Always append the BYE packet type at the end

webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
index b3ee1a6cb639371a2d76df01011da0dce764bc85..1766b0d1dd1e29b48687647233a3488af2eb4440 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -19,9 +19,11 @@
 #include "webrtc/common_types.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_sender.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h"

[danilchap, 2016/02/11 13:35:30]

Alphabetical order.

 #include "webrtc/test/rtcp_packet_parser.h"
 
 using ::testing::ElementsAre;
+using webrtc::RTCPUtility::RtcpCommonHeader;
 
 namespace webrtc {
 
@@ -216,6 +218,43 @@ class TestTransport : public Transport,
   test::RtcpPacketParser parser_;
 };
 
+// This is a mock transport class which is solely used

[danilchap, 2016/02/11 13:35:30]

This mock is used in one test only, may be move it there.
This way the main check (EXPECT_TRUE(is_last_packet)) would be inside the test.
i.e.
TEST_F(RtcpSenderTest, ByeMustBeLast) {
  class MockTransportExpectsByeIsLastInCompoundRtcpPacket : public Transport {
    ...
  } mock_transport;
  // Re-configure rtcp_sender_ with mock_transport
  rtcp_sender_.reset(new RTCPSender(..., &mock_transport));
  ...
  
or you can use gmock:
#include "webrtc/test/mock_transport.h"
using ::testing::_;
using ::testing::ElementsAre;
using ::testing::Invoke;
...
TEST_F(RtcpSenderTest, ByeMustBeLast) {
  MockTransport mock_transport;
  EXPECT_CALL(mock_transport, SendRtcp(_, _))
      .WillOnce(Invoke([](const uint8_t* data, size_t len) {
        const uint8_t* next_packet = data;
        ...
        return true;
      }));
  // Re-configure rtcp_sender_ with mock_transport
  rtcp_sender_.reset(new RTCPSender(..., &mock_transport));

+// by the ByeMustBeLast test to validate that BYE must
+// be the last packet type in a RTCP compound packet.
+class MockTransportByeIsLastInCompoundRtcpPacket : public Transport,
+  public NullRtpData {

[danilchap, 2016/02/11 13:35:30]

any reason to derive it from NullRtpData?

+ public:
+  MockTransportByeIsLastInCompoundRtcpPacket() {}
+
+  bool SendRtp(const uint8_t* /*data*/,
+    size_t /*len*/,
+    const PacketOptions& options) override {

[danilchap, 2016/02/11 13:35:30]

for consistency comment /*options*/ parameter too.

+    return false;
+  }
+  bool SendRtcp(const uint8_t* data, size_t len) override {
+    const uint8_t* next_packet = data;
+
+    while (next_packet < data + len) {
+      RtcpCommonHeader header;
+      RtcpParseCommonHeader(next_packet, len - (next_packet - data), &header);
+      next_packet = next_packet +
+        header.payload_size_bytes +
+        RtcpCommonHeader::kHeaderSizeBytes;
+      if (header.packet_type == RTCPUtility::PT_BYE) {
+        bool is_last_packet = (data + len == next_packet);
+        EXPECT_TRUE(is_last_packet) <<
+          "Bye packet should be last in a compound RTCP packet.";
+      }
+    }
+    return true;
+  }
+  int OnReceivedPayloadData(const uint8_t* payload_data,
+    const size_t payload_size,
+    const WebRtcRTPHeader* rtp_header) override {
+    return 0;
+  }
+};
+
 namespace {
 static const uint32_t kSenderSsrc = 0x11111111;
 static const uint32_t kRemoteSsrc = 0x22222222;
@@ -256,6 +295,7 @@ class RtcpSenderTest : public ::testing::Test {
 
   SimulatedClock clock_;
   TestTransport test_transport_;
+  MockTransportByeIsLastInCompoundRtcpPacket mock_transport_;
   rtc::scoped_ptr<ReceiveStatistics> receive_statistics_;
   rtc::scoped_ptr<ModuleRtpRtcpImpl> rtp_rtcp_impl_;
   rtc::scoped_ptr<RTCPSender> rtcp_sender_;
@@ -761,4 +801,45 @@ TEST_F(RtcpSenderTest, SendCompoundPliRemb) {
   EXPECT_EQ(1, parser()->pli()->num_packets());
 }
 
+
+// This test is written to verify that BYE is always the last packet
+// type in a RTCP compoud packet.  The rtcp_sender_ is recreated with
+// mock_transport_, which is used to check for whether BYE at the end
+// of a RTCP compound packet.
+// See https://bugs.chromium.org/p/webrtc/issues/detail?id=5498 for
+// details.
+TEST_F(RtcpSenderTest, ByeMustBeLast) {
+  // Re-configure rtcp_sender_ with mock_transport_
+  rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(),
+                                    nullptr, nullptr, &mock_transport_));
+  rtcp_sender_->SetSSRC(kSenderSsrc);
+  rtcp_sender_->SetRemoteSSRC(kRemoteSsrc);
+
+  // Set up XR VoIP metric to be included with BYE
+  rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound);
+  RTCPVoIPMetric metric;
+  metric.lossRate = 1;

[danilchap, 2016/02/11 13:35:30]

Are VoIPMetric values have any meaning for this test? may be can remove them to
make test shorter and concentrated on the bye packet.
i.e.
RTCPVoIPMetric metric;
rtcp_sender_->SetRTCPVoIPMetrics(&metric);

+  metric.discardRate = 2;
+  metric.burstDensity = 3;
+  metric.gapDensity = 4;
+  metric.burstDuration = 0x1111;
+  metric.gapDuration = 0x2222;
+  metric.roundTripDelay = 0x3333;
+  metric.endSystemDelay = 0x4444;
+  metric.signalLevel = 5;
+  metric.noiseLevel = 6;
+  metric.RERL = 7;
+  metric.Gmin = 8;
+  metric.Rfactor = 9;
+  metric.extRfactor = 10;
+  metric.MOSLQ = 11;
+  metric.MOSCQ = 12;
+  metric.RXconfig = 13;
+  metric.JBnominal = 0x5555;
+  metric.JBmax = 0x6666;
+  metric.JBabsMax = 0x7777;
+  EXPECT_EQ(0, rtcp_sender_->SetRTCPVoIPMetrics(&metric));
+  EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpBye));
+}
+
 }  // namespace webrtc