Support for both 44.1 kHz and 48 kHz on the client.

Support for both 44.1 kHz and 48 kHz on the client.


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

[kxing, 2012-07-23 17:53:38]

Could someone PTAL?

[Sergey Ulanov, 2012-07-23 22:01:46]

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_audio_player.cc (right):

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:37: DoStop();
Do we really need to call this explicitly? Doesn't it stop implicitly if we just
destroy |audio_|.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:41: running_ = true;
Do we want to do that even when we fail to start?

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:43: return DoStart();
That's going to fail if audio_ is null. I wonder why we need Start() method at
all? Can we start playback implicitly when the first audio packet is received?

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:47: bool
PepperAudioPlayer::DoStart() {
Doesn't seem like a good name given that it called not only from Start(). Maybe
InitializePlayback()? or StartPlayback()?

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:61: PP_AudioSampleRate
PepperAudioPlayer::ConvertToPepperSampleRate(
Doesn't need to be a class member. Just make it a static function in anonymous
namespace.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:107: DoStop();
ResetAudioPlayer() creates new pp::Audio object anyway, so I don't think you
need explicit stop here.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_audio_player.h (right):

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.h:70: // Equal to true, only if
Start() has been called.
this comment seems to be redundant. maybe just rename this variable to started_?

[kxing, 2012-07-23 22:58:46]

Could someone PTAL again?

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_audio_player.cc (right):

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:37: DoStop();
On 2012/07/23 22:01:47, sergeyu wrote:
> Do we really need to call this explicitly? Doesn't it stop implicitly if we
just
> destroy |audio_|.

Done.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:41: running_ = true;
On 2012/07/23 22:01:47, sergeyu wrote:
> Do we want to do that even when we fail to start?

Done.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:43: return DoStart();
On 2012/07/23 22:01:47, sergeyu wrote:
> That's going to fail if audio_ is null. I wonder why we need Start() method at
> all? Can we start playback implicitly when the first audio packet is received?

Done.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:47: bool
PepperAudioPlayer::DoStart() {
On 2012/07/23 22:01:47, sergeyu wrote:
> Doesn't seem like a good name given that it called not only from Start().
Maybe
> InitializePlayback()? or StartPlayback()?

Done.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:61: PP_AudioSampleRate
PepperAudioPlayer::ConvertToPepperSampleRate(
On 2012/07/23 22:01:47, sergeyu wrote:
> Doesn't need to be a class member. Just make it a static function in anonymous
> namespace.

Done.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:107: DoStop();
On 2012/07/23 22:01:47, sergeyu wrote:
> ResetAudioPlayer() creates new pp::Audio object anyway, so I don't think you
> need explicit stop here.

Done.

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_audio_player.h (right):

http://codereview.chromium.org/10795066/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.h:70: // Equal to true, only if
Start() has been called.
On 2012/07/23 22:01:47, sergeyu wrote:
> this comment seems to be redundant. maybe just rename this variable to
started_?

Done.

[Sergey Ulanov, 2012-07-23 23:32:02]

http://codereview.chromium.org/10795066/diff/5002/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_audio_player.cc (right):

http://codereview.chromium.org/10795066/diff/5002/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:86: StartPlayback();
we never use result returned from that function. It may fail and as result we
will keep queuing data in |queued_packets_|, but will never use it. I think you
need to save the result and queue data only when we were able to start
successfully.

Also you should drop data we have left in the queue, otherwise you might be
playing it with a wrong sampling rate. Alternatively you can keep it and then
AudioPlayer should be restarted later once you are finished playing what's left
for the old sampling rate. That would be more complicated solution and probably
isn't worth it as we don't expect sampling rate to change very often.

[kxing, 2012-07-24 17:20:43]

Could someone PTAL again?

http://codereview.chromium.org/10795066/diff/5002/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_audio_player.cc (right):

http://codereview.chromium.org/10795066/diff/5002/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:86: StartPlayback();
On 2012/07/23 23:32:02, sergeyu wrote:
> we never use result returned from that function. It may fail and as result we
> will keep queuing data in |queued_packets_|, but will never use it. I think
you
> need to save the result and queue data only when we were able to start
> successfully.
> 
> Also you should drop data we have left in the queue, otherwise you might be
> playing it with a wrong sampling rate. Alternatively you can keep it and then
> AudioPlayer should be restarted later once you are finished playing what's
left
> for the old sampling rate. That would be more complicated solution and
probably
> isn't worth it as we don't expect sampling rate to change very often.

Done.

[Sergey Ulanov, 2012-07-24 18:10:03]

lgtm when my last two comments are addressed

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_audio_player.cc (right):

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:54: void
PepperAudioPlayer::ResetAudioPlayer(
Don't think we need to separate StartPlayback() and ResetAudioPlayer(). Please
merge them in one method.

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_audio_player.h (right):

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.h:61: bool failed_playback_;
start_failed_?

[kxing, 2012-07-24 18:32:00]

I'll commit this soon, unless someone objects.

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_audio_player.cc (right):

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:54: void
PepperAudioPlayer::ResetAudioPlayer(
On 2012/07/24 18:10:03, sergeyu wrote:
> Don't think we need to separate StartPlayback() and ResetAudioPlayer(). Please
> merge them in one method.

Done.

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_audio_player.h (right):

http://codereview.chromium.org/10795066/diff/5004/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.h:61: bool failed_playback_;
On 2012/07/24 18:10:03, sergeyu wrote:
> start_failed_?

Done.

[commit-bot: I haz the power, 2012-07-24 19:34:34]

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

[commit-bot: I haz the power, 2012-07-24 21:23:01]

Change committed as 148203

[Wez, 2012-08-07 20:09:48]

Some belated comments.

I notice that AudioPlayer now looks very similar to the new AudioDecoder/Encoder
interfaces; you could merge them into a single AudioHandler interface with a
HandleAudioPacket() method, and have AudioEncoder/Decoders passed a target
AudioHandler when they are created; they would then process packets and pass
them to the target handler rather than returning them to the caller.

https://chromiumcodereview.appspot.com/10795066/diff/5/remoting/client/plugin...
File remoting/client/plugin/pepper_audio_player.cc (right):

https://chromiumcodereview.appspot.com/10795066/diff/5/remoting/client/plugin...
remoting/client/plugin/pepper_audio_player.cc:48: AudioPacket::SamplingRate
sampling_rate) {
nit: It seems cleaner to clear any queued packets in this function that doing it
in ProcessAudioPacket, since this function is supposed to reset the AudioPlayer.

https://chromiumcodereview.appspot.com/10795066/diff/5/remoting/client/plugin...
remoting/client/plugin/pepper_audio_player.cc:70: LOG(ERROR) << "Failed to start
Pepper audio player";
nit: Why log this here, rather than letting the caller log it?

https://chromiumcodereview.appspot.com/10795066/diff/5/remoting/client/plugin...
remoting/client/plugin/pepper_audio_player.cc:88: // Start the Pepper audio
player if this is the first packet.
nit: If you perform this check before testing start_failed_ then you don't need
the early-return on !success below; you simply set start_failed_ to
!ResetAudioPlayer(...).

https://chromiumcodereview.appspot.com/10795066/diff/5/remoting/client/plugin...
remoting/client/plugin/pepper_audio_player.cc:89: if (sampling_rate_ !=
packet->sampling_rate()) {
nit: You could move this check into ResetAudioPlayer() and have it early-exit if
the rates match, so that the rate-change logic is all in that one function;
you'd then queue the packet only if ResetAudioPlayer returned true, and
ResetAudioPlayer would be responsible for testing/setting start_failed_ whenever
the rate changed.