Add proper lifetime of encoder-specific settings.

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=mflodman@webrtc.org, stefan@webrtc.org

[pbos-webrtc, 2016-06-06 20:24:58]

typo

[pbos-webrtc, 2016-06-06 20:27:05]

PTAL, this is required to get rid of webrtc::VideoCodec and VideoCodecUnion (for
encoders).

Aiming to have VideoEncoder have a default implementation of InitEncode that
converts from VideoEncoderConfig to VideoCodec until all encoder implementations
use VideoEncoderConfig directly, then VideoCodec can be removed from the encoder
side.

[pbos-webrtc, 2016-06-06 20:27:13]

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

[commit-bot: I haz the power, 2016-06-06 20:27:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047513002/20001

[commit-bot: I haz the power, 2016-06-06 21:31:31]

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

[commit-bot: I haz the power, 2016-06-06 21:31:32]

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

[mflodman, 2016-06-16 09:05:48]

Sorry, had these comments as drafts for several days :(

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

https://codereview.webrtc.org/2047513002/diff/20001/webrtc/config.h#newcode125
webrtc/config.h:125: class EncoderSpecificSettings : public
rtc::RefCountInterface {
Add comment why ref counted and possible next steps, as we discussed.

https://codereview.webrtc.org/2047513002/diff/20001/webrtc/config.h#newcode142
webrtc/config.h:142: VideoCodecH264* vp8_settings) const override;
s/vp8/h264

https://codereview.webrtc.org/2047513002/diff/20001/webrtc/config.h#newcode160
webrtc/config.h:160: virtual void FillVideoCodecVp9(VideoCodecVP9* vp8_settings)
const override;
s/vp8/vp9

[pbos-webrtc, 2016-07-13 08:43:44]

feedback: comment and rename parameters

[pbos-webrtc, 2016-07-13 08:44:12]

Slow reply, PTAL.

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

https://codereview.webrtc.org/2047513002/diff/20001/webrtc/config.h#newcode125
webrtc/config.h:125: class EncoderSpecificSettings : public
rtc::RefCountInterface {
On 2016/06/16 09:05:48, mflodman_OOO_July_18 wrote:
> Add comment why ref counted and possible next steps, as we discussed.

Done.

https://codereview.webrtc.org/2047513002/diff/20001/webrtc/config.h#newcode142
webrtc/config.h:142: VideoCodecH264* vp8_settings) const override;
On 2016/06/16 09:05:48, mflodman_OOO_July_18 wrote:
> s/vp8/h264

Done.

https://codereview.webrtc.org/2047513002/diff/20001/webrtc/config.h#newcode160
webrtc/config.h:160: virtual void FillVideoCodecVp9(VideoCodecVP9* vp8_settings)
const override;
On 2016/06/16 09:05:48, mflodman_OOO_July_18 wrote:
> s/vp8/vp9

Done.

[pbos-webrtc, 2016-07-13 08:44:18]

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

[commit-bot: I haz the power, 2016-07-13 08:44:28]

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-07-13 09:21:16]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 09:21:17]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2016-08-01 19:24:09]

ping

[mflodman, 2016-08-04 13:53:24]

Sorry for the post-vacation delay.

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.cc#newcode170
webrtc/config.cc:170: VideoCodecVP8* vp9_settings) const {
s/vp9_settings/vp8_settings

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.cc#newcode175
webrtc/config.cc:175: VideoCodecVP9* vp8_settings) const {
s/vp8_settings/vp9_settings

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

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.h#newcode133
webrtc/config.h:133: //
Remove empty comment line.

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.h#newcode140
webrtc/config.h:140: void FillEncoderSpecificSettings(VideoCodec* codec_struct)
const;
Can this be statically implemented and the fillers below be pure virtual
instead? To avoid the base implementation with all RTC_NOT_REACHED().

[pbos-webrtc, 2016-08-08 20:42:22]

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.cc#newcode170
webrtc/config.cc:170: VideoCodecVP8* vp9_settings) const {
On 2016/08/04 13:53:23, mflodman wrote:
> s/vp9_settings/vp8_settings

Done.

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.cc#newcode175
webrtc/config.cc:175: VideoCodecVP9* vp8_settings) const {
On 2016/08/04 13:53:23, mflodman wrote:
> s/vp8_settings/vp9_settings

Done.

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

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.h#newcode133
webrtc/config.h:133: //
On 2016/08/04 13:53:24, mflodman wrote:
> Remove empty comment line.

Done.

https://codereview.webrtc.org/2047513002/diff/40001/webrtc/config.h#newcode140
webrtc/config.h:140: void FillEncoderSpecificSettings(VideoCodec* codec_struct)
const;
On 2016/08/04 13:53:23, mflodman wrote:
> Can this be statically implemented and the fillers below be pure virtual
> instead? To avoid the base implementation with all RTC_NOT_REACHED().

I have the base implementation here to avoid code duplication. Do you prefer
pure virtuals with all implementations having to add their own implementations
with RTC_NOTREACHED()?

How do you mean that the static implementation should look?

[pbos-webrtc, 2016-08-08 20:42:24]

feedback

[perkj_webrtc, 2016-09-06 09:42:20]

perkj@webrtc.org changed reviewers:
+ kthelgason@webrtc.org, perkj@webrtc.org

[perkj_webrtc, 2016-09-06 09:42:20]

I think this should be landed. 
Ping Magnus and Stefan!

pbos- do you want to continue working on this or should we kindly ask
kthelgason?

[pbos-webrtc, 2016-09-06 21:22:41]

On 2016/09/06 09:42:20, perkj_webrtc wrote:
> I think this should be landed. 
> Ping Magnus and Stefan!
> 
> pbos- do you want to continue working on this or should we kindly ask
> kthelgason?

Sure I can finish this off, I'll rebase once this is reviewed to avoid excessive
rebasing. :)

