Replace RtpStreamReceiver::DeliverRtp with OnRtpPacket.

Replace RtpStreamReceiver::DeliverRtp with OnRtpPacket.

This avoids redoing RTP header parsing already done in Call, for video.

The next step is to convert other types of receive streams, i.e.,
audio and flexfec, to use a compatible OnRtpPacket method. We can then
introduce a shared base interface, and simplify media-independent
receive processing in Call.

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

[nisse-webrtc, 2017-02-09 10:13:25]

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

[nisse-webrtc, 2017-02-09 10:13:25]

This is the video rtp header parsing change, extracted from
https://codereview.webrtc.org/2688473004/

One question: OnRecoveredPacket, is there any reason the VideoReceiveStream
needs to handle recovered differently from other arriving packets? It would be
nice to rewrite Call::OnRecoveredPacket to parse the packet header and invoke
OnRtpPacket.

Only obvious differences to Call::DeliverRtp is that we handle video only, and
that the inputs |media_type| and |packet_time| are missing.

[brandtr, 2017-02-09 14:21:50]

On 2017/02/09 10:13:25, nisse-webrtc wrote:
> This is the video rtp header parsing change, extracted from
> https://codereview.webrtc.org/2688473004/
> 
> One question: OnRecoveredPacket, is there any reason the VideoReceiveStream
> needs to handle recovered differently from other arriving packets? It would be
> nice to rewrite Call::OnRecoveredPacket to parse the packet header and invoke
> OnRtpPacket.
> 
> Only obvious differences to Call::DeliverRtp is that we handle video only, and
> that the inputs |media_type| and |packet_time| are missing.

Looks good!

Recovered packets need to be handled differently due to stats: for example, we
don't want the RTCP fraction_lost information to be changed based on how
effective we are at doing FEC or RTX. But it would indeed be nice to have
Call::OnRecoveredRtpPacket to parse the packet header.

[brandtr, 2017-02-09 14:22:17]

Missed to send the comments.

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

https://codereview.webrtc.org/2681673004/diff/1/webrtc/call/call.cc#newcode1217
webrtc/call/call.cc:1217: // Deliver media packets to FlexFEC subsystem. RTP
header extensions need
This comment can be reduced to:
"Deliver media packets to FlexFEC subsystem.", since the RTP header extensions
now _are_ being parsed.

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

https://codereview.webrtc.org/2681673004/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:338: ss << "Packet received on SSRC: " <<
header.ssrc << " with payload type: "
Might as well remove the usage of |header| here, to make it easier to remove it
completely in the future.

(RtpPacket::GetHeader is destined to be removed when it is no longer needed, I
believe.)

header.ssrc <=> packet.Ssrc(),
header.extension.hasTransmissionTimeOffset <=>
packet.HasExtension<TransmissionOffset>, etc.

https://codereview.webrtc.org/2681673004/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:356: bool ret = ReceivePacket(packet.data(),
packet.size(), header, in_order);
I believe this is correct.

If it were incorrect, it feels that lots of tests would fail?

