Fix RTPPayloadRegistry to correctly restore RTX, if a valid mapping exists.

Fix RTPPayloadRegistry to correctly restore RTX, if a valid mapping exists.

Also updated the RTPPayloadRegistry::RestoreOriginalPacket signature to not take the first arg as a **, since it isn't modified.

Committed: https://crrev.com/65220a70a38ffe252b587775c5e9104606ab7c2c
Cr-Commit-Position: refs/heads/master@{#10276}

[noahric, 2015-10-10 01:32:17]

noahric@chromium.org changed reviewers:
+ holmer@chromium.org

[noahric, 2015-10-10 01:32:17]

I think this is correct, though one e2e test still fails:
EndToEndTest.DecodesRetransmittedFrameByRedOverRtx

I've stepped through in a debugger and I can definitely see that (1) it's being
fed an RTX packet with a mapping, (2) the last_incoming_payload_type is not the
same as the result of that mapping, and (3) the test is depending on that to
pass. If you have any thoughts, that'd be greatly appreciated :) Thanks!

[stefan-webrtc, 2015-10-12 15:41:53]

On 2015/10/10 01:32:17, noahric wrote:
> I think this is correct, though one e2e test still fails:
> EndToEndTest.DecodesRetransmittedFrameByRedOverRtx
> 
> I've stepped through in a debugger and I can definitely see that (1) it's
being
> fed an RTX packet with a mapping, (2) the last_incoming_payload_type is not
the
> same as the result of that mapping, and (3) the test is depending on that to
> pass. If you have any thoughts, that'd be greatly appreciated :) Thanks!

I think the problem here is that the mapping is incorrect. All packets have the
red header (pltype 118), and the retransmissions have rtx (pltype 98) and red
headers. The underlying media payload type is 125.

Looking at what we receive and parse in rtp_payload_registry.cc, we see:
- the packets coming in have pltype 98, which is rtx
- there is a mapping from 98 to 125

This is an incorrect mapping, since the underlying header has payload type 118,
which is red. Therefore my guess is that the packet is dropped because it can't
be decoded correctly.

The reason the incorrect mapping is there is because old Chrome versions only
knew about one rtx mapping, and if announcing both rtx over red and rtx over vp8
it could get it wrong. Based on this, I'm not sure we can start doing the
correct mappings in the receiver just yet. What if an old webrtc versions starts
sending rtx+red with the incorrect (rtx -> vp8) mapping to a new webrtc which
thinks the mapping is correct and recovers the packet as vp8 when it actually is
red?

I think what we would have to do is to first make a change to the sender which
offers the correct mappings, but still looks at incoming payload types to
recover rtx. When that has been around for a few versions we can do the change
on the receiver to actually rely on the map.

Do you see any other way? I'd be very happy to solve this faster...

[noahric, 2015-10-12 18:06:37]

noahric@chromium.org changed reviewers:
+ pbos@webrtc.org

[noahric, 2015-10-12 18:06:38]

Added an option (off by default) for respecting the mapping. That will allow
clients other than Chrome to get the fixed behavior, and Chrome can follow suit
when the sender is fixed.

+pbos for the video receiver config changes.

[noahric, 2015-10-12 18:06:57]

On 2015/10/12 18:06:38, noahric wrote:
> Added an option (off by default) for respecting the mapping. That will allow
> clients other than Chrome to get the fixed behavior, and Chrome can follow
suit
> when the sender is fixed.
> 
> +pbos for the video receiver config changes.

(And the e2e test now passes, because the option is off there)

[stefan-webrtc, 2015-10-13 13:07:56]

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

[stefan-webrtc, 2015-10-13 13:07:57]

A few comments, but overall lgtm as a workaround for now.

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h (right):

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h:162: 
Remove this line

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right):

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:265: std::map<int,
int>::const_iterator apt_mapping =
auto

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right):

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:262: // The RTX
sequence number  and ssrc will use the default value of 9999. The
s/"number  and"/"number and"

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:324: << "The
restored packet should have the correct ssrc.";
You could instead use the RtpHeaderParser and simply parse the packet after
RestoreOriginalPacket. I think this test code would be a lot easier to read if
you do that.

[noahric, 2015-10-13 17:03:31]

Thanks!

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h (right):

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h:162: 
On 2015/10/13 13:07:56, stefan-webrtc (holmer) wrote:
> Remove this line

Done.

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right):

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:265: std::map<int,
int>::const_iterator apt_mapping =
On 2015/10/13 13:07:56, stefan-webrtc (holmer) wrote:
> auto

Done.

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right):

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:262: // The RTX
sequence number  and ssrc will use the default value of 9999. The
On 2015/10/13 13:07:56, stefan-webrtc (holmer) wrote:
> s/"number  and"/"number and"

Done.

https://codereview.webrtc.org/1394573004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:324: << "The
restored packet should have the correct ssrc.";
On 2015/10/13 13:07:56, stefan-webrtc (holmer) wrote:
> You could instead use the RtpHeaderParser and simply parse the packet after
> RestoreOriginalPacket. I think this test code would be a lot easier to read if
> you do that.

Done. Also cleaned up the types to remove the static casts.

I had to add in code in the packet generator to add the version, so that makes
the code a little more brittle (if the RtpHeaderParser gets more strict in what
it allows, this test will break), but it is cleaner :) Thanks!

[noahric, 2015-10-13 20:13:55]

The CQ bit was checked by noahric@chromium.org

[noahric, 2015-10-13 20:13:55]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1394573004/#ps40001 (title:
"CR comments and git cl format")

[commit-bot: I haz the power, 2015-10-13 20:14:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394573004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394573004/40001

[commit-bot: I haz the power, 2015-10-13 20:18:43]

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

[commit-bot: I haz the power, 2015-10-13 20:18:44]

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

[noahric, 2015-10-13 20:23:01]

noahric@chromium.org changed reviewers:
+ henrika@chromium.org, henrikg@webrtc.org, mflodman@webrtc.org

[noahric, 2015-10-13 20:23:02]

+ a few more people for toplevel OWNERS and voice_engine OWNERS. Thanks!

[mflodman, 2015-10-14 07:24:30]

On 2015/10/13 20:23:02, noahric wrote:
> + a few more people for toplevel OWNERS and voice_engine OWNERS. Thanks!

Stefan is an owner for rtp and video, so the best would be if Henrik or Henrik
reviews VoE/channel.cc.

[mflodman, 2015-10-14 07:24:34]

On 2015/10/13 20:23:02, noahric wrote:
> + a few more people for toplevel OWNERS and voice_engine OWNERS. Thanks!

Stefan is an owner for rtp and video, so the best would be if Henrik or Henrik
reviews VoE/channel.cc.

[mflodman, 2015-10-14 07:24:57]

mflodman@webrtc.org changed reviewers:
- mflodman@webrtc.org, pbos@webrtc.org

[mflodman, 2015-10-14 07:27:44]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2015-10-14 07:27:45]

video_receive_stream.h LGTM

[Henrik Grunell WebRTC, 2015-10-14 08:34:29]

webrtc/voice_engine/channel.cc lgtm

[noahric, 2015-10-14 17:18:59]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2015-10-14 17:19:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394573004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394573004/40001

[commit-bot: I haz the power, 2015-10-14 18:29:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-14 18:30:03]

Patchset 3 (id:??) landed as
https://crrev.com/65220a70a38ffe252b587775c5e9104606ab7c2c
Cr-Commit-Position: refs/heads/master@{#10276}