Correcting the amount of padding when send side bwe includes RTP overhead.

webrtc/modules/rtp_rtcp/source/rtp_packet_history_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.
  */
 
 #include "webrtc/modules/rtp_rtcp/source/rtp_packet_history.h"
 
 #include <memory>
 
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h"
 #include "webrtc/system_wrappers/include/clock.h"
+#include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 
 class RtpPacketHistoryTest : public ::testing::Test {
  protected:
   static constexpr uint16_t kSeqNum = 88;
 
   RtpPacketHistoryTest() : fake_clock_(123456), hist_(&fake_clock_) {}
@@ skipping 184 matching lines @@
   }
 
   fake_clock_.AdvanceTimeMilliseconds(100);
 
   // Retransmit all packets currently in buffer.
   for (size_t i = 1; i < RtpPacketHistory::kMaxCapacity + 1; ++i) {
     EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kSeqNum + i, 100, false));
   }
 }
 
+namespace {
+
+class MockRtpPacketToSend : public RtpPacketToSend {
+ public:
+  MockRtpPacketToSend() : RtpPacketToSend(nullptr) {}
+  virtual ~MockRtpPacketToSend() {}
+  MOCK_CONST_METHOD0(headers_size, size_t());
+  MOCK_CONST_METHOD0(size, size_t());

[danilchap, 2017/03/28 15:10:58]

I'm not sure it is good idea to make cheap accessor virtual, specially just for
tests. (it also breaks invariant
size() = header_size() + payload_size() + padding_size())

May be rather create helper method that use extra knowledge about RtpPacket to
create packet of given size:

std::unique_ptr<RtpPacketToSend> CreatePacket(size_t size) {
  std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(nullptr, size));
  packet->AllocatePayload(size - packet->header_size());
  EXPECT_EQ(packet->size(), size);
  return packet;
}

This will only limit to header_size of 12 bytes (minimum header size), but that
should be enough for test below.
It is possible to extend this function a bit to have custome header_size if you
prefer to be explicit (as long as they are >= 12 and multiple of 4)

[minyue-webrtc, 2017/03/29 11:39:44]

Fully agreed. I think const header size if enough for the test. So it will be
easy.

Just one comment, using the real RTP packet is a bit painful since it is not
easy to know the implementations there, e.g., only AllocatePayload can work,
even SetPayloadSize(), which sounds better, does not.

[danilchap, 2017/03/29 12:12:53]

I'm sorry for making interface of the rtp packet confusing.
AllocatePayload can increase payload size
SetPayloadSize limited to only reduce payload size.
Probably I should merge these two function into one to make it less confusing.

+};
+
+using ::testing::Return;
+using ::testing::StrictMock;
+
+}  // namespace
+
+TEST_F(RtpPacketHistoryTest, GetBestFittingPacket) {
+  constexpr int64_t kCaptureTime1 = 1234;
+  constexpr int64_t kCaptureTime2 = 5678;
+  constexpr int64_t kCaptureTime3 = 9000;
+  constexpr size_t kMinPacketRequestBytes = 50;
+
+  hist_.SetStorePacketsStatus(true, 3);
+
+  std::unique_ptr<MockRtpPacketToSend> packet;
+
+  packet.reset(new StrictMock<MockRtpPacketToSend>());
+  packet->set_capture_time_ms(kCaptureTime1);
+  EXPECT_CALL(*packet, headers_size()).WillRepeatedly(Return(3));
+  EXPECT_CALL(*packet, size())
+      .WillRepeatedly(Return(kMinPacketRequestBytes + 1));
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, false);
+
+  packet.reset(new StrictMock<MockRtpPacketToSend>());
+  packet->set_capture_time_ms(kCaptureTime2);
+  EXPECT_CALL(*packet, headers_size()).WillRepeatedly(Return(2));
+  EXPECT_CALL(*packet, size())
+      .WillRepeatedly(Return(kMinPacketRequestBytes + 2));
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, false);
+
+  packet.reset(new StrictMock<MockRtpPacketToSend>());
+  packet->set_capture_time_ms(kCaptureTime3);
+  EXPECT_CALL(*packet, headers_size()).WillRepeatedly(Return(1));
+  EXPECT_CALL(*packet, size())
+      .WillRepeatedly(Return(kMinPacketRequestBytes + 3));
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, false);
+
+  std::unique_ptr<RtpPacketToSend> fit =
+      hist_.GetBestFittingPacket(kMinPacketRequestBytes, true);

[danilchap, 2017/03/28 15:10:58]

since last parameter of the GetBestFittingPacket matter for this test, put a
comment what it means,
with an extra variable: const bool kIncludeHeader = true;
or with a comment: , true /* include_header */)

+  ASSERT_TRUE(fit);
+  EXPECT_EQ(kCaptureTime1, fit->capture_time_ms());
+  fit = hist_.GetBestFittingPacket(kMinPacketRequestBytes, false);
+  ASSERT_TRUE(fit);
+  EXPECT_EQ(kCaptureTime2, fit->capture_time_ms());
+}
+
 }  // namespace webrtc