Add FlexfecReceiveStream.

Add FlexfecReceiveStream.

This class is logically parallel with the {Audio,Video}ReceiveStream
classes. Its purpose is to describe a receive stream of FlexFEC packets,
through the corresponding config.

Functionally, this class simply forwards the received RTP packets
to its FlexfecReceiver, which returns recovered packets to the
Call level, for appropriate demultiplexing based on SSRC.

BUG=webrtc:5654
Committed: https://crrev.com/76648da8dc3ca480c117013e9e299e5f2624a71b
Cr-Commit-Position: refs/heads/master@{#14704}

[brandtr, 2016-10-07 08:24:37]

Patchset #1 (id:1) has been deleted

[brandtr, 2016-10-16 21:20:02]

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

[stefan-webrtc, 2016-10-17 17:56:04]

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/api/call/flexfec_r...
File webrtc/api/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/api/call/flexfec_r...
webrtc/api/call/flexfec_receive_stream.h:20: // WORK IN PROGRESS
Add TODO(brandtr):

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

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream.h:48: rtc::CriticalSection crit_;
Maybe move this up to before or after started_

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_unittest.cc:59:
TEST(FlexfecReceiveStreamTest, RecoversPacketWhenStarted) {
Useful test!

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/config.h#newcode71
webrtc/config.h:71: uint32_t protected_media_ssrc;
Should we make this a vector already now to avoid having to change the API
later?

[philipel, 2016-10-18 10:43:34]

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_unittest.cc:31: config.ToString());
Is the format of the string representation of the config important? Is it part
of some bigger JSON object or something like that?

It feel strange to test something that should produce some arbitrary result (in
this case, a human readable representation).

[brandtr, 2016-10-18 12:56:36]

Patchset #2 (id:40001) has been deleted

[brandtr, 2016-10-18 12:59:42]

Patchset #2 (id:60001) has been deleted

[brandtr, 2016-10-18 14:13:47]

Patchset #3 (id:100001) has been deleted

[brandtr, 2016-10-18 14:22:00]

Patchset #3 (id:120001) has been deleted

[brandtr, 2016-10-18 14:27:54]

Patchset #2 (id:80001) has been deleted

[brandtr, 2016-10-18 14:31:18]

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/api/call/flexfec_r...
File webrtc/api/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/api/call/flexfec_r...
webrtc/api/call/flexfec_receive_stream.h:20: // WORK IN PROGRESS
On 2016/10/17 17:56:04, stefan-webrtc (holmer) wrote:
> Add TODO(brandtr):

Done.

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

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream.h:48: rtc::CriticalSection crit_;
On 2016/10/17 17:56:04, stefan-webrtc (holmer) wrote:
> Maybe move this up to before or after started_

Done.

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_unittest.cc:31: config.ToString());
On 2016/10/18 10:43:34, philipel wrote:
> Is the format of the string representation of the config important? Is it part
> of some bigger JSON object or something like that?
> 
> It feel strange to test something that should produce some arbitrary result
(in
> this case, a human readable representation).

Nope, this is purely for human consumption.

Since a similar test exists here:
https://cs.chromium.org/chromium/src/third_party/webrtc/audio/audio_receive_s...,
I added this unit test here. I agree that it's not super useful though, so I
wouldn't mind removing it, if you think that is better than keeping it? (I guess
the maintenance burden of keeping it shouldn't be very big.)

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/config.h#newcode71
webrtc/config.h:71: uint32_t protected_media_ssrc;
On 2016/10/17 17:56:04, stefan-webrtc (holmer) wrote:
> Should we make this a vector already now to avoid having to change the API
> later?

