Introduce RtpStreamReceiver and RtpStreamReceiverControllerInterface.

Introduce RtpStreamReceiverInterface and RtpStreamReceiverControllerInterface.

And implementation class RtpStreamReceiverController.
It's responsible for demuxing, and acts as factory for
RtpStreamReceiverInterface.

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

[nisse-webrtc, 2017-05-17 14:30:46]

nisse@webrtc.org changed reviewers:
+ danilchap@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2017-05-17 14:30:46]

This is work-in-progress, superseding https://codereview.webrtc.org/2709723003/

The new class owns the demuxer, and is passed to the receive stream
constructors, which now get to register themselves as sinks to the controller.

Receive stream unittests not yet updated, I'm not sure how hard or easy that's
going to be.

[pthatcher1, 2017-05-17 23:24:33]

On 2017/05/17 14:30:46, nisse-webrtc wrote:
> This is work-in-progress, superseding
https://codereview.webrtc.org/2709723003/
> 
> The new class owns the demuxer, and is passed to the receive stream
> constructors, which now get to register themselves as sinks to the controller.
> 
> Receive stream unittests not yet updated, I'm not sure how hard or easy that's
> going to be.

[pthatcher1, 2017-05-17 23:24:53]

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

https://codereview.webrtc.org/2886993005/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:114:
transport->CreateReceiver(config_.rtp.remote_ssrc, this);
So CreateReceiver really means "add sink with a context that removes itself upon
destruction"?

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

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/call.cc#newcode1217
webrtc/call/call.cc:1217: video_transport_receive_.OnRtpPacket(*parsed_packet);
What is the advantage of working with the wrapper around the demuxer rather than
just working with the demuxer?

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.cc:24: demuxer_->RemoveSink(sink_);
So a Receiver is a thing that adds and removes itself from the demuxer?

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.h:35: // separate
RtpSessionReceiver classes.
Shouldn't TODOs be on their own line?

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:19: class
RtpTransportReceiver {
What is this?  No comment.  No methods.

And I'm guessing is should have the "Interface" suffix.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:25: class
RtpTransportControllerReceiveInterface {
I still don't like this name.  RtpReceiverControllerInterface might be a better
one.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:28: virtual
std::unique_ptr<RtpTransportReceiver> CreateReceiver(
This could use a method also.  You put in a sink and you get back a receiver. 
But what *is* a receiver?

[nisse-webrtc, 2017-05-18 08:45:43]

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

https://codereview.webrtc.org/2886993005/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:114:
transport->CreateReceiver(config_.rtp.remote_ssrc, this);
On 2017/05/17 23:24:53, pthatcher1 wrote:
> So CreateReceiver really means "add sink with a context that removes itself
upon
> destruction"?  

Yes. The plan is that it should get additional responsibilities later, like
ownership of rtp state and stats methods.

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

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/call.cc#newcode1217
webrtc/call/call.cc:1217: video_transport_receive_.OnRtpPacket(*parsed_packet);
On 2017/05/17 23:24:53, pthatcher1 wrote:
> What is the advantage of working with the wrapper around the demuxer rather
than
> just working with the demuxer?

At the moment, little. Responsibility for parsing should be moved to this
wrapper, but before that we need to think about responsibility for the
extensions mapping.

It would also make sense to let the wrapper object do the needed locking (while
the demuxer object itself should b a simple class unaware of multithreading
issues). Do you agree?

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.cc:24: demuxer_->RemoveSink(sink_);
On 2017/05/17 23:24:53, pthatcher1 wrote:
> So a Receiver is a thing that adds and removes itself from the demuxer?

Correct, it doesn't yet have any other responsibilities. (And it's according to
the design doc).

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.h:35: // separate
RtpSessionReceiver classes.
On 2017/05/17 23:24:53, pthatcher1 wrote:
> Shouldn't TODOs be on their own line?

Will fix in next ps.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:25: class
RtpTransportControllerReceiveInterface {
On 2017/05/17 23:24:53, pthatcher1 wrote:
> I still don't like this name.  RtpReceiverControllerInterface might be a
better
> one.

I'm open to renaming (as long as RtpTransportControllerSender(Interface)? should
be consistently renamed). Shouldn't "Transport" be part of the name. Or is
"Session" better, "RtpSessionReceiver"? 

Then Danil suggested that we should have one class responsible for the
per-packet processing, (parsing, demuxing, interfacing to the congestion
controller) and a separate "controller" class responsible for ownership,
configuration, and book-keeping above the packet level. And "Session" might be
more appropriate for the former class.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:28: virtual
std::unique_ptr<RtpTransportReceiver> CreateReceiver(
On 2017/05/17 23:24:53, pthatcher1 wrote:
> This could use a method also.  You put in a sink and you get back a receiver. 
> But what *is* a receiver?

On an abstract level, a receiver is a handle to the Rtp-specific machinery which
feeds the RtpPacketSink.

A agree the class definition (above) deserves a comment.

[danilchap, 2017-05-19 15:37:43]

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

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/call.cc#newcode1217
webrtc/call/call.cc:1217: video_transport_receive_.OnRtpPacket(*parsed_packet);
using ReceiverController feels preliminary: it should be able handle recovering
packets itself.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.cc:30: return
rtc::MakeUnique<Receiver>(&demuxer_, ssrc, sink);
returning pointer to non-thread-safe &demux is an error-prone pattern.
Do not think there is an easy safe solution.
Making demuxer thread-safe (with read-write lock or with a task queue) might be
better option.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.h:60: };
what is preventing putting ReceiveSideCC into this class?

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:31: };
OnRtpPacket or similar likely should be part of the interface too.
or RtpTransportController should implement two interfaces: one is this one and
another one is webrtc::Transport [or similar]

May be compose some tests to show how RtpTransportControllerReceiveInterface
supposed to be used.

if you keep CreateReceiver non-const, but make
OnRtpPacket const, then external read-write lock may be natural to control
synchronization of this interface.

[nisse-webrtc, 2017-05-22 07:09:37]

Comments only, no code updates yet.

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

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/call.cc#newcode1217
webrtc/call/call.cc:1217: video_transport_receive_.OnRtpPacket(*parsed_packet);
On 2017/05/19 15:37:43, danilchap wrote:
> using ReceiverController feels preliminary: it should be able handle
recovering
> packets itself.

Agreed. I think we should aim to move all per-packet processing out of Call, and
connect RtpTransportController directly to the packet transport. But there's
many steps to get there.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.cc:30: return
rtc::MakeUnique<Receiver>(&demuxer_, ssrc, sink);
On 2017/05/19 15:37:43, danilchap wrote:
> returning pointer to non-thread-safe &demux is an error-prone pattern.
> Do not think there is an easy safe solution.
> Making demuxer thread-safe (with read-write lock or with a task queue) might
be
> better option.

Note that the demuxer pointer is only passed to the Receiver object, which is
intimately related to the controller. But I agree it's not ideal; if the
receiver is destroyed on the wrong thread, we get unsafe use of the demuxer.
Still I think I'd like to have the demuxer itself simple and not thread aware.
What do you think of the following:

Make the RtpTransportControllerReceive thread-safe, and replace the &demuxer_
pointer above by this?

As for synchronization primitive, I don't think a read-write lock is the right
tool. It makes sense to replace a plain mutex by a read-write lock only if we
get lock contention from multiple threads who all need only read access. I don't
quite understand the current threading model for Call, but I guess all demuxing
happens on the same thread, likely the network thread?

And migrating to task queue, do you think that is easy to do at this point?

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.h:60: };
On 2017/05/19 15:37:43, danilchap wrote:
> what is preventing putting ReceiveSideCC into this class?

