Changed AudioPacket data to a repeated field.

Changed AudioPacket data to a repeated field.


BUG=


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

[kxing, 2012-08-14 17:24:38]

Needed for http://codereview.chromium.org/10831246/

Could someone PTAL?

[Sergey Ulanov, 2012-08-14 18:50:04]

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

http://codereview.chromium.org/10827324/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:78: if (packet->data(0).size() %
(kChannels * kSampleSizeBytes) != 0) {
data(index) dchecks that the index is correct, so you don't really need the
DCHECK above. Problem is that you still want to handle correctly the case when
when there is no data in the packet. Add condition packet->data_size() == 1 in
this if? Also I think you need to verify that encoding in the packet is set to
ENCODE_RAW.

http://codereview.chromium.org/10827324/diff/1/remoting/codec/audio_encoder_v...
File remoting/codec/audio_encoder_verbatim.cc (right):

http://codereview.chromium.org/10827324/diff/1/remoting/codec/audio_encoder_v...
remoting/codec/audio_encoder_verbatim.cc:19: DCHECK_EQ(1, packet->data_size());
Can you please modify decoder too? On the decoder side we need to drop invalid
packets instead of DCHECK'ing.

http://codereview.chromium.org/10827324/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10827324/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:298: bool
AudioCapturerWin::IsPacketOfSilence(const AudioPacket* packet) {
Maybe change this method to look at raw data instead of AudioPacket? That would
allow to avoid copying data to AudioPacket for silent packets.

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto
File remoting/proto/audio.proto (right):

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto#new...
remoting/proto/audio.proto:16: // Data is always signed and represented using
little endian.
this makes sense only for raw audio packets. Update the comment to make it
clear. And meaybe move it after the following sentence.

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto#new...
remoting/proto/audio.proto:17: // Raw data will have exactly one data field.
s/Raw data will/Packets with raw data must/

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto#new...
remoting/proto/audio.proto:18: // For encoded data, each data field represents
one frame.
It may not be necessary to frame data, depending on encoder.
Suggest rewording: Some encoders (e.g. Speex) may add multiple data fields to
separate encoded frames.

[kxing, 2012-08-14 20:56:15]

Could someone PTAL again?

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

http://codereview.chromium.org/10827324/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:78: if (packet->data(0).size() %
(kChannels * kSampleSizeBytes) != 0) {
On 2012/08/14 18:50:05, sergeyu wrote:
> data(index) dchecks that the index is correct, so you don't really need the
> DCHECK above. Problem is that you still want to handle correctly the case when
> when there is no data in the packet. Add condition packet->data_size() == 1 in
> this if? Also I think you need to verify that encoding in the packet is set to
> ENCODE_RAW.

Done.

http://codereview.chromium.org/10827324/diff/1/remoting/codec/audio_encoder_v...
File remoting/codec/audio_encoder_verbatim.cc (right):

http://codereview.chromium.org/10827324/diff/1/remoting/codec/audio_encoder_v...
remoting/codec/audio_encoder_verbatim.cc:19: DCHECK_EQ(1, packet->data_size());
On 2012/08/14 18:50:05, sergeyu wrote:
> Can you please modify decoder too? On the decoder side we need to drop invalid
> packets instead of DCHECK'ing.

Done.

http://codereview.chromium.org/10827324/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10827324/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:298: bool
AudioCapturerWin::IsPacketOfSilence(const AudioPacket* packet) {
On 2012/08/14 18:50:05, sergeyu wrote:
> Maybe change this method to look at raw data instead of AudioPacket? That
would
> allow to avoid copying data to AudioPacket for silent packets.

Done.

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto
File remoting/proto/audio.proto (right):

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto#new...
remoting/proto/audio.proto:16: // Data is always signed and represented using
little endian.
On 2012/08/14 18:50:05, sergeyu wrote:
> this makes sense only for raw audio packets. Update the comment to make it
> clear. And meaybe move it after the following sentence.

Done.

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto#new...
remoting/proto/audio.proto:17: // Raw data will have exactly one data field.
On 2012/08/14 18:50:05, sergeyu wrote:
> s/Raw data will/Packets with raw data must/

Done.

http://codereview.chromium.org/10827324/diff/1/remoting/proto/audio.proto#new...
remoting/proto/audio.proto:18: // For encoded data, each data field represents
one frame.
On 2012/08/14 18:50:05, sergeyu wrote:
> It may not be necessary to frame data, depending on encoder.
> Suggest rewording: Some encoders (e.g. Speex) may add multiple data fields to
> separate encoded frames.

Done.

[Sergey Ulanov, 2012-08-15 00:47:20]

several more nits.

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

http://codereview.chromium.org/10827324/diff/5001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:79: if (!packet.get())
You don't need this. scoped_ptr's -> operation will dcheck below if packet is
null, and we expect this method to be called only for non-null packets.

http://codereview.chromium.org/10827324/diff/5001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:83:
CHECK_EQ(AudioPacket::ENCODING_RAW, packet->encoding());
nit: this doesn't really need to be CHECK - DCHECK() would be sufficient

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

http://codereview.chromium.org/10827324/diff/5001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:23: (packet->sampling_rate() ==
AudioPacket::SAMPLING_RATE_INVALID) ||
do we require sampling_rate field to be present in each packet? maybe require it
only in the first packet and when it changes?

http://codereview.chromium.org/10827324/diff/5001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:25: return
scoped_ptr<AudioPacket>(NULL);
nit: drop NULL, just scoped_ptr<AudioPacket>()

http://codereview.chromium.org/10827324/diff/5001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10827324/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:284: if (!IsPacketOfSilence(
Now you can move this above, before |packet| is created, and pass |data| instead
of packet->data(0) to it.

[kxing, 2012-08-15 16:00:05]

Could someone PTAL again?

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

http://codereview.chromium.org/10827324/diff/5001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:79: if (!packet.get())
A packet might end up null if some malicious host sends corrupted packets.

http://codereview.chromium.org/10827324/diff/5001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:83:
CHECK_EQ(AudioPacket::ENCODING_RAW, packet->encoding());
On 2012/08/15 00:47:20, sergeyu wrote:
> nit: this doesn't really need to be CHECK - DCHECK() would be sufficient

Done.

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

http://codereview.chromium.org/10827324/diff/5001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:23: (packet->sampling_rate() ==
AudioPacket::SAMPLING_RATE_INVALID) ||
Yes, currently we require sampling rate to be present in each packet. I'm not
sure how well it would work if we only require it in the first packet and when
it changes, if we eventually switch to UDP, and we start dropping packets.

http://codereview.chromium.org/10827324/diff/5001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:25: return
scoped_ptr<AudioPacket>(NULL);
On 2012/08/15 00:47:20, sergeyu wrote:
> nit: drop NULL, just scoped_ptr<AudioPacket>()

Done.

http://codereview.chromium.org/10827324/diff/5001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10827324/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:284: if (!IsPacketOfSilence(
On 2012/08/15 00:47:20, sergeyu wrote:
> Now you can move this above, before |packet| is created, and pass |data|
instead
> of packet->data(0) to it.

Done.

[Sergey Ulanov, 2012-08-15 17:38:09]

LGTM when my nits are addressed.

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

http://codereview.chromium.org/10827324/diff/5001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:79: if (!packet.get())
On 2012/08/15 16:00:05, kxing wrote:
> A packet might end up null if some malicious host sends corrupted packets.

Oh, I see. Maybe better to handle this case in AudioDecodeScheduler?

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

http://codereview.chromium.org/10827324/diff/9001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:82: DCHECK_EQ(1,
packet->data_size());
Actually I meant that we need CHECK() here, but DCHECK() for encoding(). If
encoding() is not set to ENCODING_RAW - nothing horrible is going to happen, we
would just play some noise. If there is no data in the packet then we may end up
accessing memory at some random address when we use data(0). This potentially
could lead to a security vulnerability. It's not a big problem in this
particular case because we don't write anything to packet->data(0), still it's
better to be safe.