https://codereview.webrtc.org/2681673004/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2681673004/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:263: bool
VideoReceiveStream::OnRecoveredPacket(const uint8_t* packet,
As you suggest, it would be nice to change the signature to
OnRecoveredRtpPacket(const RtpPacketReceived&).

By doing that change, |rtp_header_parser_| could be removed from
RtpStreamReceiver. I believe that
RtpStreamReceiver::EnableReceiveRtpHeaderExtension could also be removed.

[nisse-webrtc, 2017-02-10 09:12:30]

On 2017/02/09 14:21:50, brandtr wrote:
> Recovered packets need to be handled differently due to stats: for example, we
> don't want the RTCP fraction_lost information to be changed based on how
> effective we are at doing FEC or RTX. But it would indeed be nice to have
> Call::OnRecoveredRtpPacket to parse the packet header.

I think it would be nicer to do the special stats processing based on some flag
in RtpPacketReceived. Perhaps we can just set arrival_time == -1 wne calling
OnRtpPacket, to indicate that the recovered packet didn't arrive over the
network in the usual way?

[brandtr_google, 2017-02-10 09:24:14]

On 2017/02/10 09:12:30, nisse-webrtc wrote:
> On 2017/02/09 14:21:50, brandtr wrote:
> > Recovered packets need to be handled differently due to stats: for example,
we
> > don't want the RTCP fraction_lost information to be changed based on how
> > effective we are at doing FEC or RTX. But it would indeed be nice to have
> > Call::OnRecoveredRtpPacket to parse the packet header.
> 
> I think it would be nicer to do the special stats processing based on some
flag
> in RtpPacketReceived. Perhaps we can just set arrival_time == -1 wne calling
> OnRtpPacket, to indicate that the recovered packet didn't arrive over the
> network in the usual way?

I think that makes sense. To be consistent with RtpPacketReceived::retransmit(),
maybe better to add a flag RtpPacketReceived::recovered() instead of overloading
the arrival_time? At the same time, maybe rename RtpPacketReceived::retransmit()
-> RtpPacketReceived::retransmitted().

I'm not intimately familiar with how recovered/retransmitted packets are treated
inside the RtpStreamReceiver, so it might be worth looking into that to see that
there is nothing more than the stats that are different between normal and
retransmitted/recovered packets.

[nisse-webrtc, 2017-02-10 09:39:53]

On 2017/02/10 09:24:14, brandtr_google wrote:
> I think that makes sense. To be consistent with
RtpPacketReceived::retransmit(),
> maybe better to add a flag RtpPacketReceived::recovered() instead of
overloading
> the arrival_time? At the same time, maybe rename
RtpPacketReceived::retransmit()
> -> RtpPacketReceived::retransmitted().

Sounds like a good plan for a followup cl. I'll try to address your other
comments, and see if we can get this into a landable state.

[nisse-webrtc, 2017-02-10 10:26:55]

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

https://codereview.webrtc.org/2681673004/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:338: ss << "Packet received on SSRC: " <<
header.ssrc << " with payload type: "
On 2017/02/09 14:22:17, brandtr wrote:
> Might as well remove the usage of |header| here, to make it easier to remove
it
> completely in the future.
> 
> (RtpPacket::GetHeader is destined to be removed when it is no longer needed, I
> believe.)
> 
> header.ssrc <=> packet.Ssrc(),
> header.extension.hasTransmissionTimeOffset <=>
> packet.HasExtension<TransmissionOffset>, etc.

Done, except that (i) the ReceivePacket and IncomingPacket calls below wants
|header|, and (ii) |packet| is const, so we can't call
packet.set_payload_type_frequency.

When we get to use rtp_config for both audio and video, we could move the
frequency there too.

https://codereview.webrtc.org/2681673004/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:356: bool ret = ReceivePacket(packet.data(),
packet.size(), header, in_order);
On 2017/02/09 14:22:17, brandtr wrote:
> I believe this is correct.
> 
> If it were incorrect, it feels that lots of tests would fail?

We'll see what happens to the tests.

[brandtr, 2017-02-10 11:43:00]

lgtm with a suggestion

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:343:
packet.GetExtension<TransmissionOffset>(&time_offset);
I believe you could simplify this:

int32_t time_offset;
if (packet.GetExtension<TransmissionOffset>(&time_offset)) {
  ss << ", toffset: " << time_offset;
}

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:347: uint32_t send_time;
same here.

[nisse-webrtc, 2017-02-10 11:56:28]

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:343:
packet.GetExtension<TransmissionOffset>(&time_offset);
On 2017/02/10 11:43:00, brandtr wrote:
> I believe you could simplify this:
> 
> int32_t time_offset;
> if (packet.GetExtension<TransmissionOffset>(&time_offset)) {
>   ss << ", toffset: " << time_offset;
> }

Done.

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:347: uint32_t send_time;
On 2017/02/10 11:43:00, brandtr wrote:
> same here.

Done.

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.h (right):

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.h:102: bool OnRtpPacket(const
RtpPacketReceived& packet);
I wonder, do we really need a return value? When can it return false, and is
anyone caring about the return value? E.g., WebRtcVideoChannel2 ignores
DELIVERY_PACKET_ERROR anyway, see

https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

[nisse-webrtc, 2017-02-10 12:47:52]

Stefan, can you have a look?

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.h (right):

https://codereview.webrtc.org/2681673004/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.h:102: bool OnRtpPacket(const
RtpPacketReceived& packet);
On 2017/02/10 11:56:27, nisse-webrtc wrote:
> I wonder, do we really need a return value? When can it return false, and is
> anyone caring about the return value? E.g., WebRtcVideoChannel2 ignores
> DELIVERY_PACKET_ERROR anyway, see
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

I went ahead and deleted the return type. Both video engine and voice engine
call DeliverPacket and only check for the DELIVERY_UNKNOWN_SSRC return status.
Then make no distinction between DELIVERY_OK and DELIVERY_PACKET_ERROR.

[brandtr, 2017-02-10 12:50:55]

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc#oldco...
webrtc/call/call.cc:1227: if (status == DELIVERY_OK)
Maybe check with terelius@ that this change is OK.

[nisse-webrtc, 2017-02-10 13:20:26]

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

[nisse-webrtc, 2017-02-10 13:20:27]

+terelius

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc#oldco...
webrtc/call/call.cc:1227: if (status == DELIVERY_OK)
On 2017/02/10 12:50:55, brandtr wrote:
> Maybe check with terelius@ that this change is OK.

Discussed off-line.

[stefan-webrtc, 2017-02-10 13:22:34]

