Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2016213003:
Separate MediaCodecLoop from MediaCodecAudioDecoder (Closed)
|
Created:
4 years, 6 months ago by liberato (no reviews please) Modified:
4 years, 5 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate MediaCodecLoop from MediaCodecAudioDecoder
MCAD and AndroidVideoDecodeAccelerator share lots of code
to interact with MediaCodec. This CL splits out much of
that into a new MediaCodecLoop class.
MediaCodecLoop manages driving the I/O loop for MediaCodec
and interacting with a client (AVDA or MCAD).
This CL only affects MCAD. AVDA's event loop is more
complicated, and will be merged into MediaCodecLoop as
part of a separate CL.
BUG=583082
Committed: https://crrev.com/8634a951e0946d4bf63889d888cc369dd4c2c665
Cr-Commit-Position: refs/heads/master@{#403350}
Patch Set 1 #Patch Set 2 : cleanup. #Patch Set 3 : removed changes from manifest and renderer factory #Patch Set 4 : removed some MediaCodec* args, updated docs. #Patch Set 5 : added gyp. #
Total comments: 31
Patch Set 6 : cl feedback. #
Total comments: 8
Patch Set 7 : cl feedback. #
Total comments: 14
Patch Set 8 : cl feedback. #
Total comments: 60
Patch Set 9 : cl feedback. #
Total comments: 28
Patch Set 10 : cl feedback, added unit test. #Patch Set 11 : rebased, removed include. #
Total comments: 70
Patch Set 12 : cl feedback #
Total comments: 12
Patch Set 13 : cl feedback #
Total comments: 5
Patch Set 14 : cl feedback. #Patch Set 15 : fixed |media_crypto_| #Patch Set 16 : don't allow STATE_DRAINED when flushing #
Messages
Total messages: 54 (13 generated)
Description was changed from ========== compiles, removed cdm and flush and init doesn't compile BUG= ========== to ========== compiles, removed cdm and flush and init doesn't compile BUG=583082 ==========
Description was changed from ========== compiles, removed cdm and flush and init doesn't compile BUG=583082 ========== to ========== Separate MediaCodecLoop from MediaCodecAudioDecoder MCAD and AndroidVideoDecodeAccelerator share lots of code to interact with MediaCodec. This CL splits out much of that into a new MediaCodecLoop class. MediaCodecLoop manages driving the I/O loop for MediaCodec and interacting with a client (AVDA or MCAD). This CL only affects MCAD. AVDA's event loop is more complicated, and will be merged into MediaCodecLoop as part of a separate CL. BUG=583082 ==========
liberato@chromium.org changed reviewers: + timav@chromium.org
i'll open up for others to review once you're satisfied that it's approximately sane. i've tried to use 'meld' to look for accidental changes. one thing that i know is different is when we call the decode CB on error. see the comment @254 in media_codec_loop.cc . i don't see a problem with the new behavior, but you're much more familiar with MCAD than i am. thanks! -fl
https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.cc:135: if (!client_->IsAnyInputPending()) To be equivalent to the old code this method should return true after we got NO_KEY so that we continue with this pending buffer even if there is no more input. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.cc:256: // this before? MCAD is the first decoder that uses MediaCodec and implements AudioDecoder interface, in that sense we did not run completion cb before. Completion cb is required by the protocol, the caller waits for it for the next move. In other audio decoders it is more straightforward, there is no queue there. If you look for the way to get rid of ClearInputQueue() here, I think we can call something like client_->OnError() and let the client decide. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.cc:272: // These aren't needed, and aren't guaranted to be valid after we return. I do not quite understand, do you plan to use these fields on l.210 ? Just in case: in MCAD I set |memory = nullptr| to avoid a call to FillInputBuffer() here: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... |length| is used. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:126: struct InputBufferData { Would it be possible to have a queue of same elements for both audio and video? Or we already agreed that it's more hassle than good? https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:179: I think we want another method that controls the queue advancement (pop_front), something like AdvanceInput(). At least right now the data retrieval for EnqueueInput() is separated from queue_.pop_front() and pending buffer remains in the queue. I'll try to comment in the cc. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:204: virtual void OnStateChanged(State new_state) = 0; Why would the client want to know about MediaCodecLoop states? If it is just to pass an error, maybe have OnError() instead? Then the client itself can clear the input queue, and hopefully ClearInputQueue() can be removed from the Client interface. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:212: explicit MediaCodecLoop(Client* client, nit: no "explicit". As far as I understand an "explicit" would only makes sense if constructor has exactly one parameter (and thus defines an implicit conversion) and here we have two parameters. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:237: MediaCodecBridge* GetCodec() const; I would try to avoid exposing MediaCodecBridge to the outside world. I think it is only used to get an access to the buffer's data, see my comment there. https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:142: const bool do_play = false; // Do not create AudioTrack object. nit: Min in some other review suggested to get rid of this var and just pass false. https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:372: MediaCodecBridge* media_codec = codec_loop_->GetCodec(); It seems we use MediaCodecBridge to only get access to the data. In that case I would add a method in MediaCodecLoop that does just that, e.g. GetOutputBufferAddress(const OutputBufferInfo&). media_codec->CopyFromOutputBuffer() is just a combination of getting this address and memcpy(). https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:410: // TODO(timav): Handle wrong status properly, http://crbug.com/585978. nit: Now we have a chance to implement it, maybe return bool instead of void ? https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.h:128: void CreateCodecLoop(); nit: s/CreateCodecLoop/CreateMediaCodecLoop/ ? https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.h:128: void CreateCodecLoop(); nit: I think it would be more natural to return bool for success, and even better to return unique_ptr<MediaCodecLoop> (zero pointer in case of a failure).
https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.cc:135: if (!client_->IsAnyInputPending()) On 2016/06/01 00:52:40, Tima Vaisburd wrote: > To be equivalent to the old code this method should return true after we got > NO_KEY so that we continue with this pending buffer even if there is no more > input. great catch, thanks! https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.cc:256: // this before? On 2016/06/01 00:52:40, Tima Vaisburd wrote: > MCAD is the first decoder that uses MediaCodec and implements AudioDecoder > interface, in that sense we did not run completion cb before. Completion cb is > required by the protocol, the caller waits for it for the next move. In other > audio decoders it is more straightforward, there is no queue there. > > If you look for the way to get rid of ClearInputQueue() here, I think we can > call something like client_->OnError() and let the client decide. done. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.cc:272: // These aren't needed, and aren't guaranted to be valid after we return. On 2016/06/01 00:52:40, Tima Vaisburd wrote: > I do not quite understand, do you plan to use these fields on l.210 ? > > Just in case: in MCAD I set |memory = nullptr| to avoid a call to > FillInputBuffer() here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > > |length| is used. i just meant that the ptr provided by the client isn't guaranteed to remain valid after we return. the nullptr will be re-used @210. i'll update the comment. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:126: struct InputBufferData { On 2016/06/01 00:52:40, Tima Vaisburd wrote: > Would it be possible to have a queue of same elements for both audio and video? > Or we already agreed that it's more hassle than good? now that MediaCodecLoop handles only a single MediaCodecBridge instance, it's not clear that we want this in MCL. if the client creates a new media codec bridge / MCL, then the unused part of the queue would have to be transferred to a new instance. better, i think, if we want to consolidate this, is to use a helper class that's separate from MCL. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:179: On 2016/06/01 00:52:40, Tima Vaisburd wrote: > I think we want another method that controls the queue advancement (pop_front), > something like AdvanceInput(). At least right now the data retrieval for > EnqueueInput() is separated from queue_.pop_front() and pending buffer remains > in the queue. I'll try to comment in the cc. i don't think that i understand what you mean. when would MCL call AdvanceInput? right now, the completion callback goes with the input buffer. MCAD does remove it from the queue when it hands the buffer off to MCL in ProvideInputData. MCL owns the buffer, and MCAD should never return that buffer in response to ProvideInputData again. the only exception is EOS, where it neither pops the buffer nor sends the callback. it depends that the EOS transitions to DRAINING, so MCL won't ask for input again. that seems fragile to me, but i couldn't figure out how to remove it without making it worse. :) https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:204: virtual void OnStateChanged(State new_state) = 0; On 2016/06/01 00:52:40, Tima Vaisburd wrote: > Why would the client want to know about MediaCodecLoop states? If it is just to > pass an error, maybe have OnError() instead? > > Then the client itself can clear the input queue, and hopefully > ClearInputQueue() can be removed from the Client interface. done. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:212: explicit MediaCodecLoop(Client* client, On 2016/06/01 00:52:40, Tima Vaisburd wrote: > nit: no "explicit". As far as I understand an "explicit" would only makes sense > if constructor has exactly one parameter (and thus defines an implicit > conversion) and here we have two parameters. Done. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:237: MediaCodecBridge* GetCodec() const; On 2016/06/01 00:52:40, Tima Vaisburd wrote: > I would try to avoid exposing MediaCodecBridge to the outside world. I think it > is only used to get an access to the buffer's data, see my comment there. it's also used for OnFormatChanged to query the new format. there are other cases where we might want it in avda (setOutputSurface, for example). i'm not sure that wrapping the calls in MCL really adds much abstraction -- if you're using MCL, then you already have to understand MC pretty deeply. plus. since each MCL instance is 1:1 with a MC instance, it's well defined which codec is intended. https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:142: const bool do_play = false; // Do not create AudioTrack object. On 2016/06/01 00:52:40, Tima Vaisburd wrote: > nit: Min in some other review suggested to get rid of this var and just pass > false. Done. https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:372: MediaCodecBridge* media_codec = codec_loop_->GetCodec(); On 2016/06/01 00:52:40, Tima Vaisburd wrote: > It seems we use MediaCodecBridge to only get access to the data. > In that case I would add a method in MediaCodecLoop that does just that, > e.g. GetOutputBufferAddress(const OutputBufferInfo&). > > media_codec->CopyFromOutputBuffer() is just a combination of getting this > address and memcpy(). OnFormatChanged uses it as well. please see my reply in the .h https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:410: // TODO(timav): Handle wrong status properly, http://crbug.com/585978. On 2016/06/01 00:52:40, Tima Vaisburd wrote: > nit: Now we have a chance to implement it, maybe return bool instead of void ? done. MCL transitions to STATE_ERROR if OnDecodedFrame fails. https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.h:128: void CreateCodecLoop(); On 2016/06/01 00:52:40, Tima Vaisburd wrote: > nit: s/CreateCodecLoop/CreateMediaCodecLoop/ ? Done. https://codereview.chromium.org/2016213003/diff/80001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.h:128: void CreateCodecLoop(); On 2016/06/01 00:52:40, Tima Vaisburd wrote: > nit: I think it would be more natural to return bool for success, and even > better to return unique_ptr<MediaCodecLoop> (zero pointer in case of a failure). it now returns bool. didn't want to return the ptr, since it's goal is specifically to fill in |codec_loop_|.
https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:126: struct InputBufferData { On 2016/06/01 21:47:54, liberato wrote: > On 2016/06/01 00:52:40, Tima Vaisburd wrote: > > Would it be possible to have a queue of same elements for both audio and > video? > > Or we already agreed that it's more hassle than good? > > now that MediaCodecLoop handles only a single MediaCodecBridge instance, it's > not clear that we want this in MCL. if the client creates a new media codec > bridge / MCL, then the unused part of the queue would have to be transferred to > a new instance. > > better, i think, if we want to consolidate this, is to use a helper class that's > separate from MCL. Sorry, I should have been clearer. I hinted at avoiding the InputBufferData altogether, but instead of subtle hints should have talked straight. If I get it correctly, right now you copy the DecoderBuffer from the |input_queue_| into InputBufferData variable and from this variable to QueueInputBuffer() field by field. I was wondering maybe ProvideInputData() can return a pointer into the input_queue_ directly. I'm not sure though it would be easy to provide a common format for MCAD and AVDA (or common class for DecoderBuffer and BitstreamBuffer?), so please consider these remarks as my thinking out loud. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:179: On 2016/06/01 21:47:54, liberato wrote: > On 2016/06/01 00:52:40, Tima Vaisburd wrote: > > I think we want another method that controls the queue advancement > (pop_front), > > something like AdvanceInput(). At least right now the data retrieval for > > EnqueueInput() is separated from queue_.pop_front() and pending buffer remains > > in the queue. I'll try to comment in the cc. > > i don't think that i understand what you mean. when would MCL call > AdvanceInput? > > right now, the completion callback goes with the input buffer. MCAD does remove > it from the queue when it hands the buffer off to MCL in ProvideInputData. MCL > owns the buffer, and MCAD should never return that buffer in response to > ProvideInputData again. > > the only exception is EOS, where it neither pops the buffer nor sends the > callback. it depends that the EOS transitions to DRAINING, so MCL won't ask for > input again. that seems fragile to me, but i couldn't figure out how to remove > it without making it worse. :) The idea was to decouple MCAD::input_queue_.pop_front() from ProvideInputData(). ProvideInputData() would return the same front buffer over and over until somebody calls pop_front(). In a sense, MCL does not own the buffer, the |input_queue_| does. MCL could call this pop_front() e.g. by client_->AdvanceQueue(). It is a different approach, not necessary better than what you have right now. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:237: MediaCodecBridge* GetCodec() const; On 2016/06/01 21:47:54, liberato wrote: > it's also used for OnFormatChanged to query the new format. Yes, I missed that, you are absolutely right. > there are other cases where we might want it in avda (setOutputSurface, for > example). i'm not sure that wrapping the calls in MCL really adds much > abstraction -- if you're using MCL, then you already have to understand MC > pretty deeply. Still this GetCodec() breaks encapsulation the way I understand it. I hoped MCL would conceal MediaCodecBridge. The methods like OnFormatChanged() and OnFrameDecoded() are actually good candidates for inheritance. The old pipeline used a combination of inheritance and delegation to solve this kind of problem, i.e. MCL ^ | AudioDecoder ----> AudioMCL At this point I agree to leave it as is and would like to hear what other reviewers think. https://codereview.chromium.org/2016213003/diff/100001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/100001/media/base/android/med... media/base/android/media_codec_loop.cc:265: // The client doesn't guarantee that these will remain valid after we I still do not understand this comment. Sorry me being slow :-) I think later you'd assign input_data = pending_input_buf_data_ back and pass it to QueueSecureInputBuffer() to try again. MediaCodec buffer has a copy of the data, indeed, but the API requires all the fields again, including length, key_id, iv, subsamples, pts. Why do you say we do not need them? And why do you assign |length| to 0? https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:181: DCHECK_EQ(codec_loop_->GetState(), MediaCodecLoop::STATE_READY) I think ideally we would like to keep the state of this MCAD and the state of MCL decoupled. Can we remove this DCHECK without sacrifices? If yes, MediaCodecLoop::GetState() could be private. https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:397: CHECK_EQ(status, MEDIA_CODEC_OK); Fix this CHECK_EQ too, please, now we can return false. https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:407: success &= status == MEDIA_CODEC_OK; Bitwise AND? Anyway, if status is not OK chances are good that the method did not change |interleaved_data| and |interleaved_capacity|, i.e. they are 0. l.409 will fail and l.412 might crash. I think we have to release output buffer and |return false| early.
liberato@chromium.org changed reviewers: + dalecurtis@chromium.org, watk@chromium.org
timav: thanks for all the comments! dalecurtis, watk: added for wider review. thanks -fl https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:179: On 2016/06/02 00:35:21, Tima Vaisburd wrote: > On 2016/06/01 21:47:54, liberato wrote: > > On 2016/06/01 00:52:40, Tima Vaisburd wrote: > > > I think we want another method that controls the queue advancement > > (pop_front), > > > something like AdvanceInput(). At least right now the data retrieval for > > > EnqueueInput() is separated from queue_.pop_front() and pending buffer > remains > > > in the queue. I'll try to comment in the cc. > > > > i don't think that i understand what you mean. when would MCL call > > AdvanceInput? > > > > right now, the completion callback goes with the input buffer. MCAD does > remove > > it from the queue when it hands the buffer off to MCL in ProvideInputData. > MCL > > owns the buffer, and MCAD should never return that buffer in response to > > ProvideInputData again. > > > > the only exception is EOS, where it neither pops the buffer nor sends the > > callback. it depends that the EOS transitions to DRAINING, so MCL won't ask > for > > input again. that seems fragile to me, but i couldn't figure out how to > remove > > it without making it worse. :) > > The idea was to decouple MCAD::input_queue_.pop_front() from ProvideInputData(). > ProvideInputData() would return the same front buffer over and over until > somebody calls pop_front(). In a sense, MCL does not own the buffer, the > |input_queue_| does. MCL could call this pop_front() e.g. by > client_->AdvanceQueue(). > > It is a different approach, not necessary better than what you have right now. i've given some thought to this. based on our discussion, i'm going to try to avoid the whole issue :) the MCL already knows to switch to DRAINING when an EOS arrives at the input. it might as well know to defer the EOS completion callback until the EOS is dequeued. this avoids making MCAD handle the input queue differently for data vs EOS buffers. MCAD should always advance the input queue as part of ProvideInputData(). i'm going to leave OnDecodedEos() for now, though in MCAD it does nothing. If this new approach ends up being the one we go with, then i'll probably remove it. https://codereview.chromium.org/2016213003/diff/80001/media/base/android/medi... media/base/android/media_codec_loop.h:237: MediaCodecBridge* GetCodec() const; On 2016/06/02 00:35:21, Tima Vaisburd wrote: > On 2016/06/01 21:47:54, liberato wrote: > > > it's also used for OnFormatChanged to query the new format. > > Yes, I missed that, you are absolutely right. > > > there are other cases where we might want it in avda (setOutputSurface, for > > example). i'm not sure that wrapping the calls in MCL really adds much > > abstraction -- if you're using MCL, then you already have to understand MC > > pretty deeply. > > Still this GetCodec() breaks encapsulation the way I understand it. I hoped MCL > would conceal MediaCodecBridge. The methods like OnFormatChanged() and > OnFrameDecoded() are actually good candidates for inheritance. The old pipeline > used a combination of inheritance and delegation to solve this kind of problem, > i.e. > > MCL > ^ > | > > AudioDecoder ----> AudioMCL > > At this point I agree to leave it as is and would like to hear what other > reviewers think. Acknowledged. https://codereview.chromium.org/2016213003/diff/100001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/100001/media/base/android/med... media/base/android/media_codec_loop.cc:265: // The client doesn't guarantee that these will remain valid after we On 2016/06/02 00:35:21, Tima Vaisburd wrote: > I still do not understand this comment. Sorry me being slow :-) > I think later you'd assign input_data = pending_input_buf_data_ back and pass it > to QueueSecureInputBuffer() to try again. MediaCodec buffer has a copy of the > data, indeed, but the API requires all the fields again, including length, > key_id, iv, subsamples, pts. Why do you say we do not need them? And why do you > assign |length| to 0? comment: i'll update it. key_id, iv, ...: these are preserved in |pending_input_buf_data_| memory: this is cleared, just like https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/andr... length: good catch, fixed. https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:181: DCHECK_EQ(codec_loop_->GetState(), MediaCodecLoop::STATE_READY) On 2016/06/02 00:35:21, Tima Vaisburd wrote: > I think ideally we would like to keep the state of this MCAD and the state of > MCL decoupled. Can we remove this DCHECK without sacrifices? If yes, > MediaCodecLoop::GetState() could be private. good point. i don't think that the DCHECK helps much anyway, since we're still checking MCAD state. I suppose a decode after EOS would trigger it, since we wouldn't notice the transition to draining. turns out that GetState() isn't used at all after this change. https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:397: CHECK_EQ(status, MEDIA_CODEC_OK); On 2016/06/02 00:35:21, Tima Vaisburd wrote: > Fix this CHECK_EQ too, please, now we can return false. Done. https://codereview.chromium.org/2016213003/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:407: success &= status == MEDIA_CODEC_OK; On 2016/06/02 00:35:21, Tima Vaisburd wrote: > Bitwise AND? Anyway, if status is not OK chances are good that the method did > not change |interleaved_data| and |interleaved_capacity|, i.e. they are 0. l.409 > will fail and l.412 might crash. I think we have to release output buffer and > |return false| early. done, removed |success|, replaced both branches with a call to ReleaseCodecBuffer and early-out.
Didn't finish review before needing to head out; here are partial comments. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:5: #ifndef MEDIA_FILTERS_ANDROID_MEDIA_CODEC_LOOP_H_ Fix headers or watk@ is going to yell at you :) https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:120: int buf_index; // The codec input buffers are referred to by this index. const both of these? or are they copyable? https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:126: struct InputBufferData { Again, const what can be here. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:144: bool is_eos : 1; We don't use : syntax. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:148: // Information about the MediaCodec's output buffer. Has no constructor, so probably can't const stuff, but up to you. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:204: std::unique_ptr<MediaCodecBridge>&& media_codec); No need for && here I think https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:259: // A helper function for logging. Necessary to expose here?
https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:5: #ifndef MEDIA_FILTERS_ANDROID_MEDIA_CODEC_LOOP_H_ On 2016/06/03 23:10:24, DaleCurtis wrote: > Fix headers or watk@ is going to yell at you :) done. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:120: int buf_index; // The codec input buffers are referred to by this index. On 2016/06/03 23:10:24, DaleCurtis wrote: > const both of these? or are they copyable? Done. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:126: struct InputBufferData { On 2016/06/03 23:10:24, DaleCurtis wrote: > Again, const what can be here. it's copyable, unfortunately. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:144: bool is_eos : 1; On 2016/06/03 23:10:24, DaleCurtis wrote: > We don't use : syntax. Done. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:148: // Information about the MediaCodec's output buffer. On 2016/06/03 23:10:24, DaleCurtis wrote: > Has no constructor, so probably can't const stuff, but up to you. yeah, and these fields are filled in after construction by MediaCodecBridge. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:204: std::unique_ptr<MediaCodecBridge>&& media_codec); On 2016/06/03 23:10:24, DaleCurtis wrote: > No need for && here I think turns out that we can't -- we create a std::unique_ptr<AudioCodecBridge>, and lvalue ref to MediaCodecBridge ptr doesn't work. not sure how it does the type inference, but it does. https://codereview.chromium.org/2016213003/diff/120001/media/base/android/med... media/base/android/media_codec_loop.h:259: // A helper function for logging. On 2016/06/03 23:10:24, DaleCurtis wrote: > Necessary to expose here? do you mean make it a non-member function? why? i did notice that the State enum can be made protected, which i did.
Overall looking good; though I'm worried there's still more lines in the MediaCodecAudioDecoder than I'd like to have. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:22: // TODO(liberato): the client should choose these. You should be able to replace these now with constexpr base::TimeDelta::FromXXX() kConstant; https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:39: // TODO(liberato): does anybody call this? if not, maybe they should. Delete until then? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:100: io_timer_.Stop(); Step N+1 is to switch to a common timer like AVDA? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:103: success = (media_codec_->Reset() == MEDIA_CODEC_OK); Avoid unnecessary parens. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); Should these be done in a while loop like AVDA? Note, the functions below then need to pick up watk@'s fixes for when the MediaCodec is dropped mid-loop. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:122: bool MediaCodecLoop::QueueInput() { I think the naming for these methods should be reconsidered. We have Queue, QueueOne, Enqueue, and Deque all referring to input buffers -- none of which has a clear label. How about merging QueueInput into DoIOTask() like we do for AVDA? Then rename the QueueOneInputBuffer to ProcessOneInputBuffer. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:138: if (pending_input_buf_index_ == kInvalidBufferIndex && Needs {} https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:142: if (state_ == STATE_WAITING_FOR_KEY || state_ == STATE_DRAINING || Ditto. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:293: bool did_work = false; I'd move this out of here and into DoIOTask then follow similar naming conventions to the input functions. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:366: void MediaCodecLoop::ManageTimer(bool did_work) { I think this should actually be refactored out further such that we have a MediaCodecLoopTimer singleton which manages this for all instances. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:389: if (state_ != new_state && state_ == STATE_ERROR) This offers no room for deferring errors since it is unnecessary for audio, just fyi. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:398: #undef RETURN_STRING No macros, lets avoid this from the beginning :) https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:120: const int Odd formatting. Did clang-format do this? https://codereview.chromium.org/2016213003/diff/140001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:137: DVLOG(0) << __FUNCTION__ << " failed: cannot create AudioCodecBridge"; DLOG(ERROR) https://codereview.chromium.org/2016213003/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:145: DVLOG(0) << __FUNCTION__ << " failed: cannot configure audio codec for " Ditto.
https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:398: #undef RETURN_STRING On 2016/06/08 22:55:01, DaleCurtis wrote: > No macros, lets avoid this from the beginning :) Dale, what would be your suggestion to implement AsString() then?
https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); On 2016/06/08 22:55:01, DaleCurtis wrote: > Should these be done in a while loop like AVDA? Note, the functions below then > need to pick up watk@'s fixes for when the MediaCodec is dropped mid-loop. Each one is a while loop.
https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); On 2016/06/08 at 23:24:13, Tima Vaisburd wrote: > On 2016/06/08 22:55:01, DaleCurtis wrote: > > Should these be done in a while loop like AVDA? Note, the functions below then > > need to pick up watk@'s fixes for when the MediaCodec is dropped mid-loop. > > Each one is a while loop. Yes, but I don't think each one should be. We know there are cases where inputs may be queued after an output is dequeued, so the current logic is insufficient for the most optimal pipeling. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:398: #undef RETURN_STRING On 2016/06/08 at 23:14:20, Tima Vaisburd wrote: > On 2016/06/08 22:55:01, DaleCurtis wrote: > > No macros, lets avoid this from the beginning :) > > Dale, what would be your suggestion to implement AsString() then? Hmm, I thought there'd be a nice C++ solution to this these days, but there's only duplicate your strings as far as I can tell. So how about making this a little safer: #if defined(RETURN_STRING) #error Unexpected macro collision. #else AsString() { #define RETURN_STRING(x) case x: return #x; #undef RETURN_STRING } #endif
https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); On 2016/06/09 00:30:49, DaleCurtis wrote: > On 2016/06/08 at 23:24:13, Tima Vaisburd wrote: > > Each one is a while loop. > > Yes, but I don't think each one should be. We know there are cases where inputs > may be queued after an output is dequeued I am not aware of these cases (or forgot), could you, please, remind me? Currently we repeat the round on next timer (i.e. in 10 ms), what are the cases where it would be better to do differently? My logic has been as follows. On input, enqueue all available buffers from the queue if the codec accepts them, there is no point to postpone. Independently of success of failure in the input proceed to output (that's important, otherwise decoder might block). On output, pull everything that's available at the current time (timeout == 0).
https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); On 2016/06/09 at 00:54:45, Tima Vaisburd wrote: > On 2016/06/09 00:30:49, DaleCurtis wrote: > > On 2016/06/08 at 23:24:13, Tima Vaisburd wrote: > > > Each one is a while loop. > > > > Yes, but I don't think each one should be. We know there are cases where inputs > > may be queued after an output is dequeued > > I am not aware of these cases (or forgot), could you, please, remind me? Currently we repeat the round on next timer (i.e. in 10 ms), what are the cases where it would be better to do differently? > > My logic has been as follows. On input, enqueue all available buffers from the queue if the codec accepts them, there is no point to postpone. Independently of success of failure in the input proceed to output (that's important, otherwise decoder might block). On output, pull everything that's available at the current time (timeout == 0). Pulling output may allow new input to be queued; as seen on the MotoX.
On 2016/06/09 00:56:48, DaleCurtis wrote: > https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... > File media/base/android/media_codec_loop.cc (right): > > https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... > media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); > On 2016/06/09 at 00:54:45, Tima Vaisburd wrote: > > On 2016/06/09 00:30:49, DaleCurtis wrote: > > > On 2016/06/08 at 23:24:13, Tima Vaisburd wrote: > > > > Each one is a while loop. > > > > > > Yes, but I don't think each one should be. We know there are cases where > inputs > > > may be queued after an output is dequeued > > > > I am not aware of these cases (or forgot), could you, please, remind me? > Currently we repeat the round on next timer (i.e. in 10 ms), what are the cases > where it would be better to do differently? > > > > My logic has been as follows. On input, enqueue all available buffers from the > queue if the codec accepts them, there is no point to postpone. Independently of > success of failure in the input proceed to output (that's important, otherwise > decoder might block). On output, pull everything that's available at the current > time (timeout == 0). > > Pulling output may allow new input to be queued; as seen on the MotoX. It is expected: pulling the output should release a room for more input. The current logic postpones the input to be queued till the next cycle.
On 2016/06/09 01:16:20, Tima Vaisburd wrote: > On 2016/06/09 00:56:48, DaleCurtis wrote: > > Pulling output may allow new input to be queued; as seen on the MotoX. > > It is expected: pulling the output should release a room for more input. > The current logic postpones the input to be queued till the next cycle. Do you say it would be better to change the order: process output first, and then the input?
On 2016/06/09 at 01:23:31, timav wrote: > On 2016/06/09 01:16:20, Tima Vaisburd wrote: > > On 2016/06/09 00:56:48, DaleCurtis wrote: > > > Pulling output may allow new input to be queued; as seen on the MotoX. > > > > It is expected: pulling the output should release a room for more input. > > The current logic postpones the input to be queued till the next cycle. > > Do you say it would be better to change the order: process output first, > and then the input? I'm saying it should be done in a loop to process as much input and output within a single timer wakeup; this is what we did with AVDA.
On 2016/06/09 01:24:50, DaleCurtis wrote: > On 2016/06/09 at 01:23:31, timav wrote: > > On 2016/06/09 01:16:20, Tima Vaisburd wrote: > > > On 2016/06/09 00:56:48, DaleCurtis wrote: > > > > Pulling output may allow new input to be queued; as seen on the MotoX. > > > > > > It is expected: pulling the output should release a room for more input. > > > The current logic postpones the input to be queued till the next cycle. > > > > Do you say it would be better to change the order: process output first, > > and then the input? > > I'm saying it should be done in a loop to process as much input and output > within a single timer wakeup; this is what we did with AVDA. I see. I missed that change.
On 2016/06/09 01:37:48, Tima Vaisburd wrote: > On 2016/06/09 01:24:50, DaleCurtis wrote: > > On 2016/06/09 at 01:23:31, timav wrote: > > > On 2016/06/09 01:16:20, Tima Vaisburd wrote: > > > > On 2016/06/09 00:56:48, DaleCurtis wrote: > > > > > Pulling output may allow new input to be queued; as seen on the MotoX. > > > > > > > > It is expected: pulling the output should release a room for more input. > > > > The current logic postpones the input to be queued till the next cycle. > > > > > > Do you say it would be better to change the order: process output first, > > > and then the input? > > > > I'm saying it should be done in a loop to process as much input and output > > within a single timer wakeup; this is what we did with AVDA. > > I see. I missed that change. Maybe swapping the order and not looping is enough actually? Because it seems unlikely that if you first consume all the outputs, and then queue inputs, that more outputs will become available right away. Maybe not worth worrying about, but I seem to remember calling dequeue not being as cheap as you'd hope.
Just posting my progress (mainly nits) https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:75: media_codec_.reset(); Any reason to do this explicitly? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:119: struct InputBufferInfo { I think I like this better if you drop the Info and call them InputBuffer and OutputBuffer. InputBuffer input_buffer = DequeueInputBuffer(); if (input_buffer.index != kInvalid) EnqueueInputBuffer(input_buffer); https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:121: buf_index; // The codec input buffers are referred to by this index. nit: "buf_" feels redundant now https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:127: struct InputBufferData { InputBufferInfo and InputBufferData are pretty similar, but I'm struggling to think of a better name. InputData? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:131: ~InputBufferData(); You might be able to get away with the default constructor/destructor/copy constructor and fiddle with the fields directly, unless I've missed some calls to these. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:149: // Information about the MediaCodec's output buffer. Maybe: "Information about a MediaCodec output buffer." https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:151: int buf_index; // The codec output buffers are referred to by this index. Same comment about "buf_" https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:156: bool is_key_frame; // true if this buffer is a key frame. These two comments should be capitalized. (Or deleted). I wonder if MediaCodecBridge::DequeueOutputBuffer should return a struct like this? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:159: class Client { Protected non-virtual destructor? And DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:169: // to be queued. Could be more explicit: "It is an error to call this if !IsAnyInputPending()."? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:216: // given a non-null codec during construction. Can we explicitly require that media_codec is not null in the constructor with a DCHECK so we don't have to consider this returning null? https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:220: // Possible states. Comment seems redundant
wow... it's not easy to change the world :) it'll take me a while to wade through all of these good comments from everybody. in the interim, here are a few guidelines that i've been trying to stick to for MCL: - the goal is not to bring in AVDA improvements to MCL in this CL. moving AVDA improvements into MCL is later. this CL focuses on getting the MCL interface correct, using MCAD's implementation. else, we're changing too much at once. - MCL deliberately does not subsume all of the functionality that it can from MCAD / AVDA. CDM, buffer queues, and deferred initialization of codecs are good examples. those things aren't essential to driving MediaCodec, and can be factored out separately into other helpers. that's a problem we have now -- AVDA (and, to some extent, MCAD) has a bunch of nearly orthogonal things going on. side benefit is that we can integrate MCL into AVDA more easily than getting all of those things right too. - MCL should help move us to testable code. a lot of this we get automatically from the previous point. beyond that, MCL should also not drag in too many external dependencies that would need to be mocked. anyway, thanks again for the comments. -fl
thanks for the feedback. -fl https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:22: // TODO(liberato): the client should choose these. On 2016/06/08 22:55:01, DaleCurtis wrote: > You should be able to replace these now with constexpr > base::TimeDelta::FromXXX() kConstant; nifty trick. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:39: // TODO(liberato): does anybody call this? if not, maybe they should. On 2016/06/08 22:55:01, DaleCurtis wrote: > Delete until then? Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:75: media_codec_.reset(); On 2016/06/09 02:04:33, watk wrote: > Any reason to do this explicitly? nope, removed. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:100: io_timer_.Stop(); On 2016/06/08 22:55:01, DaleCurtis wrote: > Step N+1 is to switch to a common timer like AVDA? yup. the first step aims at no function changes. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:103: success = (media_codec_->Reset() == MEDIA_CODEC_OK); On 2016/06/08 22:55:01, DaleCurtis wrote: > Avoid unnecessary parens. Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:116: const bool did_input = QueueInput(); On 2016/06/09 00:56:48, DaleCurtis wrote: > On 2016/06/09 at 00:54:45, Tima Vaisburd wrote: > > On 2016/06/09 00:30:49, DaleCurtis wrote: > > > On 2016/06/08 at 23:24:13, Tima Vaisburd wrote: > > > > Each one is a while loop. > > > > > > Yes, but I don't think each one should be. We know there are cases where > inputs > > > may be queued after an output is dequeued > > > > I am not aware of these cases (or forgot), could you, please, remind me? > Currently we repeat the round on next timer (i.e. in 10 ms), what are the cases > where it would be better to do differently? > > > > My logic has been as follows. On input, enqueue all available buffers from the > queue if the codec accepts them, there is no point to postpone. Independently of > success of failure in the input proceed to output (that's important, otherwise > decoder might block). On output, pull everything that's available at the current > time (timeout == 0). > > Pulling output may allow new input to be queued; as seen on the MotoX. hrm, i was going to keep this identical until the next CL. however, yeah, it seems pretty safe to do now and does clean up the naming a bit. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:122: bool MediaCodecLoop::QueueInput() { On 2016/06/08 22:55:01, DaleCurtis wrote: > I think the naming for these methods should be reconsidered. We have Queue, > QueueOne, Enqueue, and Deque all referring to input buffers -- none of which has > a clear label. How about merging QueueInput into DoIOTask() like we do for AVDA? > Then rename the QueueOneInputBuffer to ProcessOneInputBuffer. Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:138: if (pending_input_buf_index_ == kInvalidBufferIndex && On 2016/06/08 22:55:00, DaleCurtis wrote: > Needs {} Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:142: if (state_ == STATE_WAITING_FOR_KEY || state_ == STATE_DRAINING || On 2016/06/08 22:55:01, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:293: bool did_work = false; On 2016/06/08 22:55:01, DaleCurtis wrote: > I'd move this out of here and into DoIOTask then follow similar naming > conventions to the input functions. Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:366: void MediaCodecLoop::ManageTimer(bool did_work) { On 2016/06/08 22:55:01, DaleCurtis wrote: > I think this should actually be refactored out further such that we have a > MediaCodecLoopTimer singleton which manages this for all instances. i think that should be part of the next CL, since it's importing AVDA code. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:389: if (state_ != new_state && state_ == STATE_ERROR) On 2016/06/08 22:55:01, DaleCurtis wrote: > This offers no room for deferring errors since it is unnecessary for audio, just > fyi. i think the error deferring logic stays in the client. AVDA::OnError can still defer the post, i think. actually, i'm going to rename OnError. it's too generic. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.cc:398: #undef RETURN_STRING On 2016/06/09 00:30:49, DaleCurtis wrote: > On 2016/06/08 at 23:14:20, Tima Vaisburd wrote: > > On 2016/06/08 22:55:01, DaleCurtis wrote: > > > No macros, lets avoid this from the beginning :) > > > > Dale, what would be your suggestion to implement AsString() then? > > Hmm, I thought there'd be a nice C++ solution to this these days, but there's > only duplicate your strings as far as I can tell. So how about making this a > little safer: > > #if defined(RETURN_STRING) > #error Unexpected macro collision. > #else > AsString() { > #define RETURN_STRING(x) case x: return #x; > #undef RETURN_STRING > } > #endif done, though i skipped #else since #error. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:119: struct InputBufferInfo { On 2016/06/09 02:04:34, watk wrote: > I think I like this better if you drop the Info and call them InputBuffer and > OutputBuffer. > > InputBuffer input_buffer = DequeueInputBuffer(); > if (input_buffer.index != kInvalid) > EnqueueInputBuffer(input_buffer); Done. also moved to protected since the client never sees it. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:120: const int On 2016/06/08 22:55:01, DaleCurtis wrote: > Odd formatting. Did clang-format do this? yeah. i moved the comment above it, and now it's what one would expect. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:121: buf_index; // The codec input buffers are referred to by this index. On 2016/06/09 02:04:34, watk wrote: > nit: "buf_" feels redundant now Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:127: struct InputBufferData { On 2016/06/09 02:04:34, watk wrote: > InputBufferInfo and InputBufferData are pretty similar, but I'm struggling to > think of a better name. InputData? Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:131: ~InputBufferData(); On 2016/06/09 02:04:34, watk wrote: > You might be able to get away with the default constructor/destructor/copy > constructor and fiddle with the fields directly, unless I've missed some calls > to these. i've removed the unused constructor. the copy constructor needs to be present because chromium-style yells at me when i don't. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:149: // Information about the MediaCodec's output buffer. On 2016/06/09 02:04:33, watk wrote: > Maybe: "Information about a MediaCodec output buffer." Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:151: int buf_index; // The codec output buffers are referred to by this index. On 2016/06/09 02:04:34, watk wrote: > Same comment about "buf_" Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:156: bool is_key_frame; // true if this buffer is a key frame. On 2016/06/09 02:04:33, watk wrote: > These two comments should be capitalized. (Or deleted). > > I wonder if MediaCodecBridge::DequeueOutputBuffer should return a struct like > this? Caps: done. not sure about MediaCodecBridge. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:159: class Client { On 2016/06/09 02:04:34, watk wrote: > Protected non-virtual destructor? And DISALLOW_COPY_AND_ASSIGN. destructor: good idea. done. DISALLOW_COPY_AND_ASSIGN: there's no data to copy or assign. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:169: // to be queued. On 2016/06/09 02:04:34, watk wrote: > Could be more explicit: "It is an error to call this if !IsAnyInputPending()."? that's too strict, since IsAnyInputPending is not const. it could change the result of the next call, but it would still be okay to get the input data that was pending on the original. actually, i'll make it const and update the docs as you suggest. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:216: // given a non-null codec during construction. On 2016/06/09 02:04:33, watk wrote: > Can we explicitly require that media_codec is not null in the constructor with a > DCHECK so we don't have to consider this returning null? Done. https://codereview.chromium.org/2016213003/diff/140001/media/base/android/med... media/base/android/media_codec_loop.h:220: // Possible states. On 2016/06/09 02:04:33, watk wrote: > Comment seems redundant Done. https://codereview.chromium.org/2016213003/diff/140001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:137: DVLOG(0) << __FUNCTION__ << " failed: cannot create AudioCodecBridge"; On 2016/06/08 22:55:01, DaleCurtis wrote: > DLOG(ERROR) Done. https://codereview.chromium.org/2016213003/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:145: DVLOG(0) << __FUNCTION__ << " failed: cannot configure audio codec for " On 2016/06/08 22:55:01, DaleCurtis wrote: > Ditto. Done.
Is it possible to add a basic unit test for media_codec_loop? Nothing fancy right now, just something that spins it up and shuts it down. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:48: DVLOG(1) << __FUNCTION__; I typically drop these from production code, but up to you. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:75: bool success = false; Reorder this so that we early return instead of using bool success. I also wonder if we should have a static bool codec_reset_requires_destruction() { return base::android::BuildInfo::GetInstance()->sdk_int() < 18; } helper method for clarity. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:112: if (pending_input_buf_index_ == kInvalidBufferIndex && A single pending_input_buf_index_ isn't going to work for video right? We queue multiple inputs at once where as audio only allowed one to be queued at a time. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:118: state_ == STATE_DRAINED) { STATE_ERROR ? https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:123: InputBuffer input_buffer = DequeueInputBuffer(); Instead of return by copy should this take a InputBuffer* and return a true/false for error? That allows us to avoid mixing kInvalidBufferIndex everywhere as our error signal. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:156: DVLOG(1) << __FUNCTION__ << ": MEDIA_CODEC_ERROR from DequeInputBuffer"; DLOG(ERROR) ? https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:178: Remove line so it's clear input_data goes with conditional. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:191: // Process this buffer. Not really useful. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:232: DVLOG(0) << __FUNCTION__ << ": MEDIA_CODEC_ERROR from QueueInputBuffer"; DLOG(ERROR) https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:326: DVLOG(0) << __FUNCTION__ DLOG(ERROR) https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:375: #if defined(RETURN_STRING) Hmm, actually, is this even necessary? I.e. won't the compiler complain about these collisions already? https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.h:119: struct InputData { Structs without default values make me nervous; especially when we have memory pointers :) https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.h:151: protected: public is generally first in chromium code. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.h:225: InputBuffer(int i, bool p) : index(i), is_pending(p) {} Constructor at the top.
unittest: done, and found a bug with it. :) didn't try to mock media_codec_bridge. i'll send out a rebased one shortly, but it applied cleanly minus the s/Reset/Flush change. thanks -fl https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:48: DVLOG(1) << __FUNCTION__; On 2016/06/13 23:16:13, DaleCurtis wrote: > I typically drop these from production code, but up to you. Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:75: bool success = false; On 2016/06/13 23:16:13, DaleCurtis wrote: > Reorder this so that we early return instead of using bool success. I also > wonder if we should have a static bool codec_reset_requires_destruction() { > return base::android::BuildInfo::GetInstance()->sdk_int() < 18; } helper method > for clarity. Done, also renamed 'flush' pending rebase onto watk's recent MediaCodecBridge s/Reset/Flush/ https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:112: if (pending_input_buf_index_ == kInvalidBufferIndex && On 2016/06/13 23:16:13, DaleCurtis wrote: > A single pending_input_buf_index_ isn't going to work for video right? We queue > multiple inputs at once where as audio only allowed one to be queued at a time. this is only if we're waiting for a key, which limits us to one buffer. the same logic is in avda: https://cs.chromium.org/chromium/src/media/gpu/android_video_decode_accelerat... https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:118: state_ == STATE_DRAINED) { On 2016/06/13 23:16:13, DaleCurtis wrote: > STATE_ERROR ? Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:123: InputBuffer input_buffer = DequeueInputBuffer(); On 2016/06/13 23:16:13, DaleCurtis wrote: > Instead of return by copy should this take a InputBuffer* and return a > true/false for error? That allows us to avoid mixing kInvalidBufferIndex > everywhere as our error signal. this sounded like a good idea, but i un-did it after trying it out. the problem is that "try again later" isn't an error, but we want to do the same thing as the error cases. we really do want "if we have no input buffer". otherwise, it turns into if (Dequeue...()) return false; // error if (input_buffer.input_buf_index == kInvalidBufferIndex) return false; // no work to do i tried the middle ground of "return buf_index != kInvalidBufferIndex", but it didn't seem to add any clarity. probably made the call less clear. plus, i had to remove the InputBuffer consts to allow the assignments that were okay during initialization. so, i just put it back this way. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:156: DVLOG(1) << __FUNCTION__ << ": MEDIA_CODEC_ERROR from DequeInputBuffer"; On 2016/06/13 23:16:13, DaleCurtis wrote: > DLOG(ERROR) ? Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:178: On 2016/06/13 23:16:13, DaleCurtis wrote: > Remove line so it's clear input_data goes with conditional. Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:191: // Process this buffer. On 2016/06/13 23:16:13, DaleCurtis wrote: > Not really useful. Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:232: DVLOG(0) << __FUNCTION__ << ": MEDIA_CODEC_ERROR from QueueInputBuffer"; On 2016/06/13 23:16:13, DaleCurtis wrote: > DLOG(ERROR) Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:326: DVLOG(0) << __FUNCTION__ On 2016/06/13 23:16:13, DaleCurtis wrote: > DLOG(ERROR) Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.cc:375: #if defined(RETURN_STRING) On 2016/06/13 23:16:13, DaleCurtis wrote: > Hmm, actually, is this even necessary? I.e. won't the compiler complain about > these collisions already? yes. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.h:119: struct InputData { On 2016/06/13 23:16:13, DaleCurtis wrote: > Structs without default values make me nervous; especially when we have memory > pointers :) Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.h:151: protected: On 2016/06/13 23:16:13, DaleCurtis wrote: > public is generally first in chromium code. Done. https://codereview.chromium.org/2016213003/diff/160001/media/base/android/med... media/base/android/media_codec_loop.h:225: InputBuffer(int i, bool p) : index(i), is_pending(p) {} On 2016/06/13 23:16:13, DaleCurtis wrote: > Constructor at the top. Done.
lgtm https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:47: if (media_codec_ == nullptr) if (!media_codec_) -- but really should this be a DCHECK? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:60: static bool codec_flush_requires_destruction() { Should go above all other methods. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:174: InputData input_data; Could be written with ternary: InputData input_data = is_pending ? pending_input_buf_data_ : provide; if you prefer. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:192: media::MediaCodecStatus status = MEDIA_CODEC_OK; Ditto on ternary usage, again up to you. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:244: MediaCodecStatus status; Avoid c-style variable declarations where possible. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:264: did_work = (state_ != STATE_ERROR); Remove unnesscary parens. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:140: struct OutputBuffer { Assign defaults? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:179: ~Client() {} virtual per style guide. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:221: InputBuffer() : index(kInvalidBufferIndex), is_pending(false) {} Drop and use inline values instead? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:291: base::WeakPtrFactory<MediaCodecLoop> weak_factory_; Annotate with comment indicating this must always be the last member to avoid destruction races. See other classes for examples. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop_unittest.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop_unittest.cc:40: ASSERT_TRUE(codec_loop_->GetCodec() == nullptr); ASSERT_FALSE(codec_loop_->GetCodec());
https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:47: if (media_codec_ == nullptr) On 2016/06/14 19:29:03, DaleCurtis wrote: > if (!media_codec_) -- but really should this be a DCHECK? probably, but then i've nothing to test without a media codec bridge mock. i'll keep it as-is, since that particular test actually found a bug. i'll add a TODO to sort it out once the tests are better. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:60: static bool codec_flush_requires_destruction() { On 2016/06/14 19:29:03, DaleCurtis wrote: > Should go above all other methods. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:174: InputData input_data; On 2016/06/14 19:29:03, DaleCurtis wrote: > Could be written with ternary: > > InputData input_data = is_pending ? pending_input_buf_data_ : provide; if you > prefer. i'll keep it as is, so that the comment is clearer. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:192: media::MediaCodecStatus status = MEDIA_CODEC_OK; On 2016/06/14 19:29:03, DaleCurtis wrote: > Ditto on ternary usage, again up to you. Acknowledged. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:244: MediaCodecStatus status; On 2016/06/14 19:29:03, DaleCurtis wrote: > Avoid c-style variable declarations where possible. done, i think. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:264: did_work = (state_ != STATE_ERROR); On 2016/06/14 19:29:03, DaleCurtis wrote: > Remove unnesscary parens. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:140: struct OutputBuffer { On 2016/06/14 19:29:03, DaleCurtis wrote: > Assign defaults? thanks, meant to do it in the last one. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:179: ~Client() {} On 2016/06/14 19:29:03, DaleCurtis wrote: > virtual per style guide. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:221: InputBuffer() : index(kInvalidBufferIndex), is_pending(false) {} On 2016/06/14 19:29:03, DaleCurtis wrote: > Drop and use inline values instead? Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:291: base::WeakPtrFactory<MediaCodecLoop> weak_factory_; On 2016/06/14 19:29:03, DaleCurtis wrote: > Annotate with comment indicating this must always be the last member to avoid > destruction races. See other classes for examples. done. also, i think clang now yells at you if it isn't. at least, in some cases it does. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop_unittest.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop_unittest.cc:40: ASSERT_TRUE(codec_loop_->GetCodec() == nullptr); On 2016/06/14 19:29:03, DaleCurtis wrote: > ASSERT_FALSE(codec_loop_->GetCodec()); Done.
https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:61: // Flush if we can, otherwise completely recreate and reconfigure the codec. First sentence of this comment should be reworded or deleted. TIL about the naming style for "cheap" functions. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:67: bool MediaCodecLoop::TryFlush() { I think this should become void Flush(), because currently it's complicated to detect whether the flush failed because of a codec error (which should be considered terminal) or because it's not supported by the device, in which case you need to recreate it. The error case calls OnCodecLoopError before TryFlush returns. So you'd have to save the state variable and check it after TryFlush to detect that flush errored. So the calling code would look like: if (NeedsFlushWorkaround()) { DeleteAndRecreateLoop(); } else { loop->Flush(); } And Flush() should DCHECK that it doesn't need the flush workaround. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:72: if (state_ == STATE_ERROR || state_ == STATE_DRAINED) It's not clear to me why we can't flush in STATE_DRAINED? Or is it that we don't need to yet? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:78: // Actually try to flush! nit: Comment seems attached to an unrelated line. Probably if you delete the newline after io_time_.Stop() it won't seem weird. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:82: // Transition to the error state if the flush failed. Code is clear. I'd delete this comment. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:119: } nit: this could be != READY, and checking if the state is valid feels like it should come first, even if it probably doesn't really matter. Slightly weird for the client to possibly get a call to IsAnyInputPending after OnError(). https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:128: // STATE_ERROR. Up to you, but feels fragile to list these here. Could just say it may change the state. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:130: return state_ == STATE_READY; If state is DRAINING we still did work right? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:138: // available input buffers. We have to reuse it in QueueSecureInputBuffer(). We don't have a QueueSecureInputBuffer here (but we call one). https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:244: MediaCodecStatus status; On 2016/06/14 21:56:44, liberato wrote: > On 2016/06/14 19:29:03, DaleCurtis wrote: > > Avoid c-style variable declarations where possible. > > done, i think. I believe the point was that these declarations are earlier than they need to be. And status could be declare and initialized at the same time. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:268: // We got the decoded frame. minor nit: we didn't necessarily get a frame. Could be EOS buffer. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:302: // Next Decode() will report the error to the pipeline. pipeline comment is not applicable. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:308: // Next Decode() will report the error to the pipeline. same pipeline comment https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:338: client_->OnCodecLoopError(); Should this set the state first? To avoid any weirdness with reentrant calls to MCL by the client? Probably not a problem with the public interface we currently have. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:60: // thread. The "similar to AVDA" comment will be very soon out of date. I think the main point is that we poll at 10ms using non-blocking calls because a) we don't want to block the thread, and b) we can't tell in advance whether the calls will succeed or block indefinitely. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:73: // [Ready] Should this be any state? (not only Ready)? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:102: // | | Should we have a flushing state in this diagram? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:179: ~Client() {} On 2016/06/14 19:29:03, DaleCurtis wrote: > virtual per style guide. My bad Frank. I didn't realize the style guide says the destructor should be virtual in this case. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:196: void DoIOTask(); Should we rename this now? I'm trying to think of what the API would look like if we created MCL from the start, and I'm pretty sure it wouldn't be called DoIOTask(). Probably I would imagine something like: MCL codec_loop = new MCL(); codec_loop->StartLooping(); https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:200: // false but the state is still READY, then the codec may continue to be used. This can return false if the state is DRAINED, but the codec is also still usable? So the comment is a little imprecise. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:203: // Callback called by the client when a new key is available. This allows Comment feels focused on the implementation rather than the contract. e.g., it's not necessarily the client that has to call this. What about: "This should be called when a new key is added. Decoding will resume if it was stopped in the WAITING_FOR_KEY state." https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:207: // Return our codec. It is guaranteed that this will be non-null. Not technically true at the moment :) Maybe guaranteed non-null if state is not error? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:283: // When processing a pending input bfufer, this is the data that was returned bfufer https://codereview.chromium.org/2016213003/diff/220001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/220001/media/base/android/med... media/base/android/media_codec_loop.h:33: // (http://developer.android.com/reference/android/media/MediaCodec.html) works nit: you can probably delete this url. I doubt anyone will use it/needs it. https://codereview.chromium.org/2016213003/diff/220001/media/base/android/med... media/base/android/media_codec_loop.h:62: // This implementation detects the MediaCodec idle run (no input or output "detects the MediaCodec idle run" could be made clearer. https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:203: // we have to completely destroy and recreate the codec there. Can delete the two lines about flush having bugs since this is now written against the MCL interface, which is clear about flush maybe failing. https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:309: // EOS actually arrives on the output queue. Completion callback comment belongs with the completion_cb assignment? https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:30: // (http://crbug.com/583082). Delete https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:143: // void ManageTimer(bool start); Delete
lots of good comments. i might have misunderstood the concerns in TryFlush -- the rest i've addressed, i think. thanks -fl https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:61: // Flush if we can, otherwise completely recreate and reconfigure the codec. On 2016/06/15 00:29:17, watk wrote: > First sentence of this comment should be reworded or deleted. > > TIL about the naming style for "cheap" functions. TIL? it mentions to use unix_hacker_style for inline functions, so i added 'inline'. :) https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:67: bool MediaCodecLoop::TryFlush() { On 2016/06/15 00:29:17, watk wrote: > I think this should become void Flush(), because currently it's complicated to > detect whether the flush failed because of a codec error (which should be > considered terminal) or because it's not supported by the device, in which case > you need to recreate it. The error case calls OnCodecLoopError before TryFlush > returns. So you'd have to save the state variable and check it after TryFlush to > detect that flush errored. > > So the calling code would look like: > > if (NeedsFlushWorkaround()) { > DeleteAndRecreateLoop(); > } else { > loop->Flush(); > } > > And Flush() should DCHECK that it doesn't need the flush workaround. i don't understand why the caller cares about the difference between a codec error and not supported. either way, it has to recreate the codec if it wants to keep using it. for example, MCAD::Reset doesn't currently treat a previous codec error any differently -- it just skips the MCB::Flush() in that case and makes a new codec. if it does what to differentiate (avda might), then it can check the state before calling TryFlush. i suppose that calling OnCodecError() during TryFlush() might be confusing to the caller. MCAD already fails any pending input in the input queue at the top of Reset, which would is a side-effect of OnCodecError too otherwise. however, i think that AVDA has cases where it doesn't want to do that, so maybe this will have to change. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:72: if (state_ == STATE_ERROR || state_ == STATE_DRAINED) On 2016/06/15 00:29:17, watk wrote: > It's not clear to me why we can't flush in STATE_DRAINED? Or is it that we don't > need to yet? that's a very good question. we can, according to MC docs. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:78: // Actually try to flush! On 2016/06/15 00:29:17, watk wrote: > nit: Comment seems attached to an unrelated line. Probably if you delete the > newline after io_time_.Stop() it won't seem weird. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:82: // Transition to the error state if the flush failed. On 2016/06/15 00:29:17, watk wrote: > Code is clear. I'd delete this comment. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:119: } On 2016/06/15 00:29:17, watk wrote: > nit: this could be != READY, and checking if the state is valid feels like it > should come first, even if it probably doesn't really matter. > > Slightly weird for the client to possibly get a call to IsAnyInputPending after > OnError(). Done, good point. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:128: // STATE_ERROR. On 2016/06/15 00:29:17, watk wrote: > Up to you, but feels fragile to list these here. Could just say it may change > the state. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:130: return state_ == STATE_READY; On 2016/06/15 00:29:17, watk wrote: > If state is DRAINING we still did work right? good point. it could just return true at this point. i changed it to !=ERROR, just since it can't transition out of error so there's no point in resetting the timer. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:138: // available input buffers. We have to reuse it in QueueSecureInputBuffer(). On 2016/06/15 00:29:17, watk wrote: > We don't have a QueueSecureInputBuffer here (but we call one). Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:244: MediaCodecStatus status; On 2016/06/15 00:29:17, watk wrote: > On 2016/06/14 21:56:44, liberato wrote: > > On 2016/06/14 19:29:03, DaleCurtis wrote: > > > Avoid c-style variable declarations where possible. > > > > done, i think. > > I believe the point was that these declarations are earlier than they need to > be. And status could be declare and initialized at the same time. that's what i ended up doing. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:268: // We got the decoded frame. On 2016/06/15 00:29:17, watk wrote: > minor nit: we didn't necessarily get a frame. Could be EOS buffer. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:302: // Next Decode() will report the error to the pipeline. On 2016/06/15 00:29:17, watk wrote: > pipeline comment is not applicable. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:308: // Next Decode() will report the error to the pipeline. On 2016/06/15 00:29:17, watk wrote: > same pipeline comment Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:338: client_->OnCodecLoopError(); On 2016/06/15 00:29:17, watk wrote: > Should this set the state first? To avoid any weirdness with reentrant calls to > MCL by the client? Probably not a problem with the public interface we currently > have. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:60: // thread. On 2016/06/15 00:29:18, watk wrote: > The "similar to AVDA" comment will be very soon out of date. I think the main > point is that we poll at 10ms using non-blocking calls because a) we don't want > to block the thread, and b) we can't tell in advance whether the calls will > succeed or block indefinitely. Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:73: // [Ready] On 2016/06/15 00:29:18, watk wrote: > Should this be any state? (not only Ready)? Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:102: // | | On 2016/06/15 00:29:17, watk wrote: > Should we have a flushing state in this diagram? it has draining / drained. is that what you mean? https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:196: void DoIOTask(); On 2016/06/15 00:29:17, watk wrote: > Should we rename this now? I'm trying to think of what the API would look like > if we created MCL from the start, and I'm pretty sure it wouldn't be called > DoIOTask(). > > Probably I would imagine something like: > MCL codec_loop = new MCL(); > codec_loop->StartLooping(); agree that it should be renamed. don't like 'start looping'. it sounds too much like it won't return from the call. I'll try DoPendingWork(). https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:200: // false but the state is still READY, then the codec may continue to be used. On 2016/06/15 00:29:18, watk wrote: > This can return false if the state is DRAINED, but the codec is also still > usable? So the comment is a little imprecise. i don't think that the codec is still usable after EOS without a flush or start/stop. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:203: // Callback called by the client when a new key is available. This allows On 2016/06/15 00:29:17, watk wrote: > Comment feels focused on the implementation rather than the contract. e.g., it's > not necessarily the client that has to call this. What about: > > "This should be called when a new key is added. Decoding will resume if it was > stopped in the WAITING_FOR_KEY state." Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:207: // Return our codec. It is guaranteed that this will be non-null. On 2016/06/15 00:29:18, watk wrote: > Not technically true at the moment :) Maybe guaranteed non-null if state is not > error? Done. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:283: // When processing a pending input bfufer, this is the data that was returned On 2016/06/15 00:29:18, watk wrote: > bfufer Dnoe. https://codereview.chromium.org/2016213003/diff/220001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/220001/media/base/android/med... media/base/android/media_codec_loop.h:33: // (http://developer.android.com/reference/android/media/MediaCodec.html) works On 2016/06/15 00:29:18, watk wrote: > nit: you can probably delete this url. I doubt anyone will use it/needs it. Done. https://codereview.chromium.org/2016213003/diff/220001/media/base/android/med... media/base/android/media_codec_loop.h:62: // This implementation detects the MediaCodec idle run (no input or output On 2016/06/15 00:29:18, watk wrote: > "detects the MediaCodec idle run" could be made clearer. Done. https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:30: // (http://crbug.com/583082). On 2016/06/15 00:29:18, watk wrote: > Delete ah, thanks. i kept this around so i wouldn't have to look up the bug for the commit message, and forgot to get rid of it. https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:143: // void ManageTimer(bool start); On 2016/06/15 00:29:18, watk wrote: > Delete Done.
lgtm https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:61: // Flush if we can, otherwise completely recreate and reconfigure the codec. On 2016/06/15 20:27:29, liberato wrote: > On 2016/06/15 00:29:17, watk wrote: > > First sentence of this comment should be reworded or deleted. > > > > TIL about the naming style for "cheap" functions. > > TIL? > > it mentions to use unix_hacker_style for inline functions, so i added 'inline'. > :) Today I learned :) I looked up the style guide and saw the section about functions that are cheap to call should be named in this style. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.cc:67: bool MediaCodecLoop::TryFlush() { On 2016/06/15 20:27:30, liberato wrote: > On 2016/06/15 00:29:17, watk wrote: > > I think this should become void Flush(), because currently it's complicated to > > detect whether the flush failed because of a codec error (which should be > > considered terminal) or because it's not supported by the device, in which > case > > you need to recreate it. The error case calls OnCodecLoopError before TryFlush > > returns. So you'd have to save the state variable and check it after TryFlush > to > > detect that flush errored. > > > > So the calling code would look like: > > > > if (NeedsFlushWorkaround()) { > > DeleteAndRecreateLoop(); > > } else { > > loop->Flush(); > > } > > > > And Flush() should DCHECK that it doesn't need the flush workaround. > > i don't understand why the caller cares about the difference between a codec > error and not supported. either way, it has to recreate the codec if it wants > to keep using it. for example, MCAD::Reset doesn't currently treat a previous > codec error any differently -- it just skips the MCB::Flush() in that case and > makes a new codec. Gotcha. I assumed we wanted to treat a codec error as a terminal error, and not attempt to continue by recreating a codec. This makes sense if not. But still means we have to be careful when calling TryFlush to not care too much about the OnCodecLoopError that we might get. https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/200001/media/base/android/med... media/base/android/media_codec_loop.h:283: // When processing a pending input bfufer, this is the data that was returned On 2016/06/15 20:27:30, liberato wrote: > On 2016/06/15 00:29:18, watk wrote: > > bfufer > > Dnoe. s/Dnoe/Done :)
https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... media/base/android/media_codec_loop.h:196: std::unique_ptr<MediaCodecBridge>&& media_codec); It seems passing unique_ptr by value is more idiomatic: http://stackoverflow.com/questions/8114276/how-do-i-pass-a-unique-ptr-argumen... and http://stackoverflow.com/questions/17473900/unique-ptr-to-a-base-class What was the error message? This did compile: #include <memory> using namespace std; struct Base {}; struct Derived : public Base {}; void f(unique_ptr<Base> b) {} void g() { unique_ptr<Derived> d(new Derived()); f(move(d)); }
https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... media/base/android/media_codec_loop.cc:98: did_input = ProcessOneInputBuffer(); If the OUTPUT_FORMAT_CHANGED arrives, this code will try to push another input buffer, and only then dequeue the output buffer that is probably waiting on the output port. Although this sounds perfectly legitimate, I think we never tried that sequence. AVDA is doing approximately this: do { ProcessOneInputBuffer(); ProcessAllOutputBuffers(); // <- not quite, all/w status < 0 and then one real } while(...) I suspect the current call sequence might not be well tested with MediaCodec implementors. Can we do { one input; all outputs; } as above or do { ProcessAllInputBuffers(); ProcessAllOutputBuffers(); } while(...) ?
thanks for the comments. -fl https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:203: // we have to completely destroy and recreate the codec there. On 2016/06/15 00:29:18, watk wrote: > Can delete the two lines about flush having bugs since this is now written > against the MCL interface, which is clear about flush maybe failing. Done. https://codereview.chromium.org/2016213003/diff/220001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:309: // EOS actually arrives on the output queue. On 2016/06/15 00:29:18, watk wrote: > Completion callback comment belongs with the completion_cb assignment? Done. https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... media/base/android/media_codec_loop.cc:98: did_input = ProcessOneInputBuffer(); On 2016/06/16 21:47:29, Tima Vaisburd wrote: > If the OUTPUT_FORMAT_CHANGED arrives, this code will try to push another input > buffer, and only then dequeue the output buffer that is probably waiting on the > output port. Although this sounds perfectly legitimate, I think we never tried > that sequence. AVDA is doing approximately this: > do { > ProcessOneInputBuffer(); > ProcessAllOutputBuffers(); // <- not quite, all/w status < 0 and then one real > } > while(...) > > I suspect the current call sequence might not be well tested with MediaCodec > implementors. > Can we do { one input; all outputs; } as above or > > do { > ProcessAllInputBuffers(); > ProcessAllOutputBuffers(); > } > while(...) ? i don't think that OUTPUT_FORMAT_CHANGED processes a real buffer in avda. it always returns early. all of the messages in the switch, except OK and BUFFERS_CHANGED do. https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... File media/base/android/media_codec_loop.h (right): https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... media/base/android/media_codec_loop.h:196: std::unique_ptr<MediaCodecBridge>&& media_codec); On 2016/06/16 18:41:27, Tima Vaisburd wrote: > It seems passing unique_ptr by value is more idiomatic: > http://stackoverflow.com/questions/8114276/how-do-i-pass-a-unique-ptr-argumen... > and > http://stackoverflow.com/questions/17473900/unique-ptr-to-a-base-class > > What was the error message? > > This did compile: > > #include <memory> > using namespace std; > > struct Base {}; > struct Derived : public Base {}; > > void f(unique_ptr<Base> b) {} > > void g() { > unique_ptr<Derived> d(new Derived()); > f(move(d)); > } good -- i was really confused why it wasn't working. no idea what i mis-typed. thanks!
lgtm. Please test the audio with EME. https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... File media/base/android/media_codec_loop.cc (right): https://codereview.chromium.org/2016213003/diff/240001/media/base/android/med... media/base/android/media_codec_loop.cc:98: did_input = ProcessOneInputBuffer(); On 2016/06/27 16:13:37, liberato wrote: > On 2016/06/16 21:47:29, Tima Vaisburd wrote: > > If the OUTPUT_FORMAT_CHANGED arrives, this code will try to push another input > > buffer, and only then dequeue the output buffer that is probably waiting on > the > > output port. Although this sounds perfectly legitimate, I think we never tried > > that sequence. AVDA is doing approximately this: > > do { > > ProcessOneInputBuffer(); > > ProcessAllOutputBuffers(); // <- not quite, all/w status < 0 and then one > real > > } > > while(...) > > > > I suspect the current call sequence might not be well tested with MediaCodec > > implementors. > > Can we do { one input; all outputs; } as above or > > > > do { > > ProcessAllInputBuffers(); > > ProcessAllOutputBuffers(); > > } > > while(...) ? > > i don't think that OUTPUT_FORMAT_CHANGED processes a real buffer in avda. it > always returns early. all of the messages in the switch, except OK and > BUFFERS_CHANGED do. Yes, you are right.
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, watk@chromium.org, timav@chromium.org Link to the patchset: https://codereview.chromium.org/2016213003/#ps280001 (title: "fixed |media_crypto_|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, timav@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2016213003/#ps300001 (title: "don't allow STATE_DRAINED when flushing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patch Set 16 lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by liberato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Separate MediaCodecLoop from MediaCodecAudioDecoder MCAD and AndroidVideoDecodeAccelerator share lots of code to interact with MediaCodec. This CL splits out much of that into a new MediaCodecLoop class. MediaCodecLoop manages driving the I/O loop for MediaCodec and interacting with a client (AVDA or MCAD). This CL only affects MCAD. AVDA's event loop is more complicated, and will be merged into MediaCodecLoop as part of a separate CL. BUG=583082 ========== to ========== Separate MediaCodecLoop from MediaCodecAudioDecoder MCAD and AndroidVideoDecodeAccelerator share lots of code to interact with MediaCodec. This CL splits out much of that into a new MediaCodecLoop class. MediaCodecLoop manages driving the I/O loop for MediaCodec and interacting with a client (AVDA or MCAD). This CL only affects MCAD. AVDA's event loop is more complicated, and will be merged into MediaCodecLoop as part of a separate CL. BUG=583082 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Separate MediaCodecLoop from MediaCodecAudioDecoder MCAD and AndroidVideoDecodeAccelerator share lots of code to interact with MediaCodec. This CL splits out much of that into a new MediaCodecLoop class. MediaCodecLoop manages driving the I/O loop for MediaCodec and interacting with a client (AVDA or MCAD). This CL only affects MCAD. AVDA's event loop is more complicated, and will be merged into MediaCodecLoop as part of a separate CL. BUG=583082 ========== to ========== Separate MediaCodecLoop from MediaCodecAudioDecoder MCAD and AndroidVideoDecodeAccelerator share lots of code to interact with MediaCodec. This CL splits out much of that into a new MediaCodecLoop class. MediaCodecLoop manages driving the I/O loop for MediaCodec and interacting with a client (AVDA or MCAD). This CL only affects MCAD. AVDA's event loop is more complicated, and will be merged into MediaCodecLoop as part of a separate CL. BUG=583082 Committed: https://crrev.com/8634a951e0946d4bf63889d888cc369dd4c2c665 Cr-Commit-Position: refs/heads/master@{#403350} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/8634a951e0946d4bf63889d888cc369dd4c2c665 Cr-Commit-Position: refs/heads/master@{#403350} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
