|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by chcunningham Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org, nessy, Srirama, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove HTMLMediaElement::currentTime()
This CL introduces "current/official playback position" concepts
from the spec. currentTime will return official playback position
and will not advance while scripts are running. I've also removed
old notions of cached time that were increasingly difficult to
maintain.
This change also prevents the advancing of currentTime (and
associated timeupdate events) while 'waiting' for more data. The
underlying media pipeline may slightly advance the clock after
signalling underflow, but we should conceal this in blink to avoid
sending a mixed signal about the state of playback. This was
previously accomplished in audio_renderer_impl.cc, but can now be
moved to blink with the currentTime cleanup.
TEST=blink layout tests, especially media/video-play-stall.html
BUG=648710, 658840
Committed: https://crrev.com/a7f938399ef9a6d7c2f804848ac815bc4ba3e48a
Cr-Commit-Position: refs/heads/master@{#429637}
Patch Set 1 #Patch Set 2 : Revert AudioRenderer solution #Patch Set 3 : Cleanup #
Total comments: 1
Patch Set 4 : comment fix #
Total comments: 14
Patch Set 5 : Fix test failures (initialize to 0, not NaN) #Patch Set 6 : Feedback #
Total comments: 2
Patch Set 7 : Merge upstream. Delete REVIEWER comment. #Patch Set 8 : Fix test failure (update start position upon loadeddata) #Patch Set 9 : Merge fix for Asan tests. Oilpan heap bug. #Patch Set 10 : Merge #Messages
Total messages: 52 (36 generated)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2425463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2425463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:472: void setOfficialPlaybackPosition(double) const; The const-ness of currentTime spreads to these methods. I don't like it. Open to suggestions.
Description was changed from ========== WIP: fixing currentTime in HTMLMediaElement BUG= ========== to ========== This Cl introduces "current/official playback position" concepts from the spec. currentTime return official playback position and should not advance while scripts are running. I've also remove old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710 ==========
chcunningham@chromium.org changed reviewers: + dalecurtis@chromium.org, mlamouri@chromium.org
Adding reviewers dale for audio mounir for blink
Description was changed from ========== This Cl introduces "current/official playback position" concepts from the spec. currentTime return official playback position and should not advance while scripts are running. I've also remove old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710 ========== to ========== This Cl introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710 ==========
mlamouri@chromium.org changed reviewers: + foolip@chromium.org
+foolip@ for question below. lgtm assuming tests are happy. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3312: BLINK_MEDIA_LOG << "contextDestroyed(" << (void*)this << ")"; Why did you remove this LOG? https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1641: // movie time at the moment we ran out of data to play. movie time? https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1838: time = std::max(time, earliestPossiblePosition()); "let it be that position instead" could win the title of most confusing spec wording I've read in a while :). https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1890: setOfficialPlaybackPosition(currentPlaybackPosition()); You did this somewhere above too. Can we deal with this in a cleaner fashion? Sounds a bit odd to have random places where the official playback position is reset. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1957: // REVIEWER: Not sure what the spec expects really. If I don't do this here, +foolip@, are you familiar with the spirit of the spec for this? https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:617: mutable bool m_officialPlaybackPositionNeedsUpdate; Why are these mutable? https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:1165: return true; style: wrap in { } Would it make sense to check for currentTime(0 == std::numeric_limits<double>quiet_NaN() instead of the ready state?
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Mounir. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3312: BLINK_MEDIA_LOG << "contextDestroyed(" << (void*)this << ")"; On 2016/10/26 15:32:54, mlamouri wrote: > Why did you remove this LOG? Mistake. Fixed. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1641: // movie time at the moment we ran out of data to play. On 2016/10/26 15:32:54, mlamouri wrote: > movie time? we call it movie time in scheduleTimeupdateEvent... I think I just started spreading that. I've now changed both to mediaTime :). https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1838: time = std::max(time, earliestPossiblePosition()); On 2016/10/26 15:32:54, mlamouri wrote: > "let it be that position instead" could win the title of most confusing spec > wording I've read in a while :). Lol, agree https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1890: setOfficialPlaybackPosition(currentPlaybackPosition()); On 2016/10/26 15:32:54, mlamouri wrote: > You did this somewhere above too. Can we deal with this in a cleaner fashion? > Sounds a bit odd to have random places where the official playback position is > reset. I'm not in love with it, but I'm not sure how to improve. The need to have official position update post-seek is at odds with our blocking of automatic updates under conditions of (paused || waiting). As is, I think this is still an improvement over all of the undocumented invalidateCachedTime() calls - there are only two cases where I setOfficialPlaybackPosition() that aren't mandated by the spec, and I've documented them both :). Open to suggestions though. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:617: mutable bool m_officialPlaybackPositionNeedsUpdate; On 2016/10/26 15:32:54, mlamouri wrote: > Why are these mutable? Just like with m_cachedTime, these variables end up being updated in what is externally thought of as a const method (currentTime()). Its ugly, but its perhaps more ugly to remove the const from currentTime? Open to suggestions. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:1165: return true; On 2016/10/26 15:32:54, mlamouri wrote: > style: wrap in { } Done. > > Would it make sense to check for currentTime(0 == > std::numeric_limits<double>quiet_NaN() instead of the ready state? This NaN issue actually disappeared once I realized NaN was not the right value to initialize official/current position to (now using 0). I still think this is a nice optimization, so I'll leave it. Checking readyState is the right thing, since currentTime can be zero irrespective of whether we have data to consider evicting.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This Cl introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710 ========== to ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710 ==========
Didn't look much outside the question, but feel free to ask more if useful to you. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1957: // REVIEWER: Not sure what the spec expects really. If I don't do this here, On 2016/10/26 15:32:54, mlamouri wrote: > +foolip@, are you familiar with the spirit of the spec for this? Per spec, it's OK for the duration to change any number of times during playback. If duration was way too short or way too long, then that's clearly the only way to actually play to the end and stop there sensibly. I'm inclined to suggest always updating duration once known if it's off by more than one audio sample or one video frame, but this is probably not the best CL for it.
Overall great cleanup, but the const on setters is very smelly... Is there a way to avoid that? https://codereview.chromium.org/2425463002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2425463002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1959: // REVIEWER: Not sure what the spec expects really. If I don't do this here, Remove or fix?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/10/26 19:43:30, foolip wrote: > Didn't look much outside the question, but feel free to ask more if useful to > you. > > https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1957: // REVIEWER: Not > sure what the spec expects really. If I don't do this here, > On 2016/10/26 15:32:54, mlamouri wrote: > > +foolip@, are you familiar with the spirit of the spec for this? > > Per spec, it's OK for the duration to change any number of times during > playback. If duration was way too short or way too long, then that's clearly the > only way to actually play to the end and stop there sensibly. I'm inclined to > suggest always updating duration once known if it's off by more than one audio > sample or one video frame, but this is probably not the best CL for it. Dale actually worked on this recently, but it has some significant challenges https://codereview.chromium.org/2440563004/
Re: const smell, not sure how best to improve. I agree, its ugly. I could make currentTime non-const, but that will likely have a significant ripple into const callers. Perhaps the better thing would be to update official playback position outside of currentTime, but I'm not aware of any existing trigger that I can hook into. For instance, if we had some pre-script-execution callback on HTMLElements, that might work. Technically we should be updating currentTime whenever we reach a stable state. Philip tried a while back to introduce this concept in code but it never landed. https://codereview.chromium.org/153813002/ Philip, all of the blocking bugs for that CL seem to now be resolved. Would you support trying to land that? Do you have a better idea of how to fix our const troubles? https://codereview.chromium.org/2425463002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2425463002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1959: // REVIEWER: Not sure what the spec expects really. If I don't do this here, On 2016/10/26 19:43:44, DaleCurtis wrote: > Remove or fix? Done.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ah, I see these methods already make use of a mutable cache time, so this lgtm as is, but the whole approach should be refactored to remove this long term.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2425463002/#ps160001 (title: "Fix test failure (update start position upon loadeddata)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710 ========== to ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710,658840 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2425463002/#ps200001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710,658840 ========== to ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710,658840 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710,658840 ========== to ========== Improve HTMLMediaElement::currentTime() This CL introduces "current/official playback position" concepts from the spec. currentTime will return official playback position and will not advance while scripts are running. I've also removed old notions of cached time that were increasingly difficult to maintain. This change also prevents the advancing of currentTime (and associated timeupdate events) while 'waiting' for more data. The underlying media pipeline may slightly advance the clock after signalling underflow, but we should conceal this in blink to avoid sending a mixed signal about the state of playback. This was previously accomplished in audio_renderer_impl.cc, but can now be moved to blink with the currentTime cleanup. TEST=blink layout tests, especially media/video-play-stall.html BUG=648710,658840 Committed: https://crrev.com/a7f938399ef9a6d7c2f804848ac815bc4ba3e48a Cr-Commit-Position: refs/heads/master@{#429637} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a7f938399ef9a6d7c2f804848ac815bc4ba3e48a Cr-Commit-Position: refs/heads/master@{#429637} |
