Duplicate FecController in preparation of forking

Duplicate FecController in preparation of forking

This CL only copies existing files, in preparation of the fork. This is done so that the subsequent CLs' modifications of the existing files would be more obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid trybots warnings:
1. The controller's file's include of its header is modified to include the newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.

Except for this, no changes were made. The new files (rplr-based) are not included or compiled yet; the next CL introduces the necessary changes, then adds the files to BUILD.gn.
For the renamed files (fec_controller -> fec_controller_plr_based), a respective change is made to BUILD.gn, of course.

BUG=webrtc:7058

[elad.alon_webrtc.org, 2017-02-03 13:30:49]

Description was changed from

==========
Duplicate FecController in preparation of forking
This CL only copies existing files, in preparation of the fork. This is done so
that the subsequent CL's modifications of the existing files would be more
obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid
trybots warnings:
1. The controller's file's include of its header is modified to include the
newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.
Except for this, no changes were made. The files are not included or compiled
yet; the next CL introduces the necessary changes, then adds the files to
BUILD.gn.

BUG=webrtc:7058
==========

to

==========
Duplicate FecController in preparation of forking
This CL only copies existing files, in preparation of the fork. This is done so
that the subsequent CL's modifications of the existing files would be more
obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid
trybots warnings:
1. The controller's file's include of its header is modified to include the
newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.
Except for this, no changes were made. The files are not included or compiled
yet; the next CL introduces the necessary changes, then adds the files to
BUILD.gn.

BUG=webrtc:7058
==========

[elad.alon_webrtc.org, 2017-02-03 13:30:50]

elad.alon@webrtc.org changed reviewers:
+ michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org

[elad.alon_webrtc.org, 2017-02-03 13:31:11]

Ready for review.

[minyue-webrtc, 2017-02-03 14:10:42]

how similar will be the two controllers? Forking makes me thinking that one can
derive from the other?

[elad.alon_webrtc.org, 2017-02-03 15:19:20]

On 2017/02/03 14:10:42, minyue-webrtc wrote:
> how similar will be the two controllers? Forking makes me thinking that one
can
> derive from the other?

https://codereview.webrtc.org/2668103003/

[elad.alon_webrtc.org, 2017-02-03 15:22:23]

On 2017/02/03 15:19:20, elad.alon wrote:
> On 2017/02/03 14:10:42, minyue-webrtc wrote:
> > how similar will be the two controllers? Forking makes me thinking that one
> can
> > derive from the other?
> 
> https://codereview.webrtc.org/2668103003/

