Bug 195454 - [Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't changed
Summary: [Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't changed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Other Linux
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 03:28 PST by Enrique Ocaña
Modified: 2019-03-12 04:10 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.44 KB, patch)
2019-03-08 04:47 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.87 MB, application/zip)
2019-03-08 06:53 PST, Build Bot
no flags Details
Patch (8.53 KB, patch)
2019-03-08 09:23 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (7.06 KB, patch)
2019-03-11 07:43 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2019-03-08 03:28:24 PST
MSE 2019[1] tests "26. SFRPausedAccuracy" and "27. HFRPausedAccuracy" fail on WPE on Raspberry Pi. Those tests measure the time drift (real time vs. media time) on playback and start counting since the first timeUpdate event. In WebKit, that event happens at play(), before the pipeline has completed the transition to playing. Therefore, the real time inherits this startup delay and the test thinks that the player has drifted.

I tried to convince the YouTube MSE Conformance Tests authors to ignore that initial timeUpdate event which reports no time change and actually start measuring the time drift after the first timeUpdate greater than zero[2]. However, their response quoting the spec[3] and stating that a timeupdate event is fired if "The current playback position changed as part of normal playback or in an especially interesting way, for example discontinuously." makes sense. I'm going to submit a patch to change this behaviour in WebKit, the 3 tests that depend on the old behaviour and to improve currentTime accuracy in the GStreamer media player private implementation by forcing a currentTime update on play() and pause().

[1] https://ytlr-cert.appspot.com/2019/main.html
[2] https://github.com/youtube/js_mse_eme/pull/37
[3] https://www.w3.org/TR/html52/semantics-embedded-content.html#eventdef-media-timeupdate
Comment 1 Enrique Ocaña 2019-03-08 04:47:07 PST
Created attachment 364006 [details]
Patch
Comment 2 Build Bot 2019-03-08 06:53:00 PST
Comment on attachment 364006 [details]
Patch

Attachment 364006 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11426043

New failing tests:
compositing/video/video-clip-change-src.html
Comment 3 Build Bot 2019-03-08 06:53:02 PST
Created attachment 364009 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 4 Jer Noble 2019-03-08 07:41:33 PST
Comment on attachment 364006 [details]
Patch

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

This looks fine to me with a minor nit which could be addressed before landing.

> Source/WebCore/html/HTMLMediaElement.cpp:3594
> +        m_lastTimeUpdateEventMovieTime = currentMediaTime();

There is a slight inefficiency here, in that calling currentMediaTime() will go down all the way to the MediaPlayerPrivate again (and could have a different answer the second time!). So I would slightly refactor this so that m_lstTimeUpdateEventMovieTime was set before m_playbackStartedTime, and the playback value was just generated from the update value.
Comment 5 Eric Carlson 2019-03-08 08:45:42 PST
Comment on attachment 364006 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:3594
>> +        m_lastTimeUpdateEventMovieTime = currentMediaTime();
> 
> There is a slight inefficiency here, in that calling currentMediaTime() will go down all the way to the MediaPlayerPrivate again (and could have a different answer the second time!). So I would slightly refactor this so that m_lstTimeUpdateEventMovieTime was set before m_playbackStartedTime, and the playback value was just generated from the update value.

Or "m_lastTimeUpdateEventMovieTime = m_playbackStartedTime.toDouble();"
Comment 6 Jer Noble 2019-03-08 09:02:39 PST
(In reply to Eric Carlson from comment #5)
> Comment on attachment 364006 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364006&action=review
> 
> >> Source/WebCore/html/HTMLMediaElement.cpp:3594
> >> +        m_lastTimeUpdateEventMovieTime = currentMediaTime();
> > 
> > There is a slight inefficiency here, in that calling currentMediaTime() will go down all the way to the MediaPlayerPrivate again (and could have a different answer the second time!). So I would slightly refactor this so that m_lstTimeUpdateEventMovieTime was set before m_playbackStartedTime, and the playback value was just generated from the update value.
> 
> Or "m_lastTimeUpdateEventMovieTime = m_playbackStartedTime.toDouble();"

I think m_lastTimeUpdateEventMovieTime is a MediaTime, and m_playbackStartedTime is a double, so it has to be the other way around.
Comment 7 Enrique Ocaña 2019-03-08 09:23:38 PST
Created attachment 364017 [details]
Patch
Comment 8 Philippe Normand 2019-03-08 09:32:02 PST
Comment on attachment 364017 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:476
> +    // This will force a position update next time playbackPosition() is called.
> +    m_lastQueryTime = WTF::Seconds(0);
> +    playbackPosition();

Is it really necessary to call playbackPosition()?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:494
> +    // This will force a position update next time playbackPosition() is called.
> +    m_lastQueryTime = WTF::Seconds(0);
> +    playbackPosition();

Ditto :) Also if we end-up with copy-paste code it might be worth factoring it out to a private method, something like clearCachedPosition() maybe? It could include a GST_DEBUG_OBJECT() call as well.
Comment 9 Enrique Ocaña 2019-03-08 10:10:45 PST
Comment on attachment 364017 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:476
>> +    playbackPosition();
> 
> Is it really necessary to call playbackPosition()?

Yes, not only because that method updates the position, but because it has the logic that manages the cached time and takes decisions in some corner cases such as seeks. The m_lastQueryTime thing is just to force a cache update when playbackPosition() is called ("the cache was last updated at the begining of epoch", so it's too old now and must be updated), it has nothing to do with setting currentTime=0 or anything like that.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:494
>> +    playbackPosition();
> 
> Ditto :) Also if we end-up with copy-paste code it might be worth factoring it out to a private method, something like clearCachedPosition() maybe? It could include a GST_DEBUG_OBJECT() call as well.

Thanks for the suggestion.
Comment 10 Enrique Ocaña 2019-03-11 07:34:22 PDT
After refactoring the cached position refresh, trying the patch again on Raspberry Pi and analyzing the accuracy increase refreshing the cache vs. doing nothing (only the timeUpdate change in HTMLMediaElement) I've realized that the cached position refresh not only isn't needed but actually makes things worse.

That cached position refresh was my first attempt to fix the YouTube test, but the actual fix was the timeUpdate one. Therefore, I think it's better to keep things simple and submit only the timeUpdate part of the patch. I've also changed the bug name to reflect that.
Comment 11 Enrique Ocaña 2019-03-11 07:43:26 PDT
Created attachment 364250 [details]
Patch
Comment 12 WebKit Commit Bot 2019-03-12 04:09:51 PDT
Comment on attachment 364250 [details]
Patch

Clearing flags on attachment: 364250

Committed r242791: <https://trac.webkit.org/changeset/242791>
Comment 13 WebKit Commit Bot 2019-03-12 04:09:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-03-12 04:10:23 PDT
<rdar://problem/48803810>