PacketBuffer is now ref counted.

Since all FrameObjects have a reference to its PacketBuffer and since
the PacketBuffer can be thrown away at any moment the PacketBuffer
has to be ref counted in order to avoid FrameObjects dereferencing a potentially
destroyed object.

BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

Review URL: https://codereview.webrtc.org/2199133004 .

Cr-Commit-Position: refs/heads/master@{#13725}

webrtc/modules/BUILD.gn

@@ -326,6 +326,7 @@ if (rtc_include_tests) {
       "video_coding/percentile_filter_unittest.cc",
       "video_coding/protection_bitrate_calculator_unittest.cc",
       "video_coding/receiver_unittest.cc",
+      "video_coding/rtp_frame_reference_finder_unittest.cc",
       "video_coding/sequence_number_util_unittest.cc",
       "video_coding/session_info_unittest.cc",
       "video_coding/test/stream_generator.cc",

webrtc/modules/modules.gyp

@@ -167,14 +167,14 @@
             '<(webrtc_root)/tools/tools.gyp:agc_test_utils',
           ],
           'sources': [
-            'audio_coding/codecs/audio_decoder_factory_unittest.cc',
-            'audio_coding/codecs/cng/audio_encoder_cng_unittest.cc',
             'audio_coding/acm2/acm_receiver_unittest_oldapi.cc',
             'audio_coding/acm2/audio_coding_module_unittest_oldapi.cc',
             'audio_coding/acm2/call_statistics_unittest.cc',
             'audio_coding/acm2/codec_manager_unittest.cc',
             'audio_coding/acm2/initial_delay_manager_unittest.cc',
             'audio_coding/acm2/rent_a_codec_unittest.cc',
+            'audio_coding/codecs/audio_decoder_factory_unittest.cc',
+            'audio_coding/codecs/cng/audio_encoder_cng_unittest.cc',
             'audio_coding/codecs/cng/cng_unittest.cc',
             'audio_coding/codecs/isac/fix/source/filterbanks_unittest.cc',
             'audio_coding/codecs/isac/fix/source/filters_unittest.cc',
@@ -183,10 +183,10 @@
             'audio_coding/codecs/isac/main/source/audio_encoder_isac_unittest.cc',
             'audio_coding/codecs/isac/main/source/isac_unittest.cc',
             'audio_coding/codecs/isac/unittest.cc',
+            'audio_coding/codecs/mock/mock_audio_encoder.cc',
             'audio_coding/codecs/opus/audio_encoder_opus_unittest.cc',
             'audio_coding/codecs/opus/opus_unittest.cc',
             'audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc',
-            'audio_coding/codecs/mock/mock_audio_encoder.cc',
             'audio_coding/neteq/audio_classifier_unittest.cc',
             'audio_coding/neteq/audio_multi_vector_unittest.cc',
             'audio_coding/neteq/audio_vector_unittest.cc',
@@ -202,6 +202,18 @@
             'audio_coding/neteq/dtmf_tone_generator_unittest.cc',
             'audio_coding/neteq/expand_unittest.cc',
             'audio_coding/neteq/merge_unittest.cc',
+            'audio_coding/neteq/mock/mock_audio_decoder.h',
+            'audio_coding/neteq/mock/mock_audio_vector.h',
+            'audio_coding/neteq/mock/mock_buffer_level_filter.h',
+            'audio_coding/neteq/mock/mock_decoder_database.h',
+            'audio_coding/neteq/mock/mock_delay_manager.h',
+            'audio_coding/neteq/mock/mock_delay_peak_detector.h',
+            'audio_coding/neteq/mock/mock_dtmf_buffer.h',
+            'audio_coding/neteq/mock/mock_dtmf_tone_generator.h',
+            'audio_coding/neteq/mock/mock_expand.h',
+            'audio_coding/neteq/mock/mock_external_decoder_pcm16b.h',
+            'audio_coding/neteq/mock/mock_packet_buffer.h',
+            'audio_coding/neteq/mock/mock_payload_splitter.h',
             'audio_coding/neteq/nack_tracker_unittest.cc',
             'audio_coding/neteq/neteq_external_decoder_unittest.cc',
             'audio_coding/neteq/neteq_impl_unittest.cc',
@@ -215,20 +227,8 @@
             'audio_coding/neteq/random_vector_unittest.cc',
             'audio_coding/neteq/sync_buffer_unittest.cc',
             'audio_coding/neteq/tick_timer_unittest.cc',
-            'audio_coding/neteq/timestamp_scaler_unittest.cc',
             'audio_coding/neteq/time_stretch_unittest.cc',
-            'audio_coding/neteq/mock/mock_audio_decoder.h',
+            'audio_coding/neteq/timestamp_scaler_unittest.cc',
-            'audio_coding/neteq/mock/mock_audio_vector.h',
-            'audio_coding/neteq/mock/mock_buffer_level_filter.h',
-            'audio_coding/neteq/mock/mock_decoder_database.h',
-            'audio_coding/neteq/mock/mock_delay_manager.h',
-            'audio_coding/neteq/mock/mock_delay_peak_detector.h',
-            'audio_coding/neteq/mock/mock_dtmf_buffer.h',
-            'audio_coding/neteq/mock/mock_dtmf_tone_generator.h',
-            'audio_coding/neteq/mock/mock_expand.h',
-            'audio_coding/neteq/mock/mock_external_decoder_pcm16b.h',
-            'audio_coding/neteq/mock/mock_packet_buffer.h',
-            'audio_coding/neteq/mock/mock_payload_splitter.h',
             'audio_coding/neteq/tools/input_audio_file_unittest.cc',
             'audio_coding/neteq/tools/packet_unittest.cc',
             'audio_conference_mixer/test/audio_conference_mixer_unittest.cc',
@@ -284,8 +284,8 @@
             'pacing/paced_sender_unittest.cc',
             'pacing/packet_router_unittest.cc',
             'remote_bitrate_estimator/bwe_simulations.cc',
-            'remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h',
             'remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h',
+            'remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h',
             'remote_bitrate_estimator/inter_arrival_unittest.cc',
             'remote_bitrate_estimator/overuse_detector_unittest.cc',
             'remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc',
@@ -296,22 +296,21 @@
             'remote_bitrate_estimator/send_time_history_unittest.cc',
             'remote_bitrate_estimator/test/bwe_test_framework_unittest.cc',
             'remote_bitrate_estimator/test/bwe_unittest.cc',
-            'remote_bitrate_estimator/test/metric_recorder_unittest.cc',
             'remote_bitrate_estimator/test/estimators/nada_unittest.cc',
+            'remote_bitrate_estimator/test/metric_recorder_unittest.cc',
             'remote_bitrate_estimator/transport_feedback_adapter_unittest.cc',
-            'rtp_rtcp/source/mock/mock_rtp_payload_strategy.h',
             'rtp_rtcp/source/byte_io_unittest.cc',
             'rtp_rtcp/source/fec_receiver_unittest.cc',
             'rtp_rtcp/source/fec_test_helper.cc',
             'rtp_rtcp/source/fec_test_helper.h',
+            'rtp_rtcp/source/mock/mock_rtp_payload_strategy.h',
             'rtp_rtcp/source/nack_rtx_unittest.cc',
             'rtp_rtcp/source/packet_loss_stats_unittest.cc',
-            'rtp_rtcp/source/producer_fec_unittest.cc',
             'rtp_rtcp/source/playout_delay_oracle_unittest.cc',
+            'rtp_rtcp/source/producer_fec_unittest.cc',
             'rtp_rtcp/source/receive_statistics_unittest.cc',
             'rtp_rtcp/source/remote_ntp_time_estimator_unittest.cc',
             'rtp_rtcp/source/rtcp_format_remb_unittest.cc',
-            'rtp_rtcp/source/rtcp_packet_unittest.cc',
             'rtp_rtcp/source/rtcp_packet/app_unittest.cc',
             'rtp_rtcp/source/rtcp_packet/bye_unittest.cc',
             'rtp_rtcp/source/rtcp_packet/common_header_unittest.cc',
@@ -335,6 +334,7 @@
             'rtp_rtcp/source/rtcp_packet/tmmbr_unittest.cc',
             'rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc',
             'rtp_rtcp/source/rtcp_packet/voip_metric_unittest.cc',
+            'rtp_rtcp/source/rtcp_packet_unittest.cc',
             'rtp_rtcp/source/rtcp_receiver_unittest.cc',
             'rtp_rtcp/source/rtcp_sender_unittest.cc',
             'rtp_rtcp/source/rtcp_utility_unittest.cc',
@@ -344,11 +344,11 @@
             'rtp_rtcp/source/rtp_format_vp8_test_helper.h',
             'rtp_rtcp/source/rtp_format_vp8_unittest.cc',
             'rtp_rtcp/source/rtp_format_vp9_unittest.cc',
+            'rtp_rtcp/source/rtp_header_extension_unittest.cc',
             'rtp_rtcp/source/rtp_packet_history_unittest.cc',
             'rtp_rtcp/source/rtp_packet_unittest.cc',
             'rtp_rtcp/source/rtp_payload_registry_unittest.cc',
             'rtp_rtcp/source/rtp_rtcp_impl_unittest.cc',
-            'rtp_rtcp/source/rtp_header_extension_unittest.cc',
             'rtp_rtcp/source/rtp_sender_unittest.cc',
             'rtp_rtcp/source/time_util_unittest.cc',
             'rtp_rtcp/source/vp8_partition_aggregator_unittest.cc',
@@ -369,29 +369,30 @@
             'video_coding/codecs/vp8/simulcast_encoder_adapter_unittest.cc',
             'video_coding/codecs/vp8/simulcast_unittest.cc',
             'video_coding/codecs/vp8/simulcast_unittest.h',
-            'video_coding/frame_buffer2_unittest.cc',
-            'video_coding/include/mock/mock_vcm_callbacks.h',
             'video_coding/decoding_state_unittest.cc',
+            'video_coding/frame_buffer2_unittest.cc',
             'video_coding/histogram_unittest.cc',
+            'video_coding/include/mock/mock_vcm_callbacks.h',
             'video_coding/jitter_buffer_unittest.cc',
             'video_coding/jitter_estimator_tests.cc',
             'video_coding/nack_module_unittest.cc',
-            'video_coding/video_packet_buffer_unittest.cc',
             'video_coding/percentile_filter_unittest.cc',
             'video_coding/protection_bitrate_calculator_unittest.cc',
             'video_coding/receiver_unittest.cc',
-            'video_coding/session_info_unittest.cc',
+            'video_coding/rtp_frame_reference_finder_unittest.cc',
             'video_coding/sequence_number_util_unittest.cc',
-            'video_coding/timing_unittest.cc',
+            'video_coding/session_info_unittest.cc',
-            'video_coding/video_coding_robustness_unittest.cc',
-            'video_coding/video_receiver_unittest.cc',
-            'video_coding/video_sender_unittest.cc',
             'video_coding/test/stream_generator.cc',
             'video_coding/test/stream_generator.h',
+            'video_coding/timing_unittest.cc',
             'video_coding/utility/frame_dropper_unittest.cc',
             'video_coding/utility/h264_bitstream_parser_unittest.cc',
             'video_coding/utility/ivf_file_writer_unittest.cc',
             'video_coding/utility/quality_scaler_unittest.cc',
+            'video_coding/video_coding_robustness_unittest.cc',
+            'video_coding/video_packet_buffer_unittest.cc',
+            'video_coding/video_receiver_unittest.cc',
+            'video_coding/video_sender_unittest.cc',
             'video_processing/test/denoiser_test.cc',
             'video_processing/test/video_processing_unittest.cc',
             'video_processing/test/video_processing_unittest.h',

webrtc/modules/video_coding/frame_object.h

@@ -75,7 +75,7 @@ class RtpFrameObject : public FrameObject {
   RTPVideoTypeHeader* GetCodecHeader() const;
 
  private:
-  PacketBuffer* packet_buffer_;
+  rtc::scoped_refptr<PacketBuffer> packet_buffer_;
   enum FrameType frame_type_;
   VideoCodecType codec_type_;
   uint16_t first_seq_num_;

webrtc/modules/video_coding/packet_buffer.cc

@@ -12,7 +12,9 @@
 
 #include <algorithm>
 #include <limits>
+#include <utility>
 
+#include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/modules/video_coding/frame_object.h"
@@ -21,10 +23,19 @@
 namespace webrtc {
 namespace video_coding {
 
+rtc::scoped_refptr<PacketBuffer> PacketBuffer::Create(
+    Clock* clock,
+    size_t start_buffer_size,
+    size_t max_buffer_size,
+    OnReceivedFrameCallback* received_frame_callback) {
+  return rtc::scoped_refptr<PacketBuffer>(new PacketBuffer(
+      clock, start_buffer_size, max_buffer_size, received_frame_callback));
+}
+
 PacketBuffer::PacketBuffer(Clock* clock,
                            size_t start_buffer_size,
                            size_t max_buffer_size,
-                           OnCompleteFrameCallback* frame_callback)
+                           OnReceivedFrameCallback* received_frame_callback)
     : clock_(clock),
       size_(start_buffer_size),
       max_size_(max_buffer_size),
@@ -33,13 +44,15 @@ PacketBuffer::PacketBuffer(Clock* clock,
       first_packet_received_(false),
       data_buffer_(start_buffer_size),
       sequence_buffer_(start_buffer_size),
-      reference_finder_(frame_callback) {
+      received_frame_callback_(received_frame_callback) {
   RTC_DCHECK_LE(start_buffer_size, max_buffer_size);
   // Buffer size must always be a power of 2.
   RTC_DCHECK((start_buffer_size & (start_buffer_size - 1)) == 0);
   RTC_DCHECK((max_buffer_size & (max_buffer_size - 1)) == 0);
 }
 
+PacketBuffer::~PacketBuffer() {}
+
 bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
   rtc::CritScope lock(&crit_);
   uint16_t seq_num = packet.seqNum;
@@ -69,12 +82,6 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
   if (AheadOf(seq_num, last_seq_num_))
     last_seq_num_ = seq_num;
 
-  // If this is a padding or FEC packet, don't insert it.
-  if (packet.sizeBytes == 0) {
-    reference_finder_.PaddingReceived(packet.seqNum);
-    return true;
-  }
-
   sequence_buffer_[index].frame_begin = packet.isFirstPacket;
   sequence_buffer_[index].frame_end = packet.markerBit;
   sequence_buffer_[index].seq_num = packet.seqNum;
@@ -169,7 +176,8 @@ void PacketBuffer::FindFrames(uint16_t seq_num) {
       std::unique_ptr<RtpFrameObject> frame(
           new RtpFrameObject(this, start_seq_num, seq_num, frame_size,
                              max_nack_count, clock_->TimeInMilliseconds()));
-      reference_finder_.ManageFrame(std::move(frame));
+
+      received_frame_callback_->OnReceivedFrame(std::move(frame));
     }
 
     index = (index + 1) % size_;
@@ -239,5 +247,17 @@ void PacketBuffer::Clear() {
   first_packet_received_ = false;
 }
 
+int PacketBuffer::AddRef() const {
+  return rtc::AtomicOps::Increment(&ref_count_);
+}
+
+int PacketBuffer::Release() const {
+  int count = rtc::AtomicOps::Decrement(&ref_count_);
+  if (!count) {
+    delete this;
+  }
+  return count;
+}
+
 }  // namespace video_coding
 }  // namespace webrtc

webrtc/modules/video_coding/packet_buffer.h

@@ -12,8 +12,10 @@
 #define WEBRTC_MODULES_VIDEO_CODING_PACKET_BUFFER_H_
 
 #include <vector>
+#include <memory>
 
 #include "webrtc/base/criticalsection.h"
+#include "webrtc/base/scoped_ref_ptr.h"
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/video_coding/packet.h"
@@ -29,23 +31,37 @@ namespace video_coding {
 class FrameObject;
 class RtpFrameObject;
 
-class OnCompleteFrameCallback {
+// A received frame is a frame which has received all its packets.
+class OnReceivedFrameCallback {
  public:
-  virtual ~OnCompleteFrameCallback() {}
+  virtual ~OnReceivedFrameCallback() {}
-  virtual void OnCompleteFrame(std::unique_ptr<FrameObject> frame) = 0;
+  virtual void OnReceivedFrame(std::unique_ptr<RtpFrameObject> frame) = 0;
 };
 
 class PacketBuffer {
  public:
+  static rtc::scoped_refptr<PacketBuffer> Create(
+      Clock* clock,
+      size_t start_buffer_size,
+      size_t max_buffer_size,
+      OnReceivedFrameCallback* frame_callback);
+
+  virtual ~PacketBuffer();
+
+  // Made virtual for testing.
+  virtual bool InsertPacket(const VCMPacket& packet);
+  void ClearTo(uint16_t seq_num);
+  void Clear();
+
+  int AddRef() const;
+  int Release() const;
+
+ protected:
   // Both |start_buffer_size| and |max_buffer_size| must be a power of 2.
   PacketBuffer(Clock* clock,
                size_t start_buffer_size,
                size_t max_buffer_size,
-               OnCompleteFrameCallback* frame_callback);
+               OnReceivedFrameCallback* frame_callback);
-
-  bool InsertPacket(const VCMPacket& packet);
-  void ClearTo(uint16_t seq_num);
-  void Clear();
 
  private:
   friend RtpFrameObject;
@@ -85,13 +101,16 @@ class PacketBuffer {
   void FindFrames(uint16_t seq_num) EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
   // Copy the bitstream for |frame| to |destination|.
-  bool GetBitstream(const RtpFrameObject& frame, uint8_t* destination);
+  // Virtual for testing.
+  virtual bool GetBitstream(const RtpFrameObject& frame, uint8_t* destination);
 
   // Get the packet with sequence number |seq_num|.
-  VCMPacket* GetPacket(uint16_t seq_num);
+  // Virtual for testing.
+  virtual VCMPacket* GetPacket(uint16_t seq_num);
 
   // Mark all slots used by |frame| as not used.
-  void ReturnFrame(RtpFrameObject* frame);
+  // Virtual for testing.
+  virtual void ReturnFrame(RtpFrameObject* frame);
 
   rtc::CriticalSection crit_;
 
@@ -115,9 +134,10 @@ class PacketBuffer {
   // and information needed to determine the continuity between packets.
   std::vector<ContinuityInfo> sequence_buffer_ GUARDED_BY(crit_);
 
-  // Frames that have received all their packets are handed off to the
+  // Called when a received frame is found.
-  // |reference_finder_| which finds the dependencies between the frames.
+  OnReceivedFrameCallback* const received_frame_callback_;
-  RtpFrameReferenceFinder reference_finder_;
+
+  mutable volatile int ref_count_ = 0;
 };
 
 }  // namespace video_coding

webrtc/modules/video_coding/rtp_frame_reference_finder.h

@@ -26,8 +26,16 @@
 namespace webrtc {
 namespace video_coding {
 
+class FrameObject;
 class RtpFrameObject;
-class OnCompleteFrameCallback;
+
+// A complete frame is a frame which has received all its packets and all its
+// references are known.
+class OnCompleteFrameCallback {
+ public:
+  virtual ~OnCompleteFrameCallback() {}
+  virtual void OnCompleteFrame(std::unique_ptr<FrameObject> frame) = 0;
+};
 
 class RtpFrameReferenceFinder {
  public: