Bug 123374 – [MSE] Add MediaSource extensions to AudioTrack, VideoTrack, and TextTrack.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[MSE] Add MediaSource extensions to AudioTrack, VideoTrack, and TextTrack.

[Jer Noble]

Created attachment 215513 [details]
Patch

[Eric Carlson]

Comment on attachment 215513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215513&action=review

> Source/WebCore/Modules/mediasource/MediaSource.cpp:373
> +        return m_duration - endTime < 1;

Nit: this confused me, how about "return endTime >= m_duration"?

Isn't this is more conservative than it needs to be, won't it delay 'canplaythrough' until the entire file has been buffered?

> Source/WebCore/Modules/mediasource/MediaSource.cpp:390
> +        return endTime - m_time > 1;

Nit: this confused me, how about "return endTime > m_time"?

> Source/WebCore/html/HTMLMediaElement.cpp:1124
> +        if (m_mediaSource->attachToElement(this))

m_mediaSource is created externally and is ref'd by the media source registry, so shouldn't you also call m_mediaSource->attachToElement(nullptr) at some point?

> Source/WebCore/html/HTMLMediaElement.cpp:2906
> +    textTrack->setMediaElement(this);

Is this necessary, it is already done in TextTrackList::append?

> Source/WebCore/html/track/AudioTrackList.cpp:91
> +bool AudioTrackList::isAnyTrackEnabled() const
> +{
> +    for (size_t i = 0; i < m_inbandTracks.size(); ++i) {
> +        AudioTrack* track = toAudioTrack(m_inbandTracks[i].get());
> +        if (track->enabled())
> +            return true;
> +    }
> +    return false;
> +}

Instead of overriding isAnyTrackEnabled, can you add a virtual method that different track types override for their definition of "enabled" (eg. willRenderData() or some such) and put this in TrackListBase?

> Source/WebCore/html/track/TextTrack.cpp:178
> -    TrackBase::setKind(newKind);
> +#if ENABLE(MEDIA_SOURCE)

What happens when MEDIA_SOURCE_ENABLED is not defined?

> Source/WebCore/html/track/VideoTrack.cpp:197
> +    // FIXME: Validate the BCP47-ness of langague.

Nit: a bug number would be good.

> Source/WebCore/html/track/VideoTrack.cpp:199
> +    if (!language.isNull() && !language.isEmpty())
> +        return;

1) there is no need to check both because isEmpty() returns true for a NULL string. 2) do you really want to return early when the language is NOT empty?

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 215513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215513&action=review
> 
> > Source/WebCore/Modules/mediasource/MediaSource.cpp:373
> > +        return m_duration - endTime < 1;
> 
> Nit: this confused me, how about "return endTime >= m_duration"?

Because the reported duration may not be equal to the end time of the last sample.

> Isn't this is more conservative than it needs to be, won't it delay 'canplaythrough' until the entire file has been buffered?

Very much so!  We need some kind of playability monitor which can take network conditions into account.  I used to have a fixme here, so I'll add another one.

> > Source/WebCore/Modules/mediasource/MediaSource.cpp:390
> > +        return endTime - m_time > 1;
> 
> Nit: this confused me, how about "return endTime > m_time"?

Because technically the spec says that HAVE_FUTURE means that there is enough data to advance the time "least a little" bit.  I just defined "a little bit" to be "more than a second". :-D

> > Source/WebCore/html/HTMLMediaElement.cpp:1124
> > +        if (m_mediaSource->attachToElement(this))
> 
> m_mediaSource is created externally and is ref'd by the media source registry, so shouldn't you also call m_mediaSource->attachToElement(nullptr) at some point?

That happens as a side effect of calling close() (or whenever the readyState becomes "closed").

> > Source/WebCore/html/HTMLMediaElement.cpp:2906
> > +    textTrack->setMediaElement(this);
> 
> Is this necessary, it is already done in TextTrackList::append?

Yes, it needs to happen earlier.  Some of the side effects of the intermediate calls need to know what the attached HTMLMediaElement is.

