NetEq: Changed Packet::payload to be an rtc::Buffer

webrtc/modules/audio_coding/neteq/neteq_impl.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 543 matching lines @@
     // Create |packet| within this separate scope, since it should not be used
     // directly once it's been inserted in the packet list. This way, |packet|
     // is not defined outside of this block.
     Packet* packet = new Packet;
     packet->header.markerBit = false;
     packet->header.payloadType = rtp_header.header.payloadType;
     packet->header.sequenceNumber = rtp_header.header.sequenceNumber;
     packet->header.timestamp = rtp_header.header.timestamp;
     packet->header.ssrc = rtp_header.header.ssrc;
     packet->header.numCSRCs = 0;
-    packet->payload_length = payload.size();
+    packet->payload = rtc::Buffer(payload.data(), payload.size());

[kwiberg-webrtc, 2016/08/30 16:04:36]

You're copying data into an existing buffer, so

  packet->payload.SetData(payload);

I suspect Buffer::SetData may not actually accept an ArrayView, but it should...

[ossu, 2016/08/30 16:27:13]

Good point. Yeah, I was expecting the copy constructor to accept anything with a
data() and a size() but it might be too easy to make unintended copies of data
with that. The SetData call should be an unambiguous copy, so for that I think
it'd be fine!

[kwiberg-webrtc, 2016/08/30 16:37:05]

Yes, we explicitly don't want a copy constructor. It's too easy to make copies
without noticing it that way...

     packet->primary = true;
     // Waiting time will be set upon inserting the packet in the buffer.
     RTC_DCHECK(!packet->waiting_time);
-    packet->payload = new uint8_t[packet->payload_length];
     packet->sync_packet = is_sync_packet;
