Fix setting of recovered flag in RtxReceiveStream.

webrtc/call/rtx_receive_stream_unittest.cc

 /*
  *  Copyright (c) 2017 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/call/rtx_receive_stream.h"
 #include "webrtc/call/test/mock_rtp_packet_sink_interface.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_header_extension_map.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 
 namespace webrtc {
 
 namespace {
 
 using ::testing::_;
 using ::testing::StrictMock;
 
 constexpr int kMediaPayloadType = 100;
 constexpr int kRtxPayloadType = 98;
+constexpr int kUnknownPayloadType = 90;
 constexpr uint32_t kMediaSSRC = 0x3333333;
 constexpr uint16_t kMediaSeqno = 0x5657;
 
 constexpr uint8_t kRtxPacket[] = {
     0x80,                    // Version 2.
     98,                      // Payload type.
     0x12, 0x34,              // Seqno.
     0x11, 0x11, 0x11, 0x11,  // Timestamp.
     0x22, 0x22, 0x22, 0x22,  // SSRC.
     // RTX header.
@@ skipping 10 matching lines @@
     0x22, 0x22, 0x22, 0x22,  // SSRC.
     0xbe, 0xde, 0x00, 0x01,  // Extension header.
     0x30, 0x01, 0x00, 0x00,  // 90 degree rotation.
     // RTX header.
     0x56, 0x57,              // Orig seqno.
     // Payload.
     0xee,
 };
 
 std::map<int, int> PayloadTypeMapping() {
-  std::map<int, int> m;
-  m[kRtxPayloadType] = kMediaPayloadType;
+  static const std::map<int, int> m = {{kRtxPayloadType, kMediaPayloadType}};

[danilchap, 2017/08/30 07:54:21]

static here look unnecesary:
you copy on return anyway, so no point to keep an extra global instance of m.

[nisse-webrtc, 2017/08/30 08:27:00]

Old habit of using static const, to allow the compiler to create the object in
the read-only data section. With only const, compiler might be forced to
allocate and initialize new object on the stack for each function call due to
rules about object identity.

But I honestly don't quite understand how that would work with complex C++
objects. I can drop static here and below, if you think that looks clearer. (In
case static affects efficiency in one way or the other, that doesn't matter at
all here).

   return m;
 }
 
 template <typename T>
 rtc::ArrayView<T> Truncate(rtc::ArrayView<T> a, size_t drop) {
   return a.subview(0, a.size() - drop);
 }
 
 }  // namespace
 
 TEST(RtxReceiveStreamTest, RestoresPacketPayload) {
   StrictMock<MockRtpPacketSink> media_sink;
   RtxReceiveStream rtx_sink(&media_sink, PayloadTypeMapping(), kMediaSSRC);
   RtpPacketReceived rtx_packet;
   EXPECT_TRUE(rtx_packet.Parse(rtc::ArrayView<const uint8_t>(kRtxPacket)));
 
   EXPECT_CALL(media_sink, OnRtpPacket(_)).WillOnce(testing::Invoke(
       [](const RtpPacketReceived& packet) {
         EXPECT_EQ(packet.SequenceNumber(), kMediaSeqno);
         EXPECT_EQ(packet.Ssrc(), kMediaSSRC);
         EXPECT_EQ(packet.PayloadType(), kMediaPayloadType);
         EXPECT_THAT(packet.payload(), testing::ElementsAre(0xee));
       }));
 
   rtx_sink.OnRtpPacket(rtx_packet);
 }
 
+TEST(RtxReceiveStreamTest, SetsRecoveredFlag) {
+  StrictMock<MockRtpPacketSink> media_sink;
+  RtxReceiveStream rtx_sink(&media_sink, PayloadTypeMapping(), kMediaSSRC);
+  RtpPacketReceived rtx_packet;
+  EXPECT_TRUE(rtx_packet.Parse(rtc::ArrayView<const uint8_t>(kRtxPacket)));
+  EXPECT_FALSE(rtx_packet.recovered());
+  EXPECT_CALL(media_sink, OnRtpPacket(_)).WillOnce(testing::Invoke(
+      [](const RtpPacketReceived& packet) {

[nisse-webrtc, 2017/08/30 07:29:23]

Is there some shorter way to check the value of the flag, without
testing::Invoke?

[danilchap, 2017/08/30 07:54:21]

there are, not sure which is nicer:

EXPECT_CALL(media_sink,
OnRtpPacket(testing::Property(&RtpPacketReceived::recovered, true)));

or

webrtc::RtpPacketReceived packet_received;
EXPECT_CALL(media_sink, OnRtpPacket(_)).WillOnce(SaveArg<0>(&packet_received));
rtx_sink.OnRtpPacket(rtx_packet);
EXPECT_TRUE(packet_received.recovered());

[nisse-webrtc, 2017/08/30 08:27:00]

Hmm. I think the testing::Property variant looks nicer here. But the error
message if the test fails is much uglier than a failed EXPECT_TRUE, so I think
I'll stick with testing::Invoke, which is also similar to the test above.

+        EXPECT_TRUE(packet.recovered());
+      }));
+
+  rtx_sink.OnRtpPacket(rtx_packet);
+}
+
 TEST(RtxReceiveStreamTest, IgnoresUnknownPayloadType) {
   StrictMock<MockRtpPacketSink> media_sink;
-  RtxReceiveStream rtx_sink(&media_sink, std::map<int, int>(), kMediaSSRC);
+  static const std::map<int, int> payload_type_mapping = {

[danilchap, 2017/08/30 07:54:21]

why you made it static?

+      {kUnknownPayloadType, kMediaPayloadType}};
+
+  RtxReceiveStream rtx_sink(&media_sink, payload_type_mapping, kMediaSSRC);
   RtpPacketReceived rtx_packet;
   EXPECT_TRUE(rtx_packet.Parse(rtc::ArrayView<const uint8_t>(kRtxPacket)));
   rtx_sink.OnRtpPacket(rtx_packet);
 }
 
 TEST(RtxReceiveStreamTest, IgnoresTruncatedPacket) {
   StrictMock<MockRtpPacketSink> media_sink;
   RtxReceiveStream rtx_sink(&media_sink, PayloadTypeMapping(), kMediaSSRC);
   RtpPacketReceived rtx_packet;
   EXPECT_TRUE(
@@ skipping 22 matching lines @@
         EXPECT_THAT(packet.payload(), testing::ElementsAre(0xee));
         VideoRotation rotation = kVideoRotation_0;
         EXPECT_TRUE(packet.GetExtension<VideoOrientation>(&rotation));
         EXPECT_EQ(rotation, kVideoRotation_90);
       }));
 
   rtx_sink.OnRtpPacket(rtx_packet);
 }
 
 }  // namespace webrtc