Two things at the moment:

1. The logic prior to calling receive_side_cc_.OnReceivedPacket. I moved that in
the larger cl
(https://codereview.webrtc.org/2709723003/diff/510001/webrtc/call/rtp_transpor...),
involving the flags use_send_side_bwe and enable_receive_side_bwe, which
reviewers found confusing. To simplify things, I tried to delete the latter of
the flags in https://codereview.webrtc.org/2840833002/, but it seems that's not
possible at the moment.

2. The registration of the receive_side_cc_ and
receive_side_cc_.GetRemoteBitrateEstimator(true) as modules on two distinct
ProcessThreads, currently done by Call. Could be worked around by exposing the
receive_side_cc of course.

So I agree receive_side_cc_ belong in this class, but I think move from Call is
tricky and best left for later cls.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:31: };
On 2017/05/19 15:37:43, danilchap wrote:
> OnRtpPacket or similar likely should be part of the interface too.
> or RtpTransportController should implement two interfaces: one is this one and
> another one is webrtc::Transport [or similar]

If we add that interface, I think it should take a raw, unparsed, packet. Do you
agree? And then we'd need to move around responsibility for IdentifyExtensions
before doing that.

> May be compose some tests to show how RtpTransportControllerReceiveInterface
> supposed to be used.

Test would have to target the implementation class, right?

[nisse-webrtc, 2017-05-22 12:41:13]

New patchset, with a lock in RtpTransportControllerReceive.

Also #if:ed out the unittests which aren't yet updated.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.cc:30: return
rtc::MakeUnique<Receiver>(&demuxer_, ssrc, sink);
On 2017/05/22 07:09:37, nisse-webrtc wrote:

> Make the RtpTransportControllerReceive thread-safe, and replace the &demuxer_
> pointer above by this?

Done now.

[nisse-webrtc, 2017-05-23 11:28:58]

Description was changed from

==========
Introduce RtpTransportControllerReceiveInterface.

And implementation class RtpTransportControllerReceive.
It's responsible for demuxing, and acts as factory for
RtpTransportReceiver.

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

to

==========
Introduce RtpTransportControllerReceiveInterface.

And implementation class RtpTransportControllerReceive.
It's responsible for demuxing, and acts as factory for
RtpTransportReceiver.

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

