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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by DaleCurtis
Modified:
4 years, 1 month ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, scherkus (not reviewing)
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) Patch
M media/filters/audio_renderer_impl.h View 1 2 3 4 5 chunks +46 lines, -39 lines 0 comments Download
M media/filters/audio_renderer_impl.cc View 1 2 3 18 chunks +127 lines, -119 lines 0 comments Download
M media/filters/audio_renderer_impl_unittest.cc View 1 2 3 4 8 chunks +23 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 12 (0 generated)
DaleCurtis
Should resolve hangs in the field and on the bots while ensuring stability. An alternate ...
4 years, 1 month ago (2012-10-31 22:39:34 UTC) #1
DaleCurtis
On 2012/10/31 22:39:34, DaleCurtis wrote: > Should resolve hangs in the field and on the ...
4 years, 1 month ago (2012-11-01 00:34:16 UTC) #2
DaleCurtis
On 2012/11/01 00:34:16, DaleCurtis wrote: > On 2012/10/31 22:39:34, DaleCurtis wrote: > > Should resolve ...
4 years, 1 month ago (2012-11-01 00:56:19 UTC) #3
DaleCurtis
scherkus to cc; a challenger appears! Thanks fischman!
4 years, 1 month ago (2012-11-01 19:39:13 UTC) #4
Ami GONE FROM CHROMIUM
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 ...
4 years, 1 month ago (2012-11-01 19:57:01 UTC) #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: ...
4 years, 1 month ago (2012-11-01 22:02:59 UTC) #6
Ami GONE FROM CHROMIUM
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 ...
4 years, 1 month ago (2012-11-03 00:11:44 UTC) #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, ...
4 years, 1 month ago (2012-11-03 01:32:47 UTC) #8
Ami GONE FROM CHROMIUM
LGTM and by 'G' I mean "Garbage"... I really hope you take a flamethrower to ...
4 years, 1 month ago (2012-11-03 02:38:53 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11275087/6003
4 years, 1 month ago (2012-11-03 02:49:33 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11275087/5003
4 years, 1 month ago (2012-11-03 07:07:44 UTC) #11
commit-bot: I haz the power
4 years, 1 month ago (2012-11-03 16:51:07 UTC) #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 f8e48bd