Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1685483002:
Add stub MediaCodecAudioDecoder (Closed)
|
Created:
4 years, 10 months ago by Tima Vaisburd Modified:
4 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd stub MediaCdecAudioDecoder
MediaCodecAudioDecoder 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.
This CL only defines its interface and have notes about
possible implementation.
BUG=542910
Committed: https://crrev.com/b052c43a9c245c3c0f2b18365e5f7c1a75eca415
Cr-Commit-Position: refs/heads/master@{#374734}
Patch Set 1 #
Total comments: 75
Patch Set 2 : Addressed comments #Patch Set 3 : Fixed compilation #Patch Set 4 : Renamed the decoder to MediaCodecAudioDecoder #Patch Set 5 : Addressed comments, one question #
Messages
Total messages: 26 (8 generated)
In an offline conversation Xiaohan advised on splitting the CL https://codereview.chromium.org/1651673002/ and having just the interface with stub implementation as a separate CL. The original https://codereview.chromium.org/1651673002/ should become a dependent CL. I provided more explanations in the android_audio_decoder.h file. I hope you will find acceptable having private methods and member variables in this CL. PTAL. xhwang@: main reviewer liberato@, qinmin@, dalecurtis@, sandersd@, watk@: fyi, and please add you valuable comments!
Cool. I mostly read over all the comments. The method declarations seem reasonable to me. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:27: // AudioDecoder based on Android's MediaCodec API. AndroidAudioDecoder https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:29: // Android. It is also a way to employ hardware-accelerated decoder. s/decoder/decoding https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:67: // Every Dequeue() wakes the timer up. s/Dequeue/Decode? https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:69: // States diagram. s/States/State https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:71: // [Uninitialized] [Running] Should "Running" be "Ready"? https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:120: int buf_index; // MediaCodec's output buffers are referred by this index. s/referred/referred to/ https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:121: size_t offset; // position in the buffer where data starts. capitalize position https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:123: base::TimeDelta pts; // Presentation timestamp/ s/\//./ :P The trailing slash https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:125: bool is_key_frame; // true if this buffer is the key frame. I think you could safely remove this comment. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:136: // Enqueues one pending input buffer into MediaCodec if MediaCodec has a room. s/has a room/has room/ https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:141: // Returns true if any output buffer has been received from MediaCodec. s/has been/was/ https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:175: // run on this task runner. I think the single threaded comment would be useful at the class level instead of here. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:178: // The current state of the object. nit: comment feels unnecessary https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:182: // not be able to accept the input at the time of Dequeue(), thus all s/Dequeue/Decode https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:188: // Decoder configuration cache. Comment makes it sound like the thing is primarily a cache and not a config. Could be "Cached decoder config." https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:193: int bytes_per_frame_; It seems weird putting these in member variables because it's state that needs to be kept in sync and isn't conceptually a property of the decoder. Could you use helper methods/functions instead? https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:201: // DecodeCB that should be fired after the decoder has been flashed in case s/flashed/flushed https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:206: scoped_ptr<MediaCodecBridge> media_codec_; nit: Comment feels redundant.
I <3 those comments/doc. Thanks a lot! https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... File media/filters/android/android_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:45: // codec might not have available input buffers yet, or not every encoded frame nit: s/frame/buffer, we like to use "buffer" for input and "frame" for output. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:50: // queuing of a filled input buffer might fail due to lack of the relevant key nit: s/queuing/enqueuing to be consistent with line 53. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:51: // in the DRM module. nit: Replace "DRM module" with MediaDrm, or CDM. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:56: // tries to send the front buffer from the input queue to the MediaCodec. In Is this for enqueuing failure, which will only happen for NO_KEY? How expensive is the timer? Is it possible to only try to enqueue again when we know a new key is available? In the case of dequeuing failure, we'll try to dequeue a buffer periodically. Is that correct? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:64: // the processing is initated by the timer with short period (10 ms). This Explain why we choose this simple logic? Is it because those calls will block the current thread if the delay is non-zero? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:66: // processing) and after begin idle for a predefined time the timer stops. nit: s/begin/being? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:89: // [Error] [Ready] [Uninitialized] OOC, why after reset failure we are in Uninitialized instead of Error state? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:122: size_t size; // The size of the buffer. Is this the size of the buffer or the data? If it's the size of the buffer, does it include the offset? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:148: // and subsequent calls with false will eventually stop the timer when the So actually "subsequent calls with false" doesn't matter, as they will not start the timer and will not change the beginning of idle time. Is this correct? I feel ManagerTimer(true) and ManagerTimer(false) are not very readable. Can we actually have two calls: StartTimer() and SetIdleTime()? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:163: bool ConfigureMediaCodec(); See my comment in the other CL. Can we change this to a non-member function like: // Returns null if configuration failed. scoped_ptr<AudioCodecBridge> ConfigureMediaCodec(const AudioDecoderConfig& config); https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:175: // run on this task runner. On 2016/02/09 01:50:17, watk wrote: > I think the single threaded comment would be useful at the class level instead > of here. +1 https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:196: InitCB init_cb_; See my comment in the other CL. This is not needed. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:203: DecodeCB eos_decode_cb_; See my comment in the other CL. Can we put the EOS buffer and EOS decode_cb in the input_queue_ as well? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:216: int pending_input_buf_index_; Worth mentioning that -1 means invalid index: http://developer.android.com/reference/android/media/MediaCodec.html#dequeueI... Maybe we should define a kInvalidInputBufferIndex.
nit: In the title, should we s/Added/Add, which seems more commonly used?
Description was changed from ========== Added stub 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ========== to ========== Add stub 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ==========
https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... File media/filters/android/android_audio_decoder.h (right): https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:27: // AudioDecoder based on Android's MediaCodec API. On 2016/02/09 01:50:17, watk wrote: > AndroidAudioDecoder Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:29: // Android. It is also a way to employ hardware-accelerated decoder. On 2016/02/09 01:50:16, watk wrote: > s/decoder/decoding Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:45: // codec might not have available input buffers yet, or not every encoded frame On 2016/02/09 07:37:42, xhwang wrote: > nit: s/frame/buffer, we like to use "buffer" for input and "frame" for output. Done, although I heard "encoded frame" and "decoded frame" widely used. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:50: // queuing of a filled input buffer might fail due to lack of the relevant key On 2016/02/09 07:37:42, xhwang wrote: > nit: s/queuing/enqueuing to be consistent with line 53. Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:51: // in the DRM module. On 2016/02/09 07:37:42, xhwang wrote: > nit: Replace "DRM module" with MediaDrm, or CDM. CDM https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:56: // tries to send the front buffer from the input queue to the MediaCodec. In On 2016/02/09 07:37:42, xhwang wrote: > Is this for enqueuing failure, which will only happen for NO_KEY? If I understood your question correctly, then no, the timer is here first and foremost to cope with dequeueing failures. The enqueueing failures are expected only in NO_KEY situation, which are rare, whereas the dequeuing failures happen very often and are normal. > How expensive is the timer? I believe it is not expensive taking into account that AVDA, which has the same scheme, seems to work well. > Is it possible to only try to enqueue again when we know a new key > is available? Yes, and this is precisely what we are doing and AVDA, I believe, and what is supposed to be done here. > > In the case of dequeuing failure, we'll try to dequeue a buffer periodically. Is > that correct? Yes. Just a tiny clarification which maybe not needed: I'd say "we'll try to dequeue again". The dequeuing failure does not turn any new mechanism on, the timer just continues. We'll try again next time it fires. The dequeue/enqueue success will trigger the DecodeCB and the deletion from the front of the input queue. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:64: // the processing is initated by the timer with short period (10 ms). This On 2016/02/09 07:37:42, xhwang wrote: > Explain why we choose this simple logic? Is it because those calls will block > the current thread if the delay is non-zero? This is a very good point that I actually missed. However, I think one can chose the simple logic just by virtue of being simple yet effective enough? I added "Using no delay for enqueue operations has an extra benefit of not blocking the current thread." https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:66: // processing) and after begin idle for a predefined time the timer stops. On 2016/02/09 07:37:42, xhwang wrote: > nit: s/begin/being? Yes, corrected. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:67: // Every Dequeue() wakes the timer up. On 2016/02/09 01:50:17, watk wrote: > s/Dequeue/Decode? Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:69: // States diagram. On 2016/02/09 01:50:17, watk wrote: > s/States/State Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:71: // [Uninitialized] [Running] On 2016/02/09 01:50:16, watk wrote: > Should "Running" be "Ready"? Yes, corrected. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:89: // [Error] [Ready] [Uninitialized] On 2016/02/09 07:37:42, xhwang wrote: > OOC, why after reset failure we are in Uninitialized instead of Error state? 1. To be consistent with the initialization: if init failed, the decoder remains in the Uninitialized state. Added this to the diagram. 2. To enable recovering from the Error state. Reset() is the mean to recover. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:120: int buf_index; // MediaCodec's output buffers are referred by this index. On 2016/02/09 01:50:17, watk wrote: > s/referred/referred to/ Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:121: size_t offset; // position in the buffer where data starts. On 2016/02/09 01:50:17, watk wrote: > capitalize position Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:122: size_t size; // The size of the buffer. On 2016/02/09 07:37:42, xhwang wrote: > Is this the size of the buffer or the data? If it's the size of the buffer, does > it include the offset? Added "(includes offset)". https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:123: base::TimeDelta pts; // Presentation timestamp/ On 2016/02/09 01:50:17, watk wrote: > s/\//./ :P The trailing slash Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:125: bool is_key_frame; // true if this buffer is the key frame. On 2016/02/09 01:50:17, watk wrote: > I think you could safely remove this comment. Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:136: // Enqueues one pending input buffer into MediaCodec if MediaCodec has a room. On 2016/02/09 01:50:16, watk wrote: > s/has a room/has room/ Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:141: // Returns true if any output buffer has been received from MediaCodec. On 2016/02/09 01:50:17, watk wrote: > s/has been/was/ Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:148: // and subsequent calls with false will eventually stop the timer when the On 2016/02/09 07:37:42, xhwang wrote: > So actually "subsequent calls with false" doesn't matter, as they will not start > the timer and will not change the beginning of idle time. Is this correct? One of these "subsequent calls with false" will stop the timer. In that sense it matters. > I feel ManagerTimer(true) and ManagerTimer(false) are not very readable. Can we > actually have two calls: StartTimer() and SetIdleTime()? If you recall the implementation, the DoIOTask() looks like bool enqueued_input = QueueInput(); bool dequeued_output = DequeueOutput(); ManageTimer(enqueued_input || dequeued_output); The whole purpose of ManageTimer is to include the logic that calls either StartTimer(), StopTimer() or SetIdleTime(). Maybe it would be better to give the purpose instead of implementation details, something like "Start the timer immediately or stop it based on the elapsed idle time." ? https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:163: bool ConfigureMediaCodec(); On 2016/02/09 07:37:42, xhwang wrote: > See my comment in the other CL. Can we change this to a non-member function > like: > > // Returns null if configuration failed. > scoped_ptr<AudioCodecBridge> ConfigureMediaCodec(const AudioDecoderConfig& > config); Yes I saw it. I plan to make this change while working on implementation in the implementation CL. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:175: // run on this task runner. On 2016/02/09 07:37:42, xhwang wrote: > On 2016/02/09 01:50:17, watk wrote: > > I think the single threaded comment would be useful at the class level instead > > of here. > > +1 Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:178: // The current state of the object. On 2016/02/09 01:50:17, watk wrote: > nit: comment feels unnecessary Removed. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:182: // not be able to accept the input at the time of Dequeue(), thus all On 2016/02/09 01:50:17, watk wrote: > s/Dequeue/Decode Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:188: // Decoder configuration cache. On 2016/02/09 01:50:17, watk wrote: > Comment makes it sound like the thing is primarily a cache and not a config. > Could be "Cached decoder config." Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:193: int bytes_per_frame_; On 2016/02/09 01:50:17, watk wrote: > It seems weird putting these in member variables because it's state that needs > to be kept in sync and isn't conceptually a property of the decoder. Could you > use helper methods/functions instead? Agreed. ChannelCount(config) can be a file scope function, and bytes_per_frame needed only in OnDecode() and can be calculated there. I removed these member vars. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:196: InitCB init_cb_; On 2016/02/09 07:37:42, xhwang wrote: > See my comment in the other CL. This is not needed. Sorry I missed that. Removed. We might need this member back for CDM processing. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:201: // DecodeCB that should be fired after the decoder has been flashed in case On 2016/02/09 01:50:17, watk wrote: > s/flashed/flushed Done. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:203: DecodeCB eos_decode_cb_; On 2016/02/09 07:37:42, xhwang wrote: > See my comment in the other CL. Can we put the EOS buffer and EOS decode_cb in > the input_queue_ as well? Yes I saw it and I like the idea. As with ConfigureMediaCodec(), I plan to make this change in the implementation CL. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:206: scoped_ptr<MediaCodecBridge> media_codec_; On 2016/02/09 01:50:17, watk wrote: > nit: Comment feels redundant. Removed. https://codereview.chromium.org/1685483002/diff/1/media/filters/android/andro... media/filters/android/android_audio_decoder.h:216: int pending_input_buf_index_; On 2016/02/09 07:37:42, xhwang wrote: > Worth mentioning that -1 means invalid index: > http://developer.android.com/reference/android/media/MediaCodec.html#dequeueI... > > Maybe we should define a kInvalidInputBufferIndex. Added a line "The -1 value means there is no such buffer."
On 2016/02/09 17:41:59, xhwang wrote: > nit: In the title, should we s/Added/Add, which seems more commonly used? Done.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
I'll leave review to others, overall lg though! On naming, I suggest we call this MediaCodecAudioDecoder... but we already have that class. Long term the existing one will be removed, so I further suggest we rename the existing one to something like BrowserMediaCodecAudioDecoder and make this one MediaCodecAudioDecoder.
On 2016/02/09 20:26:44, DaleCurtis wrote: > I'll leave review to others, overall lg though! On naming, I suggest we call > this MediaCodecAudioDecoder... but we already have that class. Long term the > existing one will be removed, so I further suggest we rename the existing one to > something like BrowserMediaCodecAudioDecoder and make this one > MediaCodecAudioDecoder. Copying my comment from the other CL: MediaCodecAudioDecoder does sound a better name. The existing MediaCodecAudioDecoder should really be AudioMediaCodecDecoder since it's a MediaCodecDecoder.
On 2016/02/09 20:26:44, DaleCurtis wrote: > I'll leave review to others, overall lg though! On naming, I suggest we call > this MediaCodecAudioDecoder... but we already have that class. Long term the > existing one will be removed, so I further suggest we rename the existing one to > something like BrowserMediaCodecAudioDecoder and make this one > MediaCodecAudioDecoder. Renaming the existing MediaCodecAudioDecoder in media/base/android would pull some other files (MediaCodecDecoder, MediaCodecVideoDecoder, the unit tests). Can we have a separate pure renaming CL?
On 2016/02/09 21:20:10, xhwang wrote: > On 2016/02/09 20:26:44, DaleCurtis wrote: > > I'll leave review to others, overall lg though! On naming, I suggest we call > > this MediaCodecAudioDecoder... but we already have that class. Long term the > > existing one will be removed, so I further suggest we rename the existing one > to > > something like BrowserMediaCodecAudioDecoder and make this one > > MediaCodecAudioDecoder. > > Copying my comment from the other CL: > > MediaCodecAudioDecoder does sound a better name. The existing > MediaCodecAudioDecoder should really be AudioMediaCodecDecoder since it's a > MediaCodecDecoder. Still, can we do this renaming in a separate CL?
On 2016/02/09 21:27:07, Tima Vaisburd wrote: > On 2016/02/09 21:20:10, xhwang wrote: > > On 2016/02/09 20:26:44, DaleCurtis wrote: > > > I'll leave review to others, overall lg though! On naming, I suggest we call > > > this MediaCodecAudioDecoder... but we already have that class. Long term > the > > > existing one will be removed, so I further suggest we rename the existing > one > > to > > > something like BrowserMediaCodecAudioDecoder and make this one > > > MediaCodecAudioDecoder. > > > > Copying my comment from the other CL: > > > > MediaCodecAudioDecoder does sound a better name. The existing > > MediaCodecAudioDecoder should really be AudioMediaCodecDecoder since it's a > > MediaCodecDecoder. > > Still, can we do this renaming in a separate CL? Yes, we should. Ideally that can be done before this one so we don't need to add the new files and then immediately rename them.
Description was changed from ========== Add stub 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ========== to ========== Add stub MediaCdecAudioDecoder MediaCodecAudioDecoder 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ==========
On 2016/02/09 21:35:09, xhwang wrote: > On 2016/02/09 21:27:07, Tima Vaisburd wrote: > > On 2016/02/09 21:20:10, xhwang wrote: > > > On 2016/02/09 20:26:44, DaleCurtis wrote: > > > > I'll leave review to others, overall lg though! On naming, I suggest we > call > > > > this MediaCodecAudioDecoder... but we already have that class. Long term > > the > > > > existing one will be removed, so I further suggest we rename the existing > > one > > > to > > > > something like BrowserMediaCodecAudioDecoder and make this one > > > > MediaCodecAudioDecoder. > > > > > > Copying my comment from the other CL: > > > > > > MediaCodecAudioDecoder does sound a better name. The existing > > > MediaCodecAudioDecoder should really be AudioMediaCodecDecoder since it's a > > > MediaCodecDecoder. > > > > Still, can we do this renaming in a separate CL? > > Yes, we should. Ideally that can be done before this one so we don't need to add > the new files and then immediately rename them. Set the dependency upon browser decoders renaming CL https://codereview.chromium.org/1682923003 and renamed the decoder to MediaCodecAudioDecoder.
LGTM! https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... File media/filters/android/android_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:56: // tries to send the front buffer from the input queue to the MediaCodec. In On 2016/02/09 20:12:40, Tima Vaisburd wrote: > On 2016/02/09 07:37:42, xhwang wrote: > > Is this for enqueuing failure, which will only happen for NO_KEY? > > If I understood your question correctly, then no, the timer is here first and > foremost to cope with dequeueing failures. The enqueueing failures are expected > only in NO_KEY situation, which are rare, whereas the dequeuing failures happen > very often and are normal. > > > How expensive is the timer? > > I believe it is not expensive taking into account that AVDA, which has the same > scheme, seems to work well. Can we update the comment to make this clear? > > Is it possible to only try to enqueue again when we know a new key > > is available? > > Yes, and this is precisely what we are doing and AVDA, I believe, and what is > supposed to be done here. > > > > In the case of dequeuing failure, we'll try to dequeue a buffer periodically. > Is > > that correct? > > Yes. > Just a tiny clarification which maybe not needed: I'd say "we'll try to dequeue > again". The dequeuing failure does not turn any new mechanism on, the timer just > continues. We'll try again next time it fires. The dequeue/enqueue success will > trigger the DecodeCB and the deletion from the front of the input queue. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:89: // [Error] [Ready] [Uninitialized] On 2016/02/09 20:12:41, Tima Vaisburd wrote: > On 2016/02/09 07:37:42, xhwang wrote: > > OOC, why after reset failure we are in Uninitialized instead of Error state? > > 1. To be consistent with the initialization: if init failed, the decoder remains > in the Uninitialized state. Added this to the diagram. > 2. To enable recovering from the Error state. Reset() is the mean to recover. In what case do we try to recover from errors? Most decoders we have don't have the ability to recover from errors. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:148: // and subsequent calls with false will eventually stop the timer when the On 2016/02/09 20:12:41, Tima Vaisburd wrote: > On 2016/02/09 07:37:42, xhwang wrote: > > So actually "subsequent calls with false" doesn't matter, as they will not > start > > the timer and will not change the beginning of idle time. Is this correct? > > One of these "subsequent calls with false" will stop the timer. In that sense it > matters. > > > I feel ManagerTimer(true) and ManagerTimer(false) are not very readable. Can > we > > actually have two calls: StartTimer() and SetIdleTime()? > > If you recall the implementation, the DoIOTask() looks like > > bool enqueued_input = QueueInput(); > bool dequeued_output = DequeueOutput(); > ManageTimer(enqueued_input || dequeued_output); > > The whole purpose of ManageTimer is to include the logic that calls either > StartTimer(), StopTimer() or SetIdleTime(). > > Maybe it would be better to give the purpose instead of implementation details, > something like "Start the timer immediately or stop it based on the elapsed idle > time." ? Thanks for the explanation. Your suggested comment sgtm. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:163: bool ConfigureMediaCodec(); On 2016/02/09 20:12:41, Tima Vaisburd wrote: > On 2016/02/09 07:37:42, xhwang wrote: > > See my comment in the other CL. Can we change this to a non-member function > > like: > > > > // Returns null if configuration failed. > > scoped_ptr<AudioCodecBridge> ConfigureMediaCodec(const AudioDecoderConfig& > > config); > > Yes I saw it. I plan to make this change while working on implementation in the > implementation CL. In that case why not just remove this function from this file? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:196: InitCB init_cb_; On 2016/02/09 20:12:41, Tima Vaisburd wrote: > On 2016/02/09 07:37:42, xhwang wrote: > > See my comment in the other CL. This is not needed. > > Sorry I missed that. Removed. We might need this member back for CDM processing. Acknowledged. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:203: DecodeCB eos_decode_cb_; On 2016/02/09 20:12:41, Tima Vaisburd wrote: > On 2016/02/09 07:37:42, xhwang wrote: > > See my comment in the other CL. Can we put the EOS buffer and EOS decode_cb in > > the input_queue_ as well? > > Yes I saw it and I like the idea. As with ConfigureMediaCodec(), I plan to make > this change in the implementation CL. In that case why not just remove it from this CL?
https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... File media/filters/android/android_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:56: // tries to send the front buffer from the input queue to the MediaCodec. In On 2016/02/10 06:50:40, xhwang wrote: > On 2016/02/09 20:12:40, Tima Vaisburd wrote: > > On 2016/02/09 07:37:42, xhwang wrote: > > > Is this for enqueuing failure, which will only happen for NO_KEY? > > > > If I understood your question correctly, then no, the timer is here first and > > foremost to cope with dequeueing failures. The enqueueing failures are > expected > > only in NO_KEY situation, which are rare, whereas the dequeuing failures > happen > > very often and are normal. > > > > > How expensive is the timer? > > > > I believe it is not expensive taking into account that AVDA, which has the > same > > scheme, seems to work well. > > Can we update the comment to make this clear? What part of the explanation is confusing? Dequeue failures happening all the time? Timer with no delay has been tried? All of the above ;-) ? https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:89: // [Error] [Ready] [Uninitialized] On 2016/02/10 06:50:40, xhwang wrote: > On 2016/02/09 20:12:41, Tima Vaisburd wrote: > > On 2016/02/09 07:37:42, xhwang wrote: > > > OOC, why after reset failure we are in Uninitialized instead of Error state? > > > > 1. To be consistent with the initialization: if init failed, the decoder > remains > > in the Uninitialized state. Added this to the diagram. > > 2. To enable recovering from the Error state. Reset() is the mean to recover. > > > In what case do we try to recover from errors? Most decoders we have don't have > the ability to recover from errors. After the second thought it seems it would make better sense to go to the Error state if Reset() fails. Updated the diagram. Maybe Reset() should not clear the Error state at all. As far as I can see if Decode() returns error the whole renderer is destroyed. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:148: // and subsequent calls with false will eventually stop the timer when the On 2016/02/10 06:50:40, xhwang wrote: > On 2016/02/09 20:12:41, Tima Vaisburd wrote: > > On 2016/02/09 07:37:42, xhwang wrote: > > > So actually "subsequent calls with false" doesn't matter, as they will not > > start > > > the timer and will not change the beginning of idle time. Is this correct? > > > > One of these "subsequent calls with false" will stop the timer. In that sense > it > > matters. > > > > > I feel ManagerTimer(true) and ManagerTimer(false) are not very readable. Can > > we > > > actually have two calls: StartTimer() and SetIdleTime()? > > > > If you recall the implementation, the DoIOTask() looks like > > > > bool enqueued_input = QueueInput(); > > bool dequeued_output = DequeueOutput(); > > ManageTimer(enqueued_input || dequeued_output); > > > > The whole purpose of ManageTimer is to include the logic that calls either > > StartTimer(), StopTimer() or SetIdleTime(). > > > > Maybe it would be better to give the purpose instead of implementation > details, > > something like "Start the timer immediately or stop it based on the elapsed > idle > > time." ? > > Thanks for the explanation. Your suggested comment sgtm. Done. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:163: bool ConfigureMediaCodec(); On 2016/02/10 06:50:40, xhwang wrote: > On 2016/02/09 20:12:41, Tima Vaisburd wrote: > > On 2016/02/09 07:37:42, xhwang wrote: > > > See my comment in the other CL. Can we change this to a non-member function > > > like: > > > > > > // Returns null if configuration failed. > > > scoped_ptr<AudioCodecBridge> ConfigureMediaCodec(const AudioDecoderConfig& > > > config); > > > > Yes I saw it. I plan to make this change while working on implementation in > the > > implementation CL. > > In that case why not just remove this function from this file? My prior comment had no sense, sorry. Put the desired prototype now. https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:203: DecodeCB eos_decode_cb_; On 2016/02/10 06:50:40, xhwang wrote: > On 2016/02/09 20:12:41, Tima Vaisburd wrote: > > On 2016/02/09 07:37:42, xhwang wrote: > > > See my comment in the other CL. Can we put the EOS buffer and EOS decode_cb > in > > > the input_queue_ as well? > > > > Yes I saw it and I like the idea. As with ConfigureMediaCodec(), I plan to > make > > this change in the implementation CL. > > In that case why not just remove it from this CL? Done.
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1685483002/#ps80001 (title: "Addressed comments, one question")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685483002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685483002/80001
https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... File media/filters/android/android_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1685483002/diff/1/media/filters/androi... media/filters/android/android_audio_decoder.h:56: // tries to send the front buffer from the input queue to the MediaCodec. In On 2016/02/10 19:51:10, Tima Vaisburd wrote: > On 2016/02/10 06:50:40, xhwang wrote: > > On 2016/02/09 20:12:40, Tima Vaisburd wrote: > > > On 2016/02/09 07:37:42, xhwang wrote: > > > > Is this for enqueuing failure, which will only happen for NO_KEY? > > > > > > If I understood your question correctly, then no, the timer is here first > and > > > foremost to cope with dequeueing failures. The enqueueing failures are > > expected > > > only in NO_KEY situation, which are rare, whereas the dequeuing failures > > happen > > > very often and are normal. > > > > > > > How expensive is the timer? > > > > > > I believe it is not expensive taking into account that AVDA, which has the > > same > > > scheme, seems to work well. > > > > Can we update the comment to make this clear? > > What part of the explanation is confusing? Dequeue failures happening all the > time? Timer with no delay has been tried? All of the above ;-) ? Sorry for the confusion. I was talking about "the timer is here first and foremost to cope with dequeueing failures; the enqueueing failures are expected only in NO_KEY situation", and is handled differently IIUIC.
Message was sent while issue was closed.
Description was changed from ========== Add stub MediaCdecAudioDecoder MediaCodecAudioDecoder 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ========== to ========== Add stub MediaCdecAudioDecoder MediaCodecAudioDecoder 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add stub MediaCdecAudioDecoder MediaCodecAudioDecoder 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 ========== to ========== Add stub MediaCdecAudioDecoder MediaCodecAudioDecoder 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. This CL only defines its interface and have notes about possible implementation. BUG=542910 Committed: https://crrev.com/b052c43a9c245c3c0f2b18365e5f7c1a75eca415 Cr-Commit-Position: refs/heads/master@{#374734} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b052c43a9c245c3c0f2b18365e5f7c1a75eca415 Cr-Commit-Position: refs/heads/master@{#374734} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
