Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1815553002:
Implement MojoAudioDecoder (Closed)
|
Created:
4 years, 9 months ago by Tima Vaisburd Modified:
4 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@spitzer-audio-client-connect Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement MojoAudioDecoder
Added MojoAudioDecoder implementation for Initialize(), Decode() and Reset(). It does not accept the decoded audio buffers yet.
BUG=542910
Committed: https://crrev.com/fdd853c71ec52b9020dddb9aea68695cc1d909f6
Cr-Commit-Position: refs/heads/master@{#382070}
Patch Set 1 #Patch Set 2 : Finished client side implementation #
Total comments: 26
Patch Set 3 : Addressed some comments and added some questions #
Total comments: 2
Patch Set 4 : Added DCHECK for no pending decode_cb on reset and a comment #
Dependent Patchsets: Messages
Total messages: 17 (7 generated)
Description was changed from ========== Implement MojoAudioDecoder initialization This CL makes MojoAudioDecoder inherit from interfaces::AudioDecoderClient and passes the AudioDecoderClient to the service. BUG=542910 ========== to ========== Implement MojoAudioDecoder initialization This CL makes MojoAudioDecoder inherit from interfaces::AudioDecoderClient and passes the AudioDecoderClient to the service. BUG=542910 ==========
Description was changed from ========== Implement MojoAudioDecoder initialization This CL makes MojoAudioDecoder inherit from interfaces::AudioDecoderClient and passes the AudioDecoderClient to the service. BUG=542910 ========== to ========== Implement MojoAudioDecoder initialization Make MojoAudioDecoder inherit from interfaces::AudioDecoderClient and pass the AudioDecoderClient to the service. BUG=542910 ==========
timav@chromium.org changed reviewers: + sandersd@chromium.org, xhwang@chromium.org
Another tiny step. PTAL.
Description was changed from ========== Implement MojoAudioDecoder initialization Make MojoAudioDecoder inherit from interfaces::AudioDecoderClient and pass the AudioDecoderClient to the service. BUG=542910 ========== to ========== Implement MojoAudioDecoder Added MojoAudioDecoder implementation for Initialize(), Decode() and Reset(). It does not accept the decoded audio buffers yet. BUG=542910 ==========
On 2016/03/17 23:40:15, Tima Vaisburd wrote: > Another tiny step. PTAL. I went ahead and added implementation for other methods except the processing of the decoded results, i.e. |AudioBuffers|. It can be easily split back, please let me know if you'd like me to do that.
looking good! Just have some comments. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:90: remote_decoder_->Decode( Add a comment that this will not work. We need a DataPipe to send the real data. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:111: reset_cb_ = closure; Do you want to call remote_decoder_->Reset() here? Note that you'll want to wait for the decode_cb to be fired to fire the reset_cb: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... See example here: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr... https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:155: static media::AudioDecoder::Status ConvertDecodeStatus( FYI, we have a bug to track the work to avoid this conversion. But this is fine now. https://bugs.chromium.org/p/chromium/issues/detail?id=581405 https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:174: if (!decode_cb_.is_null()) When would this happen? Typically we assume this should not happen. Note that if connection error happened OnDecodeStarted or OnResetDone should not be dispatched. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:182: if (!reset_cb_.is_null()) ditto https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:39: // AudioDecoderClient implementation. nit: drop one space https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:43: // Callback for connection error on |remote_renderer_|. s/renderer/decoder https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:46: // Called when |remote_decored_| finished initialization. decoder https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:50: void OnDecodeStarted(interfaces::AudioDecoder::DecodeStatus decode_status); Started is not accurate. It's possible that OnBufferDecoded() is called before this callback. How about something like OnDecodeStatus? https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:55: // Data. nit: seems not necessary..
https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:90: remote_decoder_->Decode( On 2016/03/18 02:19:43, xhwang wrote: > Add a comment that this will not work. We need a DataPipe to send the real data. In the doc you said we can use MessagePipe instead of DataPipe as a first step. Please explain. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:111: reset_cb_ = closure; On 2016/03/18 02:19:43, xhwang wrote: > Do you want to call remote_decoder_->Reset() here? > > Note that you'll want to wait for the decode_cb to be fired to fire the > reset_cb: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... > > See example here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr... Yes, I forgot to call remote decoder. Thank you. I do not quite understand why do we have to do any pending treatment that you have referenced. If the order of the calls is preserved across IPC, the Decode() should arrive to the service before Reset(), and then MCAD will take care to send OnDecodeStatus() before OnReset()? What do I miss? https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:155: static media::AudioDecoder::Status ConvertDecodeStatus( On 2016/03/18 02:19:43, xhwang wrote: > FYI, we have a bug to track the work to avoid this conversion. But this is fine > now. > > https://bugs.chromium.org/p/chromium/issues/detail?id=581405 I've seen static_cast<>, maybe just cast? https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:174: if (!decode_cb_.is_null()) On 2016/03/18 02:19:43, xhwang wrote: > When would this happen? Typically we assume this should not happen. > > Note that if connection error happened OnDecodeStarted or OnResetDone should not > be dispatched. Yes, I assumed these can be called after connection error. Changed to DCHECK(). https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:182: if (!reset_cb_.is_null()) On 2016/03/18 02:19:43, xhwang wrote: > ditto Changed to DCHECK() https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:39: // AudioDecoderClient implementation. On 2016/03/18 02:19:43, xhwang wrote: > nit: drop one space Done. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:43: // Callback for connection error on |remote_renderer_|. On 2016/03/18 02:19:43, xhwang wrote: > s/renderer/decoder Done. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:46: // Called when |remote_decored_| finished initialization. On 2016/03/18 02:19:43, xhwang wrote: > decoder Done. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:50: void OnDecodeStarted(interfaces::AudioDecoder::DecodeStatus decode_status); On 2016/03/18 02:19:43, xhwang wrote: > Started is not accurate. It's possible that OnBufferDecoded() is called before > this callback. How about something like OnDecodeStatus? Started is not accurate because the buffer can be rejected. I renamed to OnDecodeStatus. However I do not see how can OnBufferDecoded() come before OnDecodeStatus() for this buffer. in MCAD everything is sequential and, I think, the order of IPC messages is preserved? Or it is preserved within one pipe only? https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:55: // Data. On 2016/03/18 02:19:43, xhwang wrote: > nit: seems not necessary.. Removed.
https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:90: remote_decoder_->Decode( On 2016/03/18 19:05:49, Tima Vaisburd wrote: > On 2016/03/18 02:19:43, xhwang wrote: > > Add a comment that this will not work. We need a DataPipe to send the real > data. > > In the doc you said we can use MessagePipe instead of DataPipe as a first step. > Please explain. There are two places we need to use DataPipe, one is to send DecoderBuffer over. One is to send AudioBuffer back. For DecoderBuffer, the data is not serialized into the message pipe at all, so this code will not work (and worth a comment). Later you can add DataPipe so it'll work. For AudioBuffer, today we are using MessagePipe to serialize the data. This should work for now. In the future, we may still want to use DataPipe for better performance. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:155: static media::AudioDecoder::Status ConvertDecodeStatus( On 2016/03/18 19:05:49, Tima Vaisburd wrote: > On 2016/03/18 02:19:43, xhwang wrote: > > FYI, we have a bug to track the work to avoid this conversion. But this is > fine > > now. > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=581405 > > I've seen static_cast<>, maybe just cast? The bug will fix the issue completely. If you do cast, you need static_assert. The currently conversion sgtm. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:50: void OnDecodeStarted(interfaces::AudioDecoder::DecodeStatus decode_status); On 2016/03/18 19:05:49, Tima Vaisburd wrote: > On 2016/03/18 02:19:43, xhwang wrote: > > Started is not accurate. It's possible that OnBufferDecoded() is called > before > > this callback. How about something like OnDecodeStatus? > > Started is not accurate because the buffer can be rejected. I renamed to > OnDecodeStatus. > However I do not see how can OnBufferDecoded() come before OnDecodeStatus() for > this buffer. in MCAD everything is sequential and, I think, the order of IPC > messages is preserved? Or it is preserved within one pipe only? mojo_audio_decoder is generic, it should work with any AudioDecoder implementation. You should code against the AudioDecoder API, which says that the output_cb may be called before or after the decode_cb: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... https://chromiumcodereview.appspot.com/1815553002/diff/40001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1815553002/diff/40001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:184: DCHECK(!reset_cb_.is_null()); DCHECK that there's no decode callback pending.
https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:90: remote_decoder_->Decode( On 2016/03/18 19:17:17, xhwang wrote: > On 2016/03/18 19:05:49, Tima Vaisburd wrote: > > On 2016/03/18 02:19:43, xhwang wrote: > > > Add a comment that this will not work. We need a DataPipe to send the real > > data. > > > > In the doc you said we can use MessagePipe instead of DataPipe as a first > step. > > Please explain. > > There are two places we need to use DataPipe, one is to send DecoderBuffer over. > One is to send AudioBuffer back. > > For DecoderBuffer, the data is not serialized into the message pipe at all, so > this code will not work (and worth a comment). Later you can add DataPipe so > it'll work. > > For AudioBuffer, today we are using MessagePipe to serialize the data. This > should work for now. In the future, we may still want to use DataPipe for better > performance. Added comment. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:155: static media::AudioDecoder::Status ConvertDecodeStatus( On 2016/03/18 19:17:17, xhwang wrote: > On 2016/03/18 19:05:49, Tima Vaisburd wrote: > > On 2016/03/18 02:19:43, xhwang wrote: > > > FYI, we have a bug to track the work to avoid this conversion. But this is > > fine > > > now. > > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=581405 > > > > I've seen static_cast<>, maybe just cast? > > The bug will fix the issue completely. If you do cast, you need static_assert. > The currently conversion sgtm. Acknowledged. https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.h (right): https://chromiumcodereview.appspot.com/1815553002/diff/20001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.h:50: void OnDecodeStarted(interfaces::AudioDecoder::DecodeStatus decode_status); On 2016/03/18 19:17:17, xhwang wrote: > On 2016/03/18 19:05:49, Tima Vaisburd wrote: > > On 2016/03/18 02:19:43, xhwang wrote: > > > Started is not accurate. It's possible that OnBufferDecoded() is called > > before > > > this callback. How about something like OnDecodeStatus? > > > > Started is not accurate because the buffer can be rejected. I renamed to > > OnDecodeStatus. > > However I do not see how can OnBufferDecoded() come before OnDecodeStatus() > for > > this buffer. in MCAD everything is sequential and, I think, the order of IPC > > messages is preserved? Or it is preserved within one pipe only? > > mojo_audio_decoder is generic, it should work with any AudioDecoder > implementation. You should code against the AudioDecoder API, which says that > the output_cb may be called before or after the decode_cb: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... I see. Acknowledged. https://chromiumcodereview.appspot.com/1815553002/diff/40001/media/mojo/servi... File media/mojo/services/mojo_audio_decoder.cc (right): https://chromiumcodereview.appspot.com/1815553002/diff/40001/media/mojo/servi... media/mojo/services/mojo_audio_decoder.cc:184: DCHECK(!reset_cb_.is_null()); On 2016/03/18 19:17:17, xhwang wrote: > DCHECK that there's no decode callback pending. Done.
lgtm
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1815553002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1815553002/60001
Message was sent while issue was closed.
Description was changed from ========== Implement MojoAudioDecoder Added MojoAudioDecoder implementation for Initialize(), Decode() and Reset(). It does not accept the decoded audio buffers yet. BUG=542910 ========== to ========== Implement MojoAudioDecoder Added MojoAudioDecoder implementation for Initialize(), Decode() and Reset(). It does not accept the decoded audio buffers yet. BUG=542910 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Implement MojoAudioDecoder Added MojoAudioDecoder implementation for Initialize(), Decode() and Reset(). It does not accept the decoded audio buffers yet. BUG=542910 ========== to ========== Implement MojoAudioDecoder Added MojoAudioDecoder implementation for Initialize(), Decode() and Reset(). It does not accept the decoded audio buffers yet. BUG=542910 Committed: https://crrev.com/fdd853c71ec52b9020dddb9aea68695cc1d909f6 Cr-Commit-Position: refs/heads/master@{#382070} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fdd853c71ec52b9020dddb9aea68695cc1d909f6 Cr-Commit-Position: refs/heads/master@{#382070} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