Nice!

lgtm % nit

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:1216: if (parsed_packet) {
Isn't this guaranteed to be true given 1190?

[stefan-webrtc, 2017-02-10 13:22:36]

Nice!

lgtm % nit

[nisse-webrtc, 2017-02-10 13:27:40]

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2681673004/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:1216: if (parsed_packet) {
On 2017/02/10 13:22:34, stefan-webrtc wrote:
> Isn't this guaranteed to be true given 1190?

Good catch. Deleted this test and the one a few lines down.

[nisse-webrtc, 2017-02-10 13:29:27]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-10 13:29:27]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2681673004/#ps80001 (title:
"Delete redundant conditionals.")

[commit-bot: I haz the power, 2017-02-10 13:29:36]

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-02-10 13:49:29]

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

[commit-bot: I haz the power, 2017-02-10 13:49:30]

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

[nisse-webrtc, 2017-02-10 13:58:00]

Deleting the return value makes the video_engine test
PacketInjectionTest.StapAPacketWithTruncatedNalUnits fails, for obvious reasons.
See 
https://cs.chromium.org/chromium/src/third_party/webrtc/call/packet_injection...

Can we have a unit test for rejection of broken NAL units at some other layer
than Call?

pbos, what's the intention with this test? Nothing in the production code cares
about DELIVERY_PACKET_ERROR, as far as I've found.

[stefan-webrtc, 2017-02-10 14:07:37]

On 2017/02/10 13:58:00, nisse-webrtc wrote:
> Deleting the return value makes the video_engine test
> PacketInjectionTest.StapAPacketWithTruncatedNalUnits fails, for obvious
reasons.
> See 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/packet_injection...
> 
> Can we have a unit test for rejection of broken NAL units at some other layer
> than Call?
> 
> pbos, what's the intention with this test? Nothing in the production code
cares
> about DELIVERY_PACKET_ERROR, as far as I've found.

I would just move it in to a lower layer somewhere. Maybe the class which does
the rejection of broken nal units?

[pbos-webrtc, 2017-02-10 21:19:39]

On 2017/02/10 14:07:37, stefan-webrtc wrote:
> On 2017/02/10 13:58:00, nisse-webrtc wrote:
> > Deleting the return value makes the video_engine test
> > PacketInjectionTest.StapAPacketWithTruncatedNalUnits fails, for obvious
> reasons.
> > See 
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/packet_injection...
> > 
> > Can we have a unit test for rejection of broken NAL units at some other
layer
> > than Call?
> > 
> > pbos, what's the intention with this test? Nothing in the production code
> cares
> > about DELIVERY_PACKET_ERROR, as far as I've found.
> 
> I would just move it in to a lower layer somewhere. Maybe the class which does
> the rejection of broken nal units?

DELIVERY_PACKET_ERROR was just a signal to drop packets that are random garbage
(too small to contain ssrc for instance)

[nisse-webrtc, 2017-02-13 12:44:57]

Moved tests to lower layer. Attempting to land this now.

https://codereview.webrtc.org/2681673004/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/2681673004/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:871: const uint8_t
kPayload[] = { 0x58, 0xCB, 0xED, 0xDF};
This is the same payload as in the deleted testcase, but without 12 bytes of
headers.

[nisse-webrtc, 2017-02-13 12:45:05]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-13 12:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2681673004/#ps100001 (title:
"Move test involving incorrect h264 data.")

[commit-bot: I haz the power, 2017-02-13 12:45:14]

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-02-13 13:59:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486989905103950,
"parent_rev": "07a050f9952b30088d9ef527a01da7722424c01f", "commit_rev":
"38cc1d6b31eff566651bc1d09b8a4e9093bcb90d"}

[commit-bot: I haz the power, 2017-02-13 13:59:49]

Description was changed from

==========
Replace RtpStreamReceiver::DeliverRtp with OnRtpPacket.

This avoids redoing RTP header parsing already done in Call, for video.

The next step is to convert other types of receive streams, i.e.,
audio and flexfec, to use a compatible OnRtpPacket method. We can then
introduce a shared base interface, and simplify media-independent
receive processing in Call.

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

to

==========
Replace RtpStreamReceiver::DeliverRtp with OnRtpPacket.

This avoids redoing RTP header parsing already done in Call, for video.

The next step is to convert other types of receive streams, i.e.,
audio and flexfec, to use a compatible OnRtpPacket method. We can then
introduce a shared base interface, and simplify media-independent
receive processing in Call.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2681673004
Cr-Commit-Position: refs/heads/master@{#16583}
Committed:
https://chromium.googlesource.com/external/webrtc/+/38cc1d6b31eff566651bc1d09...
==========

[commit-bot: I haz the power, 2017-02-13 13:59:51]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/38cc1d6b31eff566651bc1d09...