Add proper lifetime of encoder-specific settings

This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
TBR=mflodman@webrtc.org

Committed: https://crrev.com/29a44e351e1e35c29dd5ff650f62cbc4d10b3b1b
Cr-Commit-Position: refs/heads/master@{#14396}

[pbos-webrtc, 2016-09-16 18:52:35]

lgtm, but I think someone else should look/approve as well since I wrote most of
it :)

thanks for taking over

https://codereview.webrtc.org/2347843002/diff/1/webrtc/modules/audio_coding/B...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2347843002/diff/1/webrtc/modules/audio_coding/B...
webrtc/modules/audio_coding/BUILD.gn:1290: "../../base:rtc_base_approved",
Is this related?

https://codereview.webrtc.org/2347843002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2347843002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream_tests.cc:1754: encoder_settings_.frameDroppingOn
= true;
I suspect you need to reconfigure encoder_specific_settings in encoder_settings_
here, since it doesn't have a stale reference to encoder_settings_.

[perkj_webrtc, 2016-09-19 08:38:31]

lgtm

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/config.h#newcode132
webrtc/config.h:132: // TODO(pbos): Consider removing the need for copying
VideoEncoderConfig and
no todo pbos please....

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:1290: "../../base:rtc_base_approved",
is this needed?

[kthelgason, 2016-09-19 09:15:41]

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/config.h#newcode132
webrtc/config.h:132: // TODO(pbos): Consider removing the need for copying
VideoEncoderConfig and
On 2016/09/19 08:38:30, perkj_webrtc wrote:
> no todo pbos please....

Acknowledged.

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2347843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:1290: "../../base:rtc_base_approved",
On 2016/09/19 08:38:31, perkj_webrtc wrote:
> is this needed?

Yup, this CL adds `webrtc/base/checks.h` to`"webrtc/config.cc` so we need to
make sure to compile that with targets that include `webrtc/config.cc`. Perhaps
there is a more appropriate place for that but this was the "simplest thing that
would possibly work"

[kthelgason, 2016-09-19 11:06:17]

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

[commit-bot: I haz the power, 2016-09-19 11:06: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, 2016-09-19 13:06:53]

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

[commit-bot: I haz the power, 2016-09-19 13:06:54]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kthelgason, 2016-09-20 06:29:14]

Updated. Perhaps you'd like to take one last look before handing this off to CQ
@perkj?

[perkj_webrtc, 2016-09-20 08:39:42]

still lgtm but ...

https://codereview.webrtc.org/2347843002/diff/40001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2347843002/diff/40001/webrtc/config.h#newcode123
webrtc/config.h:123: // TODO(kthelgason): Apart from a special case for
two-layer screencast these
It was not this TODO I meant but I guess its great if you fix many of pbos todos
:-> ... I meant the todo you are actually adding in this cl.

https://codereview.webrtc.org/2347843002/diff/40001/webrtc/config.h#newcode132
webrtc/config.h:132: // TODO(pbos): Consider removing the need for copying
VideoEncoderConfig and
this todo.

[kthelgason, 2016-09-20 13:44:05]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2016-09-20 13:44:05]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2347843002/#ps60001 (title:
"Change TODO owner")

[commit-bot: I haz the power, 2016-09-20 13:44:12]

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-09-20 14:09:21]

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

[commit-bot: I haz the power, 2016-09-20 14:09:23]

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

[kthelgason, 2016-09-20 19:32:46]

kthelgason@webrtc.org changed reviewers:
+ tommi@webrtc.org

[kthelgason, 2016-09-20 19:32:47]

Adding tommi@ as a reviewer as he owns webrtc/config.{cc,h}

[kthelgason, 2016-09-21 10:41:55]

kthelgason@webrtc.org changed reviewers:
+ mflodman@webrtc.org
- tommi@webrtc.org

[tommi, 2016-09-21 13:20:57]

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

[tommi, 2016-09-21 13:20:58]

https://codereview.webrtc.org/2347843002/diff/60001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2347843002/diff/60001/webrtc/config.h#newcode136
webrtc/config.h:136: virtual ~EncoderSpecificSettings() {}
for reference counted classes it's preferred to have the dtor private and friend
the refcounted type that's allowed to delete the instance.

[kthelgason, 2016-09-21 16:53:09]

https://codereview.chromium.org/2347843002/diff/60001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.chromium.org/2347843002/diff/60001/webrtc/config.h#newcode136
webrtc/config.h:136: virtual ~EncoderSpecificSettings() {}
On 2016/09/21 13:20:57, tommi (webrtc) wrote:
> for reference counted classes it's preferred to have the dtor private and
friend
> the refcounted type that's allowed to delete the instance.

Done.

[tommi (sloooow) - chröme, 2016-09-21 18:36:26]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2016-09-21 18:36:27]

lgtm % one nit

https://codereview.chromium.org/2347843002/diff/80001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.chromium.org/2347843002/diff/80001/webrtc/config.h#newcode144
webrtc/config.h:144: friend struct VideoEncoderConfig;
move below "private:"

[kthelgason, 2016-09-27 08:57:00]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2016-09-27 08:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
perkj@webrtc.org, tommi@chromium.org
Link to the patchset: https://codereview.webrtc.org/2347843002/#ps100001 (title:
"nitpicks")

[commit-bot: I haz the power, 2016-09-27 08:57:08]

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-09-27 09:28:03]

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

[commit-bot: I haz the power, 2016-09-27 09:28:04]

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

[kthelgason, 2016-09-27 10:00:42]

Description was changed from

==========
This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
==========

to

==========
This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
TBR=mflodman@webrtc.org
==========

[kthelgason, 2016-09-27 10:43:05]

The CQ bit was checked by kthelgason@webrtc.org

[commit-bot: I haz the power, 2016-09-27 10:43:09]

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-09-27 10:52:04]

Description was changed from

==========
This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
TBR=mflodman@webrtc.org
==========

to

==========
This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
TBR=mflodman@webrtc.org
==========

[commit-bot: I haz the power, 2016-09-27 10:52:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-27 10:52:08]

Description was changed from

==========
This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
TBR=mflodman@webrtc.org
==========

to

==========
This is a resubmission of https://codereview.webrtc.org/2047513002/

Original description:
Add proper lifetime of encoder-specific settings.

Permits passing VideoEncoderConfig between threads and not worry about
the lifetime of an underlying void pointer. Also adds type safety to
unpacking of codec-specific settings.

These settings are not yet propagating to VideoEncoder interfaces, but
the aim is to get rid of webrtc::VideoCodec for VideoEncoder.

BUG=webrtc:3424
R=perkj@webrtc.org, pbos@webrtc.org
TBR=mflodman@webrtc.org

Committed: https://crrev.com/29a44e351e1e35c29dd5ff650f62cbc4d10b3b1b
Cr-Commit-Position: refs/heads/master@{#14396}
==========

[commit-bot: I haz the power, 2016-09-27 10:52:09]

Patchset 6 (id:??) landed as
https://crrev.com/29a44e351e1e35c29dd5ff650f62cbc4d10b3b1b
Cr-Commit-Position: refs/heads/master@{#14396}