Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
126793002:
Add Stop() to AudioDecoder. (Closed)
|
Created:
6 years, 11 months ago by rileya (GONE FROM CHROMIUM) Modified:
6 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Stop() to AudioDecoder.
BUG=329379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244994
Patch Set 1 #Patch Set 2 : remove blank lines #
Total comments: 21
Patch Set 3 : Address comments\ #Patch Set 4 : Add TODO to opus decoder. #Patch Set 5 : Call reset and stop callbacks after releasing |lock_| #
Total comments: 2
Patch Set 6 : Handle stop with pending demuxer read #
Total comments: 2
Patch Set 7 : handle Stop/Reset with pending reads for ffmpeg and opus decoders #Patch Set 8 : #
Total comments: 6
Patch Set 9 : #Patch Set 10 : Add comments to AudioDecoder.h and add a couple more BelongsToCurrentLoop's #
Total comments: 1
Patch Set 11 : Use BTCL in decoders and not ARI #
Total comments: 12
Patch Set 12 : Fix nits #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : Add comments #Patch Set 16 : #
Total comments: 5
Patch Set 17 : Cleanup + add unit tests to FFmpegAudioDecoder #Patch Set 18 : Some opus audio decoder cleanup #
Total comments: 20
Patch Set 19 : Add opus unit tests. #
Total comments: 2
Patch Set 20 : Address comments. #Patch Set 21 : remove last MessageLoop::RunUntilIdle instance #Patch Set 22 : remove opus tests (to be made into a new CL) #
Total comments: 11
Patch Set 23 : Address comments. #
Total comments: 6
Patch Set 24 : cleanup #Patch Set 25 : Move expectations up in Reset/StopFinished #Patch Set 26 : Remove extraneous #include #Messages
Total messages: 44 (0 generated)
Another step towards making the interfaces of the Audio and Video Decoders match up better. Each of the decoders seem to have their own state machines and quirks, so I'm not super confident all of the logic is 100% correct (the unit and layout tests all pass at least...).
Looks pretty good. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:192: base::AutoLock auto_lock(lock_); Previous method doesn't run this under lock. It may be important not to do that. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:199: algorithm_.reset(NULL); drop NULL. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:222: StopDecoder(); You can probably drop the StopDecoder() method in favor of inlining it here. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:226: { Just call StopDecoderDone here? https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:162: avcodec_flush_buffers(codec_context_.get()); What made you add this? We don't make a similar call in the FFmpeg video decoder for Stop, only during Reset. I'm not sure it's wrong, I'm just curious.
Thanks! Just a few comments. https://codereview.chromium.org/126793002/diff/30001/media/base/audio_decoder.h File media/base/audio_decoder.h (right): https://codereview.chromium.org/126793002/diff/30001/media/base/audio_decoder... media/base/audio_decoder.h:52: // Reset decoder state, dropping any queued encoded data. nit: since you are here, s/Reset/Resets, same as Initialize on l.31 and Request on l.38. thanks! https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:203: flush_cb_.Reset(); Wrap 199-203 in a helper function so it can be shared here and on line 227? https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:230: init_cb_.Reset(); If Init() didn't succeed, Stop() should not be called (needs to be verified). So probably we should DCHECK(init_cb_.is_null()) here. https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:167: stop_cb.Run(); Here and in Reset(), what if we have |read_cb_| pending? Usually in that case we should abort the read, then satisfy reset_cb or stop_cb, or defer the reset/stop process until Demuxer read is back. You can add a TODO and fix this in another CL. https://codereview.chromium.org/126793002/diff/30001/media/filters/opus_audio... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:338: stop_cb.Run(); ditto about Stop() during pending demuxer read.
https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:192: base::AutoLock auto_lock(lock_); On 2014/01/08 00:30:54, DaleCurtis wrote: > Previous method doesn't run this under lock. It may be important not to do > that. Which previous method do you mean? https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:203: flush_cb_.Reset(); On 2014/01/08 01:33:37, xhwang wrote: > Wrap 199-203 in a helper function so it can be shared here and on line 227? StopDecoderDone was already basically this, so per Dale's suggestion on 226 I just called it there. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:222: StopDecoder(); On 2014/01/08 00:30:54, DaleCurtis wrote: > You can probably drop the StopDecoder() method in favor of inlining it here. Done. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:226: { On 2014/01/08 00:30:54, DaleCurtis wrote: > Just call StopDecoderDone here? Makes sense, done. https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:230: init_cb_.Reset(); On 2014/01/08 01:33:37, xhwang wrote: > If Init() didn't succeed, Stop() should not be called (needs to be verified). So > probably we should DCHECK(init_cb_.is_null()) here. Alright, added a check (line 191). https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:162: avcodec_flush_buffers(codec_context_.get()); On 2014/01/08 00:30:54, DaleCurtis wrote: > What made you add this? We don't make a similar call in the FFmpeg video > decoder for Stop, only during Reset. I'm not sure it's wrong, I'm just curious. I'm not sure actually I think I saw it done in Reset and figured it couldn't hurt. In any case, I got rid of it. https://codereview.chromium.org/126793002/diff/30001/media/filters/ffmpeg_aud... media/filters/ffmpeg_audio_decoder.cc:167: stop_cb.Run(); On 2014/01/08 01:33:37, xhwang wrote: > Here and in Reset(), what if we have |read_cb_| pending? Usually in that case we > should abort the read, then satisfy reset_cb or stop_cb, or defer the reset/stop > process until Demuxer read is back. > > You can add a TODO and fix this in another CL. I'd wondered about this, added a TODO. https://codereview.chromium.org/126793002/diff/30001/media/filters/opus_audio... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/opus_audio... media/filters/opus_audio_decoder.cc:338: stop_cb.Run(); On 2014/01/08 01:33:37, xhwang wrote: > ditto about Stop() during pending demuxer read. Added a TODO.
https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:192: base::AutoLock auto_lock(lock_); On 2014/01/08 21:05:50, rileya wrote: > On 2014/01/08 00:30:54, DaleCurtis wrote: > > Previous method doesn't run this under lock. It may be important not to do > > that. > > Which previous method do you mean? See line 236 in Stop() below. callback.Run() occurs after releasing the lock_.
https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:192: base::AutoLock auto_lock(lock_); On 2014/01/08 21:51:45, DaleCurtis wrote: > On 2014/01/08 21:05:50, rileya wrote: > > On 2014/01/08 00:30:54, DaleCurtis wrote: > > > Previous method doesn't run this under lock. It may be important not to do > > > that. > > > > Which previous method do you mean? > > See line 236 in Stop() below. callback.Run() occurs after releasing the lock_. Ahh, gotcha. Looks like ResetDecoderDone also calls the reset callback before releasing the lock, would changing both to call after releasing |lock_| be reasonable?
https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/30001/media/filters/audio_rend... media/filters/audio_renderer_impl.cc:192: base::AutoLock auto_lock(lock_); On 2014/01/08 22:02:55, rileya wrote: > On 2014/01/08 21:51:45, DaleCurtis wrote: > > On 2014/01/08 21:05:50, rileya wrote: > > > On 2014/01/08 00:30:54, DaleCurtis wrote: > > > > Previous method doesn't run this under lock. It may be important not to > do > > > > that. > > > > > > Which previous method do you mean? > > > > See line 236 in Stop() below. callback.Run() occurs after releasing the lock_. > > Ahh, gotcha. Looks like ResetDecoderDone also calls the reset callback before > releasing the lock, would changing both to call after releasing |lock_| be > reasonable? sgtm
On 2014/01/08 23:56:14, DaleCurtis wrote: > sgtm Done.
https://codereview.chromium.org/126793002/diff/290001/media/filters/audio_ren... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/290001/media/filters/audio_ren... media/filters/audio_renderer_impl.cc:165: if (state_ == kStopped) State must be checked under lock; it can be changed on the audio thread. See the comment on |lock_| for which variables must only be accessed under lock. Just the callback should be run outside of lock. Also it'd be helpful if this method had a DCHECK(task_runnner_->BelongsToCurrentThread()); Ditto for StopDecoderDone(). https://codereview.chromium.org/126793002/diff/290001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/290001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:166: // TODO(rileya): Properly handle the case in which we have a pending Hmm, I suspect you may need to handle that in this CL. The previous behavior didn't clear this until destruction. Yet now you might have BufferReady() return with a valid buffer, which will wreck havok in the RunDecodeLoop() function.
On 2014/01/09 01:24:49, DaleCurtis wrote: > https://codereview.chromium.org/126793002/diff/290001/media/filters/audio_ren... > File media/filters/audio_renderer_impl.cc (right): > > https://codereview.chromium.org/126793002/diff/290001/media/filters/audio_ren... > media/filters/audio_renderer_impl.cc:165: if (state_ == kStopped) > State must be checked under lock; it can be changed on the audio thread. See the > comment on |lock_| for which variables must only be accessed under lock. > > Just the callback should be run outside of lock. Also it'd be helpful if this > method had a DCHECK(task_runnner_->BelongsToCurrentThread()); Ditto for > StopDecoderDone(). > > https://codereview.chromium.org/126793002/diff/290001/media/filters/ffmpeg_au... > File media/filters/ffmpeg_audio_decoder.cc (right): > > https://codereview.chromium.org/126793002/diff/290001/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder.cc:166: // TODO(rileya): Properly handle the > case in which we have a pending > Hmm, I suspect you may need to handle that in this CL. The previous behavior > didn't clear this until destruction. Yet now you might have BufferReady() return > with a valid buffer, which will wreck havok in the RunDecodeLoop() function. Okay, I handle this by setting the stop callback and if there's a read pending, I wait until BufferReady gets called to do the actual stop (if |stop_cb_| is non-null I discard the buffer and call the aptly named DoStop). Does this seem like a reasonable approach? If so I'll do the same in the opus decoder. Also, what is the expected behavior for Reset() in the case of a call while a read is pending? It's worth noting that this logic will have to get rearranged a fair bit in the move to AudioBufferStream (Read() will become Decode() a la VideoDecoder).
Looks okay w/ a DCHECK() change. I'm a little surprised a test didn't fail the way it was before. Can you see if it wouldn't be too hard to add a unittest either to ARI or the FFmpegAudioDecoder to test for Stop() while a Read() is outstanding? https://codereview.chromium.org/126793002/diff/430001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/430001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:121: // Don't allow further reads after Stop(). Since the renderer / AudioBufferStream will invoke Stop(), I think you can just DCHECK(stop_cb_.is_null()) here? https://codereview.chromium.org/126793002/diff/430001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:168: if (read_cb_.is_null()) { {} is not necessary on single line conditionals.
On 2014/01/09 21:10:42, DaleCurtis wrote: > Looks okay w/ a DCHECK() change. I'm a little surprised a test didn't fail the > way it was before. > > Can you see if it wouldn't be too hard to add a unittest either to ARI or the > FFmpegAudioDecoder to test for Stop() while a Read() is outstanding? > > https://codereview.chromium.org/126793002/diff/430001/media/filters/ffmpeg_au... > File media/filters/ffmpeg_audio_decoder.cc (right): > > https://codereview.chromium.org/126793002/diff/430001/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder.cc:121: // Don't allow further reads after > Stop(). > Since the renderer / AudioBufferStream will invoke Stop(), I think you can just > DCHECK(stop_cb_.is_null()) here? > > https://codereview.chromium.org/126793002/diff/430001/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder.cc:168: if (read_cb_.is_null()) { > {} is not necessary on single line conditionals. Alright, I changed that to a DCHECK and applied the same logic to Reset and also to Stop+Reset in the Opus decoder. I also added a test for a Stop on pending read to AudioRendererImpl.
https://codereview.chromium.org/126793002/diff/630001/media/filters/audio_ren... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/630001/media/filters/audio_ren... media/filters/audio_renderer_impl.cc:187: { Both ***Done() methods should have DCHECK(task_runner_->BelongsToCurrentThread()); https://codereview.chromium.org/126793002/diff/630001/media/filters/decryptin... File media/filters/decrypting_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/630001/media/filters/decryptin... media/filters/decrypting_audio_decoder.cc:236: if (state_ == kStopped) { Extra {} https://codereview.chromium.org/126793002/diff/630001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/630001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:274: // Reset() is pending, we'll ignore the buffer we just got, fire read_cb_, and Not sure this is necessary, but should be fine. https://codereview.chromium.org/126793002/diff/630001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/630001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:324: reset_cb_ = BindToCurrentLoop(closure); Do we need the BTCL here and below? Ditto for FFmpegAudioDecoder. ARI is also using BTCL, so this is now taking a couple hops. It'd be better to clarify the contract in the header file and DCHECK() things are on the right thread.
https://codereview.chromium.org/126793002/diff/630001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/630001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:274: // Reset() is pending, we'll ignore the buffer we just got, fire read_cb_, and On 2014/01/09 23:04:21, DaleCurtis wrote: > Not sure this is necessary, but should be fine. Yeah, I wasn't quite sure. DecryptingAudioDecoder does this (wait for pending read to complete before a reset), so I figured I'd match that. https://codereview.chromium.org/126793002/diff/630001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/630001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:324: reset_cb_ = BindToCurrentLoop(closure); On 2014/01/09 23:04:21, DaleCurtis wrote: > Do we need the BTCL here and below? Ditto for FFmpegAudioDecoder. ARI is also > using BTCL, so this is now taking a couple hops. It'd be better to clarify the > contract in the header file and DCHECK() things are on the right thread. I saw Reset using BTCL originally and figured there was a good reason... but I guess it does look unnecessary. How would you suggest noting this in the header? I don't completely understand how BTCL works, so I'm a little iffy on how to explain things...
On 2014/01/10 00:04:04, rileya wrote: > https://codereview.chromium.org/126793002/diff/630001/media/filters/ffmpeg_au... > File media/filters/ffmpeg_audio_decoder.cc (right): > > https://codereview.chromium.org/126793002/diff/630001/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder.cc:274: // Reset() is pending, we'll ignore > the buffer we just got, fire read_cb_, and > On 2014/01/09 23:04:21, DaleCurtis wrote: > > Not sure this is necessary, but should be fine. > > Yeah, I wasn't quite sure. DecryptingAudioDecoder does this (wait for pending > read to complete before a reset), so I figured I'd match that. > > https://codereview.chromium.org/126793002/diff/630001/media/filters/opus_audi... > File media/filters/opus_audio_decoder.cc (right): > > https://codereview.chromium.org/126793002/diff/630001/media/filters/opus_audi... > media/filters/opus_audio_decoder.cc:324: reset_cb_ = BindToCurrentLoop(closure); > On 2014/01/09 23:04:21, DaleCurtis wrote: > > Do we need the BTCL here and below? Ditto for FFmpegAudioDecoder. ARI is > also > > using BTCL, so this is now taking a couple hops. It'd be better to clarify > the > > contract in the header file and DCHECK() things are on the right thread. > > I saw Reset using BTCL originally and figured there was a good reason... but I > guess it does look unnecessary. How would you suggest noting this in the header? > I don't completely understand how BTCL works, so I'm a little iffy on how to > explain things... Okay, I added comments to the header as you suggested and I think I addressed all the other comments...
Sorry for the confusion, otherwise el-gee-tee-em. https://codereview.chromium.org/126793002/diff/700001/media/filters/audio_ren... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/700001/media/filters/audio_ren... media/filters/audio_renderer_impl.cc:223: decoder_->Stop(BindToCurrentLoop( This works, but scherkus@ pointed out that our policy is that the calling code doesn't use BTCL, it's the called code's responsibility to ensure callbacks are reentrant: https://groups.google.com/a/google.com/forum/#!msg/videostack-eng/ymbLfQtbTBM... Given this, we should remove all the BTCL's from ARI and keep / add ones to the various decoders.
On 2014/01/10 01:36:42, DaleCurtis wrote: > Sorry for the confusion, otherwise el-gee-tee-em. > > https://codereview.chromium.org/126793002/diff/700001/media/filters/audio_ren... > File media/filters/audio_renderer_impl.cc (right): > > https://codereview.chromium.org/126793002/diff/700001/media/filters/audio_ren... > media/filters/audio_renderer_impl.cc:223: decoder_->Stop(BindToCurrentLoop( > This works, but scherkus@ pointed out that our policy is that the calling code > doesn't use BTCL, it's the called code's responsibility to ensure callbacks are > reentrant: > > https://groups.google.com/a/google.com/forum/#%21msg/videostack-eng/ymbLfQtbT... > > Given this, we should remove all the BTCL's from ARI and keep / add ones to the > various decoders. Okay, added to the decoders and removed from ARI.
lgtm % nits. Wait for xhwang@ to approve for the decrypting_demuxer_stream changes. https://codereview.chromium.org/126793002/diff/740001/media/filters/audio_ren... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/audio_ren... media/filters/audio_renderer_impl.cc:150: decrypting_demuxer_stream_->Reset(BindToCurrentLoop( Fix this one too? https://codereview.chromium.org/126793002/diff/740001/media/filters/decryptin... File media/filters/decrypting_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/decryptin... media/filters/decrypting_audio_decoder.cc:167: BindToCurrentLoop(closure).Run(); Instead of BTCL.Run(), just PostTask() to avoid unnecessary binding.
Nits fixed.
We are pretty strict about callback firing in media code: 1) Callbacks must be fired. The caller may be waiting for the callback so non-satisfied-callback can cause hang. 2) Callbacks should be fired in order. For our case, it's read_cb < reset_cb < stop_cb. For example, if we call Read(), then Reset() then Stop() without waiting for the callback, the read_cb should be fired before reset_cb, which should be before stop_cb. Since FFAD and OAD are actually sync decoders, when we change Read() to Decode(), most of these complexity will go away. But unfortunately for this CL we still need to fix it. https://codereview.chromium.org/126793002/diff/740001/media/filters/audio_ren... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/audio_ren... media/filters/audio_renderer_impl.cc:150: decrypting_demuxer_stream_->Reset(BindToCurrentLoop( On 2014/01/10 02:03:00, DaleCurtis wrote: > Fix this one too? Yeah, we agreed that the callee should make sure the callback is fired acyncly. So the caller can just drop it. See: https://code.google.com/p/chromium/codesearch#search/&q=decrypting_demuxer_st... https://codereview.chromium.org/126793002/diff/740001/media/filters/audio_ren... File media/filters/audio_renderer_impl_unittest.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/audio_ren... media/filters/audio_renderer_impl_unittest.cc:839: TEST_F(AudioRendererImplTest, StopDuringFlush) { nit: PendingFlush_Stop to be consistent with the rest of the file? https://codereview.chromium.org/126793002/diff/740001/media/filters/decryptin... File media/filters/decrypting_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/decryptin... media/filters/decrypting_audio_decoder.cc:123: reset_cb_ = BindToCurrentLoop(closure); Nice catch! https://codereview.chromium.org/126793002/diff/740001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:118: DCHECK(stop_cb_.is_null()); nit: check reset_cb as well? https://codereview.chromium.org/126793002/diff/740001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:163: DoStop(); What if we are calling Stop() during a pending reset (no pending read)? For example: Read() read done Reset() Stop() reset done stop done The order of "reset done" and "stop done" should be determinstic. https://codereview.chromium.org/126793002/diff/740001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:237: check that no read and no reset is pending when we DoStop. https://codereview.chromium.org/126793002/diff/740001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:246: DCHECK(!reset_cb_.is_null()); check that no read is pending when we DoReset. https://codereview.chromium.org/126793002/diff/740001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:283: If Stop() is called during pending reset, we should DoReset() first, then DoStop() so that reset cb is always fired before the stop cb. Background: All these fancy logic used to live in FFmpegVideoDecoder as well :) https://chromiumcodereview.appspot.com/16274005 When you detach the decoder from the DemuxerStream, most of these will go away. https://codereview.chromium.org/126793002/diff/740001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:384: most of the comments in ffad apply to this file :) https://codereview.chromium.org/126793002/diff/740001/media/filters/opus_audi... File media/filters/opus_audio_decoder.h (right): https://codereview.chromium.org/126793002/diff/740001/media/filters/opus_audi... media/filters/opus_audio_decoder.h:80: base::Closure reset_cb_; nit: change the order of these two
Okay, I think I've tweaked the logic to address the comments. Now if we Stop() with a Reset() pending we'll always make sure to do the Reset() first. And as before, pending Read() callbacks will always be satisfied before Reset/Stop occur. https://codereview.chromium.org/126793002/diff/1050001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/1050001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:165: DoReset(); As-is I don't think(?) this will ever happen (Reset is only deferred if there's a pending read), but this ensures we have the proper behavior in case that changes.
The implementation looks good. Thanks! Please add tests for "Stop" for FFAD and OAD. https://codereview.chromium.org/126793002/diff/1050001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/1050001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:119: DCHECK(reset_cb_.is_null()); nit: change the order of these two lines https://codereview.chromium.org/126793002/diff/1050001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:163: if (read_cb_.is_null()) { We like to check abnormal cases and return early. e.g. // A demuxer read is pending, we'll wait until it finishes. if (!read_cb_.is_null()) return; ..... https://codereview.chromium.org/126793002/diff/1050001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:165: DoReset(); On 2014/01/10 21:35:06, rileya wrote: > As-is I don't think(?) this will ever happen (Reset is only deferred if there's > a pending read), but this ensures we have the proper behavior in case that > changes. That's correct. Feel free to DCHECK here :) https://codereview.chromium.org/126793002/diff/1050001/media/filters/opus_aud... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/1050001/media/filters/opus_aud... media/filters/opus_audio_decoder.cc:302: DCHECK(reset_cb_.is_null()); nit: switch the order to these two
I addressed everything I think. I added tests for various pending read/reset/stop cases in FFmpegAudioDecoderTest. I'm working on adding the same sort of tests for the Opus Decoder, but since no unit tests exist for Opus at the moment it's taking a bit longer to get that configured properly.
I added unit tests for the Opus decoder (basically copy-paste of ffmpeg decoder tests with some hacks to initialize opus properly). At the moment it only really tests the logic that was added here, since it seemed outside the scope of this patch to add anything else (a future CL can do something more along the lines of FFmpegAudioDecoderTest.ProduceAudioSamples). It's also worth noting that most of the logic that is tested in these new tests will be moved into AudioBufferStream once it exists, so then we'll only have to test that logic in one place... https://codereview.chromium.org/126793002/diff/1260001/media/filters/opus_aud... File media/filters/opus_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1260001/media/filters/opus_aud... media/filters/opus_audio_decoder_unittest.cc:35: static const unsigned char kOpusExtraData[] = { The decoder refuses to init properly without valid data here, so this is simply lifted from one of the test files. https://codereview.chromium.org/126793002/diff/1260001/media/filters/opus_aud... media/filters/opus_audio_decoder_unittest.cc:65: base::TimeDelta::FromMicroseconds(6500)); This is kinda ugly, but made to match the |kOpusExtraData| above so that various DCHECKs pass.
Since the test for Opus decoder will be kind of big (but similar to the FAD test), I think it's fine to add it in a separate CL. The test looks good in general, just a few more comments. Thanks for the nice work! https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:170: DoReset(); nit: ... DoReset(); return; } DoStop(); https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:102: void PendingRead() { We should be able to merge this function into Read() to simplify things. To do this we also need to merge the callbacks (DecodeFinished and PendingReadAborted) to something like ReadFinished. Then we can check that if reset or stop is pending, the read should be aborted, etc. WDYT? https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:116: void Read() { Set pending_read_ = true here to be consistent with Reset() and Stop(). https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:132: } nit: change the order of Reset() and Stop(). https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:153: run_loop_quit_.Run(); This seems too much hassle to exit the RunLoop. I saw RunLoop::RunUntilIdle() used in a lot of places. We need to find out why RunUntilIdle() isn't working for us. For example: https://code.google.com/p/chromium/codesearch#chromium/src/dbus/bus_unittest.... https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:184: DemuxerStream::ReadCB pending_read_cb_; pending_demuxer_read_cb_ just to be clear? https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:254: .WillOnce(InvokeReadPacketNoCallback(this)); This name is a bit confusing. How about HoldDemuxerRead() or EnterPendingDemuxerReadState()? https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:256: Stop(); After Stop() you can check that read and stop are still pending. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:312: EXPECT_FALSE(pending_reset_); You can probably add these 3 checks in the dtor of FADTest so that we don't need to explicitly check them for every test.
On 2014/01/14 22:25:01, xhwang wrote: > Since the test for Opus decoder will be kind of big (but similar to the FAD > test), I think it's fine to add it in a separate CL. > > The test looks good in general, just a few more comments. Thanks for the nice > work! > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > File media/filters/ffmpeg_audio_decoder.cc (right): > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder.cc:170: DoReset(); > nit: > ... > DoReset(); > return; > } > > DoStop(); > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > File media/filters/ffmpeg_audio_decoder_unittest.cc (right): > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:102: void PendingRead() { > We should be able to merge this function into Read() to simplify things. To do > this we also need to merge the callbacks (DecodeFinished and PendingReadAborted) > to something like ReadFinished. Then we can check that if reset or stop is > pending, the read should be aborted, etc. WDYT? > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:116: void Read() { > Set pending_read_ = true here to be consistent with Reset() and Stop(). > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:132: } > nit: change the order of Reset() and Stop(). > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:153: run_loop_quit_.Run(); > This seems too much hassle to exit the RunLoop. I saw RunLoop::RunUntilIdle() > used in a lot of places. We need to find out why RunUntilIdle() isn't working > for us. > > For example: > https://code.google.com/p/chromium/codesearch#chromium/src/dbus/bus_unittest.... > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:184: DemuxerStream::ReadCB > pending_read_cb_; > pending_demuxer_read_cb_ just to be clear? > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:254: > .WillOnce(InvokeReadPacketNoCallback(this)); > This name is a bit confusing. How about HoldDemuxerRead() or > EnterPendingDemuxerReadState()? > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:256: Stop(); > After Stop() you can check that read and stop are still pending. > > https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... > media/filters/ffmpeg_audio_decoder_unittest.cc:312: > EXPECT_FALSE(pending_reset_); > You can probably add these 3 checks in the dtor of FADTest so that we don't need > to explicitly check them for every test. Just saw you uploaded a new patch with Opus tests. My comments are in PS18. Let's finish the comments there first, then we can either go through the new test, or leave it to a new CL. Thanks.
https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:111: base::RunLoop run_loop; I'm wary of having mixed local global variables here. As well as the mixed usage of RunLoop.RunUntilIdle() and MessageLoop.RunUntilIdle(). It'd be nicer to have only RunLoop usage and have the RunLoop quit closure bound into the respective callbacks and owned by each test. Or if there's only one instance of RunLoop.Run per test it can be owned at a global scope. Otherwise, maybe Reset() and Stop() take the quit closure as a parameter?
https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:170: DoReset(); On 2014/01/14 22:25:02, xhwang wrote: > nit: > ... > DoReset(); > return; > } > > DoStop(); Fixed. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:102: void PendingRead() { On 2014/01/14 22:25:02, xhwang wrote: > We should be able to merge this function into Read() to simplify things. To do > this we also need to merge the callbacks (DecodeFinished and PendingReadAborted) > to something like ReadFinished. Then we can check that if reset or stop is > pending, the read should be aborted, etc. WDYT? Sounds good, I did this. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:111: base::RunLoop run_loop; On 2014/01/14 22:31:45, DaleCurtis wrote: > I'm wary of having mixed local global variables here. As well as the mixed > usage of RunLoop.RunUntilIdle() and MessageLoop.RunUntilIdle(). > > It'd be nicer to have only RunLoop usage and have the RunLoop quit closure bound > into the respective callbacks and owned by each test. Or if there's only one > instance of RunLoop.Run per test it can be owned at a global scope. Otherwise, > maybe Reset() and Stop() take the quit closure as a parameter? RunLoop::RunUntilIdle seems to behave properly (unlike the deprecated MessageLoop version...) in the tests I was having issues with. I removed all instances of MessageLoop::RunUntilIdle and replaced with the RunLoop version and things seem to work fine. So I don't think the QuitClosure stuff is even necessary at all. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:116: void Read() { On 2014/01/14 22:25:02, xhwang wrote: > Set pending_read_ = true here to be consistent with Reset() and Stop(). Done. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:132: } On 2014/01/14 22:25:02, xhwang wrote: > nit: change the order of Reset() and Stop(). Fixed. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:153: run_loop_quit_.Run(); On 2014/01/14 22:25:02, xhwang wrote: > This seems too much hassle to exit the RunLoop. I saw RunLoop::RunUntilIdle() > used in a lot of places. We need to find out why RunUntilIdle() isn't working > for us. > > For example: > https://code.google.com/p/chromium/codesearch#chromium/src/dbus/bus_unittest.... Ah, didn't know RunLoop had a RunUntilIdle, MessageLoop::RunUntilIdle seemed not to work but RunLoop's version does, fixed. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:184: DemuxerStream::ReadCB pending_read_cb_; On 2014/01/14 22:25:02, xhwang wrote: > pending_demuxer_read_cb_ just to be clear? Sounds good. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:254: .WillOnce(InvokeReadPacketNoCallback(this)); On 2014/01/14 22:25:02, xhwang wrote: > This name is a bit confusing. How about HoldDemuxerRead() or > EnterPendingDemuxerReadState()? I like EnterPendingDemuxerReadState, going with that. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:256: Stop(); On 2014/01/14 22:25:02, xhwang wrote: > After Stop() you can check that read and stop are still pending. Good catch, added checks like that to each of these tests. https://codereview.chromium.org/126793002/diff/910002/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder_unittest.cc:312: EXPECT_FALSE(pending_reset_); On 2014/01/14 22:25:02, xhwang wrote: > You can probably add these 3 checks in the dtor of FADTest so that we don't need > to explicitly check them for every test. Makes sense, done.
I also removed the opus tests (they can be added in another CL). It also looks like an email didn't get sent for the last round of comment responses, so see above for those...
Looks much cleaner! Just a few more comments I believe. https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:87: run_loop.RunUntilIdle(); You can declare run_loop_ as a member variable of the test class so that you don't need to create a RunLoop multiple times in a test. (Similar to the original message_loop_) https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:103: pending_read_ = true; You don't need this anymore. If you like, you can DCHECK it. https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:108: EXPECT_TRUE(pending_read_ && !pending_demuxer_read_cb_.is_null()); s/DCHECK/EXPECT_TRUE EXPECT_TRUE expects the implementation being tested (e.g. FAD) is correct. DCHECK asserts that this test is correctly written. In this case, this is basically saying we can only call EnterPendingDemuxerRead() when we are in pending demuxer read state, which is an assert for this test. Hence DCHECK. https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:125: &FFmpegAudioDecoderTest::ResetFinished, base::Unretained(this))); run_loop.RunUntilIdle(); https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:131: &FFmpegAudioDecoderTest::StopFinished, base::Unretained(this))); run_loop.RunUntilIdle(); https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:307: run_loop.RunUntilIdle(); You don't need this when you call RunUntilIdel() in Stop() :)
https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:87: run_loop.RunUntilIdle(); On 2014/01/14 23:23:12, xhwang wrote: > You can declare run_loop_ as a member variable of the test class so that you > don't need to create a RunLoop multiple times in a test. (Similar to the > original message_loop_) Looks like a RunLoop isn't allowed to be run multiple times (see run_loop.cc line 67, |run_called_| is set to true after a run starts and is never set to false afterwards, so initiating another run asserts). I did compact the various instances into one line by calling RunUntilIdle on a temporary RunLoop though: "base::RunLoop().RunUntilIdle()" https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:103: pending_read_ = true; On 2014/01/14 23:23:12, xhwang wrote: > You don't need this anymore. If you like, you can DCHECK it. Sounds good. https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:108: EXPECT_TRUE(pending_read_ && !pending_demuxer_read_cb_.is_null()); On 2014/01/14 23:23:12, xhwang wrote: > s/DCHECK/EXPECT_TRUE > > EXPECT_TRUE expects the implementation being tested (e.g. FAD) is correct. > DCHECK asserts that this test is correctly written. > > In this case, this is basically saying we can only call > EnterPendingDemuxerRead() when we are in pending demuxer read state, which is an > assert for this test. Hence DCHECK. Done. https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:125: &FFmpegAudioDecoderTest::ResetFinished, base::Unretained(this))); On 2014/01/14 23:23:12, xhwang wrote: > run_loop.RunUntilIdle(); Added to both Reset() and Stop(). https://codereview.chromium.org/126793002/diff/1420001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:307: run_loop.RunUntilIdle(); On 2014/01/14 23:23:12, xhwang wrote: > You don't need this when you call RunUntilIdel() in Stop() :) I actually just killed this test case off entirely, I had intended to test the case of a pending Reset() followed by a Stop(), but a pending Reset is impossible without a pending Read (and that case is covered in another test above), so it was really just a Reset followed by a Stop.
LGTM! with one last comment. Thanks! https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:157: EXPECT_TRUE(pending_stop_); Expect that no read or reset cb is pending. https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:166: EXPECT_TRUE(pending_reset_); Expect that no read cb is pending.
lgtm % cleanup nit. https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:238: Initialize(); Every test calls initialize, just fold it into the constructor? Move lines 240-244 into an SetupReadTest() method since they're reused a few times.
https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:157: EXPECT_TRUE(pending_stop_); On 2014/01/15 00:15:14, xhwang wrote: > Expect that no read or reset cb is pending. I think the checks on 161 and 162 expect this already? Short of making this a friend of FFmpegAudioDecoder and checking the callbacks directly I'm not sure what else to do here (and DoStop and DoReset in FAD already have DCHECKS that those cb's aren't pending). https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:166: EXPECT_TRUE(pending_reset_); On 2014/01/15 00:15:14, xhwang wrote: > Expect that no read cb is pending. See above.
https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder_unittest.cc (right): https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder_unittest.cc:157: EXPECT_TRUE(pending_stop_); On 2014/01/15 00:31:50, rileya wrote: > On 2014/01/15 00:15:14, xhwang wrote: > > Expect that no read or reset cb is pending. > > I think the checks on 161 and 162 expect this already? Short of making this a > friend of FFmpegAudioDecoder and checking the callbacks directly I'm not sure > what else to do here (and DoStop and DoReset in FAD already have DCHECKS that > those cb's aren't pending). Right. I didn't pay attention to those. Can you move 161-162 up to here instead?
On 2014/01/15 00:19:34, DaleCurtis wrote: > lgtm % cleanup nit. > > https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... > File media/filters/ffmpeg_audio_decoder_unittest.cc (right): > > https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... > media/filters/ffmpeg_audio_decoder_unittest.cc:238: Initialize(); > Every test calls initialize, just fold it into the constructor? Move lines > 240-244 into an SetupReadTest() method since they're reused a few times. Sounds good, applied these suggestions in the latest patchset.
On 2014/01/15 00:41:56, xhwang wrote: > https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... > File media/filters/ffmpeg_audio_decoder_unittest.cc (right): > > https://codereview.chromium.org/126793002/diff/1470001/media/filters/ffmpeg_a... > media/filters/ffmpeg_audio_decoder_unittest.cc:157: EXPECT_TRUE(pending_stop_); > On 2014/01/15 00:31:50, rileya wrote: > > On 2014/01/15 00:15:14, xhwang wrote: > > > Expect that no read or reset cb is pending. > > > > I think the checks on 161 and 162 expect this already? Short of making this a > > friend of FFmpegAudioDecoder and checking the callbacks directly I'm not sure > > what else to do here (and DoStop and DoReset in FAD already have DCHECKS that > > those cb's aren't pending). > > Right. I didn't pay attention to those. Can you move 161-162 up to here instead? Alright, moved 'em.
Unless anyone has any final thoughts, I'm gonna check 'commit' in the next hour or so.
On 2014/01/15 00:53:15, rileya wrote: > Unless anyone has any final thoughts, I'm gonna check 'commit' in the next hour > or so. lgtm!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/126793002/1560001
Retried try job too often on mac_rel for step(s) app_list_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chromedriver_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, message_center_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/126793002/1390003
Message was sent while issue was closed.
Change committed as 244994
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/139513007/ by dstockwell@chromium.org. The reason for reverting is: Causes blink layout test (media/encrypted-media/encrypted-media-needkey.html) to crash: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40.... |