Sorry, that was the wrong link. Here's the right link:
https://codereview.webrtc.org/2672933003/
Deriving is possible. My opinion is that it would be easier to keep two separate
ones for the (hopefully) short time we need for experimentation, I have no
problem doing it another way. But keep in mind that the FecController is also
going to change now (next CL I'll push for review), since it would need to
support both RTCP-PLR and TWCC-PLR.

[minyue-webrtc, 2017-02-03 15:36:53]

On 2017/02/03 15:22:23, elad.alon wrote:
> On 2017/02/03 15:19:20, elad.alon wrote:
> > On 2017/02/03 14:10:42, minyue-webrtc wrote:
> > > how similar will be the two controllers? Forking makes me thinking that
one
> > can
> > > derive from the other?
> > 
> > https://codereview.webrtc.org/2668103003/
> 
> Sorry, that was the wrong link. Here's the right link:
> https://codereview.webrtc.org/2672933003/
> Deriving is possible. My opinion is that it would be easier to keep two
separate
> ones for the (hopefully) short time we need for experimentation, I have no
> problem doing it another way. But keep in mind that the FecController is also
> going to change now (next CL I'll push for review), since it would need to
> support both RTCP-PLR and TWCC-PLR.

code duplication can be unfavorable. Thinking into future, it may be possible to
keep both implementations and let user choose?

[elad.alon_webrtc.org, 2017-02-06 15:55:42]

Description was changed from

==========
Duplicate FecController in preparation of forking
This CL only copies existing files, in preparation of the fork. This is done so
that the subsequent CL's modifications of the existing files would be more
obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid
trybots warnings:
1. The controller's file's include of its header is modified to include the
newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.
Except for this, no changes were made. The files are not included or compiled
yet; the next CL introduces the necessary changes, then adds the files to
BUILD.gn.

BUG=webrtc:7058
==========

to

==========
Duplicate FecController in preparation of forking
This CL only copies existing files, in preparation of the fork. This is done so
that the subsequent CLs' modifications of the existing files would be more
obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid
trybots warnings:
1. The controller's file's include of its header is modified to include the
newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.
Except for this, no changes were made. The files are not included or compiled
yet; the next CL introduces the necessary changes, then adds the files to
BUILD.gn.

BUG=webrtc:7058
==========

[elad.alon_webrtc.org, 2017-02-07 23:11:02]

On 2017/02/03 15:36:53, minyue-webrtc wrote:
> On 2017/02/03 15:22:23, elad.alon wrote:
> > On 2017/02/03 15:19:20, elad.alon wrote:
> > > On 2017/02/03 14:10:42, minyue-webrtc wrote:
> > > > how similar will be the two controllers? Forking makes me thinking that
> one
> > > can
> > > > derive from the other?
> > > 
> > > https://codereview.webrtc.org/2668103003/
> > 
> > Sorry, that was the wrong link. Here's the right link:
> > https://codereview.webrtc.org/2672933003/
> > Deriving is possible. My opinion is that it would be easier to keep two
> separate
> > ones for the (hopefully) short time we need for experimentation, I have no
> > problem doing it another way. But keep in mind that the FecController is
also
> > going to change now (next CL I'll push for review), since it would need to
> > support both RTCP-PLR and TWCC-PLR.
> 
> code duplication can be unfavorable. Thinking into future, it may be possible
to
> keep both implementations and let user choose?

A later CL will make sure that code shared between the two (PLR-based and
RPLR-based) would not be duplicated, as per our offline discussion.

[elad.alon_webrtc.org, 2017-02-07 23:13:33]

Description was changed from

==========
Duplicate FecController in preparation of forking
This CL only copies existing files, in preparation of the fork. This is done so
that the subsequent CLs' modifications of the existing files would be more
obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid
trybots warnings:
1. The controller's file's include of its header is modified to include the
newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.
Except for this, no changes were made. The files are not included or compiled
yet; the next CL introduces the necessary changes, then adds the files to
BUILD.gn.

BUG=webrtc:7058
==========

to

==========
Duplicate FecController in preparation of forking

This CL only copies existing files, in preparation of the fork. This is done so
that the subsequent CLs' modifications of the existing files would be more
obvious, and easier to review.
Some minor changes are introduced by this CL to the copied files to avoid
trybots warnings:
1. The controller's file's include of its header is modified to include the
newly named header.
2. The #ifdef name associated with the headefile is modified.
3. Copyright year changed from 2016 to 2017.

Except for this, no changes were made. The new files (rplr-based) are not
included or compiled yet; the next CL introduces the necessary changes, then
adds the files to BUILD.gn.
For the renamed files (fec_controller -> fec_controller_plr_based), a respective
change is made to BUILD.gn, of course.

BUG=webrtc:7058
==========

[michaelt, 2017-02-09 13:38:10]

Wouldn't it be easier then not to duplicate from the beginning ?

[elad.alon_webrtc.org, 2017-02-09 14:37:08]

On 2017/02/09 13:38:10, michaelt wrote:
> Wouldn't it be easier then not to duplicate from the beginning ?

Minyue and I have discussed it, and decided we'd like to have separate files for
the PLR-based and RPLR-based controllers. Given that RPLR-based is rather
similar to RPLR-based, it would be easier for reviewers to see what changes were
introduced between the two if there's on CL that just copies the file over, and
then another CL which only introduces the actual change. Otherwise, you need to
use the often-employed trick of comparing to patchest #1, but that becomes
problematic if any of the previous CLs in the stack end up changing the
controller, then you have to compare to patchset #n, etc. Easier to know that
his CL will always just be a simple copy, and the CLs on top of this do the
actual work (and you just need to look at their latest patchset).

[minyue-webrtc, 2017-03-22 10:52:38]

On 2017/02/09 13:38:10, michaelt wrote:
> Wouldn't it be easier then not to duplicate from the beginning ?

Hi Michael,

I think defining a superclass to take care of the commonality of the two FEC
controllers is unnecessary.

But yes, duplication is bad. To solve that, we would extract the real core of
the two controllers: threshold usage in a util class.

WDYT?

[elad.alon_webrtc.org, 2017-03-22 10:54:32]

On 2017/03/22 10:52:38, minyue-webrtc wrote:
> On 2017/02/09 13:38:10, michaelt wrote:
> > Wouldn't it be easier then not to duplicate from the beginning ?
> 
> Hi Michael,
> 
> I think defining a superclass to take care of the commonality of the two FEC
> controllers is unnecessary.
> 
> But yes, duplication is bad. To solve that, we would extract the real core of
> the two controllers: threshold usage in a util class.
> 
> WDYT?

Michael, the extraction Minyue has mentioned is done in that CL -
https://codereview.webrtc.org/2688613003/

[michaelt, 2017-03-22 11:04:46]

On 2017/03/22 10:52:38, minyue-webrtc wrote:
> On 2017/02/09 13:38:10, michaelt wrote:
> > Wouldn't it be easier then not to duplicate from the beginning ?
> 
> Hi Michael,
> 
> I think defining a superclass to take care of the commonality of the two FEC
> controllers is unnecessary.
> 
> But yes, duplication is bad. To solve that, we would extract the real core of
> the two controllers: threshold usage in a util class.
> 
> WDYT?

In general I would say composition over inheritance :)
So this is a good idea.

[minyue-webrtc, 2017-03-22 11:21:19]

lgtm

[elad.alon_webrtc.org, 2017-03-24 09:04:37]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-03-24 09:04:38]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2673543004/#ps100001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-03-24 09:04:42]

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-03-24 09:11:51]

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

