Handle padded audio packets correctly

webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc b/webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc
index acc5926e5b5829db837678226958890b0f90ebb8..abad35bab763f9d0550ae9289a7d06d8528e0b69 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc
@@ -212,11 +212,6 @@ int32_t RTPReceiverAudio::ParseAudioCodecSpecific(
     size_t payload_length,
     const AudioPayload& audio_specific,
     bool is_red) {

[minyue-webrtc, 2017/05/09 21:04:06]

I'd like to see explicit treatment of 0 and early return.

if (payload_length == 0) {	
  return data_callback_->OnReceivedPayloadData(nulltpr, 0, nullptr)
}

[hlundin-webrtc, 2017/05/10 08:57:23]

We must forward the actual WebRtcRTPHeader, can't use a nullptr for that. And,
we would be missing the setting of frameType and isCNG in the WebRtcRTPHeader on
lines 271-272 below if we did early return.

[minyue-webrtc, 2017/05/10 09:44:12]

I still prefer early return, since no need to read the parsing to know what is
going on. and the frame type should be kEmptyFrame IMO.

[hlundin-webrtc, 2017/05/10 11:19:30]

Done.

-
-  if (payload_length == 0) {
-    return 0;
-  }
-
   bool telephone_event_packet =
       TelephoneEventPayloadType(rtp_header->header.payloadType);
   if (telephone_event_packet) {
@@ -239,6 +234,7 @@ int32_t RTPReceiverAudio::ParseAudioCodecSpecific(
       number_of_events = MAX_NUMBER_OF_PARALLEL_TELEPHONE_EVENTS;
     }
     for (size_t n = 0; n < number_of_events; ++n) {
+      RTC_DCHECK_GE(payload_length, (4 * n) + 2);

[minyue-webrtc, 2017/05/09 21:04:06]

check (not dcheck) outside the loop

RTC_CHECK_GE(payload_length, number_of_events > 0 ? 0 : (4 * number_of_events) -
2)

but doesn't

size_t number_of_events = payload_length / 4;

make the check trivial

[hlundin-webrtc, 2017/05/10 08:57:23]

The DCHECK is for documentation purposes, since it is a bit complicated to
verify that the array is long enough for the index being read. That's why I
chose to have it inside the loop, when we know exactly how far we will read that
time.

[kwiberg-webrtc, 2017/05/10 09:22:50]

I agree that having a simple DCHECK inside the loop is better than having a
complicated DCHECK outside the loop. (Approximate truth: DCHECKs are by
construction not relevant to performance, so readability is pretty much the only
thing you have to consider.)

[hlundin-webrtc, 2017/05/10 11:19:30]

Acknowledged.

       bool end = (payload_data[(4 * n) + 1] & 0x80) ? true : false;
 
       std::set<uint8_t>::iterator event =
@@ -291,7 +287,7 @@ int32_t RTPReceiverAudio::ParseAudioCodecSpecific(
     }
   }
   // TODO(holmer): Break this out to have RED parsing handled generically.
-  if (is_red && !(payload_data[0] & 0x80)) {
+  if (payload_length > 0 && is_red && !(payload_data[0] & 0x80)) {

[minyue-webrtc, 2017/05/09 21:04:06]

payload_length != 0 feels to me easier to read

but early return will make this no need.

[hlundin-webrtc, 2017/05/10 08:57:23]

Done.

     // we recive only one frame packed in a RED packet remove the RED wrapper
     rtp_header->header.payloadType = payload_data[0];