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

Issue 1148473003: Fix deferred video underflow if audio underflows first. (Closed)

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

Description

Fix deferred video underflow if audio underflows first. Looks like the new video rendering path caught a pre-existing bug with how deferred video underflow is handled \o/ If audio underflowed first, the video renderer's underflow would still be deferred. If a Flush() comes in, the VideoRendererImpl would think it's in the HAVE_NOTHING state and send no update to its buffering state. Once Flush() completed and playback was restarted the renderer would start playback as soon as the audio renderer was ready since the old buffering state for video was have enough. The fix is to never defer video renderer underflow if audio has already underflowed. I've also added a DCHECK() to make it clear that the audio renderer is expected to clear deferred underflow for the video renderer during Flush(). BUG=485324 TEST=new unittest. Committed: https://crrev.com/32315ab593a4a20a7205336f66c3129658757d6d Cr-Commit-Position: refs/heads/master@{#330463}

Patch Set 1 #

Patch Set 2 : Remove CHECK. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -3 lines) Patch
M media/renderers/renderer_impl.cc View 2 chunks +8 lines, -2 lines 0 comments Download
M media/renderers/renderer_impl_unittest.cc View 1 chunk +38 lines, -0 lines 0 comments Download
M media/renderers/video_renderer_impl.cc View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 8 (2 generated)
DaleCurtis
Yay for finding old bugs :) We should probably see about getting this merged back ...
5 years, 7 months ago (2015-05-18 22:20:31 UTC) #2
xhwang
In CL description, "audio has already been deferred" should be "audio has underflowed"? otherwise lgtm!
5 years, 7 months ago (2015-05-18 23:06:16 UTC) #3
DaleCurtis
Good catch, fixed! Thanks for the fast review!
5 years, 7 months ago (2015-05-18 23:10:05 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1148473003/20001
5 years, 7 months ago (2015-05-18 23:11:41 UTC) #6
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 7 months ago (2015-05-19 00:58:18 UTC) #7
commit-bot: I haz the power
5 years, 7 months ago (2015-05-19 00:59:09 UTC) #8
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/32315ab593a4a20a7205336f66c3129658757d6d
Cr-Commit-Position: refs/heads/master@{#330463}

Powered by Google App Engine
This is Rietveld 408576698