Fix setting of recovered flag in RtxReceiveStream.

Fix setting of recovered flag in RtxReceiveStream.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/3005793002
Cr-Commit-Position: refs/heads/master@{#19599}
Committed: https://chromium.googlesource.com/external/webrtc/+/386449971a8f471e87f3844ebee1b1bc74144998

[nisse-webrtc, 2017-08-29 14:12:28]

nisse@webrtc.org changed reviewers:
+ brandtr@webrtc.org, danilchap@webrtc.org

[nisse-webrtc, 2017-08-29 14:12:29]

PTAL. ps#1 is extracted from cl https://codereview.webrtc.org/3008773002/, ps#2.

ps#2 addresses Danil's comments.

Also needs a test before landing.

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

I've added a unittest, and
I hope to land this pretty soon.

https://codereview.webrtc.org/3005793002/diff/20001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream.cc (right):

https://codereview.webrtc.org/3005793002/diff/20001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream.cc:27: << "RtxReceiveStream created with empty
payload type mapping.";
The reason I initially had a DCHECK here, was to catch any code paths where I
had missed copying the data from UplfecConfig to this mapping.

https://codereview.webrtc.org/3005793002/diff/20001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream.h (right):

https://codereview.webrtc.org/3005793002/diff/20001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream.h:34: const std::map<int, int>
associated_payload_types_;
These renames are admittedly not related to the main bugfix in this cl, but for
simplicity, I prefer to do them in the same cl.

https://codereview.webrtc.org/3005793002/diff/20001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/3005793002/diff/20001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:90: {kUnknownPayloadType,
kMediaPayloadType}};
With the DCHECK changed to a log message, this test would pass with an empty
mapping. But it makes some sense to me to keep this change, and pass a non-empty
mapping with an unrelated payload type.

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:94: [](const RtpPacketReceived&
packet) {
Is there some shorter way to check the value of the flag, without
testing::Invoke?

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

lgtm
with optional nits

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream.cc (right):

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream.cc:25: if (associated_payload_types_.size() == 0)
{
it might be better practice to check 
.empty()
rather than .size() == 0

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:59: static const std::map<int, int> m
= {{kRtxPayloadType, kMediaPayloadType}};
static here look unnecesary:
you copy on return anyway, so no point to keep an extra global instance of m.

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:94: [](const RtpPacketReceived&
packet) {
On 2017/08/30 07:29:23, nisse-webrtc wrote:
> Is there some shorter way to check the value of the flag, without
> testing::Invoke?

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());

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:103: static const std::map<int, int>
payload_type_mapping = {
why you made it static?

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

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream.cc (right):

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream.cc:25: if (associated_payload_types_.size() == 0)
{
On 2017/08/30 07:54:21, danilchap wrote:
> it might be better practice to check 
> .empty()
> rather than .size() == 0

Done.

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
File webrtc/call/rtx_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:59: static const std::map<int, int> m
= {{kRtxPayloadType, kMediaPayloadType}};
On 2017/08/30 07:54:21, danilchap wrote:
> static here look unnecesary:
> you copy on return anyway, so no point to keep an extra global instance of m.

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).

https://codereview.webrtc.org/3005793002/diff/40001/webrtc/call/rtx_receive_s...
webrtc/call/rtx_receive_stream_unittest.cc:94: [](const RtpPacketReceived&
packet) {
On 2017/08/30 07:54:21, danilchap wrote:
> On 2017/08/30 07:29:23, nisse-webrtc wrote:
> > Is there some shorter way to check the value of the flag, without
> > testing::Invoke?
> 
> 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());

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.

[brandtr, 2017-08-30 08:43:18]

lgtm

[nisse-webrtc, 2017-08-30 08:52:56]

Dropped static. Intend to land if the linux_internal bot is happy.

[nisse-webrtc, 2017-08-30 08:54:07]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-08-30 08:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3005793002/#ps80001 (title:
"Drop static. git cl format.")

[commit-bot: I haz the power, 2017-08-30 08:54:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-30 08:59:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-30 08:59:11]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/20952)

[nisse-webrtc, 2017-08-30 09:05:07]

nisse@webrtc.org changed reviewers:
+ stefan@webrtc.org

[nisse-webrtc, 2017-08-30 09:05:08]

Stefan, please take an OWNER's look.

[stefan-webrtc, 2017-08-30 10:46:10]

lgtm

[nisse-webrtc, 2017-08-30 11:11:00]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-08-30 11:11:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-30 11:16:41]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1504091460136360,
"parent_rev": "6d5b4d6fe13951a7f3f6ed3d3b29bc601920e381", "commit_rev":
"386449971a8f471e87f3844ebee1b1bc74144998"}

[commit-bot: I haz the power, 2017-08-30 11:16:46]

Description was changed from

==========
Fix setting of recovered flag in RtxReceiveStream.

BUG=webrtc:7135
==========

to

==========
Fix setting of recovered flag in RtxReceiveStream.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/3005793002
Cr-Commit-Position: refs/heads/master@{#19599}
Committed:
https://chromium.googlesource.com/external/webrtc/+/386449971a8f471e87f3844eb...
==========

[commit-bot: I haz the power, 2017-08-30 11:16:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/386449971a8f471e87f3844eb...