Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Created:
5 years ago by DaleCurtis
Modified:
5 years 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 ...
5 years 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 ...
5 years 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 ...
5 years ago (2012-11-01 00:56:19 UTC) #3
DaleCurtis
scherkus to cc; a challenger appears! Thanks fischman!
5 years 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 ...
5 years 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: ...
5 years 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 ...
5 years 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, ...
5 years 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 ...
5 years 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
5 years 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
5 years ago (2012-11-03 07:07:44 UTC) #11
commit-bot: I haz the power
5 years ago (2012-11-03 16:51:07 UTC) #12
Change committed as 165854

Powered by Google App Engine
This is Rietveld efc10ee0f