Packet buffer for the new jitter buffer.

webrtc/modules/video_coding/frame_object.cc

+/*
+ *  Copyright (c) 2016 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.
+ */
+
+#include "webrtc/modules/video_coding/frame_object.h"
+#include "webrtc/base/criticalsection.h"
+#include "webrtc/modules/video_coding/packet_buffer.h"
+
+namespace webrtc {
+namespace video_coding {
+
+RtpFrameObject::RtpFrameObject(PacketBuffer* packet_buffer,
+                uint16_t first_packet,
+                uint16_t last_packet) :
+                packet_buffer_(packet_buffer),
+                first_packet_(first_packet),
+                last_packet_(last_packet) {}
+
+RtpFrameObject::~RtpFrameObject() {
+  rtc::CritScope lock(&packet_buffer_->crit_);
+  int index = first_packet_ % packet_buffer_->size_;

[stefan-webrtc, 2016/03/17 12:06:31]

I think this code would be better to have in PacketBuffer, and maybe instead
call packet_buffer_->FreePackets(first_packet_, last_packet_) or something like
that?

Alternatively:
packet_buffer_->ReturnFrame(this);

[philipel, 2016/03/17 15:00:35]

I would like to keep this function in the frame object since I want the packet
buffer to only assemble frames.

[stefan-webrtc, 2016/03/24 09:41:21]

I understand, but I think it's pretty ugly to expose a lot of protected members
of the PacketBuffer, especially the lock since it can be difficult to determine
if a deadlock can occur if the lock can be taken from outside the class
itself...

RtpFrameObject has none of the members needed for this and GetBitstream, while
PacketBuffer has all that is needed (if you pass things in on
ReturnFrame/GetBitstream, as suggested). 

I also think it would be a lot easier to write a test for RtpFrameObject if it
doesn't rely on members of PacketBuffer as in that case you can pass in a
MockPacketBuffer instead.

[philipel, 2016/03/30 09:44:54]

Done.

+  int end = ++last_packet_ % packet_buffer_->size_;
+  uint16_t seq_num = first_packet_;
+  while (index != end) {
+    if (packet_buffer_->sequence_buffer_[index].seq_num == seq_num) {
+      packet_buffer_->sequence_buffer_[index].used = false;
+      packet_buffer_->sequence_buffer_[index].continuous = false;
+    }
+    index = (index + 1) % packet_buffer_->size_;
+    ++seq_num;
+  }
+}
+
+uint16_t RtpFrameObject::FirstPacket() const {
+  return first_packet_;
+}
+
+uint16_t RtpFrameObject::LastPacket() const {
+  return last_packet_;
+}
+
+uint16_t RtpFrameObject::PictureId() const {
+  return 1;

[stefan-webrtc, 2016/03/17 12:06:31]

Is this correct?

[philipel, 2016/03/17 15:00:35]

Added picture id to the frame object, but the corresponding logic in the packet
buffer is not yet implemented.

[stefan-webrtc, 2016/03/24 09:41:19]

Acknowledged.

+}
+
+bool RtpFrameObject::GetBitstream(uint8_t* destination) const {
+  rtc::CritScope lock(&packet_buffer_->crit_);

[stefan-webrtc, 2016/03/17 12:06:31]

Seems like most of this code also should live in packet_buffer_?

packet_buffer_->GetBitstream(destination, first_packet_, last_packet_);

[philipel, 2016/03/17 15:00:35]

Again, I would like the functionality of the packet buffer to be as close as
possible to only assemble frames.

[philipel, 2016/03/30 09:44:54]

Done.

+
+  int index = first_packet_ % packet_buffer_->size_;
+  int end = last_packet_ + 1 % packet_buffer_->size_;
+  uint16_t seq_num = first_packet_;
+  while (index != end) {
+    if (!packet_buffer_->sequence_buffer_[index].used ||
+        packet_buffer_->sequence_buffer_[index].seq_num != seq_num)
+      return false;
+
+    const uint8_t* source = packet_buffer_->data_buffer_[index].dataPtr;
+    size_t length = packet_buffer_->data_buffer_[index].sizeBytes;
+    memcpy(destination, source, length);
+    destination += length;
+    index = (index + 1) % packet_buffer_->size_;
+    ++seq_num;
+  }
+  return true;
+}
+
+}  // namespace video_coding
+}  // namespace webrtc