Add a FlexfecReceiver class.

Add a FlexfecReceiver class.

This class is split in interface/implementation classes, since it
will be referenced from the Call level. Its purpose is to interface
the erasure code decoder with a new class FlexfecReceiveStream
(for received packets), as well as with the main RTP pipeline (for
recovered packets).

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

[brandtr, 2016-10-05 11:09:57]

Patchset #1 (id:1) has been deleted

[brandtr, 2016-10-05 11:10:03]

Patchset #1 (id:20001) has been deleted

[brandtr, 2016-10-05 11:22:23]

Patchset #1 (id:40001) has been deleted

[brandtr, 2016-10-05 11:30:25]

Patchset #1 (id:60001) has been deleted

[brandtr, 2016-10-05 12:24:30]

Patchset #1 (id:80001) has been deleted

[brandtr, 2016-10-05 12:54:55]

Patchset #1 (id:100001) has been deleted

[brandtr, 2016-10-05 13:15:53]

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

[brandtr, 2016-10-05 13:15:54]

Please take a look :)

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:21: // Callback interface for
packets recovered by FlexFEC.
This interface is a subset of the RtpData interface:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/incl...

I'm open for suggestions on how to/if we should harmonize these interfaces.

(In a followup CL, I will add this interface to internal::Call. It seems like a
bad idea to add a null implementation of OnReceivedPayloadData to Call, just to
be able to reuse the RtpData interface.)

[danilchap, 2016-10-05 14:32:04]

Couldn't hold myself and took a sketchy look

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:24: virtual
~RecoveredPacketReceiver();
if you do no plan to delete RecoveredPacketReceiver having pointer to abstract
class, you might want to hide destructor into protected section. (that would be
an additional signal callback passed without ownership)

