Change ForwardErrorCorrection class to accept one received packet at a time.

Change ForwardErrorCorrection class to accept one received packet at a time.

BUG=None

Review-Url: https://codereview.webrtc.org/3012243002
Cr-Commit-Position: refs/heads/master@{#19893}
Committed: https://webrtc.googlesource.com/src/+/a5f043f9cd201fff209e96415bebcb8445ba0176

[nisse-webrtc, 2017-09-13 12:37:13]

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

[nisse-webrtc, 2017-09-13 12:37:14]

Would you like to have an intitial look? Tests not yet updated.

Possible follow up cls: Improve seqno-related logic. And delete the
ReceivedPacket class, instead passing in a is_fec flag or just have separate
methods for inserting media and fec packets into the machine.

[brandtr, 2017-09-13 15:39:23]

On 2017/09/13 12:37:14, nisse-webrtc wrote:
> Would you like to have an intitial look? Tests not yet updated.
> 
> Possible follow up cls: Improve seqno-related logic. And delete the
> ReceivedPacket class, instead passing in a is_fec flag or just have separate
> methods for inserting media and fec packets into the machine.

I have just had a quick look, but it looks like a good start. Will look more
carefully tomorrow.

Agree that it would be nicer to just pass the media and FEC packets separately
into the decoder. Since the FlexfecReceiver and UlpfecReceiver already do the
demuxing, why put the packets in a single list again...?

[brandtr, 2017-09-14 11:49:45]

I think this looks good. Hard to say why the tests fail though by just looking
at the log output.

Added a question about the refcounted packets.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:42:
std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket(
Maybe rename, since it only converts the packet now?

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:123: bool
FlexfecReceiver::ProcessReceivedPacket(
make void?

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (left):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:354:
received_packet->pkt = nullptr;
Just to verify: the refcount of |received_packet->pkt| will be decremented when
the unique_ptr owning |received_packet| goes out of scope?

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:401:
received_packet->pkt = nullptr;
And here.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: // separate
arguments.
+1 for this.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: //
UlpfecReceiverImpl::AddReceivedRedPacket may add two packets to
Right, I forgot about that! I guess it can be replaced by two calls to
DecodeFec?

I don't think we ever send RED packets with two payloads, and if we want to
support receiving 2 payloads, why not support receiving N payloads?

We just probably just try to get rid of Ulpfec at some point though.

[nisse-webrtc, 2017-09-14 12:55:51]

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:42:
std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket(
On 2017/09/14 11:49:45, brandtr wrote:
> Maybe rename, since it only converts the packet now? 

Any naming suggestion? It seems the only member variables it accesses are ssrcs
and packet counters.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:123: bool
FlexfecReceiver::ProcessReceivedPacket(
On 2017/09/14 11:49:45, brandtr wrote:
> make void?

Done.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (left):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:354:
received_packet->pkt = nullptr;
On 2017/09/14 11:49:45, brandtr wrote:
> Just to verify: the refcount of |received_packet->pkt| will be decremented
when
> the unique_ptr owning |received_packet| goes out of scope?

That's my understanding. And it would be more natural to accept a
scoped_refptr<Packet> as input.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: // separate
arguments.
On 2017/09/14 11:49:45, brandtr wrote:
> +1 for this.

For a followup, then, it's not quite trivial. Might make change to replace the
AddReceivedPacket to a method that just returns the is_fec flag depending on
config.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: //
UlpfecReceiverImpl::AddReceivedRedPacket may add two packets to
On 2017/09/14 11:49:45, brandtr wrote:
> Right, I forgot about that! I guess it can be replaced by two calls to
> DecodeFec?

Things may get easier if we merge the Add...Packet and Process...Fec methods
both here and in flexfec. It seems callers always call them together.

> I don't think we ever send RED packets with two payloads, and if we want to
> support receiving 2 payloads, why not support receiving N payloads?

No idea, I'd guess there's some interop case behind the choice to support 2?

> We just probably just try to get rid of Ulpfec at some point though.

But that's going to take a few years, I guess.

[brandtr, 2017-09-14 13:14:16]

I do not understand the purpose of
RtpFecTestUlpfecOnly.FecRecoveryWithSeqNumGapOneFrameNoRecovery.
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...

Last time I made changes to ForwardErrorCorrection, I simply left this test even
though I didn't understand the value of them. I could be some intricacy of how
sequence number wraps are handled by the bitmask in the FEC header.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:42:
std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket(
On 2017/09/14 12:55:51, nisse-webrtc wrote:
> On 2017/09/14 11:49:45, brandtr wrote:
> > Maybe rename, since it only converts the packet now? 
> 
> Any naming suggestion? It seems the only member variables it accesses are
ssrcs
> and packet counters.

Maybe 'ConvertPacket', but then it would make sense to move the
|packet_counter_| updates to ProcessReceivedPacket. This is more of a nit
though, so feel free to skip.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: // separate
arguments.
On 2017/09/14 12:55:51, nisse-webrtc wrote:
> On 2017/09/14 11:49:45, brandtr wrote:
> > +1 for this.
> 
> For a followup, then, it's not quite trivial. Might make change to replace the
> AddReceivedPacket to a method that just returns the is_fec flag depending on
> config.

Yep, no need to do now.

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right):

https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: //
UlpfecReceiverImpl::AddReceivedRedPacket may add two packets to
On 2017/09/14 12:55:51, nisse-webrtc wrote:
> On 2017/09/14 11:49:45, brandtr wrote:
> > Right, I forgot about that! I guess it can be replaced by two calls to
> > DecodeFec?
> 
> Things may get easier if we merge the Add...Packet and Process...Fec methods
> both here and in flexfec. It seems callers always call them together.

In Flexfec, outside users call FlexfecReceiver::OnRtpPacket, which always call
the two private methods together.
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/incl...

In Ulpfec, outside users could call the different public methods separately, but
they don't.
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...

> > I don't think we ever send RED packets with two payloads, and if we want to
> > support receiving 2 payloads, why not support receiving N payloads?
> 
> No idea, I'd guess there's some interop case behind the choice to support 2?

Yes, probably. But why not support the N payload case then?

> > We just probably just try to get rid of Ulpfec at some point though.
> 
> But that's going to take a few years, I guess.

Yes :(

[nisse-webrtc, 2017-09-15 06:34:57]

Updated the ulpfec test to expect success.

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/rtp_fec_unittest.cc:454: // the log message "The
recovered packet had a length larger than a typical IP
I've just checked on master, and I get the same log message there. This needs
fixing, but perhaps not in this cl?

[brandtr, 2017-09-15 08:10:34]

lgtm

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/rtp_fec_unittest.cc:454: // the log message "The
recovered packet had a length larger than a typical IP
On 2017/09/15 06:34:57, nisse-webrtc wrote:
> I've just checked on master, and I get the same log message there. This needs
> fixing, but perhaps not in this cl?

Sgtm to do this later.

[nisse-webrtc, 2017-09-15 08:18:11]

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

[commit-bot: I haz the power, 2017-09-15 08:18:21]

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-09-15 08:22:04]

nisse@webrtc.org changed reviewers:
+ holmer@chromium.org

[nisse-webrtc, 2017-09-15 08:22:05]

Stefan, would you like to have a look too?

This refactoring should make it easier to fix handling of seqno wraparound,
which currently looks broken to me.

Unlikely to land today, but I hope to get it in reasonably soon, after the dust
has settled after the great move out of webrtc/.

[commit-bot: I haz the power, 2017-09-15 08:24:11]

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

[commit-bot: I haz the power, 2017-09-15 08:24:12]

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

[stefan-webrtc, 2017-09-15 13:36:12]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2017-09-15 13:36:13]

lgtm

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/forward_error_correction.h (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/forward_error_correction.h:80: // TODO(holmer): Refactor
into a proper class.
May want to just delete this TODO, it's been there for 6 years.

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/rtp_fec_unittest.cc:453: // TODO(nisse): In this test,
recovery based on the first FEC packets fails with
FEC packet

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // TODO(nisse): But it looks
like
Please update the existing TODO instead if it's no longer correct.

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // TODO(nisse): But it looks
like
Please update the existing TODO instead if it's no longer correct.

[nisse-webrtc, 2017-09-18 12:39:06]

I'll run the fyi and internal bot, and land if things look green.

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/forward_error_correction.h (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/forward_error_correction.h:80: // TODO(holmer): Refactor
into a proper class.
On 2017/09/15 13:36:13, stefan-webrtc wrote:
> May want to just delete this TODO, it's been there for 6 years.

Done.

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/rtp_fec_unittest.cc:453: // TODO(nisse): In this test,
recovery based on the first FEC packets fails with
On 2017/09/15 13:36:13, stefan-webrtc wrote:
> FEC packet

Done.

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
File modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right):

https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/...
modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // TODO(nisse): But it looks
like
On 2017/09/15 13:36:13, stefan-webrtc wrote:
> Please update the existing TODO instead if it's no longer correct.

Done.

[nisse-webrtc, 2017-09-18 12:39:20]

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

[commit-bot: I haz the power, 2017-09-18 12:39:26]

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-09-18 13:30:16]

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

[commit-bot: I haz the power, 2017-09-18 13:30:17]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-09-18 14:56:07]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-09-18 14:56:08]

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

[commit-bot: I haz the power, 2017-09-18 14:56: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-09-18 14:58:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1505746567439560,
"parent_rev": "dd3abbb53250072dc5baac0391ebea135be6fa53", "commit_rev":
"a5f043f9cd201fff209e96415bebcb8445ba0176"}

[commit-bot: I haz the power, 2017-09-18 14:59:04]

Description was changed from

==========
Change ForwardErrorCorrection class to accept one received packet at a time.

BUG=None
==========

to

==========
Change ForwardErrorCorrection class to accept one received packet at a time.

BUG=None

Review-Url: https://codereview.webrtc.org/3012243002
Cr-Commit-Position: refs/heads/master@{#19893}
Committed:
https://webrtc.googlesource.com/src/+/a5f043f9cd201fff209e96415bebcb8445ba0176
==========

[commit-bot: I haz the power, 2017-09-18 14:59:05]

Committed patchset #8 (id:140001) as
https://webrtc.googlesource.com/src/+/a5f043f9cd201fff209e96415bebcb8445ba0176