Handle padded audio packets correctly

webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 193 matching lines @@
   }
   return 0;
 }
 
 // We are not allowed to have any critsects when calling data_callback.
 int32_t RTPReceiverAudio::ParseAudioCodecSpecific(
     WebRtcRTPHeader* rtp_header,
     const uint8_t* payload_data,
     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) {
     rtc::CritScope lock(&crit_sect_);
 
     // RFC 4733 2.3
     // 0                   1                   2                   3
     // 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     // |     event     |E|R| volume    |          duration             |
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     //
     if (payload_length % 4 != 0) {
       return -1;
     }
     size_t number_of_events = payload_length / 4;
 
     // sanity
     if (number_of_events >= MAX_NUMBER_OF_PARALLEL_TELEPHONE_EVENTS) {
       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 =
           telephone_event_reported_.find(payload_data[4 * n]);
 
       if (event != telephone_event_reported_.end()) {
         // we have already seen this event
         if (end) {
           telephone_event_reported_.erase(payload_data[4 * n]);
         }
@@ skipping 32 matching lines @@
       }
       std::set<uint8_t>::iterator first =
           telephone_event_reported_.begin();
       if (first != telephone_event_reported_.end() && *first > 15) {
         // don't forward non DTMF events
         return 0;
       }
     }
   }
   // 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];
 
     // only one frame in the RED strip the one byte to help NetEq
     return data_callback_->OnReceivedPayloadData(
         payload_data + 1, payload_length - 1, rtp_header);
   }
 
   rtp_header->type.Audio.channel = audio_specific.channels;
   return data_callback_->OnReceivedPayloadData(
       payload_data, payload_length, rtp_header);
 }
 }  // namespace webrtc