Piping for audio decoding.

Piping for audio decoding.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149990

[kxing, 2012-08-01 17:28:29]

Could someone PTAL?

[Sergey Ulanov, 2012-08-01 17:43:54]

http://codereview.chromium.org/10843031/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10843031/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.cc:147: scoped_ptr<AudioPacket> decoded_packet
=
Add DCHECK to verify that this method is called on |task_runner_|

http://codereview.chromium.org/10843031/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.cc:148: audio_decoder_->Decode(packet.Pass());
We shouldn't be decoding audio on the main plugin thread. Please add another
thread in ClientContext and use it to decode audio. It's best to move all
thread-switching logic to a new class similar to RectangleUpdateDecoder (e.g.
AudioDecodeScheduler).

http://codereview.chromium.org/10843031/diff/1/remoting/codec/audio_decoder_v...
File remoting/codec/audio_decoder_verbatim.cc (right):

http://codereview.chromium.org/10843031/diff/1/remoting/codec/audio_decoder_v...
remoting/codec/audio_decoder_verbatim.cc:17: return audio_packet.Pass();
Add DCHECK to verify codec field in audio_packet

[kxing, 2012-08-01 22:15:37]

Could someone PTAL again?

http://codereview.chromium.org/10843031/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10843031/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.cc:147: scoped_ptr<AudioPacket> decoded_packet
=
On 2012/08/01 17:43:54, sergeyu wrote:
> Add DCHECK to verify that this method is called on |task_runner_|

Done.

http://codereview.chromium.org/10843031/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.cc:148: audio_decoder_->Decode(packet.Pass());
On 2012/08/01 17:43:54, sergeyu wrote:
> We shouldn't be decoding audio on the main plugin thread. Please add another
> thread in ClientContext and use it to decode audio. It's best to move all
> thread-switching logic to a new class similar to RectangleUpdateDecoder (e.g.
> AudioDecodeScheduler).

Done.

http://codereview.chromium.org/10843031/diff/1/remoting/codec/audio_decoder_v...
File remoting/codec/audio_decoder_verbatim.cc (right):

http://codereview.chromium.org/10843031/diff/1/remoting/codec/audio_decoder_v...
remoting/codec/audio_decoder_verbatim.cc:17: return audio_packet.Pass();
On 2012/08/01 17:43:54, sergeyu wrote:
> Add DCHECK to verify codec field in audio_packet

Done.

[kxing, 2012-08-02 20:08:16]

Ping

[Sergey Ulanov, 2012-08-02 20:32:48]

