RtpPacketReceiver base class and OnRtpPacket, with a pre-parsed RTP packet.

RtpPacketReceiver base class and OnRtpPacket, with a pre-parsed RTP packet.

This is a request for comments. I think it's best to do separate cls
to convert different types of receive streams to the new OnRtpPacket
style, with pre-parsed RTP header.

And after that's landed, introduce a base class for all receive
streams, and refactor Call.

I think it should be reasonable straight forward to arrange call to
have a *single* multimap of ssrc for all types of receive streams.
(Except that I'm not sure how that fits with the

  flexfec_receive_ssrcs_media_.equal_range(ssrc)

used in the lookup of flexfec streams). The receive stream
base class should have a rtp_config member matching the current
ReceiveRtpConfig, and a media_type/transport_id to do correct demuxing
in case that audio and video use different transports and colliding
ssrcs.

BUG=None

[nisse-webrtc, 2017-02-09 08:49:10]

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

[nisse-webrtc, 2017-02-09 08:49:10]

This is the direction in which I'd like to move the RTP receive code. What do
you think?

Fredrik: Is VoENetworkImpl::ReceivedRTPPacket one of the methods you're going to
delete? It seems that's a non-Call entry point into the audio receive code.

[stefan-webrtc, 2017-02-09 13:40:06]

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:315: return
channel_proxy_->OnRTPPacket(packet);
OnRtpPacket

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/video/rtp_stream_recei...
webrtc/video/rtp_stream_receiver.cc:281: // TODO(nisse): Is .data() and .size()
right? Strip headers or not?
Yes, this looks correct, as the full packet is needed for FEC recovery etc in
ParseAndHandleEncapsulatingHeader. If not for that we could strip the header
here already.

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.h:88: const RtpConfig& rtp_config() const
override;
Seems like VideoReceiveStream could have a method to get a pointer to an
RtpPacketreceiver instead?

It however also makes me wonder how we're going to do the generic case? It
doesn't seem to scale well if we're going to have to inherit also from
webrtc::QuartcPacketReceiver for instance. Or are we somehow going to convert
this to a PacketReceiver interface?

https://codereview.webrtc.org/2688473004/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2688473004/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:214: int32_t OnRTPPacket(const RtpPacketReceived&
packet);
OnRtpPacket

[brandtr, 2017-02-09 14:51:55]

Initial comments.

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/call.cc#newcode1115
webrtc/call/call.cc:1115: pass_to_flexfec = true;
maybe_pass_to_flexfec

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/call.cc#newcode1120
webrtc/call/call.cc:1120: // TODO(nisse): Update
received_bytes_per_second_counter_ ?
Yes! This is an omission, the counters should be updated for FlexFEC as well.

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.h:54: const RtpConfig rtp_config_;
The information in |rtp_config_| is completely based on the information in
|config_|, so it feels a bit weird to save both. But on the other hand, you
probably don't want to create a new RtpConfig for every packet being parsed in
Call.

If it makes it simpler, I'm fine with changing the structure of
FlexfecReceiveStream::Config to be more similar with
{Audio,Video}ReceiveStream::Config. Then functions accessing the fields could be
templatized.

Another idea would be to move the RTP stuff in
{Audio,Video,Flexfec}ReceiveStream::Config to the RtpConfig, and then
WebRtcVideoChannel2 would populate that struct. The different stream objects
have slightly different fields in the RTP substructs, however.

[nisse-webrtc, 2017-02-09 16:20:48]

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.h:54: const RtpConfig rtp_config_;
On 2017/02/09 14:51:55, brandtr wrote:
> The information in |rtp_config_| is completely based on the information in
> |config_|, so it feels a bit weird to save both.

I agree. Main show stopper at the moment is the different representation of the
rtp extensions. 

Maybe we can unify this later. But on the other hand, purpose is different, one
struct is public and intended for applications to configure things, the other is
intended to be precisely what's needed in the rtp receive logic, and we probably
don't want to expose that. Could put a layer of accessor function on top, of
course, and let applications use those.

> But on the other hand, you
> probably don't want to create a new RtpConfig for every packet being parsed in
> Call.

No, if we unify, we should make sure the rtp_config things is a substruct of
*StreamConfig, so one can just return a pointer to it.

> Another idea would be to move the RTP stuff in
> {Audio,Video,Flexfec}ReceiveStream::Config to the RtpConfig, and then
> WebRtcVideoChannel2 would populate that struct. The different stream objects
> have slightly different fields in the RTP substructs, however.

Worth exploring, but I think it's a little premature.

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.h:88: const RtpConfig& rtp_config() const
override;
On 2017/02/09 13:40:06, stefan-webrtc wrote:
> Seems like VideoReceiveStream could have a method to get a pointer to an
> RtpPacketreceiver instead?
> 
> It however also makes me wonder how we're going to do the generic case? It
> doesn't seem to scale well if we're going to have to inherit also from
> webrtc::QuartcPacketReceiver for instance. Or are we somehow going to convert
> this to a PacketReceiver interface?

