Replace AudioReceiveStream::DeliverRtp with OnRtpPacket.

Replace AudioReceiveStream::DeliverRtp with OnRtpPacket.

This avoids redoing RTP header parsing already done in Call.

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

[nisse-webrtc, 2017-02-14 13:33:44]

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

[nisse-webrtc, 2017-02-14 13:33:45]

PTAL. I have two problems, see comments below.

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream_unittest.cc:259:
ReceivedRTPPacket(parsed_packet))
I don't understand what to do about this. I'd like the expect logic to an
address comparison, hopefully there's some magic in gmock to let me do that?

https://codereview.webrtc.org/2697833002/diff/1/webrtc/voice_engine/voe_netwo...
File webrtc/voice_engine/voe_network_impl.cc (right):

https://codereview.webrtc.org/2697833002/diff/1/webrtc/voice_engine/voe_netwo...
webrtc/voice_engine/voe_network_impl.cc:90: return false;
Is this code to be deleted?

[nisse-webrtc, 2017-02-14 13:53:51]

On 2017/02/14 13:33:45, nisse-webrtc wrote:
> PTAL. I have two problems, see comments below.
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> File webrtc/audio/audio_receive_stream_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> webrtc/audio/audio_receive_stream_unittest.cc:259:
> ReceivedRTPPacket(parsed_packet))
> I don't understand what to do about this. I'd like the expect logic to an
> address comparison, hopefully there's some magic in gmock to let me do that?
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/voice_engine/voe_netwo...
> File webrtc/voice_engine/voe_network_impl.cc (right):
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/voice_engine/voe_netwo...
> webrtc/voice_engine/voe_network_impl.cc:90: return false;
> Is this code to be deleted?

And bot failures look related to those known problems.

[the sun, 2017-02-16 13:47:23]

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:305: void
AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
RtpPacketReceived is an odd typename for me. It sounds more like a state.
ReceivedRtpPacket would have been more natural...

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:310: channel_proxy_->OnRtpPacket(packet);
Thanks for removing the return value!

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream_unittest.cc:256: // RtpPacketReceived doesn't
have any equality operator.
You could add one locally if you want:

bool operator==(const ReceivedRtpPacket& lhs, const ReceivedRtpPacket& rhs) {
...

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream_unittest.cc:259:
ReceivedRTPPacket(parsed_packet))
On 2017/02/14 13:33:45, nisse-webrtc wrote:
> I don't understand what to do about this. I'd like the expect logic to an
> address comparison, hopefully there's some magic in gmock to let me do that?

ReceivedRtpPacket is copyable, so adding op== should work. You may also be able
to use:
EXPECT_CALL(*helper.channel_proxy(), Ref(parsed_packet)).WillOnce(Return(true));

https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...

[nisse-webrtc, 2017-02-16 14:35:05]

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:305: void
AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
On 2017/02/16 13:47:22, the sun wrote:
> RtpPacketReceived is an odd typename for me. It sounds more like a state.
> ReceivedRtpPacket would have been more natural...

Agreed. But this is not the time to change that.

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream_unittest.cc:259:
ReceivedRTPPacket(parsed_packet))
On 2017/02/16 13:47:22, the sun wrote:
> On 2017/02/14 13:33:45, nisse-webrtc wrote:
> > I don't understand what to do about this. I'd like the expect logic to an
> > address comparison, hopefully there's some magic in gmock to let me do that?
> 
> ReceivedRtpPacket is copyable, so adding op== should work. You may also be
able
> to use:
> EXPECT_CALL(*helper.channel_proxy(),
Ref(parsed_packet)).WillOnce(Return(true));
> 
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...

Adding testing::Ref solved the problem. (And then I also needed to change the
name and drop return value).

[the sun, 2017-02-16 15:54:00]

On 2017/02/16 14:35:05, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> File webrtc/audio/audio_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> webrtc/audio/audio_receive_stream.cc:305: void
> AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
> On 2017/02/16 13:47:22, the sun wrote:
> > RtpPacketReceived is an odd typename for me. It sounds more like a state.
> > ReceivedRtpPacket would have been more natural...
> 
> Agreed. But this is not the time to change that.
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> File webrtc/audio/audio_receive_stream_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> webrtc/audio/audio_receive_stream_unittest.cc:259:
> ReceivedRTPPacket(parsed_packet))
> On 2017/02/16 13:47:22, the sun wrote:
> > On 2017/02/14 13:33:45, nisse-webrtc wrote:
> > > I don't understand what to do about this. I'd like the expect logic to an
> > > address comparison, hopefully there's some magic in gmock to let me do
that?
> > 
> > ReceivedRtpPacket is copyable, so adding op== should work. You may also be
> able
> > to use:
> > EXPECT_CALL(*helper.channel_proxy(),
> Ref(parsed_packet)).WillOnce(Return(true));
> > 
> >
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
> 
> Adding testing::Ref solved the problem. (And then I also needed to change the
> name and drop return value).