Looks mostly good, but I have some nits, particularly regarding
creation/ownership of AudioDecodeScheduler.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
File remoting/client/audio_decode_scheduler.cc (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:24:
AudioDecodeScheduler::~AudioDecodeScheduler() {}
please move } to the next line - it's preferred formatting for .cc files and
more readable.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:27:
decoder_.reset(AudioDecoder::CreateAudioDecoder(config).release());
Add DCHECK to verify that this is called on the main thread?

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:35: base::Passed(packet.Pass())));
FYI alternative shorter syntax for Passed: base::Passed(&packet)

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:36: done.Run();
Done should be called only when we are finished decoding it. Move it to
ProcessDecodedPacket()?

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
File remoting/client/audio_decode_scheduler.h (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.h:10: #include
"remoting/codec/audio_decoder.h"
You don't need this include. AudioDecoder can be forward-declared

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.h:41: void
DecodePacket(scoped_ptr<AudioPacket> packet);
Shouldn't this be private?

http://codereview.chromium.org/10843031/diff/6002/remoting/client/chromoting_...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/chromoting_...
remoting/client/chromoting_client.h:110: AudioDecodeScheduler*
audio_decode_scheduler_;
Can we own it here?

http://codereview.chromium.org/10843031/diff/6002/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:475:
audio_decode_scheduler_.reset(new AudioDecodeScheduler(
Do we really need to create this object here? Can it be created inside
ChromotingClient? If you need that in order so that you can get audio decode
task runner, then maybe change ChromotingClient constructor to accept
ClientContext instead of just main task runner.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:482:
audio_decode_scheduler_.get()));
Please modify ChromotingClient to take ownership of the AudioDecodeScheduler. We
should do the same for RectangleUpdateDecoder too, but it can be done in a
separate CL.

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_verbatim.cc (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:12:
AudioDecoderVerbatim::AudioDecoderVerbatim() {}
please move } to the next line - it's preferred formatting for .cc files and
more readable. Same with destructor

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_verbatim.h (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.h:16: class AudioDecoderVerbatim : public
AudioDecoder {
nit add comments to explain that this class implements decoder for raw streams

[kxing, 2012-08-03 14:51:47]

Could someone PTAL again?

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
File remoting/client/audio_decode_scheduler.cc (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:24:
AudioDecodeScheduler::~AudioDecodeScheduler() {}
On 2012/08/02 20:32:48, sergeyu wrote:
> please move } to the next line - it's preferred formatting for .cc files and
> more readable.

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:27:
decoder_.reset(AudioDecoder::CreateAudioDecoder(config).release());
On 2012/08/02 20:32:48, sergeyu wrote:
> Add DCHECK to verify that this is called on the main thread?

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:35: base::Passed(packet.Pass())));
On 2012/08/02 20:32:48, sergeyu wrote:
> FYI alternative shorter syntax for Passed: base::Passed(&packet)

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.cc:36: done.Run();
On 2012/08/02 20:32:48, sergeyu wrote:
> Done should be called only when we are finished decoding it. Move it to
> ProcessDecodedPacket()? 

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
File remoting/client/audio_decode_scheduler.h (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.h:10: #include
"remoting/codec/audio_decoder.h"
On 2012/08/02 20:32:48, sergeyu wrote:
> You don't need this include. AudioDecoder can be forward-declared

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/audio_decod...
remoting/client/audio_decode_scheduler.h:41: void
DecodePacket(scoped_ptr<AudioPacket> packet);
On 2012/08/02 20:32:48, sergeyu wrote:
> Shouldn't this be private?

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/chromoting_...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/chromoting_...
remoting/client/chromoting_client.h:110: AudioDecodeScheduler*
audio_decode_scheduler_;
On 2012/08/02 20:32:48, sergeyu wrote:
> Can we own it here?

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:475:
audio_decode_scheduler_.reset(new AudioDecodeScheduler(
On 2012/08/02 20:32:48, sergeyu wrote:
> Do we really need to create this object here? Can it be created inside
> ChromotingClient? If you need that in order so that you can get audio decode
> task runner, then maybe change ChromotingClient constructor to accept
> ClientContext instead of just main task runner.

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:482:
audio_decode_scheduler_.get()));
On 2012/08/02 20:32:48, sergeyu wrote:
> Please modify ChromotingClient to take ownership of the AudioDecodeScheduler.
We
> should do the same for RectangleUpdateDecoder too, but it can be done in a
> separate CL.

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_verbatim.cc (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:12:
AudioDecoderVerbatim::AudioDecoderVerbatim() {}
On 2012/08/02 20:32:48, sergeyu wrote:
> please move } to the next line - it's preferred formatting for .cc files and
> more readable. Same with destructor

Done.

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_verbatim.h (right):

http://codereview.chromium.org/10843031/diff/6002/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.h:16: class AudioDecoderVerbatim : public
AudioDecoder {
On 2012/08/02 20:32:48, sergeyu wrote:
> nit add comments to explain that this class implements decoder for raw streams

Done.

[Sergey Ulanov, 2012-08-03 17:59:55]

LGTM provided my nits are addressed.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
File remoting/client/audio_decode_scheduler.cc (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.cc:49: done.Run();
MessageReader will need to jump to the main thread to process this callback, so
it's be better to call this callback in ProcessDecodedPacket().

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
File remoting/client/audio_decode_scheduler.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.h:42: void
DecodePacket(scoped_ptr<AudioPacket> packet, const base::Closure& done);
please add some short comments for this and then ext function to make it clear
which thread they are called on.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
remoting/client/chromoting_client.cc:219: if
(connection_->config().is_audio_enabled()) {
remove brackets for consistency with other one-line ifs in this file.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
remoting/client/chromoting_client.h:48: const ClientContext& client_context,
It's better to pass context by pointer instead of reference. We usually don't
use reference parameters for long-lived objects.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/client_con...
File remoting/client/client_context.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/client_con...
remoting/client/client_context.h:30: base::SingleThreadTaskRunner*
main_task_runner() const;
I don't think that const is justified here. The task runners are owned by the
context, and you can mutate them by posting tasks so it's not really const. Also
see my other comment about passing ClientContext by reference.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:56: class AudioDecodeScheduler;
don't need this/

http://codereview.chromium.org/10843031/diff/11003/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:195:
scoped_ptr<AudioDecodeScheduler> audio_decode_scheduler_;
remove this line.

[kxing, 2012-08-03 20:55:13]

I'll commit this soon, unless someone objects.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
File remoting/client/audio_decode_scheduler.cc (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.cc:49: done.Run();
On 2012/08/03 17:59:55, sergeyu wrote:
> MessageReader will need to jump to the main thread to process this callback,
so
> it's be better to call this callback in ProcessDecodedPacket().

Done.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
File remoting/client/audio_decode_scheduler.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.h:42: void
DecodePacket(scoped_ptr<AudioPacket> packet, const base::Closure& done);
On 2012/08/03 17:59:55, sergeyu wrote:
> please add some short comments for this and then ext function to make it clear
> which thread they are called on.

Done.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
remoting/client/chromoting_client.cc:219: if
(connection_->config().is_audio_enabled()) {
On 2012/08/03 17:59:55, sergeyu wrote:
> remove brackets for consistency with other one-line ifs in this file.

Done.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/chromoting...
remoting/client/chromoting_client.h:48: const ClientContext& client_context,
On 2012/08/03 17:59:55, sergeyu wrote:
> It's better to pass context by pointer instead of reference. We usually don't
> use reference parameters for long-lived objects.

Done.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/client_con...
File remoting/client/client_context.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/client_con...
remoting/client/client_context.h:30: base::SingleThreadTaskRunner*
main_task_runner() const;
On 2012/08/03 17:59:55, sergeyu wrote:
> I don't think that const is justified here. The task runners are owned by the
> context, and you can mutate them by posting tasks so it's not really const.
Also
> see my other comment about passing ClientContext by reference.

Done.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/10843031/diff/11003/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:56: class AudioDecodeScheduler;
On 2012/08/03 17:59:55, sergeyu wrote:
> don't need this/

Done.

http://codereview.chromium.org/10843031/diff/11003/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:195:
scoped_ptr<AudioDecodeScheduler> audio_decode_scheduler_;
On 2012/08/03 17:59:55, sergeyu wrote:
> remove this line.

Done.

[commit-bot: I haz the power, 2012-08-03 21:40:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10843031/5018

[commit-bot: I haz the power, 2012-08-03 21:57:18]

Try job failure for 10843031-5018 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-03 22:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10843031/3006

[commit-bot: I haz the power, 2012-08-04 01:15:25]

Change committed as 149990

[Wez, 2012-08-07 18:29:04]

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
File remoting/client/audio_decode_scheduler.cc (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
remoting/client/audio_decode_scheduler.cc:30:
decoder_.reset(AudioDecoder::CreateAudioDecoder(config).release());
nit: decoder_ = AudioDecoder::CreateAudioDecoder(config); ?

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
remoting/client/audio_decode_scheduler.cc:37:
&AudioDecodeScheduler::DecodePacket, base::Unretained(this),
You can't use base::Unretained(this) unless you have some other way to guarantee
that all such tasks you posted to the thread will have been processed by the
time you delete the AudioDecodeScheduler, or will be ignored once it's gone.
You'll need to split AudioDecodeScheduler's DecodePacket handling into a
separate ref-counted class internal to the scheduler, and which posts back to
ProcessDecodedPacket via a WeakPtr to the main scheduler object.

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
remoting/client/audio_decode_scheduler.cc:47:
&AudioDecodeScheduler::ProcessDecodedPacket, base::Unretained(this),
base::Unretained(this) -> weak_ptr_, where weak_ptr_ is initialised in
Initialize() or the constructore, from a weak_factory_ member of the
AudioDecodeScheduler itself.

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
remoting/client/audio_decode_scheduler.cc:54:
audio_player_->ProcessAudioPacket(packet.Pass());
Presumably ProcessAudioPacket plays back asynchronously, so should the done task
be passed to that, so that the caller actually gets notified only when the audio
has actually been played?

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
File remoting/client/audio_decode_scheduler.h (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/aud...
remoting/client/audio_decode_scheduler.h:29:
scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
nit: It's not clear what it means for this to be the "main" TaskRunner; it's the
TaskRunner we expect our AudioStub API to be called on, and the one that we
dispatch to the AudioPlayer on, so you might call it the "playback" TaskRunner,
or simply add a comment to clarify how the two TaskRunners will be used.

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/chr...
File remoting/client/chromoting_client.cc (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/chr...
remoting/client/chromoting_client.cc:73: audio_decode_scheduler_.get());
nit: Hmmm, Connect() is getting bloated with parameters not actually related to
connecting; we should move these out into separate setters e.g.
set_audio_scheduler(), in a follow-up CL.

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/cli...
File remoting/client/client_context.cc (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/cli...
remoting/client/client_context.cc:11:
decode_thread_("ChromotingClientDecodeThread"),
nit: ChromotingClientVideoDecodeThread, etc

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/cli...
File remoting/client/client_context.h (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/cli...
remoting/client/client_context.h:31: base::SingleThreadTaskRunner*
decode_task_runner();
nit: video_decode_task_runner()

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/client/cli...
remoting/client/client_context.h:32: base::SingleThreadTaskRunner*
audio_decode_task_runner();
What is the intent in running audio decode on a separate thread from video
decode? Most client devices will have a limited number of cores, and we're
already running VP8 decode mutli-threaded, so except on high-end systems an
additional thread just adds scheduling overhead. Is the aim to ensure that long
video decodes don't block audio decode for too long, for example?

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/codec/audi...
File remoting/codec/audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/codec/audi...
remoting/codec/audio_decoder.cc:21: NOTIMPLEMENTED();
nit: NOTIMPLEMENTED isn't really appropriate here, since this function _is_
implemented and the caller has just supplied an invalid configuration.

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/codec/audi...
File remoting/codec/audio_decoder.h (right):

https://chromiumcodereview.appspot.com/10843031/diff/3006/remoting/codec/audi...
remoting/codec/audio_decoder.h:25: virtual scoped_ptr<AudioPacket>
Decode(scoped_ptr<AudioPacket> packet) = 0;
Given that the AudioEncoder and AudioDecoder interfaces are all but identical,
transcoding AudioPacket->AudioPacket, would a single AudioTranscoder interface
make more sense?