Bug 226960 – Video poster disappears prematurely on play, leaving transparent video element.

• Component: Media
• Status: RESOLVED FIXED

[Johannes Odland]

Created attachment 431302 [details]
Video showing poster disappearing prematurely

Overview:

In the latests versions of Safari on iOS the poster disappears before the video itself is ready to play.
The background is then showing through for a while, causing a flash when starting videos. 

The video element should switch seamlessly from poster to video.


Steps to reproduce: 

1. Go to test page https://codepen.io/johannesodland/pen/XWMNvdW
2. Press play


Actual results: 

Poster disappears prematurely once user presses play, leaving a transparent element with the background showing through. 
Video appears later once it has enough data to play.

Expected results: 

Poster is visible until video is ready to play, switching seamlessly to video

Build and hardware:

Safari on iOS 14.6
iPhone 11 Pro

[Radar WebKit Bug Importer]

<rdar://problem/79315114>

[Johannes Odland]

The issue is still present in Safari 15.

[Johannes Odland]

Created attachment 454331 [details]
Video of the issue on twitter

This issue makes every video on twitter and other sites flash on play. Any hope you will look into it?

[Eric Carlson]

Created attachment 454615 [details]
Patch

[Eric Carlson]

Created attachment 454626 [details]
Patch

[Jer Noble]

Comment on attachment 454626 [details]
Patch

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

r=me with nits:

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:543
> +        bool hasAvailableVideoFrame = this->hasAvailableVideoFrame();
> +        if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo && hasAvailableVideoFrame) {
> +            haveFirstVideoFrameChanged = true;
> +            m_haveReportedFirstVideoFrame = true;
> +        } else if (!hasAvailableVideoFrame)
> +            m_haveReportedFirstVideoFrame = false;

This section of code confused me. What about this instead?:

        if (!hasAvailableVideoFrame())
            m_haveReportedFirstVideoFrame = false
        else if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo) {
            m_haveReportedFirstVideoFrame = true;
            haveFirstVideoFrameChanged = true;
        }

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:561
> -                if (m_readyState != MediaPlayer::ReadyState::HaveEnoughData && maxTimeLoaded() > currentMediaTime())
> +                if (m_readyState != MediaPlayer::ReadyState::HaveEnoughData && (!m_cachedHasVideo || m_haveReportedFirstVideoFrame) && maxTimeLoaded() > currentMediaTime())

I wonder if we should have a `shouldBlockAdvancingToFutureData` variable (initialized to the same as above) here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:582
> -    if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo && hasAvailableVideoFrame()) {
> +    if (haveFirstVideoFrameChanged) {

`haveFirstVideoFrameChanged` is slightly weird (because it's really saying the media player has a video frame available for the first time). Maybe `videoFrameBecomeAvailable`?

[Eric Carlson]

Comment on attachment 454626 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:543
>> +            m_haveReportedFirstVideoFrame = false;
> 
> This section of code confused me. What about this instead?:
> 
>         if (!hasAvailableVideoFrame())
>             m_haveReportedFirstVideoFrame = false
>         else if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo) {
>             m_haveReportedFirstVideoFrame = true;
>             haveFirstVideoFrameChanged = true;
>         }

Good idea, changed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:561
>> +                if (m_readyState != MediaPlayer::ReadyState::HaveEnoughData && (!m_cachedHasVideo || m_haveReportedFirstVideoFrame) && maxTimeLoaded() > currentMediaTime())
> 
> I wonder if we should have a `shouldBlockAdvancingToFutureData` variable (initialized to the same as above) here.

I think it makes more sense to have the logic here so I'll leave this.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:582
>> +    if (haveFirstVideoFrameChanged) {
> 
> `haveFirstVideoFrameChanged` is slightly weird (because it's really saying the media player has a video frame available for the first time). Maybe `videoFrameBecomeAvailable`?

I changed it to `firstVideoFrameBecomeAvailable`.

[Eric Carlson]

Created attachment 454642 [details]
Patch for landing

[Eric Carlson]

Created attachment 454710 [details]
Patch for landing

[Eric Carlson]

Created attachment 454713 [details]
Patch for landing

[Eric Carlson]

Created attachment 454720 [details]
Patch for landing

[Eric Carlson]

Created attachment 454771 [details]
Patch for landing

[Eric Carlson]

The Windows bot test failure, streams/readable-stream-lock-after-worker-terminates-crash.html, is unrelated.

[EWS]

Committed r291326 (248465@main): <https://commits.webkit.org/248465@main>

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

[Igor Z]

Which Safari version should this patch be included in? Current Safari Technology Preview (Release 142 (Safari 15.4, WebKit 17614.1.5.16)) seems to not include those changes. At least codepen test from comment #0 still reproduces for me.
(Asking, because I experience similar bug with my own page)

[Johannes Odland]

Created attachment 459526 [details]
Video of the bug on Safari 15.5

Tested on Safari 15.5, and the bug is still there. 

Should we register a new issue, or should this issue be reopened?

[Johannes Odland]

See https://bugs.webkit.org/show_bug.cgi?id=234743