Summary: | [Media][MSE] Don't emit timeUpdate after play() if currentTime hasn't changed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrique Ocaña <eocanha> | ||||||||||
Component: | Media | Assignee: | Enrique Ocaña <eocanha> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, ews-watchlist, jer.noble, pnormand, rniwa, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Enrique Ocaña
2019-03-08 03:28:24 PST
Created attachment 364006 [details]
Patch
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 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 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 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();" (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. Created attachment 364017 [details]
Patch
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 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. 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. Created attachment 364250 [details]
Patch
Comment on attachment 364250 [details] Patch Clearing flags on attachment: 364250 Committed r242791: <https://trac.webkit.org/changeset/242791> All reviewed patches have been landed. Closing bug. |