My thinking is that some class needs to be responsible for details which are
specific to both video and rtp. We should try to minimize that class, and push
video specific details in one direction (shared with quartc), and rtp specific
details in the other direction (shared with audio).

And the corresponding class doing video over quartc, which would implement a
QuartcStreamReceiver or some such.

[Taylor Brandstetter, 2017-02-09 20:20:11]

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:310: bool
AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
Personally, I prefer the "DeliverRtpPacket" style of naming. The
"OnReceivedPacket" style of naming is mostly used for private slots (for
example,
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.h?type=cs&...).

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/rtp_packet_receiv...
File webrtc/call/rtp_packet_receiver.h (right):

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/rtp_packet_receiv...
webrtc/call/rtp_packet_receiver.h:21: class RtpPacketReceiver {
How will this relate to RtpTransportReceiver?

[nisse-webrtc, 2017-02-10 08:09:24]

See also https://codereview.webrtc.org/2681673004/, extracted from this cl.

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:310: bool
AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) {
On 2017/02/09 20:20:11, Taylor Brandstetter wrote:
> Personally, I prefer the "DeliverRtpPacket" style of naming. The
> "OnReceivedPacket" style of naming is mostly used for private slots (for
> example,
>
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.h?type=cs&...).

I picked up the On* style from the VideoSinkInterface::OnFrame which we added a
while ago, and I think about it as an analogous RTP packet sink. I'd also
consider dropping the return value, making it a fire-and-forget method, but I'm
not sure if there's some code that really handles errors (besides logging and
stats). One exception is the UNKNWO_SSRC error from Call::DeliverRtp, but I hope
to reorganize the handling of unsignalled ssrcs so that case disappears.

https://codereview.webrtc.org/2688473004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:315: return
channel_proxy_->OnRTPPacket(packet);
On 2017/02/09 13:40:06, stefan-webrtc wrote:
> OnRtpPacket

Done.

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

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/call.cc#newcode1120
webrtc/call/call.cc:1120: // TODO(nisse): Update
received_bytes_per_second_counter_ ?
On 2017/02/09 14:51:55, brandtr wrote:
> Yes! This is an omission, the counters should be updated for FlexFEC as well.

That's what you're fixing in
https://codereview.webrtc.org/2684243002/, right?

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/rtp_packet_receiv...
File webrtc/call/rtp_packet_receiver.h (right):

https://codereview.webrtc.org/2688473004/diff/1/webrtc/call/rtp_packet_receiv...
webrtc/call/rtp_packet_receiver.h:21: class RtpPacketReceiver {
On 2017/02/09 20:20:11, Taylor Brandstetter wrote:
> How will this relate to RtpTransportReceiver?

RtpTransportReceiver would keep any media-independent per-ssrc state needed in
the receive processing. While RtpPacketReceiver should be implemented by
media-dependent receive streams, and one object per stream rather than by ssrc.

If/when we introduce RtpTransportReceiver, we can move the things returned by
rtp_config below to that object, and it won't need much more than the list of
extensions and pointer to an RtpPacketReceiver. But I'm starting to think that
maybe we don't need any RtpTransportReceiver, it should be enough to let
RtpTransportController have a map from ssrc to stream/RtpPacketReceiver. 

Depends on details such as whether or not we want different treatment of the
multiple ssrcs which are used by the same stream. If it needs to be separated,
RtpTransportReceiver is the way to add one more level of indirection.

[the sun, 2017-02-10 12:37:26]

I need to know a bit more about where this is going.

E.g. why are we going to create a common base class for receive streams?

IIUC there is one SSRC space per media type, meaning we can't use a single map
for all incoming streams. There could be overlap.

[the sun, 2017-02-10 12:37:45]

nits

https://codereview.webrtc.org/2688473004/diff/20001/webrtc/call/rtp_packet_re...
File webrtc/call/rtp_packet_receiver.h (right):

https://codereview.webrtc.org/2688473004/diff/20001/webrtc/call/rtp_packet_re...
webrtc/call/rtp_packet_receiver.h:25: RtpConfig(const std::vector<RtpExtension>&
extensions,
nit: move whole impl of this ctor to .cc, then you can put UseSendSideBwe() in
an anonymous namespace in the .cc and avoid cluttering the interface.

https://codereview.webrtc.org/2688473004/diff/20001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/2688473004/diff/20001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:18: #include
"webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
Unlike the Google C++ style guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

...but since this is a mock, I'd just rely on the declaration of the class we're
mocking to pull in all required symbols.

[nisse-webrtc, 2017-05-31 13:07:09]

Closing this, it's superseded by the RtpDemuxer changes introduced recently.