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

webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
index 2a7caf188f69c2df0a2c85b6f331f7a0ad1337b0..8e4fdc470664e37012b586cea1d888b207633552 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
@@ -15,6 +15,7 @@
 #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"
 
@@ -219,4 +220,59 @@ TEST_F(RtpPacketHistoryTest, FullExpansion) {
   }
 }
 
+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