Protected streams report RTP messages directly to the FlexFec streams

Protected streams report RTP messages directly to the FlexFec streams

In preparation of making RTP packet demuxing many-to-one (one SSRC goes to one sink, but one sink may have multiple SSRCs), we need to remove FlexFEC's dependence on being able to register itself with the demuxer. Instead, we register FlexFEC streams with the streams they protect; when those streams get packets, they'll forward them to their associated FlexFEC streams, too.

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

[eladalon, 2017-07-06 15:51:56]

Description was changed from

==========
Protected streams report RTP messages directly to the FlexFec streams

In preparation of making RTP packet demuxing many-to-one (one SSRC goes to one
sink, but one sink may have multiple SSRCs), we need to remove FlexFEC's
dependence on being able to register itself with the demuxer. Instead, we
register FlexFEC streams with the streams they protect; when those streams get
packets, they'll forward them to their associated FlexFEC streams, too.

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

to

==========
Protected streams report RTP messages directly to the FlexFec streams

In preparation of making RTP packet demuxing many-to-one (one SSRC goes to one
sink, but one sink may have multiple SSRCs), we need to remove FlexFEC's
dependence on being able to register itself with the demuxer. Instead, we
register FlexFEC streams with the streams they protect; when those streams get
packets, they'll forward them to their associated FlexFEC streams, too.

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

[eladalon, 2017-07-06 15:51:57]

eladalon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org

[eladalon, 2017-07-06 15:52:38]

Ready for review.
Please not - TODOs with "!!!" are meant to be taken care of during this CL,
after discussion with reviewers. TODOs without it are for future CLs to handle.

[stefan-webrtc, 2017-07-07 09:36:24]

I think the code duplication that comes from this is a bit ugly, which makes me
lean towards having a single implementation of the secondary rtp sink container
which the streams can inherit from.

https://codereview.webrtc.org/2974453002/diff/1/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/DEPS#newcode12
webrtc/DEPS:12: "+webrtc/call/secondary_rtp_sinks_container.h",  #
TODO(eladalon): Reconsider.
Reconsider the name?

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

https://codereview.webrtc.org/2974453002/diff/1/webrtc/call/call.cc#newcode826
webrtc/call/call.cc:826: // TODO(eladalon): Return a unique_ptr instead?
Maybe it makes sense to keep this out of this CL until a discussion has been had
with some other people that care (currently on vacation), as we discussed
offline?

https://codereview.webrtc.org/2974453002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:64: // such as
|is_recovered|, which would be |true| for FlexFEC-recovered packets.
Seems a bit ugly to me since all rtp packet sinks would have to have this flag,
but only flex fec would care about it?

https://codereview.webrtc.org/2974453002/diff/1/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/test/call_test.cc#newc...
webrtc/test/call_test.cc:343: // TODO(eladalon): This is not the order in the
header; fix in separate CL.
I don't think you need to add this TODO, just create a separate CL and be done
with it. :)

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:1990: }
I think it should be done the same way it would've been done in
webrtcvideoengine2.cc

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.h:148: // TODO(eladalon): !!! Is it possible for
this to be FlexFEC-protected?
In theory I guess it is. Don't think you have to take care of it here since it's
a test and it wasn't protected before.

[eladalon, 2017-07-07 14:16:25]