[danilchap, 2017-05-23 12:53:10]

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_receive.h (right):

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive.h:26: // This class represents the
RTP receive parsing and demuxing, for a
What benefits this class has over RtpDemuxer? only thread-safety?
can you add tests to demonstrate how it supposed to be used?

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:19: class
RtpTransportReceiver {
Add documentation for this class.
e.g. TODOs what it will be responsible for in the future.

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:33: virtual void
AddSink(uint32_t ssrc, RtpPacketSinkInterface* sink) = 0;
shouldn't flexfec call CreateReceiver for each media stream it protect?

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:34: virtual size_t
RemoveSink(const RtpPacketSinkInterface* sink) = 0;
Should this function be in this interface?
May be RtpTransportReceiver should remove sinks through different interface.

[nisse-webrtc, 2017-05-23 14:33:09]

Just a few quick comments, I'm about to leave for the day.

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:33: virtual void
AddSink(uint32_t ssrc, RtpPacketSinkInterface* sink) = 0;
On 2017/05/23 12:53:10, danilchap wrote:
> shouldn't flexfec call CreateReceiver for each media stream it protect?

I thought so too at first. But that will probably not work when the receiver
gets additional responsibilities, since
we don't want flexfec to instantiate any additional rtp receiver state for the
media streams it is protecting.

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:34: virtual size_t
RemoveSink(const RtpPacketSinkInterface* sink) = 0;
On 2017/05/23 12:53:10, danilchap wrote:
> Should this function be in this interface?

Only for symmetry with AddSink, which seems needed for flexfec.

[danilchap, 2017-05-23 16:02:02]

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:33: virtual void
AddSink(uint32_t ssrc, RtpPacketSinkInterface* sink) = 0;
On 2017/05/23 14:33:09, nisse-webrtc wrote:
> On 2017/05/23 12:53:10, danilchap wrote:
> > shouldn't flexfec call CreateReceiver for each media stream it protect?
> 
> I thought so too at first. But that will probably not work when the receiver
> gets additional responsibilities, since
> we don't want flexfec to instantiate any additional rtp receiver state for the
> media streams it is protecting.

another way: CreateReceiver might take not one ssrc, but array of them.
It need to take more complicated stream id anyway (that might be mid+rsid
instead of ssrc)

[pthatcher1, 2017-05-24 03:56:40]

pthatcher@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[pthatcher1, 2017-05-24 03:57:33]

On 2017/05/23 16:02:02, danilchap wrote:
>
https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
> File webrtc/call/rtp_transport_controller_receive_interface.h (right):
> 
>
https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
> webrtc/call/rtp_transport_controller_receive_interface.h:33: virtual void
> AddSink(uint32_t ssrc, RtpPacketSinkInterface* sink) = 0;
> On 2017/05/23 14:33:09, nisse-webrtc wrote:
> > On 2017/05/23 12:53:10, danilchap wrote:
> > > shouldn't flexfec call CreateReceiver for each media stream it protect?
> > 
> > I thought so too at first. But that will probably not work when the receiver
> > gets additional responsibilities, since
> > we don't want flexfec to instantiate any additional rtp receiver state for
the
> > media streams it is protecting.
> 
> another way: CreateReceiver might take not one ssrc, but array of them.
> It need to take more complicated stream id anyway (that might be mid+rsid
> instead of ssrc)

RSID is not used for demux, but PTs are.  

Eventually, you will need MID+SSRCs+PTs.

[pthatcher1, 2017-05-24 03:57:38]

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive.cc:24: demuxer_->RemoveSink(sink_);
On 2017/05/18 08:45:43, nisse-webrtc wrote:
> On 2017/05/17 23:24:53, pthatcher1 wrote:
> > So a Receiver is a thing that adds and removes itself from the demuxer?
> 
> Correct, it doesn't yet have any other responsibilities. (And it's according
to
> the design doc).

Can you explain this in a comment in the code?

And can you send me a link to the doc?  That might help me understand all these
things and how they interact.

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller_receive_interface.h:25: class
RtpTransportControllerReceiveInterface {
On 2017/05/18 08:45:43, nisse-webrtc wrote:
> On 2017/05/17 23:24:53, pthatcher1 wrote:
> > I still don't like this name.  RtpReceiverControllerInterface might be a
> better
> > one.
> 
> I'm open to renaming (as long as RtpTransportControllerSender(Interface)?
should
> be consistently renamed). Shouldn't "Transport" be part of the name. Or is
> "Session" better, "RtpSessionReceiver"? 
> 
> Then Danil suggested that we should have one class responsible for the
> per-packet processing, (parsing, demuxing, interfacing to the congestion
> controller) and a separate "controller" class responsible for ownership,
> configuration, and book-keeping above the packet level. And "Session" might be
> more appropriate for the former class.

A controller is a thing that creates, owns, and destroys other things, like the
exiting TransportController, which create, owns, and destroys transports.  

So what does this controller create?  It seems to create RtpReceiver (thus, a
CreateReceiver method).  So then why not call it an RtpReceiverController?

Demuxing is complex enough that it ought to go in its own RtpDemuxer class.  

It wouldn't be a good idea to mix demuxing and creating receivers because then a
receiver cannot move between demuxers (which happens when we go from unbundled
to bundled).

I don't think we should use "RtpSession" except to refer to that very specific
thing: an RTP session (an SSRC space).  But that's basically what an RtpDemuxer
is, and the word Demuxer is much more clear.  Session is already very
overloaded.

[nisse-webrtc, 2017-05-29 11:18:35]

On 2017/05/24 03:57:38, pthatcher1 wrote:
> A controller is a thing that creates, owns, and destroys other things, like
the
> exiting TransportController, which create, owns, and destroys transports.  

Hmm. And what's the difference to a "factory" then? A "controller" retains
ownership to the things it creates, while a "factory" doesn't?

> So what does this controller create?  It seems to create RtpReceiver (thus, a
> CreateReceiver method).  So then why not call it an RtpReceiverController?

To be precise, it creates RtpTransportReciever (under the theory that the name
"RtpReceiver" should be reserved for the ORTC object), so it should be
RtpTransportReceiverController, implementing
RtpTransportReceiverControllerInterface? It's getting a bit unwieldy, but it
might make sense.

> It wouldn't be a good idea to mix demuxing and creating receivers because then
a
> receiver cannot move between demuxers (which happens when we go from unbundled
> to bundled).

This is a transition I don't quite understand. An RtpTransportReceiver in this
context is the rtp-specific but media-independent state needed for a receive
stream, which includes the demuxer's ssrc-to-sink association. Moving that to a
different demuxer makes little sense. Moving a *sink* between demuxers could
work, but in this model, that would imply creation of a new RtpTransportReceiver
for a different transport.