Let me know when this is ready to be reviewed in full.

[nisse-webrtc, 2017-02-17 09:03:17]

On 2017/02/16 15:54:00, the sun wrote:
> On 2017/02/16 14:35:05, nisse-webrtc wrote:
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > File webrtc/audio/audio_receive_stream.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > webrtc/audio/audio_receive_stream.cc:305: void
> > AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
> > On 2017/02/16 13:47:22, the sun wrote:
> > > RtpPacketReceived is an odd typename for me. It sounds more like a state.
> > > ReceivedRtpPacket would have been more natural...
> > 
> > Agreed. But this is not the time to change that.
> > 
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > File webrtc/audio/audio_receive_stream_unittest.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > webrtc/audio/audio_receive_stream_unittest.cc:259:
> > ReceivedRTPPacket(parsed_packet))
> > On 2017/02/16 13:47:22, the sun wrote:
> > > On 2017/02/14 13:33:45, nisse-webrtc wrote:
> > > > I don't understand what to do about this. I'd like the expect logic to
an
> > > > address comparison, hopefully there's some magic in gmock to let me do
> that?
> > > 
> > > ReceivedRtpPacket is copyable, so adding op== should work. You may also be
> > able
> > > to use:
> > > EXPECT_CALL(*helper.channel_proxy(),
> > Ref(parsed_packet)).WillOnce(Return(true));
> > > 
> > >
> >
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
> > 
> > Adding testing::Ref solved the problem. (And then I also needed to change
the
> > name and drop return value).
> 
> Let me know when this is ready to be reviewed in full.

I need advice on what to do with the call in voe_network_impl.cc. Besides that,
I think this cl is ready for landing, corresponding changes for video and
flexfex are already done.

That said, this isn't going to be the final iteration of OnRtpPacket, I'm
thinking that we will need to distinguish between simple sinks which get a const
ref to a parsed packet, and sources which are responsible for parsing extensions
and which will get a non-const pointer to a parsed packet. The current approach
of always parsing headers in Call doesn't extend easily to the handing of
packets entering via ulpfec or rtx. But I'd like to get audio and video into a
consistent state before the next steps of refactoring.

[the sun, 2017-02-17 11:51:53]

On 2017/02/17 09:03:17, nisse-webrtc wrote:
> On 2017/02/16 15:54:00, the sun wrote:
> > On 2017/02/16 14:35:05, nisse-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > > File webrtc/audio/audio_receive_stream.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > > webrtc/audio/audio_receive_stream.cc:305: void
> > > AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
> > > On 2017/02/16 13:47:22, the sun wrote:
> > > > RtpPacketReceived is an odd typename for me. It sounds more like a
state.
> > > > ReceivedRtpPacket would have been more natural...
> > > 
> > > Agreed. But this is not the time to change that.
> > > 
> > >
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > > File webrtc/audio/audio_receive_stream_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2697833002/diff/1/webrtc/audio/audio_receive_st...
> > > webrtc/audio/audio_receive_stream_unittest.cc:259:
> > > ReceivedRTPPacket(parsed_packet))
> > > On 2017/02/16 13:47:22, the sun wrote:
> > > > On 2017/02/14 13:33:45, nisse-webrtc wrote:
> > > > > I don't understand what to do about this. I'd like the expect logic to
> an
> > > > > address comparison, hopefully there's some magic in gmock to let me do
> > that?
> > > > 
> > > > ReceivedRtpPacket is copyable, so adding op== should work. You may also
be
> > > able
> > > > to use:
> > > > EXPECT_CALL(*helper.channel_proxy(),
> > > Ref(parsed_packet)).WillOnce(Return(true));
> > > > 
> > > >
> > >
> >
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
> > > 
> > > Adding testing::Ref solved the problem. (And then I also needed to change
> the
> > > name and drop return value).
> > 
> > Let me know when this is ready to be reviewed in full.
> 
> I need advice on what to do with the call in voe_network_impl.cc. Besides
that,
> I think this cl is ready for landing, corresponding changes for video and
> flexfex are already done.
> 
> That said, this isn't going to be the final iteration of OnRtpPacket, I'm
> thinking that we will need to distinguish between simple sinks which get a
const
> ref to a parsed packet, and sources which are responsible for parsing
extensions
> and which will get a non-const pointer to a parsed packet. The current
approach
> of always parsing headers in Call doesn't extend easily to the handing of
> packets entering via ulpfec or rtx. But I'd like to get audio and video into a
> consistent state before the next steps of refactoring.

