Bug 211464 – Poster set after playback begins should be ignored

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Poster set after playback begins should be ignored

[Eric Carlson]

Created attachment 398547 [details]
Patch

[Jer Noble]

Comment on attachment 398547 [details]
Patch

Nice, we got rid of setDisplayMode() entirely.

[Eric Carlson]

Created attachment 398756 [details]
Patch for landing

[Eric Carlson]

<rdar://problem/62605114>

[Eric Carlson]

Created attachment 398782 [details]
Patch for landing

[EWS]

Committed r261341: <https://trac.webkit.org/changeset/261341>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398782 [details].

[Aakash Jain]

(In reply to EWS from comment #6)
> Committed r261341: <https://trac.webkit.org/changeset/261341>
This seems to have made this test very flaky: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-video-element/intrinsic_sizes.htm
Tracked in Bug 211612.

[Ryan Haddad]

Reverted r261341 and r261392 for reason:

Caused multiple regression test failures

Committed r261409: <https://trac.webkit.org/changeset/261409>

[Eric Carlson]

Created attachment 399051 [details]
Patch

[Eric Carlson]

Created attachment 399059 [details]
Patch

[Eric Carlson]

Created attachment 399061 [details]
Patch

[Eric Carlson]

Created attachment 399133 [details]
Patch

[Darin Adler]

Comment on attachment 399133 [details]
Patch

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

Is there code that stops the poster loading once we load the first frame?

> Source/WebCore/ChangeLog:22
> +020-05-11  Antoine Quint  <[e-mail]>

Oops, deleted a "2" here.

> Source/WebCore/html/HTMLVideoElement.cpp:136
> +        if (hasAvailableVideoFrame())
> +            return;

Not really about this patch:

I wish we had a more consistent rule about when to call through to the base class in parseAttribute functions. If it was up to me, all parseAttribute functions would call through to the base class always, even when they handle the attribute, except when they are trying to disable some specific work the base class does. But it’s more efficient to *not* call through to the base class if you know it has no work to do, so some might prefer our current status quo where we often don’t call through.

> Source/WebCore/html/HTMLVideoElement.cpp:286
> +    else if (displayMode() < Poster && !hasAvailableVideoFrame())
>          setDisplayMode(Poster);

Is the idea here that if hasAvailableVideoFrame() is true, the display mode will be changing to Video very soon? Why not change it to Video right now?

[Eric Carlson]

Comment on attachment 399133 [details]
Patch

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

>> Source/WebCore/ChangeLog:22
>> +020-05-11  Antoine Quint  <[e-mail]>
> 
> Oops, deleted a "2" here.

Oops indeed, I wonder how I did that!

>> Source/WebCore/html/HTMLVideoElement.cpp:286
>>          setDisplayMode(Poster);
> 
> Is the idea here that if hasAvailableVideoFrame() is true, the display mode will be changing to Video very soon? Why not change it to Video right now?

That is the idea, but I like your suggestion - there is no reason to wait. 

I'll make that change, thanks!

[Eric Carlson]

Created attachment 399148 [details]
Patch for landing

[EWS]

Committed r261576: <https://trac.webkit.org/changeset/261576>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399148 [details].