[nisse-webrtc, 2017-05-29 13:35:50]

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/60001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_receive_interface.h:19: class
RtpTransportReceiver {
On 2017/05/23 12:53:09, danilchap wrote:
> Add documentation for this class.
> e.g. TODOs what it will be responsible for in the future.

Done.

[nisse-webrtc, 2017-05-30 11:59:44]

I and danil had a little brainstorming session on naming, considering a long
list of alternative names for RtpTransportReceiver. One short, explicit and
unique name is SsrcReceiver, this stresses that it is responsible for a single
ssrc (e.g., RTX would have its own).

And then we would also have SsrcReceiverController and
SsrcReceiverControllerInterface. Now, the implementation class would have
additional responsibilities like parsing RTP packets and coordinating with the
receive-side congestion controller, but the interface would be more narrow.

What do you think?

[pthatcher, 2017-06-02 22:44:16]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2017-06-02 22:44:17]

https://codereview.webrtc.org/2886993005/diff/120001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller_receive_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/120001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive_interface.h:20: //
media-independent state needed for receiving a stream.
"stream" is ambiguous in the RTP world.  Needs to be "RTP stream" or "Encoded
Stream" or something else.

[nisse-webrtc, 2017-06-08 12:43:03]

On 2017/05/30 11:59:44, nisse-webrtc wrote:
> I and danil had a little brainstorming session on naming, considering a long
> list of alternative names for RtpTransportReceiver. One short, explicit and
> unique name is SsrcReceiver, this stresses that it is responsible for a single
> ssrc (e.g., RTX would have its own).
> 
> And then we would also have SsrcReceiverController and
> SsrcReceiverControllerInterface. Now, the implementation class would have
> additional responsibilities like parsing RTP packets and coordinating with the
> receive-side congestion controller, but the interface would be more narrow.
> 
> What do you think?

Peter suggested RtpStreamReceiver (under the model that rtx will have its own
instance). 
Only problem is that the name is already taken, so I have to rename the existing

RtpStreamReceiver class first (it's video specific, created by
VideoReceiveStream).

[nisse-webrtc, 2017-06-08 13:16:49]

On 2017/06/08 12:43:03, nisse-webrtc wrote:
> On 2017/05/30 11:59:44, nisse-webrtc wrote:
> > I and danil had a little brainstorming session on naming, considering a long
> > list of alternative names for RtpTransportReceiver. One short, explicit and
> > unique name is SsrcReceiver, this stresses that it is responsible for a
single
> > ssrc (e.g., RTX would have its own).
> > 
> > And then we would also have SsrcReceiverController and
> > SsrcReceiverControllerInterface. Now, the implementation class would have
> > additional responsibilities like parsing RTP packets and coordinating with
the
> > receive-side congestion controller, but the interface would be more narrow.
> > 
> > What do you think?
> 
> Peter suggested RtpStreamReceiver (under the model that rtx will have its own
> instance). 
> Only problem is that the name is already taken, so I have to rename the
existing
> 
> RtpStreamReceiver class first (it's video specific, created by
> VideoReceiveStream).

Renaming cl: https://codereview.webrtc.org/2926253002/

[nisse-webrtc, 2017-06-09 12:25:08]

Description was changed from

==========
Introduce RtpTransportControllerReceiveInterface.

And implementation class RtpTransportControllerReceive.
It's responsible for demuxing, and acts as factory for
RtpTransportReceiver.

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

to

==========
Introduce RtpStreamReceiver and RtpStreamReceiverControllerInterface.

And implementation class RtpStreamReceiverController.
It's responsible for demuxing, and acts as factory for
RtpStreamReceiver.

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

[nisse-webrtc, 2017-06-09 12:30:47]

On 2017/06/08 13:16:49, nisse-webrtc wrote:
> On 2017/06/08 12:43:03, nisse-webrtc wrote:
> > On 2017/05/30 11:59:44, nisse-webrtc wrote:
> > > I and danil had a little brainstorming session on naming, considering a
long
> > > list of alternative names for RtpTransportReceiver. One short, explicit
and
> > > unique name is SsrcReceiver, this stresses that it is responsible for a
> single
> > > ssrc (e.g., RTX would have its own).
> > > 
> > > And then we would also have SsrcReceiverController and
> > > SsrcReceiverControllerInterface. Now, the implementation class would have
> > > additional responsibilities like parsing RTP packets and coordinating with
> the
> > > receive-side congestion controller, but the interface would be more
narrow.
> > > 
> > > What do you think?
> > 
> > Peter suggested RtpStreamReceiver (under the model that rtx will have its
own
> > instance). 
> > Only problem is that the name is already taken, so I have to rename the
> existing
> > 
> > RtpStreamReceiver class first (it's video specific, created by
> > VideoReceiveStream).
> 
> Renaming cl: https://codereview.webrtc.org/2926253002/

Rename done. Also updated cl description.

[nisse-webrtc, 2017-06-09 13:43:49]

Peter, can you have another look, and tell me what you think is needed to make
progress?

[danilchap, 2017-06-09 13:49:32]

looks good to me, might be time to fix tests.

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:829: delete receive_stream_impl;
by making removal from demuxer automatic, you moved it from 1st action to last
action in destruction of the receive stream.
Can that be a problem? What whould happen if packet arrives when some
deinitialization above already run (e.g. ssrc removed from bitrate estimator).
Do we have tests that check this scenario with tsan?

[pthatcher, 2017-06-10 03:12:51]