For posterity - we've discussed the duplication offline. Summary:
1. I prefer to keep SecondaryRtpSinksContainer a pure interface. See the code
here: 
void VideoReceiveStream::AddSecondarySink(RtpPacketSinkInterface* sink) {
  rtp_video_stream_receiver_.AddSecondarySink(sink);
}
Otherwise someone who inherits it just for the purpose of piping the actual
secondary sink, such as the code above, would also get some of the
implementation (a vector of observers) that it would then never use.
(Note that 
2. The other possibility would have been to have both an interface as well as a
class implementing the interface and providing a vector, the ability to
add/remove to/from it, and the ability to iterate on it. Then, some classes
would inherit just the interface, and some the interface+implementation. This
seems to me like an engineering-overkill for simple addition/removal from a
vector.

https://codereview.webrtc.org/2974453002/diff/1/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/DEPS#newcode12
webrtc/DEPS:12: "+webrtc/call/secondary_rtp_sinks_container.h",  #
TODO(eladalon): Reconsider.
On 2017/07/07 09:36:24, stefan-webrtc wrote:
> Reconsider the name?

Yes. (Despite the lack of "!!!", this one is intended to be removed before the
CL is landed, too.)

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

https://codereview.webrtc.org/2974453002/diff/1/webrtc/call/call.cc#newcode826
webrtc/call/call.cc:826: // TODO(eladalon): Return a unique_ptr instead?
On 2017/07/07 09:36:24, stefan-webrtc wrote:
> Maybe it makes sense to keep this out of this CL until a discussion has been
had
> with some other people that care (currently on vacation), as we discussed
> offline?

Done.

https://codereview.webrtc.org/2974453002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:64: // such as
|is_recovered|, which would be |true| for FlexFEC-recovered packets.
On 2017/07/07 09:36:24, stefan-webrtc wrote:
> Seems a bit ugly to me since all rtp packet sinks would have to have this
flag,
> but only flex fec would care about it?

I see now that RtpPacketReceived::recovered() exists. I've changed to TODO
accordingly.

https://codereview.webrtc.org/2974453002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:99: received_packet->is_fec =
false;
This looks suspect; TODO added and this bug filed:
https://bugs.chromium.org/p/webrtc/issues/detail?id=7954
(To be looked at in more detail later.)

https://codereview.webrtc.org/2974453002/diff/1/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/test/call_test.cc#newc...
webrtc/test/call_test.cc:343: // TODO(eladalon): This is not the order in the
header; fix in separate CL.
On 2017/07/07 09:36:24, stefan-webrtc wrote:
> I don't think you need to add this TODO, just create a separate CL and be done
> with it. :)

Done.

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:1990: }
On 2017/07/07 09:36:24, stefan-webrtc wrote:
> I think it should be done the same way it would've been done in
> webrtcvideoengine2.cc

Sorry, I was unclear - this discussion is not really about this particular call
to RemoveSecondarySink(). I'm thinking - we could also have the secondary sink
know the primary sink, and have either destructor break the connection on both
ends, for instance.
(By the way, webrtcvideoengine2.cc has been renamed to webrtcvideoengine.cc.)

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2974453002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.h:148: // TODO(eladalon): !!! Is it possible for
this to be FlexFEC-protected?
On 2017/07/07 09:36:24, stefan-webrtc wrote:
> In theory I guess it is. Don't think you have to take care of it here since
it's
> a test and it wasn't protected before.

I want the trybots to tell me if any tests were broken by this, because
thumbnail streams were Flex-FEC protected. I'll remove the TODO after that.

[eladalon, 2017-07-13 14:38:34]

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