Yes, that seems like a good idea. Done.

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:121: if (!build_with_chromium && is_clang) {
Accidental rebase!

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/call/BUILD.gn#new...
webrtc/call/BUILD.gn:21: if (!build_with_chromium && is_clang) {
Here too :(

[stefan-webrtc, 2016-10-18 18:22:24]

lgtm with my suggestion fixed.

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.cc (right):

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.cc:32: if
(config_.protected_media_ssrcs.empty()) {
Break this out to a helper function so you can do:
receiver_(MaybeCreateReceiver(config_))

That way you can keep receiver_ as a const.

[brandtr, 2016-10-19 08:37:23]

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.cc (right):

https://codereview.webrtc.org/2397843005/diff/140001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.cc:32: if
(config_.protected_media_ssrcs.empty()) {
On 2016/10/18 18:22:24, stefan-webrtc (holmer) wrote:
> Break this out to a helper function so you can do:
> receiver_(MaybeCreateReceiver(config_))
> 
> That way you can keep receiver_ as a const.

Good idea! Done.

[philipel, 2016-10-19 09:10:50]

lgtm after comment fix.

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_unittest.cc:31: config.ToString());
On 2016/10/18 14:31:18, brandtr wrote:
> On 2016/10/18 10:43:34, philipel wrote:
> > Is the format of the string representation of the config important? Is it
part
> > of some bigger JSON object or something like that?
> > 
> > It feel strange to test something that should produce some arbitrary result
> (in
> > this case, a human readable representation).
> 
> Nope, this is purely for human consumption.
> 
> Since a similar test exists here:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/audio/audio_receive_s...,
> I added this unit test here. I agree that it's not super useful though, so I
> wouldn't mind removing it, if you think that is better than keeping it? (I
guess
> the maintenance burden of keeping it shouldn't be very big.)

I think you should remove this test, or if you want to keep it add a comment
that explains that the output does not follow a strict format.

The reason for this is that we have tests that use the log output, and adding a
test like this makes it look like that might be the case here.

[brandtr, 2016-10-19 09:31:24]

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/2397843005/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_unittest.cc:31: config.ToString());
On 2016/10/19 09:10:50, philipel wrote:
> On 2016/10/18 14:31:18, brandtr wrote:
> > On 2016/10/18 10:43:34, philipel wrote:
> > > Is the format of the string representation of the config important? Is it
> part
> > > of some bigger JSON object or something like that?
> > > 
> > > It feel strange to test something that should produce some arbitrary
result
> > (in
> > > this case, a human readable representation).
> > 
> > Nope, this is purely for human consumption.
> > 
> > Since a similar test exists here:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/audio/audio_receive_s...,
> > I added this unit test here. I agree that it's not super useful though, so I
> > wouldn't mind removing it, if you think that is better than keeping it? (I
> guess
> > the maintenance burden of keeping it shouldn't be very big.)
> 
> I think you should remove this test, or if you want to keep it add a comment
> that explains that the output does not follow a strict format.
> 
> The reason for this is that we have tests that use the log output, and adding
a
> test like this makes it look like that might be the case here.

Ok, thanks for the explanation. Removing the test.

[brandtr, 2016-10-20 10:41:55]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-10-20 10:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2397843005/#ps200001 (title:
"Fix {} consistency in config.cc")

[commit-bot: I haz the power, 2016-10-20 10:42:05]

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, 2016-10-20 11:54:51]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2016-10-20 11:54:58]

Description was changed from

==========
Add FlexfecReceiveStream.

This class is logically parallel with the {Audio,Video}ReceiveStream
classes. Its purpose is to describe a receive stream of FlexFEC packets,
through the corresponding config.

Functionally, this class simply forwards the received RTP packets
to its FlexfecReceiver, which returns recovered packets to the
Call level, for appropriate demultiplexing based on SSRC.

BUG=webrtc:5654
==========

to

==========
Add FlexfecReceiveStream.

This class is logically parallel with the {Audio,Video}ReceiveStream
classes. Its purpose is to describe a receive stream of FlexFEC packets,
through the corresponding config.

Functionally, this class simply forwards the received RTP packets
to its FlexfecReceiver, which returns recovered packets to the
Call level, for appropriate demultiplexing based on SSRC.

BUG=webrtc:5654

Committed: https://crrev.com/76648da8dc3ca480c117013e9e299e5f2624a71b
Cr-Commit-Position: refs/heads/master@{#14704}
==========

[commit-bot: I haz the power, 2016-10-20 11:54:59]

Patchset 5 (id:??) landed as
https://crrev.com/76648da8dc3ca480c117013e9e299e5f2624a71b
Cr-Commit-Position: refs/heads/master@{#14704}