Overall, looks good to me.  Obviously, there are all those "if 0"s to fix.

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:24: // rather, the
corresponding implementation class). We should add have
"should add have"?

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:27: class
RtpStreamReceiver {
Should it be RtpStreamReceiverInterface?

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:32: // This class acts as
a factory for RtpTransportReceive objects.
Did you mean RtpStreamReceiver objects?

[nisse-webrtc, 2017-06-13 10:56:07]

New patchset is only typo fixes.

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:829: delete receive_stream_impl;
On 2017/06/09 13:49:32, danilchap wrote:
> by making removal from demuxer automatic, you moved it from 1st action to last
> action in destruction of the receive stream.
> Can that be a problem? What whould happen if packet arrives when some
> deinitialization above already run (e.g. ssrc removed from bitrate estimator).
> Do we have tests that check this scenario with tsan?

I don't think we have any testcase.

Hmm. We have a couple of options:

1. One could make a copy of the config struct; we could then move the delete
call a bit earlier. 

2. Or we could move both the delete and the bitrate estimator RemoveStream call
inside the WriteLockScope, which is safer in one way since those calls are about
modification of the receive-side state, but I'm not sure if there are any risk
of lock inversion if we do that. 

3. Or treat registration in receive_rtp_config_ as essential, and let
Call::ParseRtpPacket fail if the src is unknown. Then the above
receive_rtp_config_.erase should be enough to cut the demuxer and its sinks off
from the receive path.

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:24: // rather, the
corresponding implementation class). We should add have
On 2017/06/10 03:12:50, pthatcher wrote:
> "should add have"?

Deleting "have".

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:27: class
RtpStreamReceiver {
On 2017/06/10 03:12:50, pthatcher wrote:
> Should it be RtpStreamReceiverInterface?

Maybe. It is an abstract interface class. But I don't plan to add any
implementation class in the the public namespace (it's the private class
RtpStreamReceiverController::Receiver), so we could omit the "Interface" suffix
for conciseness without much confusion. 

I don't have a strong opinion here. Do you prefer "Interface"?

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:32: // This class acts as
a factory for RtpTransportReceive objects.
On 2017/06/10 03:12:50, pthatcher wrote:
> Did you mean RtpStreamReceiver objects?

Done.

[nisse-webrtc, 2017-06-13 14:04:38]

On 2017/06/13 10:56:07, nisse-webrtc wrote:
> 3. Or treat registration in receive_rtp_config_ as essential, and let
> Call::ParseRtpPacket fail if the src is unknown. Then the above
> receive_rtp_config_.erase should be enough to cut the demuxer and its sinks
off
> from the receive path.

I tried this option. Not sure what a proper test for this case (packets received
for a stream which is being torned down) should look like.

Also updated the receive stream tests, no #if 0's left.

[danilchap, 2017-06-13 15:58:32]

lgtm

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream_impl.cc:148: // TODO(nisse): Register
receiver_, and delete below OnRtpPacket method?
I'm not sure what you are suggesting to do in this comment.

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.h (right):

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:57: RtpStreamReceiverController*
controller_;
may be * const controller_; * const sink_
to stress they never change after creation.

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/video/video_recei...
webrtc/video/video_receive_stream_unittest.cc:29: using testing::_;
move these 2 usings inside unnamed namespace too (since you are expanding it
anyway)

[pthatcher1, 2017-06-13 21:49:16]

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:27: class
RtpStreamReceiver {
On 2017/06/13 10:56:07, nisse-webrtc wrote:
> On 2017/06/10 03:12:50, pthatcher wrote:
> > Should it be RtpStreamReceiverInterface?
> 
> Maybe. It is an abstract interface class. But I don't plan to add any
> implementation class in the the public namespace (it's the private class
> RtpStreamReceiverController::Receiver), so we could omit the "Interface"
suffix
> for conciseness without much confusion. 
> 
> I don't have a strong opinion here. Do you prefer "Interface"?

We use "Inteface" is private classes, too, not just public API ones.  I prefer
being consistent and using "Interface" for all interfaces like this.  At least
that's what we strive to do in cricket::.

[nisse-webrtc, 2017-06-14 06:31:06]

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/180001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:27: class
RtpStreamReceiver {
On 2017/06/13 21:49:16, pthatcher1 wrote:
> On 2017/06/13 10:56:07, nisse-webrtc wrote:
> > On 2017/06/10 03:12:50, pthatcher wrote:
> > > Should it be RtpStreamReceiverInterface?
> > 
> > Maybe. It is an abstract interface class. But I don't plan to add any
> > implementation class in the the public namespace (it's the private class
> > RtpStreamReceiverController::Receiver), so we could omit the "Interface"
> suffix
> > for conciseness without much confusion. 
> > 
> > I don't have a strong opinion here. Do you prefer "Interface"?
> 
> We use "Inteface" is private classes, too, not just public API ones.  I prefer
> being consistent and using "Interface" for all interfaces like this.  At least
> that's what we strive to do in cricket::.

I changed the class name to RtpStreamReceiverInterface. I didn't tack on
Interface and _interface in all related comments and member names, though, since
IMO that gets a bit too clumsy. Ok?

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream_impl.cc:148: // TODO(nisse): Register
receiver_, and delete below OnRtpPacket method?
On 2017/06/13 15:58:32, danilchap wrote:
> I'm not sure what you are suggesting to do in this comment.

The idea is that maybe this class doesn't need to implement
RtpPacketSinkInterface at all, since it delegates all real work to |receiver_|,
class FlexfecReceiver. The only thing OnRtpPacket of this class does is taking a
lock, and checking the |started_| flag and the |receiver_| pointer. I'll try to
clarify the comment.

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.h (right):

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:57: RtpStreamReceiverController*
controller_;
On 2017/06/13 15:58:32, danilchap wrote:
> may be * const controller_; * const sink_
> to stress they never change after creation.

Done.

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2886993005/diff/280001/webrtc/video/video_recei...
webrtc/video/video_receive_stream_unittest.cc:29: using testing::_;
On 2017/06/13 15:58:32, danilchap wrote:
> move these 2 usings inside unnamed namespace too (since you are expanding it
> anyway)

Done.

[nisse-webrtc, 2017-06-14 06:31:23]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 06:31:30]

Dry run: 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-06-14 06:47:25]

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

[commit-bot: I haz the power, 2017-06-14 06:47:26]

Dry run: Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/8796)