What part of the packet needs to be const? If this is just about lazy parsing,
can you make the actual packet data non-modifiable and attach the parsed info as
decorators?

[the sun, 2017-02-17 11:52:06]

https://codereview.webrtc.org/2697833002/diff/20001/webrtc/voice_engine/voe_n...
File webrtc/voice_engine/voe_network_impl.cc (right):

https://codereview.webrtc.org/2697833002/diff/20001/webrtc/voice_engine/voe_n...
webrtc/voice_engine/voe_network_impl.cc:89: // OnRtpPacket.
Yes it is going away. Currently we have a few tests using it though and possibly
parts of those tests should be replicated in other ways.

It'll be 2-3 weeks before it can be removed if we convert the tests first. We
could take the gun-slinger route and #if 0 out the tests and deprecate APIs, but
then incentive to add the tests is smaller so we risk reducing total test
coverage, at least for some time...

[nisse-webrtc, 2017-02-17 12:01:40]

On 2017/02/17 11:51:53, the sun wrote:
> What part of the packet needs to be const? If this is just about lazy parsing,
> can you make the actual packet data non-modifiable and attach the parsed info
as
> decorators?

It's about splitting the parsing into general rtp headers parsing,

  RtpPacketReceived::Parse

and the stream/media-dependent parsing

  RtpPacketReceived::IdentifyExtensions

I plan to use the existing RtpPacketReceived. Packet bytes are immutable,
as far as I understand.

[nisse-webrtc, 2017-02-17 12:17:48]

https://codereview.webrtc.org/2697833002/diff/20001/webrtc/voice_engine/voe_n...
File webrtc/voice_engine/voe_network_impl.cc (right):

https://codereview.webrtc.org/2697833002/diff/20001/webrtc/voice_engine/voe_n...
webrtc/voice_engine/voe_network_impl.cc:89: // OnRtpPacket.
On 2017/02/17 11:52:06, the sun wrote:
> Yes it is going away. Currently we have a few tests using it though and
possibly
> parts of those tests should be replicated in other ways.
> 
> It'll be 2-3 weeks before it can be removed if we convert the tests first. We
> could take the gun-slinger route and #if 0 out the tests and deprecate APIs,
but
> then incentive to add the tests is smaller so we risk reducing total test
> coverage, at least for some time...

So what are the options, if I'd like to land this change early next week? (Week
after next I'm off skiing, so if we can't do it pretty soon it can just as well
wait two weeks).

[nisse-webrtc, 2017-02-17 14:06:24]

New version, keeping the old method Channel::ReceivedRTPPacket, and letting both
that and OnRtpPacket call a shared helper method. 

Not particularly pretty, but I think it should work.

[the sun, 2017-02-17 15:21:27]

On 2017/02/17 14:06:24, nisse-webrtc wrote:
> New version, keeping the old method Channel::ReceivedRTPPacket, and letting
both
> that and OnRtpPacket call a shared helper method. 
> 
> Not particularly pretty, but I think it should work.

lgtm

[nisse-webrtc, 2017-02-17 15:26:22]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-02-17 15:26:34]

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-17 15:30:41]

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

[commit-bot: I haz the power, 2017-02-17 15:30:43]

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

[nisse-webrtc, 2017-02-17 15:53:44]

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

[nisse-webrtc, 2017-02-17 15:53:45]

OWNER's approval missing for webrtc/test.

[kjellander_webrtc, 2017-02-19 08:53:02]

On 2017/02/17 15:53:45, nisse-webrtc wrote:
> OWNER's approval missing for webrtc/test.

webrtc/test: lgtm

[nisse-webrtc, 2017-02-21 12:50:46]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-02-21 12:50:59]

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-21 14:28:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487681446748600,
"parent_rev": "b94491d7904feb9b072b1adaa9454c9615fba5f8", "commit_rev":
"657bab24558648a9851e8b237df6ac245bd02f66"}

[commit-bot: I haz the power, 2017-02-21 14:28:16]

Description was changed from

==========
Replace AudioReceiveStream::DeliverRtp with OnRtpPacket.

This avoids redoing RTP header parsing already done in Call.

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

to

==========
Replace AudioReceiveStream::DeliverRtp with OnRtpPacket.

This avoids redoing RTP header parsing already done in Call.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2697833002
Cr-Commit-Position: refs/heads/master@{#16750}
Committed:
https://chromium.googlesource.com/external/webrtc/+/657bab24558648a9851e8b237...
==========

[commit-bot: I haz the power, 2017-02-21 14:28:18]

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