Added more error checking for audio packets.

Added more error checking for audio packets.


BUG=


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

[kxing, 2012-08-20 20:51:31]

Could someone PTAL?

[Sergey Ulanov, 2012-08-20 22:26:19]

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

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:79: if (!packet.get())
I think I already mentioned in one of the previous reviews - it's better to
check for this in AudioDecodeScheduler and call ProcessAudioPacket() only for
not-null packets.

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:85:
DCHECK_NE(AudioPacket::BYTES_PER_SAMPLE_INVALID, packet->bytes_per_sample());
check that bytes_per_sample() == kSampleSizeBytes?

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:86:
DCHECK_NE(AudioPacket::CHANNELS_INVALID, packet->channels());
I think you want to check that channels()==kChannels. The player currently
supports only two channels.

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:87: if (packet->data(0).size() %
(kChannels * kSampleSizeBytes) != 0) {
add empty line above this one.

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

http://codereview.chromium.org/10823420/diff/1/remoting/codec/audio_decoder_v...
remoting/codec/audio_decoder_verbatim.cc:24: (packet->bytes_per_sample() ==
AudioPacket::BYTES_PER_SAMPLE_INVALID) ||
here you want to check that bytes_per_sample() == 2 as that's the only format
currently supported by the player.

http://codereview.chromium.org/10823420/diff/1/remoting/codec/audio_decoder_v...
remoting/codec/audio_decoder_verbatim.cc:25: (packet->channels() ==
AudioPacket::CHANNELS_INVALID)) {
check that we have two channels, not only that the number of channels is valid.
If the host sends us Mono stream the current player implementation will try to
play it as stereo.

[kxing, 2012-08-20 22:48:22]

Could someone PTAL again?

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

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:79: if (!packet.get())
On 2012/08/20 22:26:20, sergeyu wrote:
> I think I already mentioned in one of the previous reviews - it's better to
> check for this in AudioDecodeScheduler and call ProcessAudioPacket() only for
> not-null packets.

Done.

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:85:
DCHECK_NE(AudioPacket::BYTES_PER_SAMPLE_INVALID, packet->bytes_per_sample());
On 2012/08/20 22:26:20, sergeyu wrote:
> check that bytes_per_sample() == kSampleSizeBytes?

Done.

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:86:
DCHECK_NE(AudioPacket::CHANNELS_INVALID, packet->channels());
On 2012/08/20 22:26:20, sergeyu wrote:
> I think you want to check that channels()==kChannels. The player currently
> supports only two channels.

Done.

http://codereview.chromium.org/10823420/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_audio_player.cc:87: if (packet->data(0).size() %
(kChannels * kSampleSizeBytes) != 0) {
On 2012/08/20 22:26:20, sergeyu wrote:
> add empty line above this one.

Done.

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

http://codereview.chromium.org/10823420/diff/1/remoting/codec/audio_decoder_v...
remoting/codec/audio_decoder_verbatim.cc:24: (packet->bytes_per_sample() ==
AudioPacket::BYTES_PER_SAMPLE_INVALID) ||
On 2012/08/20 22:26:20, sergeyu wrote:
> here you want to check that bytes_per_sample() == 2 as that's the only format
> currently supported by the player.

Done.

http://codereview.chromium.org/10823420/diff/1/remoting/codec/audio_decoder_v...
remoting/codec/audio_decoder_verbatim.cc:25: (packet->channels() ==
AudioPacket::CHANNELS_INVALID)) {
On 2012/08/20 22:26:20, sergeyu wrote:
> check that we have two channels, not only that the number of channels is
valid.
> If the host sends us Mono stream the current player implementation will try to
> play it as stereo.

Done.

[Sergey Ulanov, 2012-08-20 22:53:08]

lgtm

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

http://codereview.chromium.org/10823420/diff/3002/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:26: return scoped_ptr<AudioPacket>();
log a warning that the packet is not supported or corrupt

[kxing, 2012-08-20 23:15:47]

I'll commit this soon, unless someone objects.

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

http://codereview.chromium.org/10823420/diff/3002/remoting/codec/audio_decode...
remoting/codec/audio_decoder_verbatim.cc:26: return scoped_ptr<AudioPacket>();
On 2012/08/20 22:53:08, sergeyu wrote:
> log a warning that the packet is not supported or corrupt

Done.

[Wez, 2012-08-20 23:23:51]

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

http://codereview.chromium.org/10823420/diff/10001/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.cc:54: // Only process packet if they are
non-null.
nit: Only process |packet| if it is non-NULL.

http://codereview.chromium.org/10823420/diff/10001/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.cc:54: // Only process packet if they are
non-null.
Clarify under what circumstances |packet| can be NULL, either here, or on the
AudioDecoder interface's description of Decode().

http://codereview.chromium.org/10823420/diff/10001/remoting/codec/audio_decod...
File remoting/codec/audio_decoder_verbatim.cc (right):

http://codereview.chromium.org/10823420/diff/10001/remoting/codec/audio_decod...
remoting/codec/audio_decoder_verbatim.cc:26: LOG(WARNING) << "Verbatim decoder
received a corrupt packet.";
nit: a correct -> an invalid

[kxing, 2012-08-21 14:25:53]

I'll commit this soon.

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

http://codereview.chromium.org/10823420/diff/10001/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.cc:54: // Only process packet if they are
non-null.
On 2012/08/20 23:23:51, Wez wrote:
> nit: Only process |packet| if it is non-NULL.

Done.

http://codereview.chromium.org/10823420/diff/10001/remoting/client/audio_deco...
remoting/client/audio_decode_scheduler.cc:54: // Only process packet if they are
non-null.
On 2012/08/20 23:23:51, Wez wrote:
> Clarify under what circumstances |packet| can be NULL, either here, or on the
> AudioDecoder interface's description of Decode().

Done.

http://codereview.chromium.org/10823420/diff/10001/remoting/codec/audio_decod...
File remoting/codec/audio_decoder_verbatim.cc (right):

http://codereview.chromium.org/10823420/diff/10001/remoting/codec/audio_decod...
remoting/codec/audio_decoder_verbatim.cc:26: LOG(WARNING) << "Verbatim decoder
received a corrupt packet.";
On 2012/08/20 23:23:51, Wez wrote:
> nit: a correct -> an invalid

Done.

[commit-bot: I haz the power, 2012-08-21 14:26:17]

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

[commit-bot: I haz the power, 2012-08-21 16:42:32]

Change committed as 152578