NetEq: Rectify the implementation of PacketBuffer::DiscardOldPackets

webrtc/modules/audio_coding/neteq/packet_buffer_unittest.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.
  */
 
 // Unit tests for PacketBuffer class.
 
 #include "webrtc/modules/audio_coding/neteq/packet_buffer.h"
 #include "webrtc/api/audio_codecs/builtin_audio_decoder_factory.h"
 #include "webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h"
 #include "webrtc/modules/audio_coding/neteq/mock/mock_statistics_calculator.h"
 #include "webrtc/modules/audio_coding/neteq/packet.h"
 #include "webrtc/modules/audio_coding/neteq/tick_timer.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 
 using ::testing::Return;
 using ::testing::StrictMock;
 using ::testing::_;
+using ::testing::InSequence;
+using ::testing::MockFunction;
 
 namespace webrtc {
 
 // Helper class to generate packets. Packets must be deleted by the user.
 class PacketGenerator {
  public:
   PacketGenerator(uint16_t seq_no, uint32_t ts, uint8_t pt, int frame_size);
   virtual ~PacketGenerator() {}
   void Reset(uint16_t seq_no, uint32_t ts, uint8_t pt, int frame_size);
   Packet NextPacket(int payload_size_bytes);
@@ skipping 272 matching lines @@
     buffer.InsertPacket(gen.NextPacket(payload_len));
   }
   EXPECT_EQ(10u, buffer.NumPacketsInBuffer());
 
   StrictMock<MockStatisticsCalculator> mock_stats;
   uint32_t current_ts = start_ts;
 
   // Discard them one by one and make sure that the right packets are at the
   // front of the buffer.
   constexpr int kDiscardPackets = 5;
+
+  // Interleaving the EXPECT_CALL sequence with expectations on the MockFunction
+  // check ensures that exactly one call to PacketsDiscarded happens in each
+  // DiscardNextPacket call.

[minyue-webrtc, 2017/07/05 12:11:08]

nice, another way would be avoid the for-loop. It is not needed to test
DiscardNextPacket so many times.

[hlundin-webrtc, 2017/07/05 13:42:35]

True. But I still think I would like to have the InSequence at least.

+  InSequence s;
+  MockFunction<void(int check_point_id)> check;
   for (int i = 0; i < kDiscardPackets; ++i) {
     uint32_t ts;
     EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&ts));
     EXPECT_EQ(current_ts, ts);
-    EXPECT_CALL(mock_stats, PacketsDiscarded(1)).Times(1);
+    EXPECT_CALL(mock_stats, PacketsDiscarded(1));
+    EXPECT_CALL(check, Call(i));

[minyue-webrtc, 2017/07/05 12:11:08]

why not arbitrary number

[hlundin-webrtc, 2017/07/05 13:42:35]

Indexing the calls to the check function makes the printouts in case of error
more readable.

     EXPECT_EQ(PacketBuffer::kOK, buffer.DiscardNextPacket(&mock_stats));
     current_ts += ts_increment;
+    check.Call(i);
   }
 
   constexpr int kRemainingPackets = kTotalPackets - kDiscardPackets;
-  // This will not discard any packets because the oldest packet is newer than
-  // the indicated horizon_samples.
+  // This will all remaining packets but one. The oldest packet is older than

[minyue-webrtc, 2017/07/05 12:11:08]

This will "discard" all remaining ...

[hlundin-webrtc, 2017/07/05 13:42:35]

Done.

+  // the indicated horizon_samples, and will thus be left in the buffer.
+  EXPECT_CALL(mock_stats, PacketsDiscarded(kRemainingPackets - 1));

[minyue-webrtc, 2017/07/05 12:11:08]

define 1 as kSkipPackets

[hlundin-webrtc, 2017/07/05 13:42:35]

Done.

+  EXPECT_CALL(check, Call(17));  // Arbitrary id number.
   buffer.DiscardOldPackets(start_ts + kTotalPackets * ts_increment,
                            kRemainingPackets * ts_increment, &mock_stats);
+  check.Call(17);  // Same arbitrary id number.
+
+  EXPECT_EQ(1u, buffer.NumPacketsInBuffer());
   uint32_t ts;
   EXPECT_EQ(PacketBuffer::kOK, buffer.NextTimestamp(&ts));
   EXPECT_EQ(current_ts, ts);
 
   // Discard all remaining packets.
-  EXPECT_CALL(mock_stats, PacketsDiscarded(1)).Times(kRemainingPackets);
+  EXPECT_CALL(mock_stats, PacketsDiscarded(1));
   buffer.DiscardAllOldPackets(start_ts + kTotalPackets * ts_increment,
                               &mock_stats);
 
   EXPECT_TRUE(buffer.Empty());
 }
 
 TEST(PacketBuffer, Reordering) {
   TickTimer tick_timer;
   PacketBuffer buffer(100, &tick_timer);  // 100 packets.
   const uint16_t start_seq_no = 17;
@@ skipping 337 matching lines @@
 // Test the IsObsoleteTimestamp method with different limit timestamps.
 TEST(PacketBuffer, IsObsoleteTimestamp) {
   TestIsObsoleteTimestamp(0);
   TestIsObsoleteTimestamp(1);
   TestIsObsoleteTimestamp(0xFFFFFFFF);  // -1 in uint32_t.
   TestIsObsoleteTimestamp(0x80000000);  // 2^31.
   TestIsObsoleteTimestamp(0x80000001);  // 2^31 + 1.
   TestIsObsoleteTimestamp(0x7FFFFFFF);  // 2^31 - 1.
 }
 }  // namespace webrtc