Make Call::OnRecoveredPacket parse RTP header and call OnRtpPacket.

Make Call::OnRecoveredPacket parse RTP header and call OnRtpPacket.

To make the distinction for stats, add a |recovered| flag to
RtpPacketReceived.

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

[nisse-webrtc, 2017-02-14 10:24:33]

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

[nisse-webrtc, 2017-02-14 10:24:33]

Refactoring OnRecoveredPacket as we discussed. Seems easy for flexfec, but there
are other complications.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:297: bool
RtpStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet,
Is there any easy way to get rid of this method and the |rtp_header_parser_|
now? It doesn't seem quite right to pass rtx, red or ulpfec packets (it's not
entirely clear which of these use this method, and I don't really know what they
are, either) back to call just for parsing.

It would make more sense to pass them back to an RtpTransportReceiveController,
when we get that far with the refactoring.

[brandtr, 2017-02-15 12:20:50]

Looks good! Some comments.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/call/call.cc#newcode1256
webrtc/call/call.cc:1256: bool Call::OnRecoveredPacket(const uint8_t* packet,
size_t length) {
could the return value be turned into void here as well?

https://codereview.webrtc.org/2693123002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:40: bool retransmit() const
{ return retransmit_; }
rename to 'retransmitted'/'set_retransmitted', to be consistent with the new
methods below.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:297: bool
RtpStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet,
On 2017/02/14 10:24:33, nisse-webrtc wrote:
> Is there any easy way to get rid of this method and the |rtp_header_parser_|
> now? It doesn't seem quite right to pass rtx, red or ulpfec packets (it's not
> entirely clear which of these use this method, and I don't really know what
they
> are, either) back to call just for parsing.
> 
> It would make more sense to pass them back to an
RtpTransportReceiveController,
> when we get that far with the refactoring.

No, RTX, RED, and ULPFEC should not go back to Call, because they all belong to
the stream tied to this RtpStreamReceiver. The reason that FlexFEC reception is
not implemented here is that FlexFEC will in the future be able to do
multistream protection, so then the packets _should_ go back to Call.

Given that the RtpStreamReceiver already has information about the config (*)
for the VideoReceiveStream it's owned by, you could add a
RtpStreamReceiver::ParseRtpPacket method that would be called by the
ParseAndHandleEncapsulatedHeader method. The ParseRtpPacket method would then
replace the usage of |rtp_header_parser_|. Then I believe the recovered packets
(be it RTX, ULPFEC, FlexFEC, etc.) could go through the OnRtpPacket method,
after your changes below.

