Chromium Code Reviews
Help | Chromium Project | Sign in
(1164)

Issue 11275087: Move OnDecoderInitDone() from decoder to pipeline thread. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by DaleCurtis
Modified:
1 year, 5 months ago
Reviewers:
Ami Fischman
CC:
chromium-reviews_chromium.org, feature-media-reviews_chromium.org, scherkus
Visibility:
Public.

Description

Move OnDecoderInitDone() from decoder to pipeline thread.

We need to ensure |sink_| access is thread safe so every sink implementation
doesn't have to worry about locking and out of order calls.

Failure to do so will lead to dead locks and shutdown crashes.

BUG=158848, 157793
TEST=media_unittests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165854

Patch Set 1 #

Patch Set 2 : Missed AutoUnlock. #

Total comments: 10

Patch Set 3 : Comments. #

Total comments: 18

Patch Set 4 : Correctness! #

Patch Set 5 : Fix unittest. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+196 lines, -160 lines) Lint Patch
M media/filters/audio_renderer_impl.h View 1 2 3 4 5 chunks +46 lines, -39 lines 0 comments ? errors Download
M media/filters/audio_renderer_impl.cc View 1 2 3 18 chunks +127 lines, -119 lines 0 comments 0 errors Download
M media/filters/audio_renderer_impl_unittest.cc View 1 2 3 4 8 chunks +23 lines, -2 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 12
DaleCurtis
Should resolve hangs in the field and on the bots while ensuring stability. An alternate ...
1 year, 5 months ago #1
DaleCurtis
On 2012/10/31 22:39:34, DaleCurtis wrote: > Should resolve hangs in the field and on the ...
1 year, 5 months ago #2
DaleCurtis
On 2012/11/01 00:34:16, DaleCurtis wrote: > On 2012/10/31 22:39:34, DaleCurtis wrote: > > Should resolve ...
1 year, 5 months ago #3
DaleCurtis
scherkus to cc; a challenger appears! Thanks fischman!
1 year, 5 months ago #4
Ami Fischman
https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sink.cc File media/audio/null_audio_sink.cc (right): https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sink.cc#newcode72 media/audio/null_audio_sink.cc:72: if (playing_) { This variable is set on the ...
1 year, 5 months ago #5
DaleCurtis
https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sink.cc File media/audio/null_audio_sink.cc (right): https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sink.cc#newcode72 media/audio/null_audio_sink.cc:72: if (playing_) { On 2012/11/01 19:57:01, Ami Fischman wrote: ...
1 year, 5 months ago #6
Ami Fischman
https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_renderer_impl.cc File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_renderer_impl.cc#newcode45 media/filters/audio_renderer_impl.cc:45: callback.Run(); running the callback under lock looks strange. Is ...
1 year, 5 months ago #7
DaleCurtis
Are we there yet? :) https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_renderer_impl.cc File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_renderer_impl.cc#newcode45 media/filters/audio_renderer_impl.cc:45: callback.Run(); On 2012/11/03 00:11:45, ...
1 year, 5 months ago #8
Ami Fischman
LGTM and by 'G' I mean "Garbage"... I really hope you take a flamethrower to ...
1 year, 5 months ago #9
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11275087/6003
1 year, 5 months ago #10
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11275087/5003
1 year, 5 months ago #11
I haz the power (commit-bot)
1 year, 5 months ago #12
Change committed as 165854
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6