http://codereview.chromium.org/10827324/diff/9001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10827324/diff/9001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:278: reinterpret_cast<const int16*>(data),
nit: incorrect indentation. This line should be indented 4 spaces relative to
the first ( on the previous line. Or it can fit on the previous line.

http://codereview.chromium.org/10827324/diff/9001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:303: const int16* samples,
nit: can this fit on the previous line.

[kxing, 2012-08-15 18:41:54]

I'll commit this soon, unless someone objects.

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

http://codereview.chromium.org/10827324/diff/9001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_audio_player.cc:82: DCHECK_EQ(1,
packet->data_size());
On 2012/08/15 17:38:10, sergeyu wrote:
> Actually I meant that we need CHECK() here, but DCHECK() for encoding(). If
> encoding() is not set to ENCODING_RAW - nothing horrible is going to happen,
we
> would just play some noise. If there is no data in the packet then we may end
up
> accessing memory at some random address when we use data(0). This potentially
> could lead to a security vulnerability. It's not a big problem in this
> particular case because we don't write anything to packet->data(0), still it's
> better to be safe.

Done.

http://codereview.chromium.org/10827324/diff/9001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10827324/diff/9001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:278: reinterpret_cast<const int16*>(data),
On 2012/08/15 17:38:10, sergeyu wrote:
> nit: incorrect indentation. This line should be indented 4 spaces relative to
> the first ( on the previous line. Or it can fit on the previous line.

Done.

http://codereview.chromium.org/10827324/diff/9001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:303: const int16* samples,
On 2012/08/15 17:38:10, sergeyu wrote:
> nit: can this fit on the previous line.

Done.

[commit-bot: I haz the power, 2012-08-15 19:41:47]

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

[commit-bot: I haz the power, 2012-08-15 20:54:11]

Try job failure for 10827324-8 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-15 23:12:21]

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

[commit-bot: I haz the power, 2012-08-16 01:59:50]

Change committed as 151825