[kthelgason, 2016-09-07 07:16:46]

On 2016/09/06 21:22:41, pbos-webrtc wrote:
> Sure I can finish this off, I'll rebase once this is reviewed to avoid
excessive
> rebasing. :)

I think this is a clear improvement over the void* passing, though it does
require a bit of type system boilerplate. My first intuition when I saw this was
to just turn the encoder-specific settings into a dictionary of strings.
This would have some dynamism benefits, for example that encoders can decide to
ignore a property or add new property without all clients having to know about
it and be updated accordingly. It would also simplify the code a bit. I realise
however that static checking is lost in that case so it may not be worth it. 

In any case, I just wanted to get your thoughts, and I'm totally on board with
this CL, I think it's a big win.

[perkj_webrtc, 2016-09-14 09:51:31]

Magnus, pbos- pretty ping!.

I think this can be landed as is and we later address my comment unless pbos
wants to do it now.

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

https://codereview.webrtc.org/2047513002/diff/60001/webrtc/config.h#newcode134
webrtc/config.h:134: class EncoderSpecificSettings : public
rtc::RefCountInterface {
I really think this should be landed. But I think specifics_ must be copyable
and encoder_specific_settings should be stored in a std::unique_ptr<>. No need
for ref counting as far as I can see. Note that VideoEncoderConfig has a copy
method that needs to be updated.

[pbos-webrtc, 2016-09-14 15:37:33]

On 2016/09/14 09:51:31, perkj_webrtc wrote:
> Magnus, pbos- pretty ping!.
> 
> I think this can be landed as is and we later address my comment unless pbos
> wants to do it now.
> 
> https://codereview.webrtc.org/2047513002/diff/60001/webrtc/config.h
> File webrtc/config.h (right):
> 
> https://codereview.webrtc.org/2047513002/diff/60001/webrtc/config.h#newcode134
> webrtc/config.h:134: class EncoderSpecificSettings : public
> rtc::RefCountInterface {
> I really think this should be landed. But I think specifics_ must be copyable
> and encoder_specific_settings should be stored in a std::unique_ptr<>. No need
> for ref counting as far as I can see. Note that VideoEncoderConfig has a copy
> method that needs to be updated.

Kari has agreed to take this over and push for a faster review (thanks!). I'll
remove this review when he has a CL up, which should be tomorrow or Friday.

[pbos-webrtc, 2016-09-16 18:53:01]

On 2016/09/14 15:37:33, pbos-webrtc wrote:
> On 2016/09/14 09:51:31, perkj_webrtc wrote:
> > Magnus, pbos- pretty ping!.
> > 
> > I think this can be landed as is and we later address my comment unless pbos
> > wants to do it now.
> > 
> > https://codereview.webrtc.org/2047513002/diff/60001/webrtc/config.h
> > File webrtc/config.h (right):
> > 
> >
https://codereview.webrtc.org/2047513002/diff/60001/webrtc/config.h#newcode134
> > webrtc/config.h:134: class EncoderSpecificSettings : public
> > rtc::RefCountInterface {
> > I really think this should be landed. But I think specifics_ must be
copyable
> > and encoder_specific_settings should be stored in a std::unique_ptr<>. No
need
> > for ref counting as far as I can see. Note that VideoEncoderConfig has a
copy
> > method that needs to be updated.
> 
> Kari has agreed to take this over and push for a faster review (thanks!). I'll
> remove this review when he has a CL up, which should be tomorrow or Friday.

Superseded by https://codereview.webrtc.org/2347843002/.