Remove OverUseDetectorOptions from OveruseDetector since it isn't used.

Remove OverUseDetectorOptions from OveruseDetector since it isn't used.

BUG=None

Review-Url: https://codereview.webrtc.org/2580733004
Cr-Commit-Position: refs/heads/master@{#15809}
Committed: https://chromium.googlesource.com/external/webrtc/+/84f83f8c0c7d877249ebec676c60e8a738689874

[terelius, 2016-12-15 15:10:59]

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

[nisse-webrtc, 2016-12-16 08:59:21]

On 2016/12/15 15:11:00, terelius wrote:

There's some piece missing here. It looks like the options are used in the
constructor of OveruseEstimator, see

https://cs.chromium.org/chromium/src/third_party/webrtc/modules/remote_bitrat...

If you remove the configurability, you need to move the constants here, and give
them some appropriate name.

It might make sense to group slope_, offset_, prev_offset_, avg_noise_,
var_noise_, process_noise_, and E_ into a struct, and attach initial values to
that struct? Which is more or less a move and rename of OverUseDetectorOptions.

[terelius, 2016-12-16 09:41:03]

On 2016/12/16 08:59:21, nisse-webrtc wrote:
> On 2016/12/15 15:11:00, terelius wrote:
> 
> There's some piece missing here. It looks like the options are used in the
> constructor of OveruseEstimator, see
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/remote_bitrat...
> 
> If you remove the configurability, you need to move the constants here, and
give
> them some appropriate name.
> 
> It might make sense to group slope_, offset_, prev_offset_, avg_noise_,
> var_noise_, process_noise_, and E_ into a struct, and attach initial values to
> that struct? Which is more or less a move and rename of
OverUseDetectorOptions.

Yes, see comment on lines 700-702 in common_types.h

I'm not removing any configurability. Just removing some parameters that aren't
used anyway.

[nisse-webrtc, 2016-12-16 11:38:08]

On 2016/12/16 09:41:03, terelius wrote:
> On 2016/12/16 08:59:21, nisse-webrtc wrote:
> > On 2016/12/15 15:11:00, terelius wrote:
> > 
> > There's some piece missing here. It looks like the options are used in the
> > constructor of OveruseEstimator, see
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/remote_bitrat...
> > 
> > If you remove the configurability, you need to move the constants here, and
> give
> > them some appropriate name.
> > 
> > It might make sense to group slope_, offset_, prev_offset_, avg_noise_,
> > var_noise_, process_noise_, and E_ into a struct, and attach initial values
to
> > that struct? Which is more or less a move and rename of
> OverUseDetectorOptions.
> 
> Yes, see comment on lines 700-702 in common_types.h
> 
> I'm not removing any configurability. Just removing some parameters that
aren't
> used anyway.

LGTM, then. But it would be even better to delete the type from common_types.h. 

It's not in the public api unless we think we want to expose the internals of 
remote_bitrate_estimator, and I've done an initial search 
and I don't think it's used outside of webrtc.

[terelius, 2016-12-16 11:46:40]

On 2016/12/16 11:38:08, nisse-webrtc wrote:
> On 2016/12/16 09:41:03, terelius wrote:
> > On 2016/12/16 08:59:21, nisse-webrtc wrote:
> > > On 2016/12/15 15:11:00, terelius wrote:
> > > 
> > > There's some piece missing here. It looks like the options are used in the
> > > constructor of OveruseEstimator, see
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/remote_bitrat...
> > > 
> > > If you remove the configurability, you need to move the constants here,
and
> > give
> > > them some appropriate name.
> > > 
> > > It might make sense to group slope_, offset_, prev_offset_, avg_noise_,
> > > var_noise_, process_noise_, and E_ into a struct, and attach initial
values
> to
> > > that struct? Which is more or less a move and rename of
> > OverUseDetectorOptions.
> > 
> > Yes, see comment on lines 700-702 in common_types.h
> > 
> > I'm not removing any configurability. Just removing some parameters that
> aren't
> > used anyway.
> 
> LGTM, then. But it would be even better to delete the type from
common_types.h. 
> 
> It's not in the public api unless we think we want to expose the internals of 
> remote_bitrate_estimator, and I've done an initial search 
> and I don't think it's used outside of webrtc.

Agreed, but let's do that in a follow up CL.

[terelius, 2016-12-20 15:42:44]

ping

[stefan-webrtc, 2016-12-22 09:37:25]

lgtm

https://codereview.webrtc.org/2580733004/diff/20001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2580733004/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:702: // class and delete this? See also disabled warning
at line 27
I would be happy to get rid of this struct. I've been close several times
before... :)

[terelius, 2016-12-22 20:07:51]

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

[commit-bot: I haz the power, 2016-12-22 20:08:03]

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, 2016-12-22 20:23:51]

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

[commit-bot: I haz the power, 2016-12-22 20:23:52]

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

[terelius, 2016-12-27 15:52:55]

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

[commit-bot: I haz the power, 2016-12-27 15:52:59]

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, 2016-12-27 16:51:40]

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

[commit-bot: I haz the power, 2016-12-27 16:51:41]

Dry run: This issue passed the CQ dry run.

[terelius, 2016-12-27 18:40:36]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2016-12-27 18:40:43]

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-12-27 18:43:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482864036024630,
"parent_rev": "0cd111a2ed4e8639029111f358cbf4d2769768d8", "commit_rev":
"84f83f8c0c7d877249ebec676c60e8a738689874"}

[commit-bot: I haz the power, 2016-12-27 18:43:05]

Description was changed from

==========
Remove OverUseDetectorOptions from OveruseDetector since it isn't used.

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

to

==========
Remove OverUseDetectorOptions from OveruseDetector since it isn't used.

BUG=None

Review-Url: https://codereview.webrtc.org/2580733004
Cr-Commit-Position: refs/heads/master@{#15809}
Committed:
https://chromium.googlesource.com/external/webrtc/+/84f83f8c0c7d877249ebec676...
==========

[commit-bot: I haz the power, 2016-12-27 18:43:06]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/84f83f8c0c7d877249ebec676...