[nisse-webrtc, 2017-06-14 08:16:27]

https://codereview.webrtc.org/2886993005/diff/300001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2886993005/diff/300001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:499: return rtc::Optional<RtpPacketReceived>();
It seems this change breaks ortc tests, e.g.,
OrtcFactoryIntegrationTest.BasicOneWayAudioRtpSenderAndReceiver
and probably breaks handling of unsignalled ssrcs in general.

So what's the correct way to handle rtp packets which arrive while (or just
after) a receive stream is being torned down? Just return DELIVERY_UNKNOWN_SSRC,
which might trigger creation of a new receive stream?

I'll give that a try (and I think that was the earlier behavior), to see if that
makes the tests pass.

[nisse-webrtc, 2017-06-14 08:16:28]

https://codereview.webrtc.org/2886993005/diff/300001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2886993005/diff/300001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:499: return rtc::Optional<RtpPacketReceived>();
It seems this change breaks ortc tests, e.g.,
OrtcFactoryIntegrationTest.BasicOneWayAudioRtpSenderAndReceiver
and probably breaks handling of unsignalled ssrcs in general.

So what's the correct way to handle rtp packets which arrive while (or just
after) a receive stream is being torned down? Just return DELIVERY_UNKNOWN_SSRC,
which might trigger creation of a new receive stream?

I'll give that a try (and I think that was the earlier behavior), to see if that
makes the tests pass.

[pthatcher, 2017-06-16 01:56:33]

lgtm

[nisse-webrtc, 2017-06-16 07:05:26]

Description was changed from

==========
Introduce RtpStreamReceiver and RtpStreamReceiverControllerInterface.

And implementation class RtpStreamReceiverController.
It's responsible for demuxing, and acts as factory for
RtpStreamReceiver.

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

to

==========
Introduce RtpStreamReceiverInterface and RtpStreamReceiverControllerInterface.

And implementation class RtpStreamReceiverController.
It's responsible for demuxing, and acts as factory for
RtpStreamReceiverInterface.

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

[nisse-webrtc, 2017-06-16 07:09:19]

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

[nisse-webrtc, 2017-06-16 07:09:21]

Adding OWNERs, solenberg for audio/, stefan for video/ and call/.

[holmer, 2017-06-16 07:41:41]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-06-16 07:41:43]

lgtm for video and call

[the sun, 2017-06-16 13:37:40]

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:113: rtp_stream_receiver_ =
Exposing the controller to the stream so that it can register itself with it
feels somewhat questionable architecturally. Why does the stream need to own the
receiver?

