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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by DaleCurtis
Modified:
2 years, 8 months 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. #

Messages

Total messages: 12 (0 generated)
DaleCurtis
Should resolve hangs in the field and on the bots while ensuring stability. An alternate ...
2 years, 8 months 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 ...
2 years, 8 months 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 ...
2 years, 8 months ago (2012-11-01 00:56:19 UTC) #3
DaleCurtis
scherkus to cc; a challenger appears! Thanks fischman!
2 years, 8 months 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 ...
2 years, 8 months 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: ...
2 years, 8 months 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 ...
2 years, 8 months 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, ...
2 years, 8 months 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 ...
2 years, 8 months 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
2 years, 8 months 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
2 years, 8 months ago (2012-11-03 07:07:44 UTC) #11
commit-bot: I haz the power
2 years, 8 months 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 1f9106d