Rename flexfec AddAndProcessReceivedPacket --> OnRtpPacket.

Rename flexfec AddAndProcessReceivedPacket --> OnRtpPacket.

Preparing for a media-independent RTP receive stream interface.

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

[nisse-webrtc, 2017-02-10 13:46:21]

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

[nisse-webrtc, 2017-02-10 13:46:22]

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.cc (left):

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.cc:163: if
(!receiver_->AddAndProcessReceivedPacket(packet))
Should we rename and/or drop the return value for this method too?

Is it essential to exclude broken flexfec packets from the below statistics?

[brandtr, 2017-02-10 14:12:21]

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.cc (left):

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.cc:163: if
(!receiver_->AddAndProcessReceivedPacket(packet))
On 2017/02/10 13:46:21, nisse-webrtc wrote:
> Should we rename and/or drop the return value for this method too?
Yes, might as well do both :)

> Is it essential to exclude broken flexfec packets from the below statistics?
Don't think so. What change do you want to make?

[nisse-webrtc, 2017-02-13 13:20:03]

I've now renamed FlexfecReceiver::AddAndProcessReceivedPacket too, and dropped
its return value. I had to reorg tests slightly to still be able to test
underlying functions.

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.cc (left):

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.cc:163: if
(!receiver_->AddAndProcessReceivedPacket(packet))
On 2017/02/10 14:12:21, brandtr wrote:
> On 2017/02/10 13:46:21, nisse-webrtc wrote:
> > Is it essential to exclude broken flexfec packets from the below statistics?
> Don't think so. What change do you want to make?

If I delete return value and this if statement, as a consequence the below stats
call will be invoked also for invalid packets.

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

https://codereview.webrtc.org/2686273002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: if
(!AddReceivedPacket(std::move(packet))) {
Does std::move make any sense here?

[brandtr, 2017-02-13 15:32:14]

lgtm, with request

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.cc (left):

https://codereview.webrtc.org/2686273002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.cc:163: if
(!receiver_->AddAndProcessReceivedPacket(packet))
On 2017/02/13 13:20:03, nisse-webrtc wrote:
> On 2017/02/10 14:12:21, brandtr wrote:
> > On 2017/02/10 13:46:21, nisse-webrtc wrote:
> > > Is it essential to exclude broken flexfec packets from the below
statistics?
> > Don't think so. What change do you want to make?
> 
> If I delete return value and this if statement, as a consequence the below
stats
> call will be invoked also for invalid packets.

I understand. I think it's fine to include those.

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

https://codereview.webrtc.org/2686273002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: if
(!AddReceivedPacket(std::move(packet))) {
On 2017/02/13 13:20:03, nisse-webrtc wrote:
> Does std::move make any sense here?
>  

Nope, that is a mistake. Before, |packet| was passed by value, so then it made
sense.

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

https://codereview.webrtc.org/2686273002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: ProcessReceivedPackets();
I prefer having the general case outside the braces:

if (!AddReceivedPacket(.)) {
  return;
}
ProcessReceivedPackets();

[nisse-webrtc, 2017-02-14 07:58:18]

Stefan, ptal.

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

https://codereview.webrtc.org/2686273002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: if
(!AddReceivedPacket(std::move(packet))) {
On 2017/02/13 15:32:14, brandtr wrote:
> On 2017/02/13 13:20:03, nisse-webrtc wrote:
> > Does std::move make any sense here?
> >  
> 
> Nope, that is a mistake. Before, |packet| was passed by value, so then it made
> sense.

Deleted the move.

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

https://codereview.webrtc.org/2686273002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: ProcessReceivedPackets();
On 2017/02/13 15:32:13, brandtr wrote:
> I prefer having the general case outside the braces:
> 
> if (!AddReceivedPacket(.)) {
>   return;
> }
> ProcessReceivedPackets();

Done.

[nisse-webrtc, 2017-02-15 13:38:20]

Ooops, added a patchset #4 intended for a different cl. I'll delete that. Sorry
for the confusion.

[nisse-webrtc, 2017-02-15 13:38:34]

Patchset #4 (id:60001) has been deleted

[stefan-webrtc, 2017-02-16 08:53:16]

lg, but I have some issues with the tests and protected methods...

https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:60: return
AddReceivedPacket(packet) && ProcessReceivedPackets();
Not a big fan of this. Can't we check whether expected output is produced or not
instead?

[nisse-webrtc, 2017-02-16 08:58:39]

https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:60: return
AddReceivedPacket(packet) && ProcessReceivedPackets();
On 2017/02/16 08:53:15, stefan-webrtc wrote:
> Not a big fan of this. Can't we check whether expected output is produced or
not
> instead?

I use this method in those tests which only insert data into flexfec. While the
tests that do check expected output, e.g, that OnRecoveredPacket is called,
don't use this hack and instead call the non-overridden OnRtpPacket method.

Rasmus, do you see any good way to improve on this?

[nisse-webrtc, 2017-02-16 12:21:55]

https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:60: return
AddReceivedPacket(packet) && ProcessReceivedPackets();
On 2017/02/16 08:58:39, nisse-webrtc wrote:
> On 2017/02/16 08:53:15, stefan-webrtc wrote:
> > Not a big fan of this. Can't we check whether expected output is produced or
> not
> > instead?
> 
> I use this method in those tests which only insert data into flexfec. While
the
> tests that do check expected output, e.g, that OnRecoveredPacket is called,
> don't use this hack and instead call the non-overridden OnRtpPacket method.
> 
> Rasmus, do you see any good way to improve on this?

I've now dropped the OnRtpPacketForTest method, instead calling the internal
methods directly from their tests.

[nisse-webrtc, 2017-02-16 12:22:01]

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

[commit-bot: I haz the power, 2017-02-16 12:22:10]

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-02-16 12:23:42]

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

[commit-bot: I haz the power, 2017-02-16 12:23:43]

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_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17373)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22810)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21579)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18811)

