Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581533002:
MSE: Fix logic bugs with high precision duration (Closed)
|
Created:
4 years ago by chcunningham Modified:
3 years, 10 months ago CC:
apacible+watch_chromium.org, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, erickung+watch_chromium.org, feature-media-reviews_chromium.org, fs, gasubic, miu+watch_chromium.org, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org, nessy, Srirama, vcarbune.chromium, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE: Fix logic bugs with high precision duration
Upon end of stream, chunk demuxer should decrease the duration to
match the highest buffered end time. This was not happening for
user-set durations which exceeded the highest buffered end time by
less than a microsecond.
Additionally, some style refactoring. For MSE, we pull duration
directly from the demuxer, which stores the user set value with full
precision. Moving this logic to WMPI from HTMLMediaElement ensures
duration WMPI uses matches that of HTMLMediaElement.
TEST=New layout test and manual testing of bug report steps
BUG=669997
Review-Url: https://codereview.chromium.org/2581533002
Cr-Commit-Position: refs/heads/master@{#442699}
Committed: https://chromium.googlesource.com/chromium/src/+/b92d50681a244d9cf988ea32fcdb6ea7cd096be2
Patch Set 1 #
Total comments: 23
Patch Set 2 : Feedback, simplify, rebase. #
Total comments: 1
Patch Set 3 : Fix failing test #
Total comments: 1
Patch Set 4 : Fix orientation test #
Total comments: 11
Patch Set 5 : Feedback #
Messages
Total messages: 35 (19 generated)
chcunningham@chromium.org changed reviewers: + mlamouri@chromium.org, wolenetz@chromium.org
wolenetz@ for everything mlamouri@ for HTMLMediaElement https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2017: // duration. See http://crbug.com/266644 Matt, I think this comment is stale (or I misunderstand). I checked all cases of duration changing (decrease for eos, increase for append, user specified, init segment) and found they all notify the pipeline which bubbles to WMPI and ultimately HTMLMediaElement. If you agree we should close that old bug.
lgtm https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:706: } style: no { } https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:709: return (pipeline_duration == kInfiniteDuration) nit: () are not needed https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html (right): https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:17: let increased_duration = segmentInfo.duration + /* .5 nanoseconds */ .0000000005; Unless all mediasource-* tests are using `let`, I think using `var` should be better in order to make the test re-usable by other browsers. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:26: test.waitForExpectedEvents(function() Could this simply be: ``` sourceBuffer.addEventListener('updateend', test.step_func(function() { }); ``` (ie. no need for `expectEvent` and `waitForExpectedEvents` with just one event) https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:43: assert_true(mediaSource.duration > .5); assert_greater_than(mediaSource.duration, .5); https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:49: test.waitForExpectedEvents(function() same as above but I think you can even right a one liner: ``` mediaElement.onended = test.step_func_done(); ```
https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:705: return chunk_demuxer_->GetDuration(); Is WMPI::duration() ever queried in a code path where earlier in the stack the ChunkDemuxer lock is still acquired? I seem to recall an ancient double-lock potential around this. https://codereview.chromium.org/2581533002/diff/1/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2581533002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer.cc:1255: max_duration.InSecondsF() < user_specified_duration_) { good !! https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html (right): https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:7: <link rel='stylesheet' href='/w3c/resources/testharness.css'> this isn't necessary any longer https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:30: // accurate duration). Add comment reference to existing (or new) test that demonstrates this trust please :) I'm pretty sure we have such a test downstream already, but documenting here would help. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2017: // duration. See http://crbug.com/266644 These do eventually bubble, but in the interim time, m_duration is out of date (though ::duration() with this change, will return current MSE duration). The problem is that ::duration() is not always used; there's an m_duration which is still not necessarily in agreement with WMPI, even with this change. At this time, it could be a simple fix. WDYT?
Mounir/Matt, please take a second look at HTMLMediaElement. I've reworked duration(). https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:705: return chunk_demuxer_->GetDuration(); On 2016/12/16 00:56:51, wolenetz wrote: > Is WMPI::duration() ever queried in a code path where earlier in the stack the > ChunkDemuxer lock is still acquired? I seem to recall an ancient double-lock > potential around this. Not that I'm aware of. I did some searching and couldn't find anything. This would have to be something like an event bubbled up from ChunkDemuxer. The main avenue for that is the demuxer "client", which is Pipeline::RenderWrapper, which does a good job of consistently posting things to the main thread for them to bubble up to WMPI. https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:706: } On 2016/12/15 10:20:51, mlamouri wrote: > style: no { } Done. https://codereview.chromium.org/2581533002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:709: return (pipeline_duration == kInfiniteDuration) On 2016/12/15 10:20:51, mlamouri wrote: > nit: () are not needed Done. https://codereview.chromium.org/2581533002/diff/1/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2581533002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer.cc:1255: max_duration.InSecondsF() < user_specified_duration_) { On 2016/12/16 00:56:51, wolenetz wrote: > good !! Acknowledged. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html (right): https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:7: <link rel='stylesheet' href='/w3c/resources/testharness.css'> On 2016/12/16 00:56:52, wolenetz wrote: > this isn't necessary any longer Done. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:17: let increased_duration = segmentInfo.duration + /* .5 nanoseconds */ .0000000005; On 2016/12/15 10:20:51, mlamouri wrote: > Unless all mediasource-* tests are using `let`, I think using `var` should be > better in order to make the test re-usable by other browsers. We have a lot of let usage already. https://cs.chromium.org/search/?q=f:LayoutTests+%22let+%22&sq=package:chromiu... https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:26: test.waitForExpectedEvents(function() On 2016/12/15 10:20:51, mlamouri wrote: > Could this simply be: > ``` > sourceBuffer.addEventListener('updateend', test.step_func(function() { > }); > ``` > > (ie. no need for `expectEvent` and `waitForExpectedEvents` with just one event) I started down this path but it makes the test less readable because the functions that handle the events must then be defined above the code that fires the events. I thought about working around that by having the event handlers just resolve a promise which I could hook into below, but thats just re-inventing the expectEvent/waitForExpectedEvents infra. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:30: // accurate duration). On 2016/12/16 00:56:52, wolenetz wrote: > Add comment reference to existing (or new) test that demonstrates this trust > please :) > I'm pretty sure we have such a test downstream already, but documenting here > would help. Done. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:43: assert_true(mediaSource.duration > .5); On 2016/12/15 10:20:52, mlamouri wrote: > assert_greater_than(mediaSource.duration, .5); Done. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-precise-duration.html:49: test.waitForExpectedEvents(function() On 2016/12/15 10:20:51, mlamouri wrote: > same as above but I think you can even right a one liner: > ``` > mediaElement.onended = test.step_func_done(); > ``` simplified, but continued using the waitFor. See comment above. https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2581533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2017: // duration. See http://crbug.com/266644 On 2016/12/16 00:56:52, wolenetz wrote: > These do eventually bubble, but in the interim time, m_duration is out of date > (though ::duration() with this change, will return current MSE duration). The > problem is that ::duration() is not always used; there's an m_duration which is > still not necessarily in agreement with WMPI, even with this change. At this > time, it could be a simple fix. WDYT? At this point m_duration is only used in OnDurationChanged as a guard against spamming that event. Having said that, I have taken this opportunity to clean this up. HTMLMediaElement::duration() will now simply return m_duration. m_duration will be explicitly set to NaN during invokeResourceSelectionAlgorithm() m_duration will be explicitly set to WMPI->duration() during transition to HAVE_METADATA. m_duration will be updated (already is) whenever duration changes occur from the lower layer https://codereview.chromium.org/2581533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2581533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1965: double newPosition = std::min(duration(), currentPlaybackPosition()); Moved this min logic into setOfficialPlaybackPosition() so that other callers will also clamp.
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...
Hold on that review - I've got a failing content_browsertest to look into.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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...
Go ahead with review - test is fixed. https://codereview.chromium.org/2581533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2581533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1991: !std::isnan(duration()) ? std::min(duration(), position) : position; This fixes the broken test. We initial set official playback position to 0 during load algo. NaN is apparently < 0 so this ended up clamping it to NaN which hit some DCHECKS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
It looks like the most recent patch set is failing some tests: MediaControlsOrientationLockDelegateTest.ReceivedMetadataLater MediaControlsOrientationLockDelegateTest.ReceivedMetadataAfterExitingFullscreen
Fixed orientation tests. IMO tests should not force readyState values before setting up the WebMediaPlayer. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp (right): https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:338: ReceivedMetadataAfterExitingFullscreen) { Mounir, some questions for you here. This test claims to exit full screen, but I don't see that it does. Should I add a call to simulateExitFullscreen()? https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:342: // State set to PendingMetadata. This comment is dangling. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:360: // State set to PendingMetadata. I don't follow this comment.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm back after being sick. I'll do another review round on this today.
lgtm % nits and % mlamouri@ responding to the orientation unit tests' questions. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1991: !std::isnan(duration()) ? std::min(duration(), position) : position; nit: swap the ternary clauses to remove leading '!' and simplify readability? Also, when duration() is NaN, must position be 0? (For MSE, I think so, but I'm less certain about non-MSE where earliest playback position might not be 0.) If so, sounds like a good DCHECK to add. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3061: // durationChanged() triggered by media player. nit: s/() trig/() is trig/ ? https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3071: void HTMLMediaElement::durationChanged(double duration, bool requestSeek) { aside: the .h comment for this method indicates it's for MSE, but it looks like it's at least in the common path for any durationChange coming from webMediaPlayer now. Are we certain that no non-MSE media player ever calls durationChanged? This is just a drive-by and not critical to this particular CL.
Changes to MediaControlsOrientationLockDelegateTest lgtm. Feel free to rephrase the comments that didn't look clear. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp (right): https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:342: // State set to PendingMetadata. On 2017/01/04 at 00:11:54, chcunningham_OOO_Jan_9 wrote: > This comment is dangling. It's not though I agree it's unclear: the internal state is now `PendingMetadata` after the `simulateEnterFullscreen()` call. I don't test it because it was tested somewhere else. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegateTest.cpp:360: // State set to PendingMetadata. On 2017/01/04 at 00:11:54, chcunningham_OOO_Jan_9 wrote: > I don't follow this comment. As above.
Thanks https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1991: !std::isnan(duration()) ? std::min(duration(), position) : position; On 2017/01/09 22:19:35, wolenetz_slow_jan9 wrote: > nit: swap the ternary clauses to remove leading '!' and simplify readability? Done. > Also, when duration() is NaN, must position be 0? (For MSE, I think so, but I'm > less certain about non-MSE where earliest playback position might not be 0.) If > so, sounds like a good DCHECK to add. I think you might be right about non-MSE, so leaving no DCHECK. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3061: // durationChanged() triggered by media player. On 2017/01/09 22:19:34, wolenetz_slow_jan9 wrote: > nit: s/() trig/() is trig/ ? Done. https://codereview.chromium.org/2581533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3071: void HTMLMediaElement::durationChanged(double duration, bool requestSeek) { On 2017/01/09 22:19:34, wolenetz_slow_jan9 wrote: > aside: the .h comment for this method indicates it's for MSE, but it looks like > it's at least in the common path for any durationChange coming from > webMediaPlayer now. Are we certain that no non-MSE media player ever calls > durationChanged? This is just a drive-by and not critical to this particular CL. The only external caller is MSE's durationChangeAlgorithm
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, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2581533002/#ps80001 (title: "Feedback")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484080232778650,
"parent_rev": "f87fdaa12b09ff286d873e7a3ea459507d7adb83", "commit_rev":
"b92d50681a244d9cf988ea32fcdb6ea7cd096be2"}
Message was sent while issue was closed.
Description was changed from ========== MSE: Fix logic bugs with high precision duration Upon end of stream, chunk demuxer should decrease the duration to match the highest buffered end time. This was not happening for user-set durations which exceeded the highest buffered end time by less than a microsecond. Additionally, some style refactoring. For MSE, we pull duration directly from the demuxer, which stores the user set value with full precision. Moving this logic to WMPI from HTMLMediaElement ensures duration WMPI uses matches that of HTMLMediaElement. TEST=New layout test and manual testing of bug report steps BUG=669997 ========== to ========== MSE: Fix logic bugs with high precision duration Upon end of stream, chunk demuxer should decrease the duration to match the highest buffered end time. This was not happening for user-set durations which exceeded the highest buffered end time by less than a microsecond. Additionally, some style refactoring. For MSE, we pull duration directly from the demuxer, which stores the user set value with full precision. Moving this logic to WMPI from HTMLMediaElement ensures duration WMPI uses matches that of HTMLMediaElement. TEST=New layout test and manual testing of bug report steps BUG=669997 Review-Url: https://codereview.chromium.org/2581533002 Cr-Commit-Position: refs/heads/master@{#442699} Committed: https://chromium.googlesource.com/chromium/src/+/b92d50681a244d9cf988ea32fcdb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/b92d50681a244d9cf988ea32fcdb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