> > Source/WebCore/html/track/AudioTrackList.cpp:91
> > +bool AudioTrackList::isAnyTrackEnabled() const
> > +{
> > +    for (size_t i = 0; i < m_inbandTracks.size(); ++i) {
> > +        AudioTrack* track = toAudioTrack(m_inbandTracks[i].get());
> > +        if (track->enabled())
> > +            return true;
> > +    }
> > +    return false;
> > +}
> 
> Instead of overriding isAnyTrackEnabled, can you add a virtual method that different track types override for their definition of "enabled" (eg. willRenderData() or some such) and put this in TrackListBase?

I don't really like "willRenderData()"; it sounds like a notification method. How about we just add an "enabled()" virtual method in TrackBase and have every track type implement it?

> > Source/WebCore/html/track/TextTrack.cpp:178
> > -    TrackBase::setKind(newKind);
> > +#if ENABLE(MEDIA_SOURCE)
> 
> What happens when MEDIA_SOURCE_ENABLED is not defined?

Whoops, fixed.

> > Source/WebCore/html/track/VideoTrack.cpp:197
> > +    // FIXME: Validate the BCP47-ness of langague.
> 
> Nit: a bug number would be good.

Sure thing; added.

> > Source/WebCore/html/track/VideoTrack.cpp:199
> > +    if (!language.isNull() && !language.isEmpty())
> > +        return;
> 
> 1) there is no need to check both because isEmpty() returns true for a NULL string. 2) do you really want to return early when the language is NOT empty?

Well, it made sense at the time. ;)

Seriously though, that should be something like:

if (!language.isEmpty() || !isValidBCP47(language))
    return;

So maybe i'll just remove this until the FIXME is fixed.

[Jer Noble]

Created attachment 216239 [details]
Patch for landing

[EFL EWS Bot]

Comment on attachment 216239 [details]
Patch for landing

Attachment 216239 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22778019

[Jer Noble]

Created attachment 216252 [details]
Patch for landing

Fixed build errors on platforms which do not enable MEDIA_SOURCE.

[Thiago de Barros Lacerda]

Comment on attachment 216252 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=216252&action=review

> Source/WebCore/Modules/mediasource/MediaSource.cpp:264
> +    }

This method is way too big, maybe you could split it in three: one that deals with audio tracks, other that deals with video tracks and other to deal with text tracks. You can also reuse some code

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:540
> +

This can be spitted too

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:586
> +

All that for loops above can be in a method that receives the vector to be analyzed as parameter

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:609
> +

Maybe the method that will deal with the for loop can also perform that check in the same vector?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:889
> +    } while (1);

Can't this be splitted in methods that can be reused?

[Jer Noble]

Comment on attachment 216252 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=216252&action=review

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:264
>> +    }
> 
> This method is way too big, maybe you could split it in three: one that deals with audio tracks, other that deals with video tracks and other to deal with text tracks. You can also reuse some code

I don't think this method would benefit from being broken up:

* None of this code is re-usable, so splitting it into three would be pointless and wasteful.
* The steps are also subtly different between Video, Audio, and TextTracks, so templating this code will be impossible as well.
* If you ignore the comments, there are a reasonable number of lines of code in this function.
* The algorithm isn't broken up, so breaking up this method would just confuse future developers comparing the implementation to the spec.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:540
>> +
> 
> This can be spitted too

The same reasons for not splitting the code apply here.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:586
>> +
> 
> All that for loops above can be in a method that receives the vector to be analyzed as parameter

No, that won't work because the vectors are not of the same type. See InitializationSegment in SourceBufferPrivateClient.h.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:609
>> +
> 
> Maybe the method that will deal with the for loop can also perform that check in the same vector?

Again, the vectors are not of the same type.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:889
>> +    } while (1);
> 
> Can't this be splitted in methods that can be reused?

The same reasons apply above.

[Jer Noble]

Committed r158821: <http://trac.webkit.org/changeset/158821>