[nisse-webrtc, 2017-02-16 12:32:48]

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

[commit-bot: I haz the power, 2017-02-16 12:32:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[stefan-webrtc, 2017-02-16 12:36:49]

On 2017/02/16 08:58:39, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
> File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2686273002/diff/40001/webrtc/modules/rtp_rtcp/s...
> webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:60: return
> AddReceivedPacket(packet) && ProcessReceivedPackets();
> On 2017/02/16 08:53:15, stefan-webrtc wrote:
> > Not a big fan of this. Can't we check whether expected output is produced or
> not
> > instead?
> 
> I use this method in those tests which only insert data into flexfec. While
the
> tests that do check expected output, e.g, that OnRecoveredPacket is called,
> don't use this hack and instead call the non-overridden OnRtpPacket method.
> 
> Rasmus, do you see any good way to improve on this?

I wonder if these tests provide enough value that we should actually keep those
methods...

[stefan-webrtc, 2017-02-16 12:37:17]

lgtm for now, but I would want to clean this up at some point.

[commit-bot: I haz the power, 2017-02-16 12:43:05]

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

[commit-bot: I haz the power, 2017-02-16 12:43:06]

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

[nisse-webrtc, 2017-02-16 12:50:20]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-16 12:50:20]

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/2686273002/#ps120001 (title:
"Update flexfec fuzzer.")

[commit-bot: I haz the power, 2017-02-16 12:50:23]

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-02-16 14:52:34]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487249420047250,
"parent_rev": "c1fa32d0d283ed3c7563690f0e6e6f7474d575d4", "commit_rev":
"5c29a7aad13809caaaa29f02519daea432586280"}

[commit-bot: I haz the power, 2017-02-16 14:52:37]

Description was changed from

==========
Rename flexfec AddAndProcessReceivedPacket --> OnRtpPacket.

Preparing for a media-independent RTP receive stream interface.

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

to

==========
Rename flexfec AddAndProcessReceivedPacket --> OnRtpPacket.

Preparing for a media-independent RTP receive stream interface.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2686273002
Cr-Commit-Position: refs/heads/master@{#16646}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5c29a7aad13809caaaa29f025...
==========

[commit-bot: I haz the power, 2017-02-16 14:52:38]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/5c29a7aad13809caaaa29f025...