-    if (!packet->payload) {
-      LOG_F(LS_ERROR) << "Payload pointer is NULL.";
-    }
-    assert(!payload.empty());  // Already checked above.
-    memcpy(packet->payload, payload.data(), packet->payload_length);
     // Insert packet in a packet list.
     packet_list.push_back(packet);
     // Save main payloads header for later.
     memcpy(&main_header, &packet->header, sizeof(main_header));
   }
 
   bool update_sample_rate_and_channels = false;
   // Reinitialize NetEq if it's needed (changed SSRC or first call).
   if ((main_header.ssrc != ssrc_) || first_packet_) {
     // Note: |first_packet_| will be cleared further down in this method, once
@@ skipping 49 matching lines @@
 
   // Scale timestamp to internal domain (only for some codecs).
   timestamp_scaler_->ToInternal(&packet_list);
 
   // Process DTMF payloads. Cycle through the list of packets, and pick out any
   // DTMF payloads found.
   PacketList::iterator it = packet_list.begin();
   while (it != packet_list.end()) {
     Packet* current_packet = (*it);
     assert(current_packet);
-    assert(current_packet->payload);
+    assert(!current_packet->payload.empty());
     if (decoder_database_->IsDtmf(current_packet->header.payloadType)) {
       assert(!current_packet->sync_packet);  // We had a sanity check for this.
       DtmfEvent event;
-      int ret = DtmfBuffer::ParseEvent(
-          current_packet->header.timestamp,
-          current_packet->payload,
-          current_packet->payload_length,
-          &event);
+      int ret = DtmfBuffer::ParseEvent(current_packet->header.timestamp,
+                                       current_packet->payload.data(),
+                                       current_packet->payload.size(), &event);
       if (ret != DtmfBuffer::kOK) {
         PacketBuffer::DeleteAllPackets(&packet_list);
         return kDtmfParsingError;
       }
       if (dtmf_buffer_->InsertEvent(event) != DtmfBuffer::kOK) {
         PacketBuffer::DeleteAllPackets(&packet_list);
         return kDtmfInsertError;
       }
-      // TODO(hlundin): Let the destructor of Packet handle the payload.
-      delete [] current_packet->payload;

[kwiberg-webrtc, 2016/08/30 16:04:36]

Whatever happens, this CL is going to earn you a heap of karma points. :-)

[ossu, 2016/08/30 16:27:14]

Yay! \o/

       delete current_packet;
       it = packet_list.erase(it);
     } else {
       ++it;
     }
   }
 
   // Check for FEC in packets, and separate payloads into several packets.
   int ret = payload_splitter_->SplitFec(&packet_list, decoder_database_.get());
   if (ret != PayloadSplitter::kOK) {
@@ skipping 24 matching lines @@
 
   // Update bandwidth estimate, if the packet is not sync-packet nor comfort
   // noise.
   if (!packet_list.empty() && !packet_list.front()->sync_packet &&
       !decoder_database_->IsComfortNoise(main_header.payloadType)) {
     // The list can be empty here if we got nothing but DTMF payloads.
     AudioDecoder* decoder =
         decoder_database_->GetDecoder(main_header.payloadType);
     assert(decoder);  // Should always get a valid object, since we have
     // already checked that the payload types are known.
-    decoder->IncomingPacket(packet_list.front()->payload,
-                            packet_list.front()->payload_length,
+    decoder->IncomingPacket(packet_list.front()->payload.data(),
+                            packet_list.front()->payload.size(),
                             packet_list.front()->header.sequenceNumber,
                             packet_list.front()->header.timestamp,
                             receive_timestamp);
   }
 
   if (nack_enabled_) {
     RTC_DCHECK(nack_);
     if (update_sample_rate_and_channels) {
       nack_->Reset();
     }
@@ skipping 743 matching lines @@
       !decoder_database_->IsComfortNoise(packet->header.payloadType)) {
     assert(decoder);  // At this point, we must have a decoder object.
     // The number of channels in the |sync_buffer_| should be the same as the
     // number decoder channels.
     assert(sync_buffer_->Channels() == decoder->Channels());
     assert(decoded_buffer_length_ >= kMaxFrameSize * decoder->Channels());
     assert(operation == kNormal || operation == kAccelerate ||
            operation == kFastAccelerate || operation == kMerge ||
            operation == kPreemptiveExpand);
     packet_list->pop_front();
-    size_t payload_length = packet->payload_length;
+    const size_t payload_length = packet->payload.size();
     int decode_length;
     if (packet->sync_packet) {
       // Decode to silence with the same frame size as the last decode.
       memset(&decoded_buffer_[*decoded_length], 0,
              decoder_frame_length_ * decoder->Channels() *
                  sizeof(decoded_buffer_[0]));
       decode_length = rtc::checked_cast<int>(decoder_frame_length_);
     } else if (!packet->primary) {
       // This is a redundant payload; call the special decoder method.
       decode_length = decoder->DecodeRedundant(
-          packet->payload, packet->payload_length, fs_hz_,
+          packet->payload.data(), packet->payload.size(), fs_hz_,
           (decoded_buffer_length_ - *decoded_length) * sizeof(int16_t),
           &decoded_buffer_[*decoded_length], speech_type);
     } else {
-      decode_length =
-          decoder->Decode(
-              packet->payload, packet->payload_length, fs_hz_,
-              (decoded_buffer_length_ - *decoded_length) * sizeof(int16_t),
-              &decoded_buffer_[*decoded_length], speech_type);
+      decode_length = decoder->Decode(
+          packet->payload.data(), packet->payload.size(), fs_hz_,
+          (decoded_buffer_length_ - *decoded_length) * sizeof(int16_t),
+          &decoded_buffer_[*decoded_length], speech_type);
     }
 
-    delete[] packet->payload;
     delete packet;
     packet = NULL;
     if (decode_length > 0) {
       *decoded_length += decode_length;
       // Update |decoder_frame_length_| with number of samples per channel.
       decoder_frame_length_ =
           static_cast<size_t>(decode_length) / decoder->Channels();
     } else if (decode_length < 0) {
       // Error.
       LOG(LS_WARNING) << "Decode " << decode_length << " " << payload_length;
@@ skipping 452 matching lines @@
     Packet* packet = packet_buffer_->GetNextPacket(&discard_count);
     // |header| may be invalid after the |packet_buffer_| operation.
     header = NULL;
     if (!packet) {
       LOG(LS_ERROR) << "Should always be able to extract a packet here";
       assert(false);  // Should always be able to extract a packet here.
       return -1;
     }
     stats_.PacketsDiscarded(discard_count);
     stats_.StoreWaitingTime(packet->waiting_time->ElapsedMs());
-    assert(packet->payload_length > 0);
+    assert(!packet->payload.empty());
     packet_list->push_back(packet);  // Store packet in list.
 
     if (first_packet) {
       first_packet = false;
       if (nack_enabled_) {
         RTC_DCHECK(nack_);
         // TODO(henrik.lundin): Should we update this for all decoded packets?
         nack_->UpdateLastDecodedPacket(packet->header.sequenceNumber,
                                        packet->header.timestamp);
       }
       prev_sequence_number = packet->header.sequenceNumber;
       prev_timestamp = packet->header.timestamp;
       prev_payload_type = packet->header.payloadType;
     }
 
     // Store number of extracted samples.
     int packet_duration = 0;
     AudioDecoder* decoder = decoder_database_->GetDecoder(
         packet->header.payloadType);
     if (decoder) {
       if (packet->sync_packet) {
         packet_duration = rtc::checked_cast<int>(decoder_frame_length_);
       } else {
         if (packet->primary) {
-          packet_duration = decoder->PacketDuration(packet->payload,
-                                                    packet->payload_length);
+          packet_duration = decoder->PacketDuration(packet->payload.data(),
+                                                    packet->payload.size());
         } else {
-          packet_duration = decoder->
-              PacketDurationRedundant(packet->payload, packet->payload_length);
+          packet_duration = decoder->PacketDurationRedundant(
+              packet->payload.data(), packet->payload.size());
           stats_.SecondaryDecodedSamples(packet_duration);
         }
       }
     } else if (!decoder_database_->IsComfortNoise(packet->header.payloadType)) {
       LOG(LS_WARNING) << "Unknown payload type "
                       << static_cast<int>(packet->header.payloadType);
       assert(false);
     }
     if (packet_duration <= 0) {
       // Decoder did not return a packet duration. Assume that the packet
@@ skipping 128 matching lines @@
   }
 }
 
 void NetEqImpl::CreateDecisionLogic() {
   decision_logic_.reset(DecisionLogic::Create(
       fs_hz_, output_size_samples_, playout_mode_, decoder_database_.get(),
       *packet_buffer_.get(), delay_manager_.get(), buffer_level_filter_.get(),
       tick_timer_.get()));
 }
 }  // namespace webrtc