Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 2425463002: Improve HTMLMediaElement::currentTime() (Closed)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -102 lines) Patch
M media/renderers/audio_renderer_impl.h View 1 2 9 1 chunk +0 lines, -3 lines 0 comments Download
M media/renderers/audio_renderer_impl.cc View 1 2 9 4 chunks +0 lines, -19 lines 0 comments Download
M media/renderers/audio_renderer_impl_unittest.cc View 1 9 1 chunk +1 line, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html View 1 2 1 chunk +6 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/media/video-seek-to-middle.html View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.h View 1 2 3 4 5 6 3 chunks +12 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp View 1 2 3 4 5 6 7 8 26 chunks +136 lines, -65 lines 0 comments Download
M third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 52 (36 generated)
chcunningham
https://codereview.chromium.org/2425463002/diff/60001/third_party/WebKit/Source/core/html/HTMLMediaElement.h File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2425463002/diff/60001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode472 third_party/WebKit/Source/core/html/HTMLMediaElement.h:472: void setOfficialPlaybackPosition(double) const; The const-ness of currentTime spreads to ...
4 years, 1 month ago (2016-10-25 22:46:19 UTC) #4
chcunningham
Adding reviewers dale for audio mounir for blink
4 years, 1 month ago (2016-10-25 23:32:58 UTC) #7
mlamouri (slow - plz ping)
+foolip@ for question below. lgtm assuming tests are happy. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#oldcode3312 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3312: ...
4 years, 1 month ago (2016-10-26 15:32:54 UTC) #10
chcunningham
Thanks Mounir. https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2425463002/diff/80001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#oldcode3312 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3312: BLINK_MEDIA_LOG << "contextDestroyed(" << (void*)this << ")"; ...
4 years, 1 month ago (2016-10-26 19:29:41 UTC) #17
foolip
Didn't look much outside the question, but feel free to ask more if useful to ...
4 years, 1 month ago (2016-10-26 19:43:30 UTC) #21
DaleCurtis
Overall great cleanup, but the const on setters is very smelly... Is there a way ...
4 years, 1 month ago (2016-10-26 19:43:44 UTC) #22
chcunningham
On 2016/10/26 19:43:30, foolip wrote: > Didn't look much outside the question, but feel free ...
4 years, 1 month ago (2016-10-26 21:36:27 UTC) #25
chcunningham
Re: const smell, not sure how best to improve. I agree, its ugly. I could ...
4 years, 1 month ago (2016-10-26 21:57:07 UTC) #26
DaleCurtis
Ah, I see these methods already make use of a mutable cache time, so this ...
4 years, 1 month ago (2016-10-26 22:44:42 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2425463002/160001
4 years, 1 month ago (2016-10-28 19:04:52 UTC) #34
commit-bot: I haz the power
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_asan_rel_ng/builds/252681)
4 years, 1 month ago (2016-10-28 22:05:43 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2425463002/200001
4 years, 1 month ago (2016-11-03 06:52:04 UTC) #44
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
4 years, 1 month ago (2016-11-03 08:52:49 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2425463002/200001
4 years, 1 month ago (2016-11-03 17:36:33 UTC) #48
commit-bot: I haz the power
Committed patchset #10 (id:200001)
4 years, 1 month ago (2016-11-03 17:43:45 UTC) #50
commit-bot: I haz the power
4 years, 1 month ago (2016-11-03 17:48:21 UTC) #52
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/a7f938399ef9a6d7c2f804848ac815bc4ba3e48a
Cr-Commit-Position: refs/heads/master@{#429637}

Powered by Google App Engine
This is Rietveld 408576698