Bug 168268 – Video elements with MediaSource objects set by srcObject are not cleared when srcObject is set to null

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Video elements with MediaSource objects set by srcObject are not cleared when srcObject is set to null

[Radar WebKit Bug Importer]

<rdar://problem/30503796>

[Jer Noble]

Created attachment 301427 [details]
Patch

[WebKit Commit Bot]

Attachment 301427 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.h:96:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 301427 [details]
Patch

Great to see this fixed!
Interim review as some tests do not seem to like the patch.

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

> Source/WebCore/ChangeLog:13
> +        HTMLMediaElement.idl. Then bring the "media elemenst load" and "resource selection" algorithms up

element

> Source/WebCore/html/HTMLMediaElement.cpp:1077
> +    // value, and then invoke the elementâs media element load algorithm.

I am not sure this comment adds much here. Matter of style I guess.

> Source/WebCore/html/HTMLMediaElement.cpp:1134
> +    Ref<HTMLMediaElement> protectedThis(*this); // selectMediaResource may result in a 'beforeload' event, which can make arbitrary DOM mutations.

It is not clear that selectMediaResource is called in prepareForLoad.
Change this statement, and maybe put it after the next if statement, or just before prepareForLoad.

> Source/WebCore/html/HTMLMediaElement.cpp:1144
> +    m_resourceSelectionTaskQueue.enqueueTask([this] {

I guess m_resourceSelectionTaskQueue is owned by 'this' so there is no chance that the task be executed with a dead 'this', right?

> Source/WebCore/html/HTMLMediaElement.cpp:1260
> +    // 1. Set the elementâs networkState attribute to the NETWORK_NO_SOURCE value.

I see elementas here and in some other places. Maybe it is my browser that is messing things.

HTMLMediaElement::selectMediaResource seems like a big function.
Can it be split somehow so that we can have a better view at what it is doing?

> Source/WebCore/html/HTMLMediaElement.cpp:1289
> +    RefPtr<HTMLSourceElement> candidate;

Is 'candidate' used anywhere?

> Source/WebCore/html/HTMLMediaElement.cpp:1301
> +            for (unsigned i = 0; i < m_textTracks->length(); ++i) {

It would be nice for TextTrackList to have an iterator API for such things.

> Source/WebCore/html/HTMLMediaElement.cpp:1302
> +                TextTrack* track = m_textTracks->item(i);

It would be good to have an API where we would have a reference and not a pointer. Maybe use auto* here as well.

> Source/WebCore/html/HTMLMediaElement.cpp:1404
> +            ContentType contentType((String()));

This looks weird to have double parenthesis.
I would expect "ContentType contentType" to be similar also.
If it is not, maybe ContentType could be updated?

> Source/WebCore/html/HTMLMediaElement.h:103
> +    RefPtr<Blob>>>;

Use Ref<> instead of RefPtr<> for all cases?
If there is a null possibility, we could use nullptr to store it.

[Build Bot]

Comment on attachment 301427 [details]
Patch

Attachment 301427 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3082544

New failing tests:
fast/mediastream/MediaStream-video-element-track-stop.html
fast/mediastream/MediaStream-video-element.html

[Build Bot]

Created attachment 301435 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Eric Carlson]

Comment on attachment 301427 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:1077
>> +    // value, and then invoke the elementâs media element load algorithm.
> 
> I am not sure this comment adds much here. Matter of style I guess.

Because it is spec text, I think it helps future readers see if the spec logic has changed.

> Source/WebCore/html/HTMLMediaElement.cpp:1552
> +    if (!loadAttempted) {
> +        if (m_blob) {

Nit: these lines can be combined.

[Jer Noble]

(In reply to comment #4)
> Comment on attachment 301427 [details]
> Patch
> 
> Great to see this fixed!
> Interim review as some tests do not seem to like the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301427&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        HTMLMediaElement.idl. Then bring the "media elemenst load" and "resource selection" algorithms up
> 
> element

Fixed.

> > Source/WebCore/html/HTMLMediaElement.cpp:1077
> > +    // value, and then invoke the elementâs media element load algorithm.
> 
> I am not sure this comment adds much here. Matter of style I guess.

We've diverged a lot from the specced algorithms, in a large part because none of the algorithms are documented (at least by what version of the spec the implement).

> > Source/WebCore/html/HTMLMediaElement.cpp:1134
> > +    Ref<HTMLMediaElement> protectedThis(*this); // selectMediaResource may result in a 'beforeload' event, which can make arbitrary DOM mutations.
> 
> It is not clear that selectMediaResource is called in prepareForLoad.
> Change this statement, and maybe put it after the next if statement, or just
> before prepareForLoad.

I'll change this to read "prepareForLoad".

> > Source/WebCore/html/HTMLMediaElement.cpp:1144
> > +    m_resourceSelectionTaskQueue.enqueueTask([this] {
> 
> I guess m_resourceSelectionTaskQueue is owned by 'this' so there is no
> chance that the task be executed with a dead 'this', right?

Yes, m_resourceSelectionTaskQueue has a weakPtr in all of it's tasks; when the GenericTaskQueue is destroyed, all the weakPtrs are invalidated, and none of the enqueued tasks are executed.

> > Source/WebCore/html/HTMLMediaElement.cpp:1260
> > +    // 1. Set the elementâs networkState attribute to the NETWORK_NO_SOURCE value.
> 
> I see elementas here and in some other places. Maybe it is my browser that
> is messing things.

Looks like the HTML specification is using &lsquo;'s rather than simple apostrophes, and our "show-pretty-diff" script isn't UTF-8-aware.

> HTMLMediaElement::selectMediaResource seems like a big function.
> Can it be split somehow so that we can have a better view at what it is
> doing?

It's only about 80 non-blank, non-comment lines. I don't think that's too long.

> > Source/WebCore/html/HTMLMediaElement.cpp:1289
> > +    RefPtr<HTMLSourceElement> candidate;
> 
> Is 'candidate' used anywhere?

Good catch! I added that for the "mode == Source" part of the algorithm, but we already have an ivar for that purpose: m_nextChildNodeToConsider.  Will Delete.

> > Source/WebCore/html/HTMLMediaElement.cpp:1301
> > +            for (unsigned i = 0; i < m_textTracks->length(); ++i) {
> 
> It would be nice for TextTrackList to have an iterator API for such things.

Since this is just moving existing code to a new location, I didn't optimize or modernize the code at all. You're right that it should really be iterable.

> > Source/WebCore/html/HTMLMediaElement.cpp:1302
> > +                TextTrack* track = m_textTracks->item(i);
> 
> It would be good to have an API where we would have a reference and not a
> pointer. Maybe use auto* here as well.

Ditto above; this is just moving code to a different location.

> > Source/WebCore/html/HTMLMediaElement.cpp:1404
> > +            ContentType contentType((String()));
> 
> This looks weird to have double parenthesis.
> I would expect "ContentType contentType" to be similar also.
> If it is not, maybe ContentType could be updated?

ContentType doesn't have a default constructor, though this could be changed to: "ContentType contentType {};"

> > Source/WebCore/html/HTMLMediaElement.h:103
> > +    RefPtr<Blob>>>;
> 
> Use Ref<> instead of RefPtr<> for all cases?
>
> If there is a null possibility, we could use nullptr to store it.

We can't. I initially had the typedef use Ref<> objects, but the bindings generator requires Variant<RefPtr<>, ...>.

[Jer Noble]

Created attachment 301443 [details]
Patch

Updated with review comments; also fixed non-MEDIA_SOURCE, non-MEDIA_STREAM builds.

[WebKit Commit Bot]

Attachment 301443 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.h:96:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 301454 [details]
Patch

Addressed additional review feedback.

[WebKit Commit Bot]

Attachment 301454 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.h:96:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 301454 [details]
Patch

please fix the Windows build before landing

[Jer Noble]

Created attachment 301509 [details]
Patch for landing

One more patch to fix the Windows EWS

[WebKit Commit Bot]

Attachment 301509 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.h:96:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 301509 [details]
Patch for landing

Clearing flags on attachment: 301509

Committed r212311: <http://trac.webkit.org/changeset/212311>

[Diego Caravana]

This bug is preventing us to implement to manage video streams and detach them from any video element they have been attached to previously, basically forcing us to only build quite a basic UX.

Any recent progress here please?

[Ahmad Saleem]

Landed and didn't backed(In reply to Diego Caravana from comment #17)
> This bug is preventing us to implement to manage video streams and detach
> them from any video element they have been attached to previously, basically
> forcing us to only build quite a basic UX.
> 
> Any recent progress here please?

If it is causing issues in your web case, please open new bug with reduced testcase to show the failures / concern.

This patch has landed and didn't backed out:

https://github.com/WebKit/WebKit/commit/2f99ef618f4fea2d1abe2808751ee78eef167a21

Marking this as "RESOLVED FIXED".