Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSE] media/media-source/media-source-video-playback-quality.html is a constant failure with MockMSE in GPUP #11915

Conversation

jyavenard
Copy link
Member

@jyavenard jyavenard commented Mar 24, 2023

ad2552f

[MSE] media/media-source/media-source-video-playback-quality.html is a constant failure with MockMSE in GPUP
https://bugs.webkit.org/show_bug.cgi?id=254405
rdar://107182984

Reviewed by Youenn Fablet.

To get a valid VideoPlaybackQuality object about all the frames contained
in the SourceBuffer we must get the entire content first.
We modify the test so that the video is played to the end first and only
then get the VideoPlaybackQuality.

* LayoutTests/media/media-source/media-source-video-playback-quality-expected.txt:
* LayoutTests/media/media-source/media-source-video-playback-quality.html:

Canonical link: https://commits.webkit.org/262071@main

36a5457

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
  πŸ§ͺ api-ios   πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 ⏳ πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@jyavenard jyavenard self-assigned this Mar 24, 2023
@jyavenard jyavenard changed the title ??? [MSE] media/media-source/media-source-video-playback-quality.html is a constant failure with MockMSE in GPUP Mar 24, 2023
@jyavenard jyavenard added the Media Bugs related to the HTML 5 Media elements. label Mar 24, 2023
@youennf
Copy link
Contributor

youennf commented Mar 24, 2023

s/we must the entire content first./we must get the entire content first./

run('source.endOfStream()');
run('video.play()');
await waitFor(video, 'ended');
setTimeout(videoEnded, 50); // VideoPlaybackQuality is refreshed every 0.25s, wait a bit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a risk for flakiness?
Should we do the try test and if failing retry until 100 times approach?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this. What if 100 loops is done under 0.25s?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, with our current implementation there's no risk of flakiness:

  • With WK1 or WK2 with MockMSE in the CP, the object is updated synchronously when you call getVideoPlaybackQuality()
  • With the MockMSE in the GPU Process, the VPQ is updated in the next event loop
void RemoteMediaPlayerProxy::updateCachedState(bool forceCurrentTimeUpdate)
{
    if (!m_observingTimeChanges || forceCurrentTimeUpdate)
        currentTimeChanged(m_player->currentTime());

    m_cachedState.paused = m_player->paused();
    maybeUpdateCachedVideoMetrics();
    if (m_bufferedChanged) {
        m_bufferedChanged = false;
        if ((m_engineIdentifier != MediaPlayerEnums::MediaEngineIdentifier::AVFoundationMSE)
            && (m_engineIdentifier != MediaPlayerEnums::MediaEngineIdentifier::MockMSE)) {
            m_cachedState.bufferedRanges = *m_player->buffered();
        }
    }
}

it sends the time , and immediately after the player state, which contains the VPQ

@jyavenard jyavenard force-pushed the eng/MSE-mediamedia-sourcemedia-source-video-playback-quality-html-is-a-constant-failure-with-MockMSE-in-GPUP branch from 84e4339 to 36a5457 Compare March 24, 2023 14:02
@jyavenard jyavenard added the merge-queue Applied to send a pull request to merge-queue label Mar 24, 2023
…a constant failure with MockMSE in GPUP

https://bugs.webkit.org/show_bug.cgi?id=254405
rdar://107182984

Reviewed by Youenn Fablet.

To get a valid VideoPlaybackQuality object about all the frames contained
in the SourceBuffer we must get the entire content first.
We modify the test so that the video is played to the end first and only
then get the VideoPlaybackQuality.

* LayoutTests/media/media-source/media-source-video-playback-quality-expected.txt:
* LayoutTests/media/media-source/media-source-video-playback-quality.html:

Canonical link: https://commits.webkit.org/262071@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/MSE-mediamedia-sourcemedia-source-video-playback-quality-html-is-a-constant-failure-with-MockMSE-in-GPUP branch from 36a5457 to ad2552f Compare March 24, 2023 14:46
@webkit-commit-queue
Copy link
Collaborator

Committed 262071@main (ad2552f): https://commits.webkit.org/262071@main

Reviewed commits have been landed. Closing PR #11915 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit ad2552f into WebKit:main Mar 24, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
4 participants