Fix setting of recovered flag in RtxReceiveStream.

webrtc/call/rtx_receive_stream_unittest.cc

Index: webrtc/call/rtx_receive_stream_unittest.cc
diff --git a/webrtc/call/rtx_receive_stream_unittest.cc b/webrtc/call/rtx_receive_stream_unittest.cc
index e9c82105212adec197b49f6298c1e345b5df3be5..2b2eabc423e8f4316ad575a8278c278bfe3584e4 100644
--- a/webrtc/call/rtx_receive_stream_unittest.cc
+++ b/webrtc/call/rtx_receive_stream_unittest.cc
@@ -25,6 +25,7 @@ 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;
 
@@ -55,8 +56,7 @@ constexpr uint8_t kRtxPacketWithCVO[] = {
 };
 
 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;
 }
 
@@ -84,9 +84,26 @@ TEST(RtxReceiveStreamTest, RestoresPacketPayload) {
   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);