Also, in this case (pure interface, no members) it is ok to have destructor
implementation in the .h file.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h:53: rtc::ThreadChecker
thread_checker_;
may be use SequentialThreadChecker to support upcoming ThreadQueue.

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

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:98: EXPECT_EQ(true,
receiver_->AddReceivedPacket(media_packet->data,
s/EXPECT_EQ(true,/EXPECT_TRUE(/g

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:112:
EXPECT_EQ(false, receiver_->AddReceivedPacket(media_packet->data,
EXPECT_FALSE(

[stefan-webrtc, 2016-10-05 14:45:59]

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:21: // Callback interface for
packets recovered by FlexFEC.
On 2016/10/05 13:15:54, brandtr wrote:
> This interface is a subset of the RtpData interface:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/incl...
> 
> I'm open for suggestions on how to/if we should harmonize these interfaces.
> 
> (In a followup CL, I will add this interface to internal::Call. It seems like
a
> bad idea to add a null implementation of OnReceivedPayloadData to Call, just
to
> be able to reuse the RtpData interface.)

Fine with me to have a new interface.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:44: virtual bool
ProcessReceivedPackets() = 0;
Is it worth having this method, or should we simply always process if we have
received a packet?

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/rtp_rtcp.gypi (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/rtp_rtcp.gypi:34: 'source/flexfec_receiver_impl.h',
The mock wasn't added.

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

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.cc:134: 0) {
Will you update this method to return bool later?

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.cc:140: if
(recovered_packet->returned) {
If you don't prefer this, I prefer removing {} :)

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h:46:
RecoveredPacketReceiver* callback_;
RecoveredPacketReceiver* const callback_

[brandtr, 2016-10-06 08:36:31]

On 2016/10/05 14:32:04, danilchap wrote:
> Couldn't hold myself and took a sketchy look

Haha :) Feel free to add yourself as a formal reviewer if you want!

My plan is to send you the send side CLs for review, since these interface with
your changes in RTPSenderVideo. I'll do that after I land the receive side,
though.

[brandtr, 2016-10-06 08:38:58]

Addressed comments in different patchsets.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:21: // Callback interface for
packets recovered by FlexFEC.
On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> On 2016/10/05 13:15:54, brandtr wrote:
> > This interface is a subset of the RtpData interface:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/incl...
> > 
> > I'm open for suggestions on how to/if we should harmonize these interfaces.
> > 
> > (In a followup CL, I will add this interface to internal::Call. It seems
like
> a
> > bad idea to add a null implementation of OnReceivedPayloadData to Call, just
> to
> > be able to reuse the RtpData interface.)
> 
> Fine with me to have a new interface.

Acknowledged.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:24: virtual
~RecoveredPacketReceiver();
On 2016/10/05 14:32:04, danilchap wrote:
> if you do no plan to delete RecoveredPacketReceiver having pointer to abstract
> class, you might want to hide destructor into protected section. (that would
be
> an additional signal callback passed without ownership)
> 
> Also, in this case (pure interface, no members) it is ok to have destructor
> implementation in the .h file.

Good idea, done!

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:44: virtual bool
ProcessReceivedPackets() = 0;
On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> Is it worth having this method, or should we simply always process if we have
> received a packet?

Currently, we always process whenever we receive a packet. I've simplified the
public interface to make this clear.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/rtp_rtcp.gypi (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/rtp_rtcp.gypi:34: 'source/flexfec_receiver_impl.h',
On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> The mock wasn't added.

:(

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

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.cc:134: 0) {
On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> Will you update this method to return bool later?

This method currently always returns 0, so it could even be void... I'll write
it down in my TODO list.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.cc:140: if
(recovered_packet->returned) {
On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> If you don't prefer this, I prefer removing {} :)

I prefer it :)

All of the existing FEC source code uses {} to wrap single-line statements, so
I'm using that here as well. In Call and other places, where the {}'s are
excluded, I do not add them.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h (right):

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h:46:
RecoveredPacketReceiver* callback_;
On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> RecoveredPacketReceiver* const callback_

Done.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.h:53: rtc::ThreadChecker
thread_checker_;
On 2016/10/05 14:32:04, danilchap wrote:
> may be use SequentialThreadChecker to support upcoming ThreadQueue.

Sounds good! I wasn't aware that the SequencedTaskChecker worked for threads as
well.

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

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:98: EXPECT_EQ(true,
receiver_->AddReceivedPacket(media_packet->data,
On 2016/10/05 14:32:04, danilchap wrote:
> s/EXPECT_EQ(true,/EXPECT_TRUE(/g

Done.

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:112:
EXPECT_EQ(false, receiver_->AddReceivedPacket(media_packet->data,
On 2016/10/05 14:32:04, danilchap wrote:
> EXPECT_FALSE(

Done.

[stefan-webrtc, 2016-10-06 08:43:55]

lgtm

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

https://codereview.webrtc.org/2392663006/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_impl.cc:140: if
(recovered_packet->returned) {
On 2016/10/06 08:38:57, brandtr wrote:
> On 2016/10/05 14:45:59, stefan-webrtc (holmer) wrote:
> > If you don't prefer this, I prefer removing {} :)
> 
> I prefer it :)
> 
> All of the existing FEC source code uses {} to wrap single-line statements, so
> I'm using that here as well. In Call and other places, where the {}'s are
> excluded, I do not add them.

Acknowledged.

[brandtr, 2016-10-10 23:14:27]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2016-10-10 23:14:38]

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-10 23:45:00]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-10-10 23:45:06]

Description was changed from

==========
Add a FlexfecReceiver class.

This class is split in interface/implementation classes, since it
will be referenced from the Call level. Its purpose is to interface
the erasure code decoder with a new class FlexfecReceiveStream
(for received packets), as well as with the main RTP pipeline (for
recovered packets).

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

to

==========
Add a FlexfecReceiver class.

This class is split in interface/implementation classes, since it
will be referenced from the Call level. Its purpose is to interface
the erasure code decoder with a new class FlexfecReceiveStream
(for received packets), as well as with the main RTP pipeline (for
recovered packets).

BUG=webrtc:5654

Committed: https://crrev.com/a8b38559a570599a44e3bfa55a870904db02bde9
Cr-Commit-Position: refs/heads/master@{#14594}
==========

[commit-bot: I haz the power, 2016-10-10 23:45:07]

Patchset 3 (id:??) landed as
https://crrev.com/a8b38559a570599a44e3bfa55a870904db02bde9
Cr-Commit-Position: refs/heads/master@{#14594}