Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1651673002:
Add MediaCodecAudioDecoder implementation (Closed)
|
Created:
4 years, 10 months ago by Tima Vaisburd Modified:
4 years, 10 months ago Reviewers:
sandersd (OOO until July 31), watk, liberato (no reviews please), qinmin, DaleCurtis, xhwang, no sievers CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd MediaCodecAudioDecoder implementation
MediaCodecAudioDecoder is an audio decoder that uses Android
MediaCodec API and implements the AudioDecoder interface.
This CL adds the implementation.
BUG=542910
Committed: https://crrev.com/a2d1a85ac6693abd2f59ea321293aa012207c381
Cr-Commit-Position: refs/heads/master@{#375942}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Removed CDM stuff, fixed Opus #
Total comments: 63
Patch Set 3 : Addressed most comments #Patch Set 4 : Removed all changes to decoder_buffer.cc #
Total comments: 44
Patch Set 5 : Rebased, addressed comments, some questions #Patch Set 6 : Modified a comment #
Total comments: 52
Patch Set 7 : Fixes with some questions #
Total comments: 29
Patch Set 8 : Addressed comments #
Total comments: 14
Patch Set 9 : Reset pending input buffer immediately after use, addressed comments. #Patch Set 10 : Fixed component compilaton #Patch Set 11 : A better linking issue fix #
Messages
Total messages: 65 (14 generated)
timav@chromium.org changed reviewers: + liberato@chromium.org, qinmin@chromium.org, sandersd@chromium.org, xhwang@chromium.org
Here is the first draft of AudioDecoderAndroid as we discussed with Xiaohan and Dan. It passes compilation but never ran yet. The CDM part is not complete. At this stage I'm concerned that the CL may be too big for review, and would gladly accept advice on how to make it more tractable. Maybe the CDM part should be taken out and added later. However, the decoder is the cross-breed between AVDA and MediaCodecAudioDecoder and most of you have already seen both.
i'd really like to get some of your improvements from AudioDecoder into AVDA. what would you think about pulling all of the MediaCodec configuration / cdm / timer / QueueInput / DequeueOutput / SetState logic into one place? it looks like you've already gone down this route a little with some of the generalizations you've added (e.g., OnDecodedFrame). the resulting class, something like MediaCodecLoop, would manage a MediaCodecBridge. AVDA / audio decoder would have an instance of MediaCodecLoop. they would also retain the ConfigureMediaCodec logic, i think. there might be a bit of trickery around the input buffer types, but it's probably not too bad. ultimately, they both are used identically as input to MediaCodec. MediaCodecBridge might have a client interface that it could call into for OnDecodedFrame, etc. https://codereview.chromium.org/1651673002/diff/1/media/filters/android_audio... File media/filters/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/1/media/filters/android_audio... media/filters/android_audio_decoder.cc:374: bool AndroidAudioDecoder::DequeueOutput() { DequeueOutput looks like it's an improvement over AVDA's implementation. it also seems to have little or no dependencies on any types that aren't common to both use-cases. i particularly like the On* callbacks and SetState. https://codereview.chromium.org/1651673002/diff/1/media/filters/android_audio... media/filters/android_audio_decoder.cc:380: OutputBufferInfo out; OutputBufferInfo: good idea for avda. https://codereview.chromium.org/1651673002/diff/1/media/filters/android_audio... media/filters/android_audio_decoder.cc:466: void AndroidAudioDecoder::SetState(State new_state) { this is an improvement over avda.
timav@chromium.org changed reviewers: + dalecurtis@chromium.org, watk@chromium.org
+watk@, dalecurtis@
+1 to Frank's suggestion. It'd be great to have a MediaCodecLoop generic class (templated if need be) -- that was my first thought upon seeing this as well. We now have ~5 implementations of this loop if I'm counting correctly :) https://codereview.chromium.org/1651673002/diff/1/media/base/audio_buffer.h File media/base/audio_buffer.h (right): https://codereview.chromium.org/1651673002/diff/1/media/base/audio_buffer.h#n... media/base/audio_buffer.h:140: uint8_t* interleaved_data() const { return data_.get(); } You can just use channel_data()[0] in this case, seems reasonable to return data_size().
I didn't review the details, just general comments! 1. Thanks for putting these together in such a short time! 2. It seems you have too many reviewers on this CL. It'd nice to understand who should review the details, who should provide general comments and who are FYI'ed, etc. 3. The CL is a bit too large to review comfortably (maybe only for me :)), especially since we also want more docs and tests (see below). It'd be nice to split this CL into multiple smaller CLs. For example, as you pointed out, the EME part can be in a different CL. 4. Can you add a link to a design doc, or some comments that describe a big picture how the AAD works? 5. Can we add unittests to cover some basic decoding cases? 6. (+dalecurtis): Any thoughts/comments on where to put AAD? media/filters seems fine. But I was also thinking about putting it together with all other android classes (e.g. MediaCodecBridge). However, they are under media/base/android. Now looking at it, I don't feel they belong to media/base. Maybe we should clean it up at certain point? 7. (nit) typo in CL description: "udes". https://chromiumcodereview.appspot.com/1651673002/diff/1/media/base/decoder_b... File media/base/decoder_buffer.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/1/media/base/decoder_b... media/base/decoder_buffer.h:179: std::string AsShortString(); How about just add a "bool verbose" parameter in AsHumanReadableString()? https://chromiumcodereview.appspot.com/1651673002/diff/1/media/filters/androi... File media/filters/android_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/1/media/filters/androi... media/filters/android_audio_decoder.h:72: void SetCdm(CdmContext* cdm_context, const CdmAttachedCB& cdm_attached_cb); As you pointed out. Leaving EME part to a separate CL sgtm.
On 2016/02/01 15:18:06, liberato wrote: > i'd really like to get some of your improvements from AudioDecoder into AVDA. I'm glad you see this as an improvement :) When we talked with Xiaohan and Dan Sanders (offline) we did realize that the upcoming VDAv2 video decoder and this audio decoder would have similar MediaCodec processing logic and need to share some code. I thought it would be better to do in two steps, now I see I was mistaken. However, why would you like to improve AVDA if it is planned to be replaced with VDAv2? If the corresponding v2 decoder will derive from VideoDecoder the common part (MediaCodecLoop) might be easier to write. > what would you think about pulling all of the MediaCodec configuration / cdm / > timer / QueueInput / DequeueOutput / SetState logic into one place? it looks > like you've already gone down this route a little with some of the > generalizations you've added (e.g., OnDecodedFrame). Yes, On... methods are planned for virtualization for audio/video. > the resulting class, something like MediaCodecLoop, would manage a > MediaCodecBridge. AVDA / audio decoder would have an instance of > MediaCodecLoop. Yes, I like this idea. > they would also retain the ConfigureMediaCodec logic, i think. Configuration would have to be different - at least for now - because of underlying differences in the MediaCodecBridge and its derived classes.
On 2016/02/01 19:30:55, xhwang wrote: > 2. It seems you have too many reviewers on this CL. It'd nice to understand who > should review the details, who should provide general comments and who are > FYI'ed, etc. As far as I understand: xhwang@, sandersd@ : please review. I expect sandersd@ to use/modify upcoming MediaCodecLoop (or MediaCodecProcessor?) class that should be written liberato@, qinmin@, dalecurtis@ : please provide general comments. watk@: fyi, please participate in design and plans discussions. > 3. The CL is a bit too large to review comfortably (maybe only for me :)), > especially since we also want more docs and tests (see below). It'd be nice to > split this CL into multiple smaller CLs. For example, as you pointed out, the > EME part can be in a different CL. This was my major concern, this is why I presented the CL which does not work yet. I will take the EME part out, however, I do not know what else to remove. Please advise.
I think something like media/filters/android would fit with our existing
strategy. (I.e. media/audio/{win/android/mac})
> However, why would you like to improve AVDA if it is planned to be replaced with > VDAv2? If the corresponding v2 decoder will derive from VideoDecoder the common > part (MediaCodecLoop) might be easier to write. this is a good question. doing it now will involve at least some throwaway abstraction work (around bitstream buffer/DecoderBuffer, mostly). however, doing it now has the benefit of making sure that these things don't diverge further before VDAv2 happens. as we add deferred initialization and full screen support, we could easily make AVDA harder to re-integrate. for that reason, i made the suggestion of doing it now. it forces us to keep this stuff in mind.
On 2016/02/01 19:57:05, Tima Vaisburd wrote: > On 2016/02/01 19:30:55, xhwang wrote: > > 2. It seems you have too many reviewers on this CL. It'd nice to understand > who > > should review the details, who should provide general comments and who are > > FYI'ed, etc. > > As far as I understand: > > xhwang@, sandersd@ : please review. I expect sandersd@ to use/modify upcoming > MediaCodecLoop (or MediaCodecProcessor?) class that should be written > > liberato@, qinmin@, dalecurtis@ : please provide general comments. > > watk@: fyi, please participate in design and plans discussions. > > > 3. The CL is a bit too large to review comfortably (maybe only for me :)), > > especially since we also want more docs and tests (see below). It'd be nice to > > split this CL into multiple smaller CLs. For example, as you pointed out, the > > EME part can be in a different CL. > > This was my major concern, this is why I presented the CL which does not work > yet. > I will take the EME part out, however, I do not know what else to remove. Please > advise. if you do decide to go with the MCL thing -- moving AVDA to that might be a good first standalone CL.
> However, why would you like to improve AVDA if it is planned to be replaced with > VDAv2? If the corresponding v2 decoder will derive from VideoDecoder the common > part (MediaCodecLoop) might be easier to write. Sadly the priority of VDAv2 is low enough that it keeps getting bumped by other more pressing issues. I would recommend not waiting for it. (It is still a team OKR for this quarter though.)
On 2016/02/01 20:01:31, liberato wrote: > On 2016/02/01 19:57:05, Tima Vaisburd wrote: > > On 2016/02/01 19:30:55, xhwang wrote: > > > 2. It seems you have too many reviewers on this CL. It'd nice to understand > > who > > > should review the details, who should provide general comments and who are > > > FYI'ed, etc. > > > > As far as I understand: > > > > xhwang@, sandersd@ : please review. I expect sandersd@ to use/modify upcoming > > MediaCodecLoop (or MediaCodecProcessor?) class that should be written > > > > liberato@, qinmin@, dalecurtis@ : please provide general comments. > > > > watk@: fyi, please participate in design and plans discussions. > > > > > 3. The CL is a bit too large to review comfortably (maybe only for me :)), > > > especially since we also want more docs and tests (see below). It'd be nice > to > > > split this CL into multiple smaller CLs. For example, as you pointed out, > the > > > EME part can be in a different CL. > > > > This was my major concern, this is why I presented the CL which does not work > > yet. > > I will take the EME part out, however, I do not know what else to remove. > Please > > advise. > > if you do decide to go with the MCL thing -- moving AVDA to that might be a good > first standalone CL. Unless MCL work is super trivial, I would suggest doing it later after the audio path is in a good shape. Having to work on two paths will likely complicate things. The audio work is blocking Spitzer+EME which is blocking Spitzer+MSE. While the VDAv2 is on hold now...
On 2016/02/01 19:58:54, DaleCurtis wrote:
> I think something like media/filters/android would fit with our existing
> strategy. (I.e. media/audio/{win/android/mac})
media/filters/android sgtm!
On 2016/02/01 20:13:57, xhwang wrote: > Unless MCL work is super trivial, I would suggest doing it later after the audio > path is in a good shape. Having to work on two paths will likely complicate > things. The audio work is blocking Spitzer+EME which is blocking Spitzer+MSE. > While the VDAv2 is on hold now... In an offline conversation with xhwang@ we agreed to postpone refactoring MediaCodecLoop (MCL) out at least till audio path works.
> > While the VDAv2 is on hold now... the refactor and vdav2 aren't dependent on each other, in either direction. it's only about not having the same media codec loop in multiple places. > In an offline conversation with xhwang@ we agreed to postpone refactoring fair enough. however, please do keep in mind that keeping these two paths separate risks parallel maintenance and divergence, so i'd greatly prefer a small value of "postponed" :) . i've opened crbug.com/583082 to capture this. could you add a reference to it in the code? thanks -fl
Description was changed from ========== Added AndroidAudioDecoder AndroidAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface, i.e. it can be attached to the DecoderStream and udes in the Spitzer pipeline directly. It is meant to be the first step towards audio EME in Spitzer. COMPILED BUT NOT TESTED. BUG=542910 ========== to ========== Added AndroidAudioDecoder AndroidAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface, i.e. it can be attached to the DecoderStream and uses in the Spitzer pipeline directly. It is meant to be the first step towards audio EME in Spitzer. BUG=542910 ==========
The major change in this PS is removal most of CDM stuff. Unit tests are not done yer. Please take a look. On 2016/02/01 19:30:55, xhwang wrote: > 3. The CL is a bit too large to review comfortably (maybe only for me :)), > especially since we also want more docs and tests (see below). It'd be nice to > split this CL into multiple smaller CLs. For example, as you pointed out, the > EME part can be in a different CL. Done, see my inline comment. > 4. Can you add a link to a design doc, or some comments that describe a big > picture how the AAD works? Added a link and some more explanation. > 5. Can we add unittests to cover some basic decoding cases? This is not done yet. > 7. (nit) typo in CL description: "udes". liberato@ wrote: > reference to http://crbug.com/583082 Done. https://codereview.chromium.org/1651673002/diff/1/media/base/audio_buffer.h File media/base/audio_buffer.h (right): https://codereview.chromium.org/1651673002/diff/1/media/base/audio_buffer.h#n... media/base/audio_buffer.h:140: uint8_t* interleaved_data() const { return data_.get(); } On 2016/02/01 19:08:17, DaleCurtis wrote: > You can just use channel_data()[0] in this case, seems reasonable to return > data_size(). Removed these methods and added data_size() method instead, I hope this is what you meant? https://codereview.chromium.org/1651673002/diff/1/media/base/decoder_buffer.h File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1651673002/diff/1/media/base/decoder_buffer.h... media/base/decoder_buffer.h:179: std::string AsShortString(); On 2016/02/01 19:30:55, xhwang wrote: > How about just add a "bool verbose" parameter in AsHumanReadableString()? Added a flag instead. AsHumanReadableString(true) will require an explanation, and AsHumanReadableString(kShortFormat) hopefully not. But it is quite verbose for a programmer... https://codereview.chromium.org/1651673002/diff/1/media/filters/android_audio... File media/filters/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/1/media/filters/android_audio... media/filters/android_audio_decoder.h:72: void SetCdm(CdmContext* cdm_context, const CdmAttachedCB& cdm_attached_cb); On 2016/02/01 19:30:55, xhwang wrote: > As you pointed out. Leaving EME part to a separate CL sgtm. > Done, but I kept the NO_KEY related stuff that I feel belongs to the buffer processing (pending input index and its usage, kStateWaitingForKey, OnKeyAdded()). https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:6: #define MEDIA_FILTERS_ANDROID_ANDROID_AUDIO_DECODER_H_ Moved the file into media/filters/android. The ANDROID_ANDROID suggests that we might rename this decoder... https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:37: // d/1YwMZdC-eWe6EkAshSqy8P1b7h2XU469RvbyGPW6EpZs This link points to the Google restricted doc, is this ok?
Description was changed from ========== Added AndroidAudioDecoder AndroidAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface, i.e. it can be attached to the DecoderStream and uses in the Spitzer pipeline directly. It is meant to be the first step towards audio EME in Spitzer. BUG=542910 ========== to ========== Added AndroidAudioDecoder AndroidAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface, i.e. it can be attached to the DecoderStream and used in the Spitzer pipeline directly. It is meant to be the first step towards audio EME in Spitzer. BUG=542910 ==========
Nice work! A few small comments. https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_decode... media/base/audio_decoder_config.h:114: // Modifies sampling rate. Used by decoders based on Android MediaCodec. We don't have setters for any other values; if possible we should avoid introducing one just for sample rate. Can you just create a new config instead? Also for audio we don't support unexpected sample rate changes (i.e. sample rate changes are not supported with src=, only with MSE where a Flush and reconfigure occur), so you might be able to simplify some code here. https://codereview.chromium.org/1651673002/diff/20001/media/renderers/default... File media/renderers/default_renderer_factory.cc (right): https://codereview.chromium.org/1651673002/diff/20001/media/renderers/default... media/renderers/default_renderer_factory.cc:58: #if defined(OS_ANDROID) This should be pushed after FFmpeg/Opus so they are preferred (faster to initialize too). Please remove the DVLOGs from the final patch set too.
Great work! I reviewed everything except the real implementation (on purpose). Most comments are nits and asking for more comments/documentation. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/base/audio... File media/base/audio_buffer.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/base/audio... media/base/audio_buffer.h:136: // the channels. For interleaved formats only only the first element is s/only only/only/ https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/base/audio... media/base/audio_buffer.h:138: const std::vector<uint8_t*>& channel_data() const { return channel_data_; } This could be confusing as a public API. For example, for interleaved formats, will the returned vector actually contain multiple elements? https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/base/decod... File media/base/decoder_buffer.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/base/decod... media/base/decoder_buffer.h:46: // A flag gor AsHumanReadableString() s/gor/for/ https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/base/decod... media/base/decoder_buffer.h:178: std::string AsHumanReadableString(int flags = 0); |flags| is very ambiguous... I would like "bool verbose = false" more. Also that can save an extra enum. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... File media/filters/android/android_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.cc:492: } Add NOTREACHED() here so that we crash even earlier. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... File media/filters/android/android_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:6: #define MEDIA_FILTERS_ANDROID_ANDROID_AUDIO_DECODER_H_ On 2016/02/04 22:59:12, Tima Vaisburd wrote: > Moved the file into media/filters/android. The ANDROID_ANDROID suggests that we > might rename this decoder... *_audio_decoder.h is more media-ish and *_android.h is more clank-ish. Personally I feel this is fine, nobody really reads the guards here.. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:8: #include <deque> add an empty line here https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:33: // encrypted stream and produce the decrypted and decoded output. When encrypted audio streams work, I suppose clear streams will also work. We'd like to be as general as possible (especially when it doesn't involve extra work, which I suppose is the case here). So in this comment we don't really need limit ourselves to encrypted audio streams. Also, it'll actually be harder to test using encrypted stream + widevine since you can't get widevine licenses from unittests. We'll figure that out later. But for now you can at least add unittest for clear streams. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:37: // d/1YwMZdC-eWe6EkAshSqy8P1b7h2XU469RvbyGPW6EpZs On 2016/02/04 22:59:12, Tima Vaisburd wrote: > This link points to the Google restricted doc, is this ok? We'd like to avoid it. Actually, you don't really need this comment. The AAD is a generic class that can be used anywhere for audio decoding (while the sandbox rules allow). For example, someone may want to use it in the browser process some day. We shouldn't care how it will be used. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:37: // d/1YwMZdC-eWe6EkAshSqy8P1b7h2XU469RvbyGPW6EpZs nit: A link that across two lines with "//" and a space in the middle is very hard to use. In this case, consider using a short URL (goo.gl/). https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:41: AndroidAudioDecoder(scoped_refptr<base::SingleThreadTaskRunner> task_runner); explicit https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:41: AndroidAudioDecoder(scoped_refptr<base::SingleThreadTaskRunner> task_runner); in media/ we like to pass scoped_refptr by const-ref to avoid an extra AddRef/Release. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:56: enum State { Can we have comments/doc about how these states work? https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:62: kStateError, Use MACRO_STYLE for enums: https://www.chromium.org/developers/coding-style "Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. In enums that are actually enumerations (i.e. have multiple values), continue to use this style for consistency. Use kConstantNaming when using the "enum hack" to define a single constant, as you would for a const int or the like." https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:83: // Enqueue one pending input buffer into MediaCodec if MediaCodec has a room. nit: here and below, Enqueue_s_ https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:87: // Dequeue all output buffers from MediaCodec that are immediately available. what's the return value? https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:93: void ManageTimer(bool did_work); This seems complicated: "will start the timer" and "may stop the timer". Again I feel we should have some high level doc explaining how things are supposed to work, including IOTask, timer, etc. (BTW, I am looking at the implementation yet on purpose.) https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:104: // Configures |media_codec_| with the given codec parameters from the client. Can we s/given codec parameters from the client/|config_|/? What's the return value? https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:108: void OnDecodedFrame(const OutputBufferInfo* out); We don't typically use const-pointer. Can we use const-ref here? https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:114: static const char* AsString(State state); Can you make this a file-scope function in the .cc file? https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:116: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Is this class single threaded? Will everything run on |task_runner_|? Maybe worth a comment. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:122: using BufferCBPair = std::pair<scoped_refptr<DecoderBuffer>, DecodeCB>; Add include for std::pair: <utility> https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:124: InputQueue input_queue_; Can you comment on why we need to put input buffers in a queue? (e.g. we don't need a queue for other decoders.) https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/filters/an... media/filters/android/android_audio_decoder.h:150: base::TimeTicks most_recent_work_; nit: |most_recent_work_| doesn't sounds like a time. https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/renderers/... File media/renderers/default_renderer_factory.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/renderers/... media/renderers/default_renderer_factory.cc:60: audio_decoders.push_back(new AndroidAudioDecoder(media_task_runner)); The DefaultRendererFactory will be used in the render process side. So here we should push a MojoAudioDecoder. And as Dale mentioned, it should be after other decoders. Note that all other decoders doesn't support encrypted streams, so they will fail immediately and we'll fall back to the MojoAudioDecoder which supports encrypted streams. Since we don't have a MojoAudioDecoder yet, we can make this change later. That being said, this code can be used to test your current CL (with --no-sandbox). https://chromiumcodereview.appspot.com/1651673002/diff/20001/media/renderers/... media/renderers/default_renderer_factory.cc:69: DVLOG(1) << __FUNCTION__ << ": adding OpusAudioDecoder"; nit: It's not very interesting to know what decoders we pushed. I feel it more interesting to know which decoder is selected.... That being said, if you want to list all decoders that has been pushed, we can print it out in DecoderSelector before we start selecting decoders. The code will be something like iterate the vector and call GetDisplayName() on each decoder.
https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_buffer.h File media/base/audio_buffer.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_buffer... media/base/audio_buffer.h:136: // the channels. For interleaved formats only only the first element is On 2016/02/05 21:37:36, xhwang wrote: > s/only only/only/ This is gone. https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_buffer... media/base/audio_buffer.h:138: const std::vector<uint8_t*>& channel_data() const { return channel_data_; } On 2016/02/05 21:37:36, xhwang wrote: > This could be confusing as a public API. For example, for interleaved formats, > will the returned vector actually contain multiple elements? Reformulated. https://codereview.chromium.org/1651673002/diff/20001/media/base/decoder_buff... File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/base/decoder_buff... media/base/decoder_buffer.h:46: // A flag gor AsHumanReadableString() On 2016/02/05 21:37:36, xhwang wrote: > s/gor/for/ Deleted. https://codereview.chromium.org/1651673002/diff/20001/media/base/decoder_buff... media/base/decoder_buffer.h:178: std::string AsHumanReadableString(int flags = 0); On 2016/02/05 21:37:36, xhwang wrote: > |flags| is very ambiguous... I would like "bool verbose = false" more. Also that > can save an extra enum. I humbly disagree. The usage ... << AsHumanReadableString(false) ... would stumble me (why false? what is false? ah, he meant short format..) and even if you remember now that false means short soon you will wonder what this means exactly. AsHumanReadableString(SHORT_FORMAT) is more descriptive. Because of the disagreement let's revert this change, I can deal with long format. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:492: } On 2016/02/05 21:37:37, xhwang wrote: > Add NOTREACHED() here so that we crash even earlier. I think it should not compile if a state is missing. Removed "crash early" comment. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:6: #define MEDIA_FILTERS_ANDROID_ANDROID_AUDIO_DECODER_H_ On 2016/02/05 21:37:37, xhwang wrote: > On 2016/02/04 22:59:12, Tima Vaisburd wrote: > > Moved the file into media/filters/android. The ANDROID_ANDROID suggests that > we > > might rename this decoder... > > *_audio_decoder.h is more media-ish and *_android.h is more clank-ish. > Personally I feel this is fine, nobody really reads the guards here.. Just to clarify: "Android" in the name of the class became redundant since every file in this directory is suppoed to be about Android. MediaCodecAudioDecoder would be more precise, but this name is taken. If you have a better idea of a name please let me know, otherwise keeping as is. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:8: #include <deque> On 2016/02/05 21:37:37, xhwang wrote: > add an empty line here Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:33: // encrypted stream and produce the decrypted and decoded output. On 2016/02/05 21:37:37, xhwang wrote: > When encrypted audio streams work, I suppose clear streams will also work. We'd > like to be as general as possible (especially when it doesn't involve extra > work, which I suppose is the case here). > > So in this comment we don't really need limit ourselves to encrypted audio > streams. Also, it'll actually be harder to test using encrypted stream + > widevine since you can't get widevine licenses from unittests. We'll figure that > out later. But for now you can at least add unittest for clear streams. I did not mean to limit, rather I wanted to say that it can accept encrypted as well as non-encrypted streams. Rewrote the comment a little, please take a look. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:37: // d/1YwMZdC-eWe6EkAshSqy8P1b7h2XU469RvbyGPW6EpZs On 2016/02/05 21:37:37, xhwang wrote: > nit: A link that across two lines with "//" and a space in the middle is very > hard to use. In this case, consider using a short URL (goo.gl/). Acknowledged. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:37: // d/1YwMZdC-eWe6EkAshSqy8P1b7h2XU469RvbyGPW6EpZs On 2016/02/05 21:37:37, xhwang wrote: > On 2016/02/04 22:59:12, Tima Vaisburd wrote: > > This link points to the Google restricted doc, is this ok? > > We'd like to avoid it. > > Actually, you don't really need this comment. The AAD is a generic class that > can be used anywhere for audio decoding (while the sandbox rules allow). For > example, someone may want to use it in the browser process some day. We > shouldn't care how it will be used. Removed https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:41: AndroidAudioDecoder(scoped_refptr<base::SingleThreadTaskRunner> task_runner); On 2016/02/05 21:37:37, xhwang wrote: > explicit Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:41: AndroidAudioDecoder(scoped_refptr<base::SingleThreadTaskRunner> task_runner); On 2016/02/05 21:37:37, xhwang wrote: > in media/ we like to pass scoped_refptr by const-ref to avoid an extra > AddRef/Release. Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:62: kStateError, On 2016/02/05 21:37:37, xhwang wrote: > Use MACRO_STYLE for enums: > > https://www.chromium.org/developers/coding-style > > "Though the Google C++ Style Guide now says to use kConstantNaming for enums, > Chromium was written using MACRO_STYLE naming. In enums that are actually > enumerations (i.e. have multiple values), continue to use this style for > consistency. Use kConstantNaming when using the "enum hack" to define a single > constant, as you would for a const int or the like." This rationale argues for deviation from the Google style guide for the sake of consistency, but the decoders in this very directory are already using kConstantNaming: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vpx_... https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr... It seems I have followed an established convention. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:83: // Enqueue one pending input buffer into MediaCodec if MediaCodec has a room. On 2016/02/05 21:37:37, xhwang wrote: > nit: here and below, Enqueue_s_ Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:87: // Dequeue all output buffers from MediaCodec that are immediately available. On 2016/02/05 21:37:37, xhwang wrote: > what's the return value? Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:93: void ManageTimer(bool did_work); On 2016/02/05 21:37:37, xhwang wrote: > This seems complicated: "will start the timer" and "may stop the timer". > > Again I feel we should have some high level doc explaining how things are > supposed to work, including IOTask, timer, etc. > > (BTW, I am looking at the implementation yet on purpose.) Rewrote the explanation. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:104: // Configures |media_codec_| with the given codec parameters from the client. On 2016/02/05 21:37:37, xhwang wrote: > Can we s/given codec parameters from the client/|config_|/? > > What's the return value? Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:108: void OnDecodedFrame(const OutputBufferInfo* out); On 2016/02/05 21:37:37, xhwang wrote: > We don't typically use const-pointer. Can we use const-ref here? Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:114: static const char* AsString(State state); On 2016/02/05 21:37:37, xhwang wrote: > Can you make this a file-scope function in the .cc file? Wouldn't it require making the State public? https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:122: using BufferCBPair = std::pair<scoped_refptr<DecoderBuffer>, DecodeCB>; On 2016/02/05 21:37:37, xhwang wrote: > Add include for std::pair: <utility> Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:124: InputQueue input_queue_; On 2016/02/05 21:37:37, xhwang wrote: > Can you comment on why we need to put input buffers in a queue? (e.g. we don't > need a queue for other decoders.) Done. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:150: base::TimeTicks most_recent_work_; On 2016/02/05 21:37:37, xhwang wrote: > nit: |most_recent_work_| doesn't sounds like a time. s/most_recent_work_/idle_time_begin_/ https://codereview.chromium.org/1651673002/diff/20001/media/renderers/default... File media/renderers/default_renderer_factory.cc (right): https://codereview.chromium.org/1651673002/diff/20001/media/renderers/default... media/renderers/default_renderer_factory.cc:58: #if defined(OS_ANDROID) On 2016/02/04 23:15:31, DaleCurtis wrote: > This should be pushed after FFmpeg/Opus so they are preferred (faster to > initialize too). Please remove the DVLOGs from the final patch set too. I just forgot to remove debugging code. I'm keeping the call to AndroidAudioDecoder for now, but it has to be replaced with MojoAudioDecoder later, as Xiaohan said. https://codereview.chromium.org/1651673002/diff/20001/media/renderers/default... media/renderers/default_renderer_factory.cc:60: audio_decoders.push_back(new AndroidAudioDecoder(media_task_runner)); On 2016/02/05 21:37:37, xhwang wrote: > The DefaultRendererFactory will be used in the render process side. So here we > should push a MojoAudioDecoder. And as Dale mentioned, it should be after other > decoders. Note that all other decoders doesn't support encrypted streams, so > they will fail immediately and we'll fall back to the MojoAudioDecoder which > supports encrypted streams. > > Since we don't have a MojoAudioDecoder yet, we can make this change later. > > That being said, this code can be used to test your current CL (with > --no-sandbox). Acknowledged. https://codereview.chromium.org/1651673002/diff/20001/media/renderers/default... media/renderers/default_renderer_factory.cc:69: DVLOG(1) << __FUNCTION__ << ": adding OpusAudioDecoder"; On 2016/02/05 21:37:37, xhwang wrote: > nit: It's not very interesting to know what decoders we pushed. I feel it more > interesting to know which decoder is selected.... > > That being said, if you want to list all decoders that has been pushed, we can > print it out in DecoderSelector before we start selecting decoders. The code > will be something like iterate the vector and call GetDisplayName() on each > decoder. > As I said before, I just forgot to remove the debugging code. Deleted now.
sorry for the delay, meant to send this friday afternoon. lgtm. -fl https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:158: base::TimeTicks idle_time_begin_; i apologize, i didn't press send on friday about this. please remember that this is common code from avda, and will be merged back. comment changes make sense, but at least comment that this is renamed from the old variable. also, that means that many of the bad naming / comments in this file are my doing, so don't blame tima. :)
I stopped at AndroidAudioDecoder::Decode(). Some comments are on code that you copied from existing files. But since this is a new file, I'd like to make it as good as we can (and don't let existing issues spread). As you can see the comments start to grow. I strongly recommend you to land non-AndroidAudioDecoder-implementation changes in a CL first, then we can iterate on the detailed implementation. Some comments are in PS2. https://codereview.chromium.org/1651673002/diff/20001/media/base/decoder_buff... File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/base/decoder_buff... media/base/decoder_buffer.h:178: std::string AsHumanReadableString(int flags = 0); On 2016/02/06 03:54:11, Tima Vaisburd wrote: > On 2016/02/05 21:37:36, xhwang wrote: > > |flags| is very ambiguous... I would like "bool verbose = false" more. Also > that > > can save an extra enum. > > I humbly disagree. The usage ... << AsHumanReadableString(false) ... would > stumble me (why false? what is false? ah, he meant short format..) and even if > you remember now that false means short soon you will wonder what this means > exactly. AsHumanReadableString(SHORT_FORMAT) is more descriptive. > > Because of the disagreement let's revert this change, I can deal with long > format. Good point. But I still feel AsHumanReadableString(DecoderBuffer::kShortFormat) is too long. How about we have two functions ToVerboseString() and ToString()? I think you had something like this before. Sorry for going back and forth on this. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:492: } On 2016/02/06 03:54:11, Tima Vaisburd wrote: > On 2016/02/05 21:37:37, xhwang wrote: > > Add NOTREACHED() here so that we crash even earlier. > > I think it should not compile if a state is missing. Removed "crash early" > comment. A NOTREACHED() here is still valuable and we have a lot of examples doing this, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/dbus/message.cc&l=89 https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:6: #define MEDIA_FILTERS_ANDROID_ANDROID_AUDIO_DECODER_H_ On 2016/02/06 03:54:12, Tima Vaisburd wrote: > On 2016/02/05 21:37:37, xhwang wrote: > > On 2016/02/04 22:59:12, Tima Vaisburd wrote: > > > Moved the file into media/filters/android. The ANDROID_ANDROID suggests that > > we > > > might rename this decoder... > > > > *_audio_decoder.h is more media-ish and *_android.h is more clank-ish. > > Personally I feel this is fine, nobody really reads the guards here.. > > Just to clarify: "Android" in the name of the class became redundant since every > file in this directory is suppoed to be about Android. MediaCodecAudioDecoder > would be more precise, but this name is taken. If you have a better idea of a > name please let me know, otherwise keeping as is. MediaCodecAudioDecoder does sound a better name. The existing MediaCodecAudioDecoder should really be AudioMediaCodecDecoder since it's a MediaCodecDecoder. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:33: // encrypted stream and produce the decrypted and decoded output. On 2016/02/06 03:54:12, Tima Vaisburd wrote: > On 2016/02/05 21:37:37, xhwang wrote: > > When encrypted audio streams work, I suppose clear streams will also work. > We'd > > like to be as general as possible (especially when it doesn't involve extra > > work, which I suppose is the case here). > > > > So in this comment we don't really need limit ourselves to encrypted audio > > streams. Also, it'll actually be harder to test using encrypted stream + > > widevine since you can't get widevine licenses from unittests. We'll figure > that > > out later. But for now you can at least add unittest for clear streams. > > I did not mean to limit, rather I wanted to say that it can accept encrypted as > well as non-encrypted streams. Rewrote the comment a little, please take a look. I guess "this decoder is a decrypting decoder" is a bit ambiguous to me. Mainly because we already have a DecryptingAudioDecoder which only accepts encrypted streams. Can we somehow make the comment more explicit, e.g. This decoder can handle clear and encrypted audio streams. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:56: enum State { On 2016/02/05 21:37:37, xhwang wrote: > Can we have comments/doc about how these states work? This is not done :) I'll continue to review the implementation after learning how the code is supposed to work. https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.h:62: kStateError, On 2016/02/06 03:54:11, Tima Vaisburd wrote: > On 2016/02/05 21:37:37, xhwang wrote: > > Use MACRO_STYLE for enums: > > > > https://www.chromium.org/developers/coding-style > > > > "Though the Google C++ Style Guide now says to use kConstantNaming for enums, > > Chromium was written using MACRO_STYLE naming. In enums that are actually > > enumerations (i.e. have multiple values), continue to use this style for > > consistency. Use kConstantNaming when using the "enum hack" to define a > single > > constant, as you would for a const int or the like." > > This rationale argues for deviation from the Google style guide for the sake of > consistency, but the decoders in this very directory are already using > kConstantNaming: > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vpx_... > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr... > > It seems I have followed an established convention. Once upon a time kCamelCase was acceptable for enums and that's why you see them in these "old" decoders. These code should be updated but nobody have the time to do it. All new file should follow the style guide. Some new code is already using the UPPER_CASE style: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/deco... https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/fake... https://codereview.chromium.org/1651673002/diff/60001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1651673002/diff/60001/media/base/audio_decode... media/base/audio_decoder_config.h:115: void set_samples_per_second(int value) { samples_per_second_ = value; } Please see Dales' comment on this. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:12: no empty line here https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:23: const int64_t kNanoMultiplier = 1000LL * 1000 * 1000; You can just use kNanosecondsPerSecond: https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:77: // Keep this consistent with AudioCodecBridge. Probably not necessary. AudioCodecBridge will go away in the future and it's not needed to be consistent with it. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:86: In this CL, if config is encrypted, we should fail initialization. Then in the next CL you can remove this check. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:92: init_cb_ = bound_init_cb; |init_cb_| will be always be fired in this call. So it seems we don't need it as a member variable. Then it seems we don't need bound_init_cb either. We can just PostTask(init_cb). https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:93: output_cb_ = BindToCurrentLoop(output_cb); Will |output_cb_| be fired on other threads? If not, we probably don't need an extra BindToCurrentLoop. Also see the comment around Decode(). https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:95: const bool success = ConfigureMediaCodec(); ddorwin@ and I talked about this before (the same code pattern in FFmpegAudioDecoder [1]), and we felt that it would be cleaner to set the member variables (e.g. config_, channel_count_ etc) after the initialization succeeds. In this particular case, e.g. we could be in an kStateUninitialized state (line 97) while having config_ set. How about having ConfigureMediaCodec() be a non-member function like: // Returns null if configuration failed. scoped_ptr<AudioCodecBridge> ConfigureMediaCodec(const AudioDecoderConfig& config); Then we can move line 87-93 after this configuration call. In line 144, we can call ConfigureMediaCodec(config_); [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:103: DecodeCB bound_decode_cb = BindToCurrentLoop(decode_cb); We BindToCurrentLoop for two reasons: 1. Make sure the callback is fired on the correct thread. 2. Avoid reentering the caller in the same callstack by force posting the callback. It's very convenient to use, however it always involve an extra post, even if we are already on the same thread. In this case, it seems to me AAD is single threaded, so (1) isn't a concern. The only case we can reenter the caller in the same callstack is the DoIOTask() call on line 116. Instead of calling DoIOTask() directly, we can probably PostTask(base::Bind(&DoIOTask)). The reason I *think* this is better is that when DoIOTask() is called in other cases (e.g. OnKeyAdded, timer fired), we don't force post the decoder and output callbacks, which seems unnecessary. p.s. line 108 can be a PostTask. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:104: DCHECK that we are not in kStateUninitialized state. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:105: if (state_ == kStateError) { In what case can this happen? If an error has been reported to the pipeline, then Decode() should never be called again. It seems this could happen after line 352, but without knowing how the state machine is supposed to work I am not sure. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:394: media_codec_.reset(AudioCodecBridge::Create(config_.codec())); OOC and not related to this CL: why AudioCodecBridge::Create() is returning a raw pointer instead of a scoped_ptr<>? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:419: << " play audio:" << play_audio << " media_crypto:" << media_crypto; Shouldn't this log be a part of AudioCodecBridge::ConfigureAndStart()? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:423: &config_.extra_data()[0], config_.extra_data().size(), codec_delay_ns, &config_.extra_data()[0] is not safe if extra_data is empty. Use config_.extra_data().data() instead. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:149: DecodeCB eos_decode_cb_; Is it possible to replace this with a (null, DecodeCB) in |input_queue_|? https://codereview.chromium.org/1651673002/diff/60001/media/renderers/default... File media/renderers/default_renderer_factory.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/renderers/default... media/renderers/default_renderer_factory.cc:68: #endif Thanks! This probably should not be checked in because it will not work in the render process. But feel free to use this for local test.
On 2016/02/08 20:03:35, xhwang wrote: > I stopped at AndroidAudioDecoder::Decode(). Some comments are on code that you > copied from existing files. But since this is a new file, I'd like to make it as > good as we can (and don't let existing issues spread). > > As you can see the comments start to grow. I strongly recommend you to land > non-AndroidAudioDecoder-implementation changes in a CL first, then we can > iterate on the detailed implementation. Do you mean keeping the header (android_audio_decoder.h) here but have a stub implementation in android_audio_decoder.cc, and have a second CL that fills the implementation in?
On 2016/02/08 20:25:56, Tima Vaisburd wrote: > On 2016/02/08 20:03:35, xhwang wrote: > > I stopped at AndroidAudioDecoder::Decode(). Some comments are on code that you > > copied from existing files. But since this is a new file, I'd like to make it > as > > good as we can (and don't let existing issues spread). > > > > As you can see the comments start to grow. I strongly recommend you to land > > non-AndroidAudioDecoder-implementation changes in a CL first, then we can > > iterate on the detailed implementation. > > Do you mean keeping the header (android_audio_decoder.h) here but have a stub > implementation in android_audio_decoder.cc, and have a second CL that fills the > implementation in? Yes, so in the next CL we have only one file to iterate on.
On 2016/02/08 21:02:20, xhwang wrote: > On 2016/02/08 20:25:56, Tima Vaisburd wrote: > > On 2016/02/08 20:03:35, xhwang wrote: > > > I stopped at AndroidAudioDecoder::Decode(). Some comments are on code that > you > > > copied from existing files. But since this is a new file, I'd like to make > it > > as > > > good as we can (and don't let existing issues spread). > > > > > > As you can see the comments start to grow. I strongly recommend you to land > > > non-AndroidAudioDecoder-implementation changes in a CL first, then we can > > > iterate on the detailed implementation. > > > > Do you mean keeping the header (android_audio_decoder.h) here but have a stub > > implementation in android_audio_decoder.cc, and have a second CL that fills > the > > implementation in? > > Yes, so in the next CL we have only one file to iterate on. Correction: in the next CL we'll only have AAD.h, AAD.cc and AAD_unittest.cc.
https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:60: for (const auto& entry : input_queue_) { nit: no {} needed https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:128: for (const auto& entry : input_queue_) { ditto https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:17: #include "media/base/android/media_drm_bridge.h" do we need this here? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:21: #include "media/base/media_export.h" is this going to be exported?
Description was changed from ========== Added AndroidAudioDecoder AndroidAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface, i.e. it can be attached to the DecoderStream and used in the Spitzer pipeline directly. It is meant to be the first step towards audio EME in Spitzer. BUG=542910 ========== to ========== Add MediaCodecAudioDecoder implementation MediaCodecAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface. This CL adds the implementation. BUG=542910 ==========
I tried to address all comments that are here so far, either with a change or with a question. If I missed some, do let me know. The unit tests are still missing. PTAL again. https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1651673002/diff/20001/media/base/audio_decode... media/base/audio_decoder_config.h:114: // Modifies sampling rate. Used by decoders based on Android MediaCodec. On 2016/02/04 23:15:31, DaleCurtis wrote: > We don't have setters for any other values; if possible we should avoid > introducing one just for sample rate. Can you just create a new config instead? > > Also for audio we don't support unexpected sample rate changes (i.e. sample rate > changes are not supported with src=, only with MSE where a Flush and reconfigure > occur), so you might be able to simplify some code here. Changed the code not to support sample rate changes on the fly. Thus the method set_samples_per_second() is also gone. Will the pipeline destroy and recreate the decoder in case of MSE sample rate change? https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/20001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:492: } On 2016/02/08 20:03:33, xhwang wrote: > On 2016/02/06 03:54:11, Tima Vaisburd wrote: > > On 2016/02/05 21:37:37, xhwang wrote: > > > Add NOTREACHED() here so that we crash even earlier. > > > > I think it should not compile if a state is missing. Removed "crash early" > > comment. > > A NOTREACHED() here is still valuable and we have a lot of examples doing this, > e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/dbus/message.cc&l=89 Done. https://codereview.chromium.org/1651673002/diff/60001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/1651673002/diff/60001/media/base/audio_decode... media/base/audio_decoder_config.h:115: void set_samples_per_second(int value) { samples_per_second_ = value; } On 2016/02/08 20:03:34, xhwang wrote: > Please see Dales' comment on this. Removed. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:12: On 2016/02/08 20:03:34, xhwang wrote: > no empty line here Done. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:23: const int64_t kNanoMultiplier = 1000LL * 1000 * 1000; On 2016/02/08 20:03:34, xhwang wrote: > You can just use kNanosecondsPerSecond: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... Used in sdk_media_codec_bridge.cc now. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:60: for (const auto& entry : input_queue_) { On 2016/02/09 00:31:56, qinmin wrote: > nit: no {} needed Done. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:77: // Keep this consistent with AudioCodecBridge. On 2016/02/08 20:03:34, xhwang wrote: > Probably not necessary. AudioCodecBridge will go away in the future and it's not > needed to be consistent with it. I changed the comment to "We can support only the codecs that AudioCodecBridge can decode". I think as long as we use AudioCodecBridge it makes sense? OOC: why will it go away? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:86: On 2016/02/08 20:03:34, xhwang wrote: > In this CL, if config is encrypted, we should fail initialization. Then in the > next CL you can remove this check. Done. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:92: init_cb_ = bound_init_cb; On 2016/02/08 20:03:34, xhwang wrote: > |init_cb_| will be always be fired in this call. So it seems we don't need it as > a member variable. Then it seems we don't need bound_init_cb either. We can just > PostTask(init_cb). I removed |init_cb_|, but not BindToCurrentLoop(). Both OpusAudioDecoder and FFmpegAudioDecoder do this, I think because they care not to call the |init_cb| from Initialize(). Is it equivalent to PostTask? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:93: output_cb_ = BindToCurrentLoop(output_cb); On 2016/02/08 20:03:34, xhwang wrote: > Will |output_cb_| be fired on other threads? If not, we probably don't need an > extra BindToCurrentLoop. Also see the comment around Decode(). Yes, we do not need BindToCurrentLoop for output_cb_, strictly speaking. I wrote it that way to make the timing of the DoIOTask() independent of how long is |output_cb_|. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:103: DecodeCB bound_decode_cb = BindToCurrentLoop(decode_cb); On 2016/02/08 20:03:34, xhwang wrote: > We BindToCurrentLoop for two reasons: > > 1. Make sure the callback is fired on the correct thread. > 2. Avoid reentering the caller in the same callstack by force posting the > callback. > > It's very convenient to use, however it always involve an extra post, even if we > are already on the same thread. > > In this case, it seems to me AAD is single threaded, so (1) isn't a concern. > > The only case we can reenter the caller in the same callstack is the DoIOTask() > call on line 116. Instead of calling DoIOTask() directly, we can probably > PostTask(base::Bind(&DoIOTask)). > > The reason I *think* this is better is that when DoIOTask() is called in other > cases (e.g. OnKeyAdded, timer fired), we don't force post the decoder and output > callbacks, which seems unnecessary. > > p.s. line 108 can be a PostTask. Yes, (1) is not a concern. Just (2), and, as I said above, it felt safer not to call a callback which duration I cannot control directly from DoIOTask(). Still, I do not (yet?) see a benefit of moving the extra post from DecodeCB to Decode(). In my understanding OnKeyAdded cases can be neglected. After that the number of Decode() calls should be the same as the DecodeCB calls. Am I missing something? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:104: On 2016/02/08 20:03:34, xhwang wrote: > DCHECK that we are not in kStateUninitialized state. It seems only STATE_READY is possible here. Added that. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:105: if (state_ == kStateError) { On 2016/02/08 20:03:34, xhwang wrote: > In what case can this happen? If an error has been reported to the pipeline, > then Decode() should never be called again. It seems this could happen after > line 352, but without knowing how the state machine is supposed to work I am not > sure. Yes, this is exactly right. If MediaCodec error happens in DequeueOutput() or Reset() only the next Decode will report the error. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:128: for (const auto& entry : input_queue_) { On 2016/02/09 00:31:56, qinmin wrote: > ditto Done. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:394: media_codec_.reset(AudioCodecBridge::Create(config_.codec())); On 2016/02/08 20:03:34, xhwang wrote: > OOC and not related to this CL: why AudioCodecBridge::Create() is returning a > raw pointer instead of a scoped_ptr<>? I do not know exactly. Maybe because media_codec_.reset(AudioCodecBridge::Create()) looks better than media_codec_ = AudioCodecBridge::Create().Pass() ? We could do the same if CreateMediaCodec(config) returned a raw pointer. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:419: << " play audio:" << play_audio << " media_crypto:" << media_crypto; On 2016/02/08 20:03:34, xhwang wrote: > Shouldn't this log be a part of AudioCodecBridge::ConfigureAndStart()? Done. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:423: &config_.extra_data()[0], config_.extra_data().size(), codec_delay_ns, On 2016/02/08 20:03:34, xhwang wrote: > &config_.extra_data()[0] is not safe if extra_data is empty. Use > config_.extra_data().data() instead. Done. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:17: #include "media/base/android/media_drm_bridge.h" On 2016/02/09 00:31:56, qinmin wrote: > do we need this here? Removed. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:21: #include "media/base/media_export.h" On 2016/02/09 00:31:56, qinmin wrote: > is this going to be exported? I think not, removed. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.h:149: DecodeCB eos_decode_cb_; On 2016/02/08 20:03:34, xhwang wrote: > Is it possible to replace this with a (null, DecodeCB) in |input_queue_|? It seems there is no need to remove the pair from the |input_queue_| upon QueueEOS(), it can be done later when EOS comes out in the output buffer. So this callback seems not needed at all. Removed. https://codereview.chromium.org/1651673002/diff/60001/media/renderers/default... File media/renderers/default_renderer_factory.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/renderers/default... media/renderers/default_renderer_factory.cc:68: #endif On 2016/02/08 20:03:35, xhwang wrote: > Thanks! > > This probably should not be checked in because it will not work in the render > process. But feel free to use this for local test. All changes to this file are removed. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:61: // The failures in both steps are normal and they happen periodically since This is where I tried to clarify the timer function.
Looking pretty good. Just have some comments, mostly nits. There are a few comments in the old PS (replies). Since this CL is getting large and nobody is using it yet. I think if fine to land the implementation first, then we work on a CL that add unittests immediately after it. WDYT? https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:77: // Keep this consistent with AudioCodecBridge. On 2016/02/11 20:23:23, Tima Vaisburd wrote: > On 2016/02/08 20:03:34, xhwang wrote: > > Probably not necessary. AudioCodecBridge will go away in the future and it's > not > > needed to be consistent with it. > > I changed the comment to "We can support only the codecs that AudioCodecBridge > can decode". I think as long as we use AudioCodecBridge it makes sense? > OOC: why will it go away? Sorry, I was wrong :) Apparently you are using it here. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:92: init_cb_ = bound_init_cb; On 2016/02/11 20:23:22, Tima Vaisburd wrote: > On 2016/02/08 20:03:34, xhwang wrote: > > |init_cb_| will be always be fired in this call. So it seems we don't need it > as > > a member variable. Then it seems we don't need bound_init_cb either. We can > just > > PostTask(init_cb). > > I removed |init_cb_|, but not BindToCurrentLoop(). Both OpusAudioDecoder and > FFmpegAudioDecoder do this, I think because they care not to call the |init_cb| > from Initialize(). Is it equivalent to PostTask? Yes, it's just an easier way to PostTask. https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:103: DecodeCB bound_decode_cb = BindToCurrentLoop(decode_cb); On 2016/02/11 20:23:23, Tima Vaisburd wrote: > On 2016/02/08 20:03:34, xhwang wrote: > > We BindToCurrentLoop for two reasons: > > > > 1. Make sure the callback is fired on the correct thread. > > 2. Avoid reentering the caller in the same callstack by force posting the > > callback. > > > > It's very convenient to use, however it always involve an extra post, even if > we > > are already on the same thread. > > > > In this case, it seems to me AAD is single threaded, so (1) isn't a concern. > > > > The only case we can reenter the caller in the same callstack is the > DoIOTask() > > call on line 116. Instead of calling DoIOTask() directly, we can probably > > PostTask(base::Bind(&DoIOTask)). > > > > The reason I *think* this is better is that when DoIOTask() is called in other > > cases (e.g. OnKeyAdded, timer fired), we don't force post the decoder and > output > > callbacks, which seems unnecessary. > > > > p.s. line 108 can be a PostTask. > > Yes, (1) is not a concern. Just (2), and, as I said above, it felt safer not to > call a callback which duration I cannot control directly from DoIOTask(). > > Still, I do not (yet?) see a benefit of moving the extra post from DecodeCB to > Decode(). In my understanding OnKeyAdded cases can be neglected. After that the > number of Decode() calls should be the same as the DecodeCB calls. Am I missing > something? If you bind the callback, it will always do a post, which is not necessary if you know it's not going to have reentrency issues. For example, all the decoding work happens in DoIOTask(), which can be called in 3 cases: 1. In Decode(). 2. In OnKeyAdded() 3. Fired by the timer. Only in (1) we could have the reentrency issue, that's why I suggest a post there. I agreed that the number of Decode() calls is the same as the DecodeCB calls. But then you don't need to post all OutputCB, which could save us a bit cycles... https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... File media/base/android/sdk_media_codec_bridge.cc (right): https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.cc:381: << " play audio:" << play_audio << " media_crypto:" << media_crypto; nit: If you don't care about this overloaded version, you can put the log at line 351, and use config.AsHumanReadableString(). https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.cc:508: } Is this for local test only? https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... File media/base/android/sdk_media_codec_bridge.h (right): https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.h:109: bool play_audio, What is "play_audio" for? Add a comment? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:44: scoped_ptr<MediaCodecBridge> media_codec( You can have "scoped_ptr<AudioCodecBridge> audio_codec_bridge" here. Then you don't need to cast on line 51. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:82: media_codec_.reset(); Is this needed? Can we just let the scoped_ptr dtor do the work? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:119: media_codec_ = std::move(CreateMediaCodec(config)); Will it work if you remove std::move? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:167: base::android::BuildInfo::GetInstance()->sdk_int() < 18) { Why we only Reset/Flush when SDK < 18? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:233: decode_cb.Run(kDecodeError); Do you want to clear |input_queue_| and fire all pending decode callbacks instead? If you do that, you actually don't need line 214 and line 215. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:243: DCHECK_NE(input_buf_index, -1); QueueInput() is too long. Can you wrap line 217-243 to a DequeueInputBuffer() method? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:244: See above. Move line 214 here. You don't need line 215 until you get an output successfully. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:256: DCHECK(!decoder_buffer->end_of_stream()); Not needed given line 245 and 252? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:311: break; Should we go to STATE_ERROR and bail out? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:312: } ditto, wrap this into a QueueInputBuffer() method? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:315: // |decode_cb|", we call |decode_cb| when the buffer is accepted by The comment actually means that once the buffer is accepted by the decoder. We can update audio_decoder.h to make this more clear. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:326: DCHECK(media_codec_); Not needed. If this fires we'll just crash on line 332. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:330: bool work_done = false; use |did_work| to be consistent? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:362: input_queue_ should be empty now? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:365: SetState(STATE_DRAINED); The switch/case block is too long. Wrap this block to OnEosFrame()? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:403: if (now - idle_time_begin_ > IdleTimerTimeOut()) The initial value of idle_time_begin_ would be 0. So if for the first decode we didn't do any work, |now - idle_time_begin_| could be pretty large. Then we'll not start the timer. Is this expected? Maybe instead of checking !did_work, we should check if (!did_work && !idle_time_begin_) https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:426: DCHECK(media_codec_); nit: usually we DVLOG first, then DCHECK. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:61: // The failures in both steps are normal and they happen periodically since On 2016/02/11 20:23:23, Tima Vaisburd wrote: > This is where I tried to clarify the timer function. Acknowledged. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:101: class MediaCodecAudioDecoder : public AudioDecoder { You'll need MEDIA_EXPORT when you add unittest. I am not sure about Android though.
https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... File media/filters/android/android_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/60001/media/filters/android/a... media/filters/android/android_audio_decoder.cc:103: DecodeCB bound_decode_cb = BindToCurrentLoop(decode_cb); On 2016/02/12 10:15:04, xhwang wrote: > On 2016/02/11 20:23:23, Tima Vaisburd wrote: > > On 2016/02/08 20:03:34, xhwang wrote: > > > We BindToCurrentLoop for two reasons: > > > > > > 1. Make sure the callback is fired on the correct thread. > > > 2. Avoid reentering the caller in the same callstack by force posting the > > > callback. > > > > > > It's very convenient to use, however it always involve an extra post, even > if > > we > > > are already on the same thread. > > > > > > In this case, it seems to me AAD is single threaded, so (1) isn't a concern. > > > > > > The only case we can reenter the caller in the same callstack is the > > DoIOTask() > > > call on line 116. Instead of calling DoIOTask() directly, we can probably > > > PostTask(base::Bind(&DoIOTask)). > > > > > > The reason I *think* this is better is that when DoIOTask() is called in > other > > > cases (e.g. OnKeyAdded, timer fired), we don't force post the decoder and > > output > > > callbacks, which seems unnecessary. > > > > > > p.s. line 108 can be a PostTask. > > > > Yes, (1) is not a concern. Just (2), and, as I said above, it felt safer not > to > > call a callback which duration I cannot control directly from DoIOTask(). > > > > Still, I do not (yet?) see a benefit of moving the extra post from DecodeCB to > > Decode(). In my understanding OnKeyAdded cases can be neglected. After that > the > > number of Decode() calls should be the same as the DecodeCB calls. Am I > missing > > something? > > If you bind the callback, it will always do a post, which is not necessary if > you know it's not going to have reentrency issues. > > For example, all the decoding work happens in DoIOTask(), which can be called in > 3 cases: > 1. In Decode(). > 2. In OnKeyAdded() > 3. Fired by the timer. > > Only in (1) we could have the reentrency issue, that's why I suggest a post > there. > > I agreed that the number of Decode() calls is the same as the DecodeCB calls. > But then you don't need to post all OutputCB, which could save us a bit > cycles... I unbound |output_cb_|, but kept |bound_decode_cb|, and deleted |weak_factory_|. Let me know what you think. https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... File media/base/android/sdk_media_codec_bridge.cc (right): https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.cc:381: << " play audio:" << play_audio << " media_crypto:" << media_crypto; On 2016/02/12 10:15:04, xhwang wrote: > nit: If you don't care about this overloaded version, you can put the log at > line 351, and use config.AsHumanReadableString(). I wanted to see that conversion to nanoseconds is done right. I think it is nice to have the log like this for every code path. https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.cc:508: } On 2016/02/12 10:15:04, xhwang wrote: > Is this for local test only? Oops! Reverted. https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... File media/base/android/sdk_media_codec_bridge.h (right): https://codereview.chromium.org/1651673002/diff/100001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.h:109: bool play_audio, On 2016/02/12 10:15:04, xhwang wrote: > What is "play_audio" for? Add a comment? Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:44: scoped_ptr<MediaCodecBridge> media_codec( On 2016/02/12 10:15:05, xhwang wrote: > You can have "scoped_ptr<AudioCodecBridge> audio_codec_bridge" here. Then you > don't need to cast on line 51. How to return scoped_ptr<MediaCodecBridge> on l.65? My compilation complains that it cannot convert scoped_ptr<Audio> into scoped_ptr<Media>, although Audio is derived from Media. Maybe we can just return raw pointer? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:82: media_codec_.reset(); On 2016/02/12 10:15:05, xhwang wrote: > Is this needed? Can we just let the scoped_ptr dtor do the work? As far as I understand this is not needed right now because there is no specific need to destroy MediaCodec before any other field. I would still prefer to keep explicit resets, I find myself searching for it when looking into various bugs as MediaCodec is quite fragile. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:119: media_codec_ = std::move(CreateMediaCodec(config)); On 2016/02/12 10:15:05, xhwang wrote: > Will it work if you remove std::move? Yes! Unnamed temporary? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:167: base::android::BuildInfo::GetInstance()->sdk_int() < 18) { On 2016/02/12 10:15:05, xhwang wrote: > Why we only Reset/Flush when SDK < 18? Bug! Thank you, should be SDK >= 18. I updated the comment as well. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:233: decode_cb.Run(kDecodeError); On 2016/02/12 10:15:04, xhwang wrote: > Do you want to clear |input_queue_| and fire all pending decode callbacks > instead? > > If you do that, you actually don't need line 214 and line 215. I created DequeueInputBuffer() and EnqueueInputBuffer(), but slightly differently than you've suggested. Please take a look. Right now the code is quite conservative in removing the stuff from |input_queue_|. This way potentially the caller might try to recover with Reset() and continue. On the other hand, as far as I know no one is doing that, and the errors from DequeueOutput() would require special handling. I prefer to keep it as is right now and revisit at the time of a planned common part refactoring, but let me know what you think. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:243: DCHECK_NE(input_buf_index, -1); On 2016/02/12 10:15:04, xhwang wrote: > QueueInput() is too long. Can you wrap line 217-243 to a DequeueInputBuffer() > method? See my comment above. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:244: On 2016/02/12 10:15:05, xhwang wrote: > See above. Move line 214 here. > > You don't need line 215 until you get an output successfully. See my comment above. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:256: DCHECK(!decoder_buffer->end_of_stream()); On 2016/02/12 10:15:05, xhwang wrote: > Not needed given line 245 and 252? Removed. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:311: break; On 2016/02/12 10:15:05, xhwang wrote: > Should we go to STATE_ERROR and bail out? Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:312: } On 2016/02/12 10:15:05, xhwang wrote: > ditto, wrap this into a QueueInputBuffer() method? Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:315: // |decode_cb|", we call |decode_cb| when the buffer is accepted by On 2016/02/12 10:15:05, xhwang wrote: > The comment actually means that once the buffer is accepted by the decoder. We > can update audio_decoder.h to make this more clear. Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:326: DCHECK(media_codec_); On 2016/02/12 10:15:04, xhwang wrote: > Not needed. If this fires we'll just crash on line 332. Done. I've always wanted to ask though: there are various precondition DCHECKs, including pointer testing. They would produce helpful debug message if violated. Why this particular kind is redundant? https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:330: bool work_done = false; On 2016/02/12 10:15:05, xhwang wrote: > use |did_work| to be consistent? Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:362: On 2016/02/12 10:15:05, xhwang wrote: > input_queue_ should be empty now? For this audio decoder probably yes, but in general case we can have input buffers after EOS. AVDA, for instance, uses artificial EOSes diring the resolution change. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:365: SetState(STATE_DRAINED); On 2016/02/12 10:15:05, xhwang wrote: > The switch/case block is too long. Wrap this block to OnEosFrame()? Yes, I called OnDecodedEos() and renamed OnDecoderFrame() -> OnDecodedContentFrame() to stress the decoded side. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:403: if (now - idle_time_begin_ > IdleTimerTimeOut()) On 2016/02/12 10:15:05, xhwang wrote: > The initial value of idle_time_begin_ would be 0. So if for the first decode we > didn't do any work, |now - idle_time_begin_| could be pretty large. Then we'll > not start the timer. Is this expected? > > Maybe instead of checking !did_work, we should check > > if (!did_work && !idle_time_begin_) I think you mean if (!did_work && idle_time_begin_) Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:426: DCHECK(media_codec_); On 2016/02/12 10:15:05, xhwang wrote: > nit: usually we DVLOG first, then DCHECK. My bad! Done. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:101: class MediaCodecAudioDecoder : public AudioDecoder { On 2016/02/12 10:15:06, xhwang wrote: > You'll need MEDIA_EXPORT when you add unittest. I am not sure about Android > though. Acknowledged.
> Since this CL is getting large and nobody is using it yet. I think if fine to > land the implementation first, then we work on a CL that add unittests > immediately after it. WDYT? That would be very nice of you.
Looks like a cleaner version of AVDA :) https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:12: Usually no gap between headers https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:28: inline const base::TimeDelta NoWaitTimeOut() { nit: this probably applies to AVDA too, but it should be Timeout not TimeOut. For IdleTimerTimeOut as well. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:175: media_codec_ = std::move(CreateMediaCodec(config_)); minor: I think you can do media_codec_.reset(CreateMediaCodec()) https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:461: DCHECK_GE(out.buf_index, 0); // |out->buf_index| must be valid nit: Comment seems redundant. Maybe you could use "kInvalidBufferIndex" if you wanted a more self documenting condition? https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:480: SetState(STATE_DRAINED); Could you clarify this comment? I'm not sure what it means. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:488: DCHECK_GE(out.buf_index, 0); // |out->buf_index| must be valid Same as above https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:504: DCHECK_LE(out.size, audio_buffer->data_size()); Seems like this should be handled with a CHECK if it should be impossible, because it looks like a potential buffer overflow. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:514: output_cb_.Run(audio_buffer); Am I missing something or is the input buffer never removed from the queue?
https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/base/andr... File media/base/android/sdk_media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/base/andr... media/base/android/sdk_media_codec_bridge.cc:381: << " play audio:" << play_audio << " media_crypto:" << media_crypto; On 2016/02/13 01:31:24, Tima Vaisburd wrote: > On 2016/02/12 10:15:04, xhwang wrote: > > nit: If you don't care about this overloaded version, you can put the log at > > line 351, and use config.AsHumanReadableString(). > > I wanted to see that conversion to nanoseconds is done right. I think it is nice > to have the log like this for every code path. Acknowledged. https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/filters/a... File media/filters/android/media_codec_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:44: scoped_ptr<MediaCodecBridge> media_codec( On 2016/02/13 01:31:24, Tima Vaisburd wrote: > On 2016/02/12 10:15:05, xhwang wrote: > > You can have "scoped_ptr<AudioCodecBridge> audio_codec_bridge" here. Then you > > don't need to cast on line 51. > > How to return scoped_ptr<MediaCodecBridge> on l.65? My compilation complains > that it cannot convert scoped_ptr<Audio> into scoped_ptr<Media>, although Audio > is derived from Media. Maybe we can just return raw pointer? Can you try? return std::move(media_codec); https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:82: media_codec_.reset(); On 2016/02/13 01:31:24, Tima Vaisburd wrote: > On 2016/02/12 10:15:05, xhwang wrote: > > Is this needed? Can we just let the scoped_ptr dtor do the work? > > As far as I understand this is not needed right now because there is no specific > need to destroy MediaCodec before any other field. I would still prefer to keep > explicit resets, I find myself searching for it when looking into various bugs > as MediaCodec is quite fragile. Acknowledged. https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:119: media_codec_ = std::move(CreateMediaCodec(config)); On 2016/02/13 01:31:24, Tima Vaisburd wrote: > On 2016/02/12 10:15:05, xhwang wrote: > > Will it work if you remove std::move? > > Yes! Unnamed temporary? rvalue: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... https://chromiumcodereview.appspot.com/1651673002/diff/100001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:233: decode_cb.Run(kDecodeError); On 2016/02/13 01:31:24, Tima Vaisburd wrote: > On 2016/02/12 10:15:04, xhwang wrote: > > Do you want to clear |input_queue_| and fire all pending decode callbacks > > instead? > > > > If you do that, you actually don't need line 214 and line 215. > > I created DequeueInputBuffer() and EnqueueInputBuffer(), but slightly > differently than you've suggested. Please take a look. > > Right now the code is quite conservative in removing the stuff from > |input_queue_|. This way potentially the caller might try to recover with > Reset() and continue. > > On the other hand, as far as I know no one is doing that, and the errors from > DequeueOutput() would require special handling. > > I prefer to keep it as is right now and revisit at the time of a planned common > part refactoring, but let me know what you think. In Reset() you'll clear |input_queue_| anyways, right? When a decode error happens, there's pretty no way to keep decoding the rest of the buffers. In the video case, the next buffer may be a B frame, which cannot be decoded if the previous frame isn't decoded successfully. Also, holding callbacks seems bad in general. Currently VideoDecoder supports parallel decoding and it requires all decode_cb to be fired by the decoder, even if the caller doesn't call into the decoder again [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d... https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... File media/filters/android/media_codec_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:147: input_queue_.push_back(std::make_pair(buffer, bound_decode_cb)); This would work and is totally legit according to the current AudioDecoder API. However now we could have a reentrancy case where the call stack looks like: DecoderStream::Decode() -> MCAD::Decode() -> output_cb_.Run() -> DecoderStream::OnDecodeDone() -> e.g. MCAD::Initialize() (for reinitialization for config change). This would still work (I think) but is a pattern that we want to avoid. Long stacks with reentrancy sometimes can cause tricky issues. I think we should update the AudioDecoder API such that output_cb are always called asynchronously after Decode() calls. For this CL, either we post the DoIOTask below, or your original approach is fine (use BindToCurrentLoop() for both decode_cb an output_cb). Just the latter would involve more posts. https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:216: return false; nit: You can bind these 3 state checks together. https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:263: decode_cb.Run(kDecodeError); pop_front()? It seems to me decode_cb.Run(*) and input_queue_.pop_front() should always come together. Maybe put them in a function so that we won't forget it. https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:483: void MediaCodecAudioDecoder::OnDecodedContentFrame( Why "ContentFrame"? How about just OnOutputBufferDecoded or OnFrameDecoded? https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... File media/filters/android/media_codec_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... media/filters/android/media_codec_audio_decoder.h:13: #include "base/memory/weak_ptr.h" remove? https://chromiumcodereview.appspot.com/1651673002/diff/120001/media/filters/a... media/filters/android/media_codec_audio_decoder.h:201: InputQueue input_queue_; Actually I just realized that for AudioDecoder, you can have at most one Decode() call at a time [1, 2]. Parallel decoding is only supported for video. So in theory you don't need the queue here. But if you want to share this code with the future MediaCodecVideoDecoder, we can keep the queue. But in the audio case, let's DCHECK that the queue size is <=1? [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... [2] https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/deco...
Thank you for the prompt review. I tried to address all the comments, please take a look. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:44: scoped_ptr<MediaCodecBridge> media_codec( On 2016/02/13 07:43:13, xhwang wrote: > On 2016/02/13 01:31:24, Tima Vaisburd wrote: > > On 2016/02/12 10:15:05, xhwang wrote: > > > You can have "scoped_ptr<AudioCodecBridge> audio_codec_bridge" here. Then > you > > > don't need to cast on line 51. > > > > How to return scoped_ptr<MediaCodecBridge> on l.65? My compilation complains > > that it cannot convert scoped_ptr<Audio> into scoped_ptr<Media>, although > Audio > > is derived from Media. Maybe we can just return raw pointer? > > Can you try? > return std::move(media_codec); That works! https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:119: media_codec_ = std::move(CreateMediaCodec(config)); On 2016/02/13 07:43:13, xhwang wrote: > On 2016/02/13 01:31:24, Tima Vaisburd wrote: > > On 2016/02/12 10:15:05, xhwang wrote: > > > Will it work if you remove std::move? > > > > Yes! Unnamed temporary? > > rvalue: > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... > Thank you for the link. https://codereview.chromium.org/1651673002/diff/100001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:233: decode_cb.Run(kDecodeError); On 2016/02/13 07:43:13, xhwang wrote: > On 2016/02/13 01:31:24, Tima Vaisburd wrote: > > On 2016/02/12 10:15:04, xhwang wrote: > > > Do you want to clear |input_queue_| and fire all pending decode callbacks > > > instead? > > > > > > If you do that, you actually don't need line 214 and line 215. > > > > I created DequeueInputBuffer() and EnqueueInputBuffer(), but slightly > > differently than you've suggested. Please take a look. > > > > Right now the code is quite conservative in removing the stuff from > > |input_queue_|. This way potentially the caller might try to recover with > > Reset() and continue. > > > > On the other hand, as far as I know no one is doing that, and the errors from > > DequeueOutput() would require special handling. > > > > I prefer to keep it as is right now and revisit at the time of a planned > common > > part refactoring, but let me know what you think. > > In Reset() you'll clear |input_queue_| anyways, right? > > When a decode error happens, there's pretty no way to keep decoding the rest of > the buffers. In the video case, the next buffer may be a B frame, which cannot > be decoded if the previous frame isn't decoded successfully. > > Also, holding callbacks seems bad in general. Currently VideoDecoder supports > parallel decoding and it requires all decode_cb to be fired by the decoder, even > if the caller doesn't call into the decoder again [1]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d... Agreed. Created a new method ClearInputQueue() and called it in the error situations (except after DequeueOutputBuffer() error and delete/recreate error in Reset()). https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:12: On 2016/02/13 02:34:52, watk wrote: > Usually no gap between headers Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:28: inline const base::TimeDelta NoWaitTimeOut() { On 2016/02/13 02:34:52, watk wrote: > nit: this probably applies to AVDA too, but it should be Timeout not TimeOut. > For IdleTimerTimeOut as well. Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:147: input_queue_.push_back(std::make_pair(buffer, bound_decode_cb)); On 2016/02/13 07:43:13, xhwang wrote: >I think we should update the AudioDecoder API such that output_cb are always > called asynchronously after Decode() calls. There is a phrase "Implementations guarantee that the callbacks will not be called from within this method [i.e. Decode]" already. I did not pay attention that they say about both DecodeCB and OutputCB. > For this CL, either we post the DoIOTask below, or your original approach is > fine (use BindToCurrentLoop() for both decode_cb an output_cb). Reverted to use BindToCurrentLoop() for output_cb. > Just the latter would involve more posts. I think you are talking about decode_cb, i.e. that keeping BindForCurrentLoop() for decode_cb and calling DoIOTask() here directly would involve more posts than not binding decode_cb and posting DoIOTask instead? Unfortunately I still cannot see why is this true. Isn't the amount of Decode() calls is exactly equal to the number of decode_cb.Run() calls? https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:175: media_codec_ = std::move(CreateMediaCodec(config_)); On 2016/02/13 02:34:52, watk wrote: > minor: I think you can do media_codec_.reset(CreateMediaCodec()) It does not compile: "no known conversion for argument 1 from 'scoped_ptr<media::MediaCodecBridge>' to 'scoped_ptr<media::MediaCodecBridge>::element_type* {aka media::MediaCodecBridge*}'", that is, reset() accepts a raw pointer, but not another scoped_ptr. However, even if it did (we could make CreateMediaCodec() returning a raw pointer) I wanted to release the old codec first, and then create the new one, i.e. avoid the situation where there are two MediaCodec objects for some period of time. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:216: return false; On 2016/02/13 07:43:13, xhwang wrote: > nit: You can bind these 3 state checks together. Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:263: decode_cb.Run(kDecodeError); On 2016/02/13 07:43:13, xhwang wrote: > pop_front()? > > It seems to me decode_cb.Run(*) and input_queue_.pop_front() should always come > together. Maybe put them in a function so that we won't forget it. Now we nave a new function ClearInputQueue() that clears the whole input queue and is used in error cases. This leaves only one place where we pop one buffer only, it is STATE_READY above. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:461: DCHECK_GE(out.buf_index, 0); // |out->buf_index| must be valid On 2016/02/13 02:34:53, watk wrote: > nit: Comment seems redundant. Maybe you could use "kInvalidBufferIndex" if you > wanted a more self documenting condition? Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:480: SetState(STATE_DRAINED); On 2016/02/13 02:34:52, watk wrote: > Could you clarify this comment? I'm not sure what it means. Changed to "Set state STATE_DRAINED after we have received EOS frame at the output." https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:483: void MediaCodecAudioDecoder::OnDecodedContentFrame( On 2016/02/13 07:43:13, xhwang wrote: > Why "ContentFrame"? How about just OnOutputBufferDecoded or OnFrameDecoded? Going back to OnDecodedFrame(). I wanted to nitpick on something that's not that important. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:488: DCHECK_GE(out.buf_index, 0); // |out->buf_index| must be valid On 2016/02/13 02:34:53, watk wrote: > Same as above Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:504: DCHECK_LE(out.size, audio_buffer->data_size()); On 2016/02/13 02:34:53, watk wrote: > Seems like this should be handled with a CHECK if it should be impossible, > because it looks like a potential buffer overflow. Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:514: output_cb_.Run(audio_buffer); On 2016/02/13 02:34:52, watk wrote: > Am I missing something or is the input buffer never removed from the queue? The removal happens after EnqueueInputBuffer() in this PS on l.240 and l.257. In the next PS it is moved to ClearInputQueue() for error cases. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:13: #include "base/memory/weak_ptr.h" On 2016/02/13 07:43:13, xhwang wrote: > remove? Done. https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.h:201: InputQueue input_queue_; On 2016/02/13 07:43:13, xhwang wrote: > Actually I just realized that for AudioDecoder, you can have at most one > Decode() call at a time [1, 2]. Parallel decoding is only supported for video. > > So in theory you don't need the queue here. But if you want to share this code > with the future MediaCodecVideoDecoder, we can keep the queue. But in the audio > case, let's DCHECK that the queue size is <=1? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... > > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/deco... I added DCHECK into Decode().
timav@google.com changed reviewers: + timav@google.com
https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/120001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:147: input_queue_.push_back(std::make_pair(buffer, bound_decode_cb)); On 2016/02/15 23:25:43, Tima Vaisburd wrote: > On 2016/02/13 07:43:13, xhwang wrote: > > >I think we should update the AudioDecoder API such that output_cb are always > > called asynchronously after Decode() calls. > > There is a phrase "Implementations guarantee that the callbacks will not be > called from within this method [i.e. Decode]" already. I did not pay attention > that they say about both DecodeCB and OutputCB. > > > For this CL, either we post the DoIOTask below, or your original approach is > > fine (use BindToCurrentLoop() for both decode_cb an output_cb). > > Reverted to use BindToCurrentLoop() for output_cb. > > > Just the latter would involve more posts. > > I think you are talking about decode_cb, i.e. that keeping BindForCurrentLoop() > for decode_cb and calling DoIOTask() here directly would involve more posts than > not binding decode_cb and posting DoIOTask instead? > > Unfortunately I still cannot see why is this true. Isn't the amount of Decode() > calls is exactly equal to the number of decode_cb.Run() calls? Maybe I got it: if we post DoIOTask we do not need BindToCurrentLoop() for either callback. Still, I would like to avoid the above reentrance chain even within DoIOTask.
LGTM % nits. Thank you for the great work! https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... File media/filters/android/media_codec_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:46: scoped_ptr<AudioCodecBridge> media_codec( s/media_codec/audio_codec_bridge/ https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:146: DCHECK_EQ(input_queue_.size(), 1U); maybe it's more straightforward to check at the beginning of this function that the queue is empty. https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:170: media_codec_ = std::move(CreateMediaCodec(config_)); Can you try: media_codec_ = CreateMediaCodec(config_); https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:361: void MediaCodecAudioDecoder::ClearInputQueue(Status frame_decode_status) { nit: s/frame_decode_status/decode_status/ ?
Min, Since this CL modifies sdk_media_codec_bridge.*, could you, please, give your response? Thank you!
If this is working now can you try adding it to AudioDecoderUnittest? https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi...
https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/base/andr... File media/base/android/sdk_media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/base/andr... media/base/android/sdk_media_codec_bridge.h:115: // We might want to remove this method when these classes go away. can we remove this method? We can pack in all the params inside AudioDecoderConfig, just need to change AudioDecoderJob and AudioMediaCodecDecoder https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... File media/filters/android/media_codec_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:222: pending_input_buf_index_ = if we reset the pending_input_buf_index_ inside DequeueInputBuffer(), we can move this statement to either EnqueueInputBuffer() or the switch statement below(), and change it to case MEDIA_CODEC_NO_KEY: pending_input_buf_index_ = input_buf_index; break; https://chromiumcodereview.appspot.com/1651673002/diff/140001/media/filters/a... media/filters/android/media_codec_audio_decoder.cc:265: return pending_input_buf_index_; we can reset pending_input_buf_index_ here.
https://codereview.chromium.org/1651673002/diff/140001/media/base/android/sdk... File media/base/android/sdk_media_codec_bridge.h (right): https://codereview.chromium.org/1651673002/diff/140001/media/base/android/sdk... media/base/android/sdk_media_codec_bridge.h:115: // We might want to remove this method when these classes go away. On 2016/02/16 19:25:09, qinmin wrote: > can we remove this method? We can pack in all the params inside > AudioDecoderConfig, just need to change AudioDecoderJob and > AudioMediaCodecDecoder One way to do this would be to make a method that converts DemuxerConfigs into AudioDecoderConfig, and make AudioDecoderJob keep DemuxerConfigs as a member. May I do this in a separate CL? I've included TODO. https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:46: scoped_ptr<AudioCodecBridge> media_codec( On 2016/02/16 17:40:35, xhwang wrote: > s/media_codec/audio_codec_bridge/ Done. https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:146: DCHECK_EQ(input_queue_.size(), 1U); On 2016/02/16 17:40:35, xhwang wrote: > maybe it's more straightforward to check at the beginning of this function that > the queue is empty. Done. https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:170: media_codec_ = std::move(CreateMediaCodec(config_)); On 2016/02/16 17:40:35, xhwang wrote: > Can you try: > > media_codec_ = CreateMediaCodec(config_); Done. https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:222: pending_input_buf_index_ = On 2016/02/16 19:25:09, qinmin wrote: > if we reset the pending_input_buf_index_ inside DequeueInputBuffer(), we can > move this statement to either EnqueueInputBuffer() or the switch statement > below(), and change it to > > case MEDIA_CODEC_NO_KEY: > pending_input_buf_index_ = input_buf_index; > break; Created new struct InputBufferInfo, the method DequeueInputBuffer now returns it. PTAL. https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:265: return pending_input_buf_index_; On 2016/02/16 19:25:09, qinmin wrote: > we can reset pending_input_buf_index_ here. Done. https://codereview.chromium.org/1651673002/diff/140001/media/filters/android/... media/filters/android/media_codec_audio_decoder.cc:361: void MediaCodecAudioDecoder::ClearInputQueue(Status frame_decode_status) { On 2016/02/16 17:40:35, xhwang wrote: > nit: s/frame_decode_status/decode_status/ ? Done.
On 2016/02/16 18:48:40, DaleCurtis wrote: > If this is working now can you try adding it to AudioDecoderUnittest? > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi... This is planned in the next CL.
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1651673002/#ps160001 (title: "Reset pending input buffer immediately after use, addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651673002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651673002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
timav@chromium.org changed reviewers: + sievers@chromium.org - timav@google.com
+sievers@: Daniel, could you, please, review content/ change? Thank you.
We shouldn't need to update content/ for media/ changes. It seems the right fix is to let "player_android" depend on "shared_memory_support", which contains channel_layout.* that defines ChannelLayoutToChannelCount. dalecurtis: It seems ChannelLayoutToChannelCount is a generic operation that has nothing to do with "share memory". Should channel_layout.* be part of "media" instead of "shared_memory_support"?
content lgtm
Does BUILD.gn need a fix too? shared_memory_support really means "All the things needed to build AudioBus for NaCl / Pepper." It's an intentionally minimal target since it's compiled standalone with he NaCl generators for audio support. lgtm % gn fix if necessary.
On 2016/02/17 18:18:03, DaleCurtis wrote: > Does BUILD.gn need a fix too? shared_memory_support really means "All the things > needed to build AudioBus for NaCl / Pepper." It's an intentionally minimal > target since it's compiled standalone with he NaCl generators for audio support. > > lgtm % gn fix if necessary. gn seems okay: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
On 2016/02/17 18:38:15, xhwang wrote: > On 2016/02/17 18:18:03, DaleCurtis wrote: > > Does BUILD.gn need a fix too? shared_memory_support really means "All the > things > > needed to build AudioBus for NaCl / Pepper." It's an intentionally minimal > > target since it's compiled standalone with he NaCl generators for audio > support. > > > > lgtm % gn fix if necessary. > > gn seems okay: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... gn build worked even before this fix
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, qinmin@chromium.org, xhwang@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1651673002/#ps200001 (title: "A better linking issue fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651673002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651673002/200001
Message was sent while issue was closed.
Description was changed from ========== Add MediaCodecAudioDecoder implementation MediaCodecAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface. This CL adds the implementation. BUG=542910 ========== to ========== Add MediaCodecAudioDecoder implementation MediaCodecAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface. This CL adds the implementation. BUG=542910 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add MediaCodecAudioDecoder implementation MediaCodecAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface. This CL adds the implementation. BUG=542910 ========== to ========== Add MediaCodecAudioDecoder implementation MediaCodecAudioDecoder is an audio decoder that uses Android MediaCodec API and implements the AudioDecoder interface. This CL adds the implementation. BUG=542910 Committed: https://crrev.com/a2d1a85ac6693abd2f59ea321293aa012207c381 Cr-Commit-Position: refs/heads/master@{#375942} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a2d1a85ac6693abd2f59ea321293aa012207c381 Cr-Commit-Position: refs/heads/master@{#375942} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