Also, giving the receiver a pointer to "this" before the object is fully
constructed, is brittle even if it will work in this situation.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.cc:38: bool
RtpStreamReceiverController::OnRtpPacket(const RtpPacketReceived& packet) {
Please add thread checkers so we can lock down usage as much as possible with
the longer term goal of removing ad-hoc lock usage.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.h (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:13: #include <memory>
Not needed (unique_ptr should be included in
rtp_stream_receiver_controller_interface.h already)

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:15: #include
"webrtc/base/array_view.h"
Not needed

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:17: 
dd

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:23: class
ReceiveSideCongestionController;
not used

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:48: class Receiver : public
RtpStreamReceiverInterface {
Is there any reason to not put this in an anonymous namespace in the .cc?

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:25: // methods for
getting rtp receive stats, and for sending RTCP
I thought RTCP messages were usually sent for a collection of streams?

[nisse-webrtc, 2017-06-16 14:33:26]

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:113: rtp_stream_receiver_ =
On 2017/06/16 13:37:39, the sun wrote:
> Exposing the controller to the stream so that it can register itself with it
> feels somewhat questionable architecturally.

The point is that the controller should own media-independent state, while the
receive stream should hook that up to any processing which depends on *both* rtp
and the media type.

The corresponding method on the video stream creates two receivers, one for
media and one for rtx, and the idea is that neither the controller or call
should need to know that's how we do video.

> Why does the stream need to own the
> receiver?

Who else should own the receiver(s)? In any case, we need to deregister from the
demuxer, and letting the receiver's destructor be responsible for that seems
reasonable to me (it was originally danil's suggestion, iirc).

> Also, giving the receiver a pointer to "this" before the object is fully
> constructed, is brittle even if it will work in this situation.

It looks like we could pass channel_proxy_ instead, and delete the OnRtpPacket
method from this class. (But I think this way to use this is reasonable, even if
the subtleties could perhaps be better documented).

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.cc:38: bool
RtpStreamReceiverController::OnRtpPacket(const RtpPacketReceived& packet) {
On 2017/06/16 13:37:39, the sun wrote:
> Please add thread checkers so we can lock down usage as much as possible with
> the longer term goal of removing ad-hoc lock usage.

I'll try to sort it out. But I kind-of expect that OnRtpPacket and
AddSink/RemoveSink used on different threads.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.h (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:13: #include <memory>
On 2017/06/16 13:37:39, the sun wrote:
> Not needed (unique_ptr should be included in
> rtp_stream_receiver_controller_interface.h already)

I agree, but presubmit doesn't... If I delete that line, I get

.../rtp_stream_receiver_controller.cc:31:  Add #include <memory> for
unique_ptr<>  [build/include_what_you_use] [4]
.../rtp_stream_receiver_controller.h:32:  Add #include <memory> for unique_ptr<>
 [build/include_what_you_use] [4]

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:15: #include
"webrtc/base/array_view.h"
On 2017/06/16 13:37:39, the sun wrote:
> Not needed

Done.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:17: 
On 2017/06/16 13:37:39, the sun wrote:
> dd

I'm not following you.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:23: class
ReceiveSideCongestionController;
On 2017/06/16 13:37:39, the sun wrote:
> not used

Done.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:48: class Receiver : public
RtpStreamReceiverInterface {
On 2017/06/16 13:37:39, the sun wrote:
> Is there any reason to not put this in an anonymous namespace in the .cc?

I think it's tied to this class in such a way that it makes sense for it to be
declared here. And we get the option of making the RemoveSink method private
(only use is in the Receiver destructor), and we might want to add more private
methods later.

Is it possibly to forward declare it as an inner class here (for namespace
reasons), but move the rest of the declaration to the implementation file? 

I don't have a strong opinion either way, and I must admit that I don't really
know what a private class implies for C++ access rules, so I might have
misunderstood. So I'll do whatever you prefer.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller_interface.h (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller_interface.h:25: // methods for
getting rtp receive stats, and for sending RTCP
On 2017/06/16 13:37:39, the sun wrote:
> I thought RTCP messages were usually sent for a collection of streams?

The intended interface would like "I want a key frame for this stream", and then
the Controller should be responsible for how that gets packed into an RTCP
packet, possibly together with information from other send and receive streams.

Maybe there's some confusion between what I call an "RTCP message" (one piece of
info with precise meaning, communicted using the RTCP protocol) and an RTCP
packet (data corresponding to a single RTCP packet on the wire). Can you suggest
better wording?

[the sun, 2017-06-16 15:00:13]

lgtm

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:113: rtp_stream_receiver_ =
On 2017/06/16 14:33:25, nisse-webrtc wrote:
> On 2017/06/16 13:37:39, the sun wrote:
> > Exposing the controller to the stream so that it can register itself with it
> > feels somewhat questionable architecturally.
> 
> The point is that the controller should own media-independent state, while the
> receive stream should hook that up to any processing which depends on *both*
rtp
> and the media type.
> 
> The corresponding method on the video stream creates two receivers, one for
> media and one for rtx, and the idea is that neither the controller or call
> should need to know that's how we do video.

That sgtm.

> > Why does the stream need to own the
> > receiver?
> 
> Who else should own the receiver(s)? In any case, we need to deregister from
the
> demuxer, and letting the receiver's destructor be responsible for that seems
> reasonable to me (it was originally danil's suggestion, iirc).

I guess the Receiver could have shared ownership of the stream and be created
outside of the stream class. That would achieve the objective of decoupling the
transport layer from stream objects too. But very well...

> > Also, giving the receiver a pointer to "this" before the object is fully
> > constructed, is brittle even if it will work in this situation.
> 
> It looks like we could pass channel_proxy_ instead, and delete the OnRtpPacket
> method from this class. (But I think this way to use this is reasonable, even
if
> the subtleties could perhaps be better documented).

No, please don't pass ChannelProxy. That's an interim solution.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.cc:38: bool
RtpStreamReceiverController::OnRtpPacket(const RtpPacketReceived& packet) {
On 2017/06/16 14:33:25, nisse-webrtc wrote:
> On 2017/06/16 13:37:39, the sun wrote:
> > Please add thread checkers so we can lock down usage as much as possible
with
> > the longer term goal of removing ad-hoc lock usage.
> 
> I'll try to sort it out. But I kind-of expect that OnRtpPacket and
> AddSink/RemoveSink used on different threads.

Yes, we just want to document that.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.h (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:17: 
On 2017/06/16 14:33:26, nisse-webrtc wrote:
> On 2017/06/16 13:37:39, the sun wrote:
> > dd
> 
> I'm not following you.

Sorry, vim-speak for delete a line. It's the only thing from my 5 weeks with vim
that stuck. :)

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:48: class Receiver : public
RtpStreamReceiverInterface {
On 2017/06/16 14:33:26, nisse-webrtc wrote:
> On 2017/06/16 13:37:39, the sun wrote:
> > Is there any reason to not put this in an anonymous namespace in the .cc?
> 
> I think it's tied to this class in such a way that it makes sense for it to be
> declared here. And we get the option of making the RemoveSink method private
> (only use is in the Receiver destructor), and we might want to add more
private
> methods later.

Ah, didn't see that. Good call.

> 
> Is it possibly to forward declare it as an inner class here (for namespace
> reasons), but move the rest of the declaration to the implementation file? 
> 
> I don't have a strong opinion either way, and I must admit that I don't really
> know what a private class implies for C++ access rules, so I might have
> misunderstood. So I'll do whatever you prefer.

[nisse-webrtc, 2017-06-19 07:21:01]

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:113: rtp_stream_receiver_ =
On 2017/06/16 15:00:13, the sun wrote:

> No, please don't pass ChannelProxy. That's an interim solution.

I did that change in ps#18. Please have a look, and tell me if you still want me
to undo that part. Note that this change is only about the internal division of
work between AudioReceiveStrean and ChannelProxy, and not in any other
interfaces. Whenever ChannelProxy is replaced by something else, the
CreateReceiver call needs to be updated with whatever is the replacement. 

ChannelProxy already has an OnRtpPacket method, to pass it to CreateReceiver all
that's needed is to tell the compiler it actually implements the
RtpPacketSinkInterface.

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.h (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.h:17: 
On 2017/06/16 15:00:13, the sun wrote:
> On 2017/06/16 14:33:26, nisse-webrtc wrote:
> > On 2017/06/16 13:37:39, the sun wrote:
> > > dd
> > 
> > I'm not following you.
> 
> Sorry, vim-speak for delete a line. It's the only thing from my 5 weeks with
vim
> that stuck. :)

Done. I would have understood C-k (short for M-x kill-line RET) better ;-)

[the sun, 2017-06-20 07:11:36]

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:113: rtp_stream_receiver_ =
On 2017/06/19 07:21:01, nisse-webrtc wrote:
> On 2017/06/16 15:00:13, the sun wrote:
> 
> > No, please don't pass ChannelProxy. That's an interim solution.
> 
> I did that change in ps#18. Please have a look, and tell me if you still want
me
> to undo that part. Note that this change is only about the internal division
of
> work between AudioReceiveStrean and ChannelProxy, and not in any other
> interfaces. Whenever ChannelProxy is replaced by something else, the
> CreateReceiver call needs to be updated with whatever is the replacement. 
> 
> ChannelProxy already has an OnRtpPacket method, to pass it to CreateReceiver
all
> that's needed is to tell the compiler it actually implements the
> RtpPacketSinkInterface.

The problem is that it muddies the image of what ChannelProxy is - it is not
longer just a proxy for voe::Channel, mirroring what Channel provides. Now it is
an actor all of its own.

Please revert to the previous state.

[nisse-webrtc, 2017-06-20 09:03:13]

On 2017/06/20 07:11:36, the sun wrote:
>
https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
> File webrtc/audio/audio_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/2886993005/diff/320001/webrtc/audio/audio_recei...
> webrtc/audio/audio_receive_stream.cc:113: rtp_stream_receiver_ =
> On 2017/06/19 07:21:01, nisse-webrtc wrote:
> > On 2017/06/16 15:00:13, the sun wrote:
> > 
> > > No, please don't pass ChannelProxy. That's an interim solution.
> > 
> > I did that change in ps#18. Please have a look, and tell me if you still
want
> me
> > to undo that part. Note that this change is only about the internal division
> of
> > work between AudioReceiveStrean and ChannelProxy, and not in any other
> > interfaces. Whenever ChannelProxy is replaced by something else, the
> > CreateReceiver call needs to be updated with whatever is the replacement. 
> > 
> > ChannelProxy already has an OnRtpPacket method, to pass it to CreateReceiver
> all
> > that's needed is to tell the compiler it actually implements the
> > RtpPacketSinkInterface.
> 
> The problem is that it muddies the image of what ChannelProxy is - it is not
> longer just a proxy for voe::Channel, mirroring what Channel provides. Now it
is
> an actor all of its own.
> 
> Please revert to the previous state.

Decided to not revert, after offline discussion.

[nisse-webrtc, 2017-06-20 09:04:35]

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
File webrtc/call/rtp_stream_receiver_controller.cc (right):

https://codereview.webrtc.org/2886993005/diff/320001/webrtc/call/rtp_stream_r...
webrtc/call/rtp_stream_receiver_controller.cc:38: bool
RtpStreamReceiverController::OnRtpPacket(const RtpPacketReceived& packet) {
On 2017/06/16 15:00:13, the sun wrote:
> On 2017/06/16 14:33:25, nisse-webrtc wrote:
> > On 2017/06/16 13:37:39, the sun wrote:
> > > Please add thread checkers so we can lock down usage as much as possible
> with
> > > the longer term goal of removing ad-hoc lock usage.
> > 
> > I'll try to sort it out. But I kind-of expect that OnRtpPacket and
> > AddSink/RemoveSink used on different threads.
> 
> Yes, we just want to document that.

Added thread checkers, and it seems to pass tests.

[nisse-webrtc, 2017-06-20 10:43:45]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 10:43:49]

Dry run: 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-06-20 10:45:38]

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

[commit-bot: I haz the power, 2017-06-20 10:45:40]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21324)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26644)

[nisse-webrtc, 2017-06-20 13:29:31]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 13:29:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-06-20 13:35:35]

Final(?) fix. I'd like Fredrik or Stefan to have a look at change in ps#24, I'll
land if you think it's safe.

https://codereview.webrtc.org/2886993005/diff/460001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2886993005/diff/460001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream_impl.cc:159:
receiver_controller->CreateReceiver(config_.remote_ssrc, this);
This is another case of passing |this| before the object is ready. For the time
being solved by protecting the construction by Call's |receive_crit_| lock.

I think the proper solution is to move statistics up a level or two, to
RtpReceiveStreamController/RtpTransportController, and then delete the
OnRtpPacket method of this class.

[commit-bot: I haz the power, 2017-06-20 13:57:51]

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

[commit-bot: I haz the power, 2017-06-20 13:57:52]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-06-21 07:42:56]

lgtm

[nisse-webrtc, 2017-06-21 08:02:11]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-06-21 08:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, holmer@google.com, pthatcher@google.com,
solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2886993005/#ps460001 (title:
"Protect construction of FlexfecReceiveStreamImpl.")

[commit-bot: I haz the power, 2017-06-21 08:02:17]

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-06-21 08:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1498032131653840,
"parent_rev": "130ca7e783d123ed04758056b5fceb3dbbf5c1fa", "commit_rev":
"0f15f926e3468f9d0d8f09da6f3760cbd22da219"}

[commit-bot: I haz the power, 2017-06-21 08:05:29]

Description was changed from

==========
Introduce RtpStreamReceiverInterface and RtpStreamReceiverControllerInterface.

And implementation class RtpStreamReceiverController.
It's responsible for demuxing, and acts as factory for
RtpStreamReceiverInterface.

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

to

==========
Introduce RtpStreamReceiverInterface and RtpStreamReceiverControllerInterface.

And implementation class RtpStreamReceiverController.
It's responsible for demuxing, and acts as factory for
RtpStreamReceiverInterface.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2886993005
Cr-Commit-Position: refs/heads/master@{#18696}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0f15f926e3468f9d0d8f09da6...
==========

[commit-bot: I haz the power, 2017-06-21 08:05:31]

Committed patchset #24 (id:460001) as
https://chromium.googlesource.com/external/webrtc/+/0f15f926e3468f9d0d8f09da6...