Bug 226960 - Video poster disappears prematurely on play, leaving transparent video element.
Summary: Video poster disappears prematurely on play, leaving transparent video element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 14
Hardware: iPhone / iPad iOS 14
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-13 22:29 PDT by Johannes Odland
Modified: 2022-05-22 22:58 PDT (History)
12 users (show)

See Also:


Attachments
Video showing poster disappearing prematurely (1.33 MB, video/mp4)
2021-06-13 22:29 PDT, Johannes Odland
no flags Details
Video of the issue on twitter (2.45 MB, video/mp4)
2022-03-10 03:19 PST, Johannes Odland
no flags Details
Patch (19.26 KB, patch)
2022-03-14 13:17 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (19.44 KB, patch)
2022-03-14 15:04 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (19.43 KB, patch)
2022-03-14 17:46 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (19.36 KB, patch)
2022-03-15 08:37 PDT, Eric Carlson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (19.36 KB, patch)
2022-03-15 08:59 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (19.37 KB, patch)
2022-03-15 10:10 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (21.70 KB, patch)
2022-03-15 16:31 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Video of the bug on Safari 15.5 (630.07 KB, video/mp4)
2022-05-17 22:38 PDT, Johannes Odland
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Johannes Odland 2021-06-13 22:29:41 PDT
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
Comment 1 Radar WebKit Bug Importer 2021-06-14 17:04:41 PDT
<rdar://problem/79315114>
Comment 2 Johannes Odland 2021-11-08 22:05:33 PST
The issue is still present in Safari 15.
Comment 3 Johannes Odland 2022-03-10 03:19:24 PST
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?
Comment 4 Eric Carlson 2022-03-14 13:17:57 PDT
Created attachment 454615 [details]
Patch
Comment 5 Eric Carlson 2022-03-14 15:04:15 PDT
Created attachment 454626 [details]
Patch
Comment 6 Jer Noble 2022-03-14 16:10:52 PDT
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`?
Comment 7 Eric Carlson 2022-03-14 17:45:01 PDT
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`.
Comment 8 Eric Carlson 2022-03-14 17:46:24 PDT
Created attachment 454642 [details]
Patch for landing
Comment 9 Eric Carlson 2022-03-15 08:37:40 PDT
Created attachment 454710 [details]
Patch for landing
Comment 10 Eric Carlson 2022-03-15 08:59:26 PDT
Created attachment 454713 [details]
Patch for landing
Comment 11 Eric Carlson 2022-03-15 10:10:32 PDT
Created attachment 454720 [details]
Patch for landing
Comment 12 Eric Carlson 2022-03-15 16:31:34 PDT
Created attachment 454771 [details]
Patch for landing
Comment 13 Eric Carlson 2022-03-15 18:00:22 PDT
The Windows bot test failure, streams/readable-stream-lock-after-worker-terminates-crash.html, is unrelated.
Comment 14 EWS 2022-03-15 20:22:45 PDT
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].
Comment 15 Igor Z 2022-04-04 08:01:25 PDT
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)
Comment 16 Johannes Odland 2022-05-17 22:38:45 PDT
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?
Comment 17 Johannes Odland 2022-05-22 22:58:27 PDT
See https://bugs.webkit.org/show_bug.cgi?id=234743