[danilchap, 2017-07-19 12:07:25]

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:55: // We should probably just use an
atomic-bool.
is this TODO somehow related to the CL topic?

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/secondary_rtp...
File webrtc/call/secondary_rtp_sinks_container.h (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/secondary_rtp...
webrtc/call/secondary_rtp_sinks_container.h:24: class SecondaryRtpSinksContainer
{
I can see several implementation of this interface, but no users.
(i.e. whenever AddSecondarySink is called, full type is known)
How the interface is intended to be used?

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/video/rtp_video_st...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/video/rtp_video_st...
webrtc/video/rtp_video_stream_receiver.cc:369:
secondary_sink->OnRtpPacket(packet);
can recovered packet trigger secondary_sink? should it?

[eladalon, 2017-07-21 14:42:08]

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:55: // We should probably just use an
atomic-bool.
On 2017/07/19 12:07:25, danilchap wrote:
> is this TODO somehow related to the CL topic?

1. Looking at it again, I see I have mis-worded; the critical section only
protects started_.
2. Not really related to the CL topic, which is why I'm just keeping it as a
TODO, rather than fix. Or do you suggest filing a bug instead of adding the
TODO?

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/secondary_rtp...
File webrtc/call/secondary_rtp_sinks_container.h (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/secondary_rtp...
webrtc/call/secondary_rtp_sinks_container.h:24: class SecondaryRtpSinksContainer
{
On 2017/07/19 12:07:25, danilchap wrote:
> I can see several implementation of this interface, but no users.
> (i.e. whenever AddSecondarySink is called, full type is known)
> How the interface is intended to be used?

This TODO is about this:
  // TODO(eladalon): !!! Good? Bad? Do we want to have these functions
explicitly
  // part of the interface, without inheriting them, perhaps?
  #include "webrtc/call/secondary_rtp_sinks_container.h"
(FYI - "!!!" is my way of noting that I do not intend to land the CL until the
TODO is resolved within this CL, as opposed to normal TODOs, which I leave for
later handling.)

My intuition is that if any stream can be FlexFEC-protected, then there is a
concept of "protectable", which also in our implementation demands the concept
of "can register sub-sinks, some of which might be protectors." But this is
gut-feeling. I don't currently plan to have FlexFEC register itself with a list
of objects that implement this interface, for instance.

So wdyt?

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/video/rtp_video_st...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/video/rtp_video_st...
webrtc/video/rtp_video_stream_receiver.cc:369:
secondary_sink->OnRtpPacket(packet);
On 2017/07/19 12:07:25, danilchap wrote:
> can recovered packet trigger secondary_sink? should it?

* This is in line with the behavior before this CL, where OnRtpPacket() got
triggered on those very sinks. The only difference here is that before it got
triggered from within RtpDemuxer, and now it's triggered from an intermediary
(RtpVideoStreamReceiver) which was itself triggered by RtpDemuxer.
* Please see here
https://codereview.webrtc.org/2974453002/diff/20001/webrtc/modules/rtp_rtcp/s...,
where I have a TODO in place for this general issue:
  // TODO(eladalon): Consider using packet.recovered() to avoid processing
  // recovered packets here.
* In either case, IMHO handling this issue had better be done in a separate CL
which would be based on top of this one. It would (1) keep the CLs small and
manageable, (2) allow partial rollback and (3) unblock other CLs which depends
on this CL, but not on the recovery issue.

[danilchap, 2017-07-24 09:03:46]

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:55: // We should probably just use an
atomic-bool.
On 2017/07/21 14:42:08, eladalon wrote:
> On 2017/07/19 12:07:25, danilchap wrote:
> > is this TODO somehow related to the CL topic?
> 
> 1. Looking at it again, I see I have mis-worded; the critical section only
> protects started_.
> 2. Not really related to the CL topic, which is why I'm just keeping it as a
> TODO, rather than fix. Or do you suggest filing a bug instead of adding the
> TODO?

this is issue looks too small to me to deserve an own bug.
Just create a separate CL and move discussion there.

Keeping it in this CL is a bit distracting. (There are lots of odd dependencies
in the codebase, so reading this TODO by default I assume it is one of those and
start searching for connection)

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/rtp_video_st...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/rtp_video_st...
webrtc/video/rtp_video_stream_receiver.h:213:
std::vector<RtpPacketSinkInterface*> secondary_sinks_;
How do you plan to support protecting audio with flexfec? Adding same secondary
sinks to RtpAudioStreamReceiver?

Do you plan to support anything else other than flexfec with secondary sinks?
(e.g. receive-side bwe where only some streams participate in it)

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:2051: // something else to be in charge of
removing the secondary sinks?
what are the alternatives to choose from?

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.h:152: // TODO(eladalon): !!! Is it possible for
this to be FlexFEC-protected?
why not? May be there are no tests for now, but it look like it will make sense
to add quality tests with flexfec.

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video_receive_stre...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video_receive_stre...
webrtc/video_receive_stream.h:21: // part of the interface, without inheriting
them, perhaps?
I do not see a value in an interface that no code use.

[eladalon, 2017-07-24 13:15:48]

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2974453002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:55: // We should probably just use an
atomic-bool.
On 2017/07/24 09:03:45, danilchap wrote:
> On 2017/07/21 14:42:08, eladalon wrote:
> > On 2017/07/19 12:07:25, danilchap wrote:
> > > is this TODO somehow related to the CL topic?
> > 
> > 1. Looking at it again, I see I have mis-worded; the critical section only
> > protects started_.
> > 2. Not really related to the CL topic, which is why I'm just keeping it as a
> > TODO, rather than fix. Or do you suggest filing a bug instead of adding the
> > TODO?
> 
> this is issue looks too small to me to deserve an own bug.
> Just create a separate CL and move discussion there.
> 
> Keeping it in this CL is a bit distracting. (There are lots of odd
dependencies
> in the codebase, so reading this TODO by default I assume it is one of those
and
> start searching for connection)

Created https://codereview.webrtc.org/2991533002/

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/rtp_video_st...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/rtp_video_st...
webrtc/video/rtp_video_stream_receiver.h:213:
std::vector<RtpPacketSinkInterface*> secondary_sinks_;
On 2017/07/24 09:03:46, danilchap wrote:
> How do you plan to support protecting audio with flexfec? Adding same
secondary
> sinks to RtpAudioStreamReceiver?
> 
> Do you plan to support anything else other than flexfec with secondary sinks?
> (e.g. receive-side bwe where only some streams participate in it)

1.
For audio - yes, its own vector of sinks. I considered putting the vector in
SecondaryRtpSinksContainer, but that would have meant VideoReceiveStream, which
only pipes the sink, would have had an unused vector:
  void VideoReceiveStream::AddSecondarySink(RtpPacketSinkInterface* sink) {
    rtp_video_stream_receiver_.AddSecondarySink(sink);
  }
This was solvable, but it was overkill for just having this simple vector.

2.
I don't currently plan anything else. I'm trying to keep things general when it
doesn't increase complexity (secondary-sink vs. flexfec-sink adds no complexity,
imho), to reduce future refactoring if anything else is needed.

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:2051: // something else to be in charge of
removing the secondary sinks?
On 2017/07/24 09:03:46, danilchap wrote:
> what are the alternatives to choose from?

I found it unclear that DestroyFlexfecReceiveStream() is called from a different
class than CreateFlexfecReceiveStream(), but I don't think that's in this CL's
scope to fix. Other than that, it's probably better to RemoveSecondarySink()
explicitly, rather than have it triggered by DestroyFlexfecReceiveStream(). I'm
removing the TODO.

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.h:152: // TODO(eladalon): !!! Is it possible for
this to be FlexFEC-protected?
On 2017/07/24 09:03:46, danilchap wrote:
> why not? May be there are no tests for now, but it look like it will make
sense
> to add quality tests with flexfec.

Since this isn't protected at the moment, I'll just remove the TODO. Let me know
if you think we should file a bug for adding tests here.

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video_receive_stre...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/40001/webrtc/video_receive_stre...
webrtc/video_receive_stream.h:21: // part of the interface, without inheriting
them, perhaps?
On 2017/07/24 09:03:46, danilchap wrote:
> I do not see a value in an interface that no code use.

Acknowledged.

[eladalon, 2017-07-24 14:19:00]

https://codereview.webrtc.org/2974453002/diff/100001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/100001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:434: // TODO(eladalon): !!!
Discuss - do we want to allow secondaries to be
Danil, your thoughts?

[danilchap, 2017-07-24 15:02:07]

https://codereview.webrtc.org/2974453002/diff/80001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/2974453002/diff/80001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.h:28: #include "webrtc/rtc_base/buffer.h"
reorder this include alpabetically

https://codereview.webrtc.org/2974453002/diff/100001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/100001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:434: // TODO(eladalon): !!!
Discuss - do we want to allow secondaries to be
On 2017/07/24 14:19:00, eladalon wrote:
> Danil, your thoughts?

I do not have strong opinion here.
Probably not: seems that flag is explicitly designed for better
construction/destruction of the stream, so should be fine to ignore packets
then.
But may be secondary sinks should have own way to deal with that periods.

[danilchap, 2017-07-24 15:37:26]

https://codereview.webrtc.org/2974453002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:98: // TODO(eladalon): It
seems like, because of return-statement above,
received_packet is a local variable: if return above, it will be destroyed.
only below it is pushed into member variable for later use.

[eladalon, 2017-07-24 15:53:55]

https://codereview.webrtc.org/2974453002/diff/80001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/2974453002/diff/80001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.h:28: #include "webrtc/rtc_base/buffer.h"
On 2017/07/24 15:02:07, danilchap wrote:
> reorder this include alpabetically

It's ordered on the latest revision.

https://codereview.webrtc.org/2974453002/diff/100001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/100001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:434: // TODO(eladalon): !!!
Discuss - do we want to allow secondaries to be
On 2017/07/24 15:02:07, danilchap wrote:
> On 2017/07/24 14:19:00, eladalon wrote:
> > Danil, your thoughts?
> 
> I do not have strong opinion here.
> Probably not: seems that flag is explicitly designed for better
> construction/destruction of the stream, so should be fine to ignore packets
> then.
> But may be secondary sinks should have own way to deal with that periods.

Acknowledged.

https://codereview.webrtc.org/2974453002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:98: // TODO(eladalon): It
seems like, because of return-statement above,
On 2017/07/24 15:37:25, danilchap wrote:
> received_packet is a local variable: if return above, it will be destroyed.
> only below it is pushed into member variable for later use.

Right you are. Looks like I was confused between |received_packet| and |packet|
(which is const) when I filed this bug. At least, that's one theory for the
mistake.

[danilchap, 2017-07-25 17:29:54]

lgtm

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.h:87: virtual ~FlexfecReceiveStream() =
default;
virtual -> override
now that it inherit from another interface.

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.cc:451: LOG(LS_WARNING) << "Removal of
unknown sink (" << sink << ").";
why stream to text log address of the sink pointer?

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.h:208: // TODO(eladalon): !!! Unit-tests,
here and elsewhere, after initial approach
remove the comment

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:98: class MockRtpPacketSink :
public RtpPacketSinkInterface {
there are already two Mocks of the interface.
May be it is time to create mock source set near the interface and include it
rather than reimplement it.
(not in this CL though - either in sequel or prequel)

[eladalon, 2017-07-26 08:20:29]

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.h:87: virtual ~FlexfecReceiveStream() =
default;
On 2017/07/25 17:29:54, danilchap wrote:
> virtual -> override
> now that it inherit from another interface.

Done.

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.cc:451: LOG(LS_WARNING) << "Removal of
unknown sink (" << sink << ").";
On 2017/07/25 17:29:54, danilchap wrote:
> why stream to text log address of the sink pointer?

I thought it might sometimes help, but I agree it's not terribly likely.
Removed.

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.h:208: // TODO(eladalon): !!! Unit-tests,
here and elsewhere, after initial approach
On 2017/07/25 17:29:54, danilchap wrote:
> remove the comment

Done.

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/160001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:98: class MockRtpPacketSink :
public RtpPacketSinkInterface {
On 2017/07/25 17:29:54, danilchap wrote:
> there are already two Mocks of the interface.
> May be it is time to create mock source set near the interface and include it
> rather than reimplement it.
> (not in this CL though - either in sequel or prequel)

https://codereview.webrtc.org/2988853002

[eladalon, 2017-07-26 09:09:02]

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

[the sun, 2017-07-26 09:44:12]

https://codereview.webrtc.org/2974453002/diff/180001/webrtc/video_receive_str...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/180001/webrtc/video_receive_str...
webrtc/video_receive_stream.h:225: virtual void
AddSecondarySink(RtpPacketSinkInterface* sink) = 0;
For me, this business with a "Secondary" sink fires the alarm that the solution
may be to specific. Could the flexfec sink instead be inserted as a filter and
take the video stream's place in the package forwarding chain?

[the sun, 2017-07-26 10:24:25]

solenberg@webrtc.org changed reviewers:
- solenberg@webrtc.org

[the sun, 2017-07-26 10:25:18]

On 2017/07/26 09:44:12, the sun wrote:
>
https://codereview.webrtc.org/2974453002/diff/180001/webrtc/video_receive_str...
> File webrtc/video_receive_stream.h (right):
> 
>
https://codereview.webrtc.org/2974453002/diff/180001/webrtc/video_receive_str...
> webrtc/video_receive_stream.h:225: virtual void
> AddSecondarySink(RtpPacketSinkInterface* sink) = 0;
> For me, this business with a "Secondary" sink fires the alarm that the
solution
> may be to specific. Could the flexfec sink instead be inserted as a filter and
> take the video stream's place in the package forwarding chain?

Removing myself from reviewers - don't have enough context.

[eladalon, 2017-07-26 10:47:33]

On 2017/07/26 09:44:12, the sun wrote:
>
https://codereview.webrtc.org/2974453002/diff/180001/webrtc/video_receive_str...
> File webrtc/video_receive_stream.h (right):
> 
>
https://codereview.webrtc.org/2974453002/diff/180001/webrtc/video_receive_str...
> webrtc/video_receive_stream.h:225: virtual void
> AddSecondarySink(RtpPacketSinkInterface* sink) = 0;
> For me, this business with a "Secondary" sink fires the alarm that the
solution
> may be to specific. Could the flexfec sink instead be inserted as a filter and
> take the video stream's place in the package forwarding chain?

FlexFEC will eventually protect multiple streams. When that happens, such a
solution would require it to do secondary demuxing.

[eladalon, 2017-07-31 11:36:01]

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

[Taylor Brandstetter, 2017-07-31 15:59:25]

lgtm

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine.cc:2375: }
Would it be feasible to cover this with a basic test or couple of tests in
webrtcvideoengine_unittest.cc?

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.h:147: void RemoveSecondarySink(const
RtpPacketSinkInterface* sink);
Can you add comments here documenting what a secondary sink represents?

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:434:
rtp_video_stream_receiver_->StopReceive();
Good tests; especially things like this, and the death test.

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video_receive_str...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video_receive_str...
webrtc/video_receive_stream.h:226: virtual void RemoveSecondarySink(const
RtpPacketSinkInterface* sink) = 0;
Again comments would be nice.

[eladalon, 2017-07-31 16:46:04]

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine.cc:2375: }
On 2017/07/31 15:59:25, Taylor Brandstetter wrote:
> Would it be feasible to cover this with a basic test or couple of tests in
> webrtcvideoengine_unittest.cc?

I will look into this tomorrow; pushing this batch of comments before I have an
answer to this because we're in different timezones, and I have a question
attached to one of the other comments.

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.h:147: void RemoveSecondarySink(const
RtpPacketSinkInterface* sink);
On 2017/07/31 15:59:25, Taylor Brandstetter wrote:
> Can you add comments here documenting what a secondary sink represents?

I've added this in video_receive_stream.h (following your comment; see latest
revision). Because this function isn't virtual, I've also copied it here, but
comment-duplication, while not as bad as code-duplication, is undesirable. Wdyt
should be done, if anything?
+danilchap@

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver_unittest.cc (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver_unittest.cc:434:
rtp_video_stream_receiver_->StopReceive();
On 2017/07/31 15:59:25, Taylor Brandstetter wrote:
> Good tests; especially things like this, and the death test.

Thanks. :-)

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video_receive_str...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video_receive_str...
webrtc/video_receive_stream.h:226: virtual void RemoveSecondarySink(const
RtpPacketSinkInterface* sink) = 0;
On 2017/07/31 15:59:25, Taylor Brandstetter wrote:
> Again comments would be nice.

Done % question attached to the other comment about comment-duplication.

[Taylor Brandstetter, 2017-07-31 17:31:29]

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.h (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.h:147: void RemoveSecondarySink(const
RtpPacketSinkInterface* sink);
On 2017/07/31 16:46:03, eladalon wrote:
> On 2017/07/31 15:59:25, Taylor Brandstetter wrote:
> > Can you add comments here documenting what a secondary sink represents?
> 
> I've added this in video_receive_stream.h (following your comment; see latest
> revision). Because this function isn't virtual, I've also copied it here, but
> comment-duplication, while not as bad as code-duplication, is undesirable.
Wdyt
> should be done, if anything?
> +danilchap@

Comments in one place are good, as long as we're consistent about where they go.

[eladalon, 2017-08-01 08:15:30]

Taylor, normally I'd put them in the topmost interface, but here we have a
separate function (not inherited) with the same signature and function. I
therefore wonder:
1. If that's enough motivation for making the non-virtual one inherit the
virtual from the interface.
2. Barring #1, if the comment should be duplicated, since a future reader will
not reach one from the other.

[eladalon, 2017-08-01 08:19:39]

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.cc:314:
RTC_DCHECK_RUN_ON(&worker_thread_checker_);
This will not work for video_loopback, because DirectTransport creates its own
thread, "NetworkProcess". I don't want to put a lock in actual code just to get
rid of a problem in tests, and I don't want to remove this check, either.
Refactoring DirectTransport would likely not be an easy task. Any suggestions?
The simplest I can think of is checking for (ON-WORKER-THREAD ||
IN-SPECIFIC-BUILDS-LIST), where the latter would whitelist the loopback,
unit-tests, etc. Wdyt?
+danilchap@, +stefan@, +solenberg@

[brandtr, 2017-08-01 08:34:16]

Looks good, but the test(s) requested by Taylor would be useful to have. Other
than that, I just have some minor comments.

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/2974453002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine.cc:2375: }
On 2017/07/31 16:46:03, eladalon wrote:
> On 2017/07/31 15:59:25, Taylor Brandstetter wrote:
> > Would it be feasible to cover this with a basic test or couple of tests in
> > webrtcvideoengine_unittest.cc?
> 
> I will look into this tomorrow; pushing this batch of comments before I have
an
> answer to this because we're in different timezones, and I have a question
> attached to one of the other comments.

This would be great to have, as the current VideoEngine unit tests only check
for the existence of the FlexfecReceiveStream object and the validity of the
corresponding FlexfecReceiveStream::Config.

Our perf and end-to-end tests in video/ do not involve the
WebRtcVideoEngine/WebRtcVideoChannel layer, so this is a place for potential
configuration bugs to appear in the future.

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:367: for (auto* flexfec_receive_stream :
flexfec_receive_streams_) {
nit: flexfec_recv_stream for consistency

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:368: for (auto* video_recv_stream :
video_receive_streams_) {
I'd prefer the explicit type names here, even if it means you have to do a line
break.

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:376: for (FlexfecReceiveStream* flexfec_recv_stream :
flexfec_receive_streams_) {
Does it matter which order you loop over video/flexfec here? If not, maybe use
the same order as above?

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.cc:314:
RTC_DCHECK_RUN_ON(&worker_thread_checker_);
Nice! This addition makes the code faster to read.

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.cc:370: for (auto* secondary_sink :
secondary_sinks_) {
RtpPacketSinkInterface* ?

[eladalon, 2017-08-01 09:30:29]

Taylor, Rasmus, what do you think about adding the tests in a later CL, seeing
as 1. they're not 100% related to this change and 2. they would provide coverage
that was lacking before, so missing them in this CL is probably not a step
backwards?

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:367: for (auto* flexfec_receive_stream :
flexfec_receive_streams_) {
On 2017/08/01 08:34:15, brandtr wrote:
> nit: flexfec_recv_stream for consistency

Done.

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:368: for (auto* video_recv_stream :
video_receive_streams_) {
On 2017/08/01 08:34:15, brandtr wrote:
> I'd prefer the explicit type names here, even if it means you have to do a
line
> break.

Done.

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:376: for (FlexfecReceiveStream* flexfec_recv_stream :
flexfec_receive_streams_) {
On 2017/08/01 08:34:15, brandtr wrote:
> Does it matter which order you loop over video/flexfec here? If not, maybe use
> the same order as above?

Yeah, I just saw this while responding to your previous comments and spent a few
seconds scratching my head, wondering if I did this intentionally. :-)

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/320001/webrtc/video/rtp_video_s...
webrtc/video/rtp_video_stream_receiver.cc:370: for (auto* secondary_sink :
secondary_sinks_) {
On 2017/08/01 08:34:15, brandtr wrote:
> RtpPacketSinkInterface* ?

Done.

[brandtr, 2017-08-01 15:12:27]

lgtm. I think the test can be added in a follow-up.

[Taylor Brandstetter, 2017-08-01 20:19:53]

On 2017/08/01 09:30:29, eladalon wrote:
> Taylor, Rasmus, what do you think about adding the tests in a later CL, seeing
> as 1. they're not 100% related to this change and 2. they would provide
coverage
> that was lacking before, so missing them in this CL is probably not a step
> backwards?

Sounds fine; patchset 19 lgtm.

[stefan-webrtc, 2017-08-02 08:20:44]

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:63: // recovered packets
here.
If FlexfecReceiver recovers a packet, will that packet be handed to the
VideoReceiveStream, which then again will pass it down to FlexfecReceiver?

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:110: void
AddSecondarySink(RtpPacketSinkInterface* sink) override;
Add a comment indicating what interface this implements.

[eladalon, 2017-08-02 09:16:27]

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:63: // recovered packets
here.
On 2017/08/02 08:20:44, stefan-webrtc wrote:
> If FlexfecReceiver recovers a packet, will that packet be handed to the
> VideoReceiveStream, which then again will pass it down to FlexfecReceiver?

When I inject a trace of packet.recovered() into this function
(FlexfecReceiver::AddReceivedPacket), I see that recovered media packets do end
up here. I've talked to Rasmus, and he agrees that this would be good to avoid.

Did you mean VideoReceiveStream or RtpVideoStreamReceiver, btw? We definitely go
through the latter (it's the secondary-sink-carrier), and it also has this
comment attached to it:
  // This method handles both regular RTP packets and packets recovered
  // via FlexFEC.
  void RtpVideoStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) {

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2974453002/diff/360001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:110: void
AddSecondarySink(RtpPacketSinkInterface* sink) override;
On 2017/08/02 08:20:44, stefan-webrtc wrote:
> Add a comment indicating what interface this implements.

Moved this to the webrtc::VideoReceiveStream section (both .h file and .cc
file).

[stefan-webrtc, 2017-08-02 12:29:27]

lgtm

[eladalon, 2017-08-02 13:35:32]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-02 13:35:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, brandtr@webrtc.org, deadbeef@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2974453002/#ps400001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-08-02 13:35:40]

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-08-02 13:41:09]

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

[commit-bot: I haz the power, 2017-08-02 13:41:10]

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

[eladalon, 2017-08-02 14:09:49]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-02 14:09:50]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org,
danilchap@webrtc.org, deadbeef@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2974453002/#ps420001 (title:
"Appease lint.")

[commit-bot: I haz the power, 2017-08-02 14:09:53]

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-08-02 14:39:07]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1501682989377790,
"parent_rev": "a67bd9d641929df8d42d93348fd17cbfa3839bd5", "commit_rev":
"c0d481a4a60243d9d05e024fc43c8533b8c26c1b"}

[commit-bot: I haz the power, 2017-08-02 14:39:13]

Description was changed from

==========
Protected streams report RTP messages directly to the FlexFec streams

In preparation of making RTP packet demuxing many-to-one (one SSRC goes to one
sink, but one sink may have multiple SSRCs), we need to remove FlexFEC's
dependence on being able to register itself with the demuxer. Instead, we
register FlexFEC streams with the streams they protect; when those streams get
packets, they'll forward them to their associated FlexFEC streams, too.

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

to

==========
Protected streams report RTP messages directly to the FlexFec streams

In preparation of making RTP packet demuxing many-to-one (one SSRC goes to one
sink, but one sink may have multiple SSRCs), we need to remove FlexFEC's
dependence on being able to register itself with the demuxer. Instead, we
register FlexFEC streams with the streams they protect; when those streams get
packets, they'll forward them to their associated FlexFEC streams, too.

BUG=webrtc:7135

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

[commit-bot: I haz the power, 2017-08-02 14:39:16]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/external/webrtc/+/c0d481a4a60243d9d05e024fc...