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

Issue 1863373002: Fix underflow and av/sync issues in low delay and stall presence. (Closed)

Created:
4 years, 8 months ago by DaleCurtis
Modified:
4 years, 8 months ago
Reviewers:
xhwang
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
http://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix underflow and av/sync issues in low delay and stall presence. When in low delay mode we were previously transitioning to HAVE_ENOUGH if any frame was available; this may include frames which are bad. I.e. frames that are too far in the past to be effective. Instead we need to enter the underflow state in these cases and wait for effective frames to enter the queue. In low delay this should just add a frame or two of latency while we catchup since the frames should be readily available in most cases. In the stalled case, i.e. VideoFrameStream::CanReadWithoutStalling() is false, this will attempt to purge frames as fast as possible such that the decoder can catch up. This is done by taking advantage of a new VideoRendererAlgorithm::Reset() call which allows the frame queue to be purged while maintaining some of the next frame estimates. In all cases this patch also adds the ability to try and avoid ever reaching the paused underflow state by removing expired frames as they are enqueued if the decoder is not keeping. BUG=599908, 601066 TEST=new unittest, manual testing w/ AVDA adaptivePlayback=false. Committed: https://crrev.com/675f8b4aacfec7f7fb1d33f430996672c4dc3eec Cr-Commit-Position: refs/heads/master@{#389564}

Patch Set 1 #

Patch Set 2 : Fixes. #

Patch Set 3 : Rebase. #

Patch Set 4 : Moar tests. #

Total comments: 1

Patch Set 5 : Rebase. Rename enum. #

Patch Set 6 : Clarified comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+268 lines, -114 lines) Patch
M content/common/gpu/media/android_video_decode_accelerator.cc View 1 2 3 4 5 1 chunk +9 lines, -8 lines 0 comments Download
M content/common/gpu/media/gpu_video_decode_accelerator.cc View 1 2 3 4 5 1 chunk +5 lines, -3 lines 0 comments Download
M media/base/video_decoder.h View 1 2 3 4 5 2 chunks +7 lines, -3 lines 0 comments Download
M media/filters/ffmpeg_video_decoder.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/filters/gpu_video_decoder.cc View 1 2 3 4 5 3 chunks +19 lines, -4 lines 0 comments Download
M media/filters/vpx_video_decoder.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/renderers/video_renderer_impl.h View 1 2 3 2 chunks +19 lines, -0 lines 0 comments Download
M media/renderers/video_renderer_impl.cc View 1 2 3 4 chunks +62 lines, -39 lines 0 comments Download
M media/renderers/video_renderer_impl_unittest.cc View 1 2 3 4 2 chunks +145 lines, -55 lines 0 comments Download

Messages

Total messages: 25 (13 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863373002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863373002/20001
4 years, 8 months ago (2016-04-09 00:55:00 UTC) #2
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 8 months ago (2016-04-09 04:08:58 UTC) #4
DaleCurtis
Preliminary version of the underflow fix. Needs working tests.
4 years, 8 months ago (2016-04-11 20:52:00 UTC) #7
DaleCurtis
Okay this is all done now; test all teh things! I've confirmed the new tests ...
4 years, 8 months ago (2016-04-11 23:50:57 UTC) #9
xhwang
LGTM with tiny nit https://chromiumcodereview.appspot.com/1863373002/diff/80001/media/renderers/video_renderer_impl_unittest.cc File media/renderers/video_renderer_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/1863373002/diff/80001/media/renderers/video_renderer_impl_unittest.cc#newcode282 media/renderers/video_renderer_impl_unittest.cc:282: enum class UnderflowTestType { kNormal, ...
4 years, 8 months ago (2016-04-14 21:28:00 UTC) #11
DaleCurtis
I hate upper case :) But will do it before landing. Going to hold off ...
4 years, 8 months ago (2016-04-15 01:29:10 UTC) #12
DaleCurtis
Landing this now that the other CL has landed and I have a final fix ...
4 years, 8 months ago (2016-04-25 19:06:27 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863373002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863373002/100001
4 years, 8 months ago (2016-04-25 19:07:02 UTC) #17
commit-bot: I haz the power
Failed to commit the patch.
4 years, 8 months ago (2016-04-25 20:25:45 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863373002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863373002/100001
4 years, 8 months ago (2016-04-25 21:24:13 UTC) #22
commit-bot: I haz the power
Committed patchset #5 (id:100001)
4 years, 8 months ago (2016-04-25 21:30:04 UTC) #23
commit-bot: I haz the power
4 years, 8 months ago (2016-04-25 21:32:39 UTC) #25
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/675f8b4aacfec7f7fb1d33f430996672c4dc3eec
Cr-Commit-Position: refs/heads/master@{#389564}

Powered by Google App Engine
This is Rietveld 408576698