[commit-bot: I haz the power, 2017-03-24 09:11:52]

Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24167)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/23273)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/20138)

[elad.alon_webrtc.org, 2017-03-24 09:19:26]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-03-24 09:19:27]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2673543004/#ps120001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-03-24 09:19:32]

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-03-24 09:26:41]

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

[commit-bot: I haz the power, 2017-03-24 09:26:42]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22734)
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)

[minyue-webrtc, 2017-03-24 09:26:49]

On 2017/03/24 09:19:32, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

I was careless on this one. Is FecController as a class defined twice?

[elad.alon_webrtc.org, 2017-03-24 09:27:39]

On 2017/03/24 09:26:49, minyue-webrtc wrote:
> On 2017/03/24 09:19:32, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> 
> I was careless on this one. Is FecController as a class defined twice?

Only if I had made a mistake while rebasing. Let me check.

[minyue-webrtc, 2017-03-24 09:53:53]

On 2017/03/24 09:27:39, elad.alon wrote:
> On 2017/03/24 09:26:49, minyue-webrtc wrote:
> > On 2017/03/24 09:19:32, commit-bot: I haz the power wrote:
> > > CQ is trying da patch. Follow status at
> > >  
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> > 
> > I was careless on this one. Is FecController as a class defined twice?
> 
> Only if I had made a mistake while rebasing. Let me check.

-lgtm
and I will take a look when it is ready again.

[elad.alon_webrtc.org, 2017-03-24 09:55:03]

On 2017/03/24 09:53:53, minyue-webrtc wrote:
> On 2017/03/24 09:27:39, elad.alon wrote:
> > On 2017/03/24 09:26:49, minyue-webrtc wrote:
> > > On 2017/03/24 09:19:32, commit-bot: I haz the power wrote:
> > > > CQ is trying da patch. Follow status at
> > > >  
> > > >
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> > > 
> > > I was careless on this one. Is FecController as a class defined twice?
> > 
> > Only if I had made a mistake while rebasing. Let me check.
> 
> -lgtm
> and I will take a look when it is ready again.

I think this was a false alarm. Nothing seems to have been messed up during
rebasing - everything is as is by design. Please re-read the CL's description.

[minyue-webrtc, 2017-03-24 10:10:03]

On 2017/03/24 09:55:03, elad.alon wrote:
> On 2017/03/24 09:53:53, minyue-webrtc wrote:
> > On 2017/03/24 09:27:39, elad.alon wrote:
> > > On 2017/03/24 09:26:49, minyue-webrtc wrote:
> > > > On 2017/03/24 09:19:32, commit-bot: I haz the power wrote:
> > > > > CQ is trying da patch. Follow status at
> > > > >  
> > > > >
> > > >
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> > > > 
> > > > I was careless on this one. Is FecController as a class defined twice?
> > > 
> > > Only if I had made a mistake while rebasing. Let me check.
> > 
> > -lgtm
> > and I will take a look when it is ready again.
> 
> I think this was a false alarm. Nothing seems to have been messed up during
> rebasing - everything is as is by design. Please re-read the CL's description.

I see, it works because the new file is not added to Build.gn.

A CL that is not really complete and needs significant follow-up changes is not
recommended. The best is that you just merge it into the next one, and abandon
this. It shouldn't be complicated, otherwise, I can take this as an exception.

[elad.alon_webrtc.org, 2017-03-24 10:35:03]

On 2017/03/24 10:10:03, minyue-webrtc wrote:
> On 2017/03/24 09:55:03, elad.alon wrote:
> > On 2017/03/24 09:53:53, minyue-webrtc wrote:
> > > On 2017/03/24 09:27:39, elad.alon wrote:
> > > > On 2017/03/24 09:26:49, minyue-webrtc wrote:
> > > > > On 2017/03/24 09:19:32, commit-bot: I haz the power wrote:
> > > > > > CQ is trying da patch. Follow status at
> > > > > >  
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> > > > > 
> > > > > I was careless on this one. Is FecController as a class defined twice?
> > > > 
> > > > Only if I had made a mistake while rebasing. Let me check.
> > > 
> > > -lgtm
> > > and I will take a look when it is ready again.
> > 
> > I think this was a false alarm. Nothing seems to have been messed up during
> > rebasing - everything is as is by design. Please re-read the CL's
description.
> 
> I see, it works because the new file is not added to Build.gn.
> 
> A CL that is not really complete and needs significant follow-up changes is
not
> recommended. The best is that you just merge it into the next one, and abandon
> this. It shouldn't be complicated, otherwise, I can take this as an exception.

Merged with https://codereview.webrtc.org/2672933003/.