Limit audio buffer size in the audio player used by the chromoting client.

Limit audio buffer size in the audio player used by the chromoting client.


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

[Sergey Ulanov, 2012-09-12 20:30:39]

wez: ping

[Wez, 2012-09-12 20:47:57]

LGTM w/ nits

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

http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:61: kFrameSizeMs * sampling_rate /
base::Time::kMillisecondsPerSecond);
nit: Since you do a similar calculation based on the max latency later on, wrap
this into a trivial static helper function GetFrameSize()?

http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:83:
DCHECK_EQ(packet->data(0).size() % (kChannels * kSampleSizeBytes), 0u);
Is this check correct; do we not allow splitting of samples across packet
boundaries in any of our Audio capturers?

http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:115: queued_samples_ +=
packet->data(0).size() / (kChannels * kSampleSizeBytes);
nit: Since you use kSampleSizeBytes * kChannels in several places, consider
defining it as its own constant e.g. kAllChannelsSampleBytes.

[Sergey Ulanov, 2012-09-12 21:43:34]

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

http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:61: kFrameSizeMs * sampling_rate /
base::Time::kMillisecondsPerSecond);
On 2012/09/12 20:47:57, Wez wrote:
> nit: Since you do a similar calculation based on the max latency later on,
wrap
> this into a trivial static helper function GetFrameSize()?

Done. Called the new function MsecToSamples()

http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:83:
DCHECK_EQ(packet->data(0).size() % (kChannels * kSampleSizeBytes), 0u);
On 2012/09/12 20:47:57, Wez wrote:
> Is this check correct; do we not allow splitting of samples across packet
> boundaries in any of our Audio capturers?

Yes, it's correct. There is no way you can encode/decode packets with partial
samples. I added check for this in the verbatim decoder and the capturers
guarantee that the packets they produce always contain integer number of
samples.

http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:115: queued_samples_ +=
packet->data(0).size() / (kChannels * kSampleSizeBytes);
On 2012/09/12 20:47:57, Wez wrote:
> nit: Since you use kSampleSizeBytes * kChannels in several places, consider
> defining it as its own constant e.g. kAllChannelsSampleBytes.

Done. I wish there was a term for set of samples for all channels. Media code
calls it a frame, but that would be confusing here, as we use "frame" for
different purposes.

[Wez, 2012-09-12 23:50:18]

On 2012/09/12 21:43:34, sergeyu wrote:
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> File remoting/client/plugin/pepper_audio_player.cc (right):
> 
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> remoting/client/plugin/pepper_audio_player.cc:61: kFrameSizeMs * sampling_rate
/
> base::Time::kMillisecondsPerSecond);
> On 2012/09/12 20:47:57, Wez wrote:
> > nit: Since you do a similar calculation based on the max latency later on,
> wrap
> > this into a trivial static helper function GetFrameSize()?
> 
> Done. Called the new function MsecToSamples()
> 
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> remoting/client/plugin/pepper_audio_player.cc:83:
> DCHECK_EQ(packet->data(0).size() % (kChannels * kSampleSizeBytes), 0u);
> On 2012/09/12 20:47:57, Wez wrote:
> > Is this check correct; do we not allow splitting of samples across packet
> > boundaries in any of our Audio capturers?
> 
> Yes, it's correct. There is no way you can encode/decode packets with partial
> samples. I added check for this in the verbatim decoder and the capturers
> guarantee that the packets they produce always contain integer number of
> samples.
> 
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> remoting/client/plugin/pepper_audio_player.cc:115: queued_samples_ +=
> packet->data(0).size() / (kChannels * kSampleSizeBytes);
> On 2012/09/12 20:47:57, Wez wrote:
> > nit: Since you use kSampleSizeBytes * kChannels in several places, consider
> > defining it as its own constant e.g. kAllChannelsSampleBytes.
> 
> Done. I wish there was a term for set of samples for all channels. Media code
> calls it a frame, but that would be confusing here, as we use "frame" for
> different purposes.

I think kAudioFrameSizeBytes would be OK here, although you'd want a comment to
briefly explain the term "frame".

[Sergey Ulanov, 2012-09-12 23:59:48]

On 2012/09/12 23:50:18, Wez wrote:
> On 2012/09/12 21:43:34, sergeyu wrote:
> >
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> > File remoting/client/plugin/pepper_audio_player.cc (right):
> > 
> >
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> > remoting/client/plugin/pepper_audio_player.cc:61: kFrameSizeMs *
sampling_rate
> /
> > base::Time::kMillisecondsPerSecond);
> > On 2012/09/12 20:47:57, Wez wrote:
> > > nit: Since you do a similar calculation based on the max latency later on,
> > wrap
> > > this into a trivial static helper function GetFrameSize()?
> > 
> > Done. Called the new function MsecToSamples()
> > 
> >
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> > remoting/client/plugin/pepper_audio_player.cc:83:
> > DCHECK_EQ(packet->data(0).size() % (kChannels * kSampleSizeBytes), 0u);
> > On 2012/09/12 20:47:57, Wez wrote:
> > > Is this check correct; do we not allow splitting of samples across packet
> > > boundaries in any of our Audio capturers?
> > 
> > Yes, it's correct. There is no way you can encode/decode packets with
partial
> > samples. I added check for this in the verbatim decoder and the capturers
> > guarantee that the packets they produce always contain integer number of
> > samples.
> > 
> >
>
http://codereview.chromium.org/10914210/diff/1/remoting/client/plugin/pepper_...
> > remoting/client/plugin/pepper_audio_player.cc:115: queued_samples_ +=
> > packet->data(0).size() / (kChannels * kSampleSizeBytes);
> > On 2012/09/12 20:47:57, Wez wrote:
> > > nit: Since you use kSampleSizeBytes * kChannels in several places,
consider
> > > defining it as its own constant e.g. kAllChannelsSampleBytes.
> > 
> > Done. I wish there was a term for set of samples for all channels. Media
code
> > calls it a frame, but that would be confusing here, as we use "frame" for
> > different purposes.
> 
> I think kAudioFrameSizeBytes would be OK here, although you'd want a comment
to
> briefly explain the term "frame".

There is also kFrameSizeMs in the same file, so kAudioFrameSizeBytes would be
confusing

[commit-bot: I haz the power, 2012-09-13 00:00:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10914210/1

[commit-bot: I haz the power, 2012-09-13 02:11:25]

Change committed as 156465