(*) Alternatively, you could pass down the new RtpReceiveConfig struct?

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:324: // This method handles both regular Rtp
packets and packets recovered
Nits: "RTP packets". "FlexFEC".

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:368: if (!packet.recovered()) {
Could you reorder the lines in this method, so that this if clause is merged
with the one above?

Alternatively: call OnRecoveredPacket here, since the functionality seems the
same. Then that method should probably be renamed.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:377: // TODO(nisse): Should we pass a
recovered flag to stats?
That would be one way of implementing bugs.webrtc.org/6339.

[nisse-webrtc, 2017-02-15 13:59:15]

https://codereview.webrtc.org/2693123002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/call/call.cc#newcode1256
webrtc/call/call.cc:1256: bool Call::OnRecoveredPacket(const uint8_t* packet,
size_t length) {
On 2017/02/15 12:20:50, brandtr wrote:
> could the return value be turned into void here as well?

That's a change of the RecoveredPacketReceiver interface. I'll do it.

The logic to keep calling OnRecoveredPacket again and again if it returns false
doesn't look particularly useful.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:40: bool retransmit() const
{ return retransmit_; }
On 2017/02/15 12:20:50, brandtr wrote:
> rename to 'retransmitted'/'set_retransmitted', to be consistent with the new
> methods below.

On the receive side, that verb doesn't seem quite right to me. *We* haven't
retransmitted anything; it's just that we received the packet as an rtx payload,
supposedly because the other end retransmitted it.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:297: bool
RtpStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet,
On 2017/02/15 12:20:50, brandtr wrote:
> On 2017/02/14 10:24:33, nisse-webrtc wrote:
> > Is there any easy way to get rid of this method and the |rtp_header_parser_|
> > now? It doesn't seem quite right to pass rtx, red or ulpfec packets (it's
not
> > entirely clear which of these use this method, and I don't really know what
> they
> > are, either) back to call just for parsing.
> > 
> > It would make more sense to pass them back to an
> RtpTransportReceiveController,
> > when we get that far with the refactoring.
> 
> No, RTX, RED, and ULPFEC should not go back to Call, because they all belong
to
> the stream tied to this RtpStreamReceiver. The reason that FlexFEC reception
is
> not implemented here is that FlexFEC will in the future be able to do
> multistream protection, so then the packets _should_ go back to Call.
> 
> Given that the RtpStreamReceiver already has information about the config (*)
> for the VideoReceiveStream it's owned by, you could add a
> RtpStreamReceiver::ParseRtpPacket method that would be called by the
> ParseAndHandleEncapsulatedHeader method. The ParseRtpPacket method would then
> replace the usage of |rtp_header_parser_|. Then I believe the recovered
packets
> (be it RTX, ULPFEC, FlexFEC, etc.) could go through the OnRtpPacket method,
> after your changes below.
> 
> (*) Alternatively, you could pass down the new RtpReceiveConfig struct?

This is tricky. Parsing extensions logically belongs to the stream. But then
Call (or the generic RTP receive code) needs the extensions to feed information
to the congestion controller.

One alternative might be to let call to only parse out the ssrc, invoke
OnRtpPacket with a non-const RtpPacketReceived with unparsed extension headers,
and let OnRtpPacket return with extensions parsed. We'd then have to add back
the return value, to indicate that parsing failed.

I'll have to think a bit more about this. But we really should parse the
extension headers at a single place.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:324: // This method handles both regular Rtp
packets and packets recovered
On 2017/02/15 12:20:50, brandtr wrote:
> Nits: "RTP packets". "FlexFEC".

Done.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:368: if (!packet.recovered()) {
On 2017/02/15 12:20:50, brandtr wrote:
> Could you reorder the lines in this method, so that this if clause is merged
> with the one above?

I'm not sure how order matters. And I'd prefer to not move the packet.GetHeader
earlier. Let's discuss.
 
> Alternatively: call OnRecoveredPacket here, since the functionality seems the
> same. Then that method should probably be renamed.

Not easy, OnRecoveredPacket has other callers which wants it to parse the
packet.

So I think we'll either have to live with two recovered-packet paths for now, or
postpone this change until header parsing has settled more.

https://codereview.webrtc.org/2693123002/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:377: // TODO(nisse): Should we pass a
recovered flag to stats?
On 2017/02/15 12:20:50, brandtr wrote:
> That would be one way of implementing bugs.webrtc.org/6339.

I'm adding a TODO and bug reference.

[nisse-webrtc, 2017-05-09 12:37:56]

Hi, I'm trying to get this change moving again. Would you like to have a new
look?

https://codereview.webrtc.org/2693123002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:40: bool retransmit() const
{ return retransmit_; }
On 2017/02/15 13:59:14, nisse-webrtc wrote:
> On 2017/02/15 12:20:50, brandtr wrote:
> > rename to 'retransmitted'/'set_retransmitted', to be consistent with the new
> > methods below.
> 
> On the receive side, that verb doesn't seem quite right to me. *We* haven't
> retransmitted anything; it's just that we received the packet as an rtx
payload,
> supposedly because the other end retransmitted it. 

Danil suggested that we use only a single "recovered" flag, and use it for both
rtx and fec. So I'll delete retransmit.

[brandtr, 2017-05-10 14:27:29]

lgtm with some comments. Thanks for doing this.

RtpPacket::recovered sgtm.

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/call/call.cc#oldc...
webrtc/call/call.cc:1259: uint32_t ssrc =
ByteReader<uint32_t>::ReadBigEndian(&packet[8]);
Nice to get rid of this!

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:39: // Flag if packet was
recovered via rtx or fec.
Capitals? "RTX or FEC".

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream_unittest.cc (left):

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/video/video_recei...
webrtc/video/video_receive_stream_unittest.cc:133:
video_receive_stream_->OnRecoveredPacket(rtppacket.data(),
This looks weird to me. Why is OnRecoveredPacket called here in the first place?

[nisse-webrtc, 2017-05-11 07:36:14]

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

[nisse-webrtc, 2017-05-11 07:36:15]

Erik, can you have a look at the video_receive_stream_unittest.cc? Is there any
easy way to make it work with OnRtpPacket rather than OnRecoveredPacket?
You reviewed it at the time.

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:39: // Flag if packet was
recovered via rtx or fec.
On 2017/05/10 14:27:29, brandtr wrote:
> Capitals? "RTX or FEC".

Done.

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream_unittest.cc (left):

https://codereview.webrtc.org/2693123002/diff/100001/webrtc/video/video_recei...
webrtc/video/video_receive_stream_unittest.cc:133:
video_receive_stream_->OnRecoveredPacket(rtppacket.data(),
On 2017/05/10 14:27:29, brandtr wrote:
> This looks weird to me. Why is OnRecoveredPacket called here in the first
place?

No idea. The test was added a few months ago, see
https://codereview.webrtc.org/2721653002

[sprang_webrtc, 2017-05-11 08:48:45]

On 2017/05/11 07:36:15, nisse-webrtc wrote:
> Erik, can you have a look at the video_receive_stream_unittest.cc? Is there
any
> easy way to make it work with OnRtpPacket rather than OnRecoveredPacket?
> You reviewed it at the time.
 
What you already added there looks fine to me, for a test.
Did you have some specific issue with it?

[sprang_webrtc, 2017-05-11 08:48:59]

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:39: // Flag if packet was
recovered via RTX or FEC.
Do we want a single flag or should we perhaps separate the two so we can more
easily distinguish them in stats?

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:341: }
nit: can we clean up the extra scopes for locking here? I don't think it's
justified to have one for checking if we're receiving above, then release it
only to (potentially) grab it again after checking the time....

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:353: // TODO(nisse): Why isn't this done for
recovered packets?
Will the header here have the actual payload type, or will it have ulpfec or red
or rtx some other payload type we don't want to register for this ssrc?
Anyway, that logic might be better put in
rtp_payload_registry_.SetIncomingPayloadType() since that's where rtx is
filtered out...

[nisse-webrtc, 2017-05-11 08:51:38]

On 2017/05/11 08:48:45, sprang_webrtc wrote:
> On 2017/05/11 07:36:15, nisse-webrtc wrote:
> > Erik, can you have a look at the video_receive_stream_unittest.cc? Is there
> any
> > easy way to make it work with OnRtpPacket rather than OnRecoveredPacket?
> > You reviewed it at the time.
>  
> What you already added there looks fine to me, for a test.
> Did you have some specific issue with it?

I'm curious for an answer to Rasmus' question "Why is OnRecoveredPacket called
here in the first place?"

[nisse-webrtc, 2017-05-11 09:04:01]

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet_received.h (right):

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet_received.h:39: // Flag if packet was
recovered via RTX or FEC.
On 2017/05/11 08:48:58, sprang_webrtc wrote:
> Do we want a single flag or should we perhaps separate the two so we can more
> easily distinguish them in stats?

Discussed with danil, and single flag seems to be good enough for now.

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:341: }
On 2017/05/11 08:48:58, sprang_webrtc wrote:
> nit: can we clean up the extra scopes for locking here? I don't think it's
> justified to have one for checking if we're receiving above, then release it
> only to (potentially) grab it again after checking the time....

Done.

https://codereview.webrtc.org/2693123002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:353: // TODO(nisse): Why isn't this done for
recovered packets?
On 2017/05/11 08:48:58, sprang_webrtc wrote:
> Will the header here have the actual payload type, or will it have ulpfec or
red
> or rtx some other payload type we don't want to register for this ssrc?
> Anyway, that logic might be better put in
> rtp_payload_registry_.SetIncomingPayloadType() since that's where rtx is
> filtered out...

No idea. At the moment I don't think rtx or ulpfec enter this code path.

[sprang_webrtc, 2017-05-11 10:56:25]

lgtm

[nisse-webrtc, 2017-05-11 10:58:51]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-11 10:58:51]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2693123002/#ps140001 (title:
"Merge two locking scopes.")

[commit-bot: I haz the power, 2017-05-11 10:59:01]

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-05-11 11:04:14]

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

[commit-bot: I haz the power, 2017-05-11 11:04:15]

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

[nisse-webrtc, 2017-05-11 11:12:03]

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

[nisse-webrtc, 2017-05-11 11:12:04]

Magnus: OWNER's approval needed.

[mflodman, 2017-05-11 11:17:45]

On 2017/05/11 11:12:04, nisse-webrtc wrote:
> Magnus: OWNER's approval needed.

Owner's approval for (a) specific folder(s) or all of the CL?

[nisse-webrtc, 2017-05-11 11:21:39]

On 2017/05/11 11:17:45, mflodman wrote:
> On 2017/05/11 11:12:04, nisse-webrtc wrote:
> > Magnus: OWNER's approval needed.
> 
> Owner's approval for (a) specific folder(s) or all of the CL?

For call/, modules/rtp_rtcp, (and the trivial fuzzer update in ps#9 also lack
OWNER's approval).

[mflodman, 2017-05-11 13:26:29]

one ask, then lgtm

https://codereview.webrtc.org/2693123002/diff/160001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/160001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:317: if (!packet.recovered()) {
Why do we only want to log non-recovered packets?

Can you comment?

[nisse-webrtc, 2017-05-11 13:31:55]

https://codereview.webrtc.org/2693123002/diff/160001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2693123002/diff/160001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:317: if (!packet.recovered()) {
On 2017/05/11 13:26:29, mflodman wrote:
> Why do we only want to log non-recovered packets?
> 
> Can you comment?

Only to keep the previous behavior unchanged.

[nisse-webrtc, 2017-05-11 13:57:54]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-11 13:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2693123002/#ps160001 (title:
"Update fuzzer.")

[commit-bot: I haz the power, 2017-05-11 13:58:00]

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-05-11 15:00:59]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494511074319850,
"parent_rev": "198c80d0afd919939b782360ff10eca80a9fe704", "commit_rev":
"d2ef314292b338e558e8aa8a886525189d0a4fab"}

[commit-bot: I haz the power, 2017-05-11 15:01:02]

Description was changed from

==========
Make Call::OnRecoveredPacket parse RTP header and call OnRtpPacket.

To make the distinction for stats, add a |recovered| flag to
RtpPacketReceived.

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

to

==========
Make Call::OnRecoveredPacket parse RTP header and call OnRtpPacket.

To make the distinction for stats, add a |recovered| flag to
RtpPacketReceived.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2693123002
Cr-Commit-Position: refs/heads/master@{#18103}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d2ef314292b338e558e8aa8a8...
==========

[commit-bot: I haz the power, 2017-05-11 15:01:03]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/d2ef314292b338e558e8aa8a8...