Refactoring: Hide VideoCodec.codecSpecific as "private"

Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=webrtc:6603
Committed: https://crrev.com/257dc39841c4d29e6a360f2dd10af336a45072a3
Cr-Commit-Position: refs/heads/master@{#14775}

[hta-webrtc, 2016-05-20 07:42:40]

Description was changed from

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=
==========

to

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=
==========

[hta-webrtc, 2016-05-20 07:42:40]

hta@webrtc.org changed reviewers:
+ pbos@webrtc.org

[hta-webrtc, 2016-05-20 07:43:03]

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

[hta-webrtc, 2016-05-20 07:43:45]

I decided to try something a lot less ambitious for a first piece of refactoring
in the video coding APIs.

WDYT?

[hta-webrtc, 2016-05-20 07:52:32]

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

[commit-bot: I haz the power, 2016-05-20 07:52:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/1

[commit-bot: I haz the power, 2016-05-20 07:57:05]

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

[commit-bot: I haz the power, 2016-05-20 07:57:06]

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

[hta-webrtc, 2016-05-20 08:29:47]

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

[commit-bot: I haz the power, 2016-05-20 08:29:56]

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

[commit-bot: I haz the power, 2016-05-20 08:38:57]

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

[commit-bot: I haz the power, 2016-05-20 08:38:58]

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

[hta-webrtc, 2016-05-20 08:52:34]

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

[commit-bot: I haz the power, 2016-05-20 08:52:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/40001

[commit-bot: I haz the power, 2016-05-20 09:26:56]

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

[commit-bot: I haz the power, 2016-05-20 09:26:57]

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

[hta-webrtc, 2016-05-20 10:30:45]

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

[commit-bot: I haz the power, 2016-05-20 10:30:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/60001

[commit-bot: I haz the power, 2016-05-20 10:38:51]

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

[commit-bot: I haz the power, 2016-05-20 10:38:52]

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

[hta-webrtc, 2016-05-20 11:17:22]

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

[commit-bot: I haz the power, 2016-05-20 11:17:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/80001

[commit-bot: I haz the power, 2016-05-20 11:20:42]

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

[commit-bot: I haz the power, 2016-05-20 11:20:43]

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

[tommi, 2016-05-20 11:34:55]

I think this change looks like a good thing in general, but I wouldn't spend too
much time on this since it's all going to change.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:65: memset(&simulcastStream, 0, sizeof(simulcastStream));
initialize these in the initializer list instead of using memset.  memset() can
have worse side effects if the type changes.  If you could also fix the memset()
above, then that would be good.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:21: #include "webrtc/base/checks.h"
Let's avoid including this header here.  DCHECKs are for implementation not
declarations.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:719: VideoCodecVP8* VP8() {
move all of these to a source file

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:720: RTC_CHECK(codecType == kVideoCodecVP8);
I notice that some of these are CHECK while others are DCHECK.  I think they
should all be RTC_DCHECK_EQ

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:745: // TODO(hta): Consider replacing the union with a
pointer type.
having an extra pointer might not be worth it.  I think we should discuss the
future of this class/struct with the video team. As I understand it, it might be
on the way out anyway.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:747: VideoCodecUnion codecSpecific_;
since you're changing the name according to the style guide, you might as well
go all the way: codec_specific_

[hta-webrtc, 2016-05-20 12:10:47]

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

[commit-bot: I haz the power, 2016-05-20 12:11:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/120001

[hta-webrtc, 2016-05-20 12:11:32]

Addressed all comments except use of memset.
Queried about array initialization.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:53: plName(""),
BTW, plName is a 16-character array. The Android compiler barfed on this
initialization, while the others accepted it - advice?

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:65: memset(&simulcastStream, 0, sizeof(simulcastStream));
On 2016/05/20 11:34:55, tommi-webrtc wrote:
> initialize these in the initializer list instead of using memset.  memset()
can
> have worse side effects if the type changes.  If you could also fix the
memset()
> above, then that would be good.

All of these are arrays. Is there a good way to initialize arrays with an
initializer?

(I'd like to make them all have constructors that zeroed them, but that's too
many changes for one little CL.)

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:21: #include "webrtc/base/checks.h"
On 2016/05/20 11:34:55, tommi-webrtc wrote:
> Let's avoid including this header here.  DCHECKs are for implementation not
> declarations.

Done.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:719: VideoCodecVP8* VP8() {
On 2016/05/20 11:34:55, tommi-webrtc wrote:
> move all of these to a source file

Done.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:720: RTC_CHECK(codecType == kVideoCodecVP8);
On 2016/05/20 11:34:55, tommi-webrtc wrote:
> I notice that some of these are CHECK while others are DCHECK.  I think they
> should all be RTC_DCHECK_EQ

Done.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:745: // TODO(hta): Consider replacing the union with a
pointer type.
On 2016/05/20 11:34:55, tommi-webrtc wrote:
> having an extra pointer might not be worth it.  I think we should discuss the
> future of this class/struct with the video team. As I understand it, it might
be
> on the way out anyway.

Acknowledged.

Although I remember 2 years ago that people said the rtp_rtcp module would be
refactored Real Soon Now, so I shouldn't worry about it - it's still there.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:747: VideoCodecUnion codecSpecific_;
On 2016/05/20 11:34:55, tommi-webrtc wrote:
> since you're changing the name according to the style guide, you might as well
> go all the way: codec_specific_

Done.

[pbos-webrtc, 2016-05-20 12:26:24]

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:745: // TODO(hta): Consider replacing the union with a
pointer type.
On 2016/05/20 12:11:31, hta-webrtc wrote:
> On 2016/05/20 11:34:55, tommi-webrtc wrote:
> > having an extra pointer might not be worth it.  I think we should discuss
the
> > future of this class/struct with the video team. As I understand it, it
might
> be
> > on the way out anyway.
> 
> Acknowledged.
> 
> Although I remember 2 years ago that people said the rtp_rtcp module would be
> refactored Real Soon Now, so I shouldn't worry about it - it's still there.

If that's the case, then I think it makes more sense to assist with the
transition than fixing up the old stuff. There's some related refactoring here:

https://bugs.chromium.org/p/webrtc/issues/detail?id=3424

I think we need a way to get rid of the void* in
VideoEncoderConfig::encoder_specific_settings, to handle ownership more properly
(it needs to be posted between threads and remain non-blocking). I think
refactoring in this area and helping deprecate VideoCodec is time better spent.

This struct is a non-pod that doesn't yet have any unions, so if we get rid of
VideoCodec that'll also achieve your goal. :)

[commit-bot: I haz the power, 2016-05-20 14:11:40]

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

[commit-bot: I haz the power, 2016-05-20 14:11:41]

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

[hta-webrtc, 2016-05-20 14:54:18]

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

[commit-bot: I haz the power, 2016-05-20 14:54:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/120001

[commit-bot: I haz the power, 2016-05-20 16:54:52]

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

[commit-bot: I haz the power, 2016-05-20 16:54:53]

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

[tommi, 2016-05-21 11:17:08]

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:53: plName(""),
On 2016/05/20 12:11:31, hta-webrtc wrote:
> BTW, plName is a 16-character array. The Android compiler barfed on this
> initialization, while the others accepted it - advice?

try plName(),

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:65: memset(&simulcastStream, 0, sizeof(simulcastStream));
On 2016/05/20 12:11:31, hta-webrtc wrote:
> On 2016/05/20 11:34:55, tommi-webrtc wrote:
> > initialize these in the initializer list instead of using memset.  memset()
> can
> > have worse side effects if the type changes.  If you could also fix the
> memset()
> > above, then that would be good.
> 
> All of these are arrays. Is there a good way to initialize arrays with an
> initializer?
> 
> (I'd like to make them all have constructors that zeroed them, but that's too
> many changes for one little CL.)

Same here. Try:

simulcastStream(),
spatialLayers(),
codecSpecific(),

in the initializer list

[hta-webrtc, 2016-05-23 13:25:32]

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

[commit-bot: I haz the power, 2016-05-23 13:25:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/140001

[hta-webrtc, 2016-05-23 13:30:16]

Now running the CQ to see if breakage can be observed. I guess that if there is,
we'll have to have more explicit constructors.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:53: plName(""),
On 2016/05/21 11:17:08, tommi-webrtc wrote:
> On 2016/05/20 12:11:31, hta-webrtc wrote:
> > BTW, plName is a 16-character array. The Android compiler barfed on this
> > initialization, while the others accepted it - advice?
> 
> try plName(),

Done.

https://codereview.webrtc.org/2001533003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:65: memset(&simulcastStream, 0, sizeof(simulcastStream));
On 2016/05/21 11:17:07, tommi-webrtc wrote:
> On 2016/05/20 12:11:31, hta-webrtc wrote:
> > On 2016/05/20 11:34:55, tommi-webrtc wrote:
> > > initialize these in the initializer list instead of using memset. 
memset()
> > can
> > > have worse side effects if the type changes.  If you could also fix the
> > memset()
> > > above, then that would be good.
> > 
> > All of these are arrays. Is there a good way to initialize arrays with an
> > initializer?
> > 
> > (I'd like to make them all have constructors that zeroed them, but that's
too
> > many changes for one little CL.)
> 
> Same here. Try:
> 
> simulcastStream(),
> spatialLayers(),
> codecSpecific(),
> 
> in the initializer list

Done.

[commit-bot: I haz the power, 2016-05-23 15:26:18]

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

[commit-bot: I haz the power, 2016-05-23 15:26:21]

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

[tommi, 2016-05-23 17:05:09]

lgtm with a couple of nits

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.cc#n...
webrtc/common_types.cc:49: memset(&arrOfCSRCs, 0, sizeof(arrOfCSRCs));
can we get rid of this memset as well?

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.h#ne...
webrtc/common_types.h:689: // Public variables. TODO(hta): Make them private
with accessors.
nit: TODO on a line of its own

[hta-webrtc, 2016-05-23 17:19:33]

Nits addressed.

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.cc#n...
webrtc/common_types.cc:49: memset(&arrOfCSRCs, 0, sizeof(arrOfCSRCs));
On 2016/05/23 17:05:08, tommi-webrtc wrote:
> can we get rid of this memset as well?

If the tests still pass....

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2001533003/diff/140001/webrtc/common_types.h#ne...
webrtc/common_types.h:689: // Public variables. TODO(hta): Make them private
with accessors.
On 2016/05/23 17:05:09, tommi-webrtc wrote:
> nit: TODO on a line of its own

Done.

[hta-webrtc, 2016-05-23 17:20:01]

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

[commit-bot: I haz the power, 2016-05-23 17:20:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001533003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001533003/160001

[commit-bot: I haz the power, 2016-05-23 19:20:56]

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

[commit-bot: I haz the power, 2016-05-23 19:20:57]

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

[tommi, 2016-05-27 13:18:50]

pbos - can we land this as is or is there something more that remains to do?

[hta-webrtc, 2016-10-25 13:00:17]

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

[commit-bot: I haz the power, 2016-10-25 13:00:25]

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-10-25 13:03:15]

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

[commit-bot: I haz the power, 2016-10-25 13:03:16]

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

[hta-webrtc, 2016-10-25 13:16:34]

Description was changed from

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=
==========

to

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=webrtc:6603
==========

[hta-webrtc, 2016-10-25 13:22:47]

hta@webrtc.org changed reviewers:
+ magjed@webrtc.org
- pbos@webrtc.org

[magjed_webrtc, 2016-10-25 15:24:04]

lgtm. I think this is a good first step and at least we get runtime type checks.

[hta-webrtc, 2016-10-25 16:03:38]

The CQ bit was checked by hta@webrtc.org

[hta-webrtc, 2016-10-25 16:03:38]

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

[commit-bot: I haz the power, 2016-10-25 16:03:48]

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-10-25 16:05:09]

Description was changed from

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=webrtc:6603
==========

to

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=webrtc:6603
==========

[commit-bot: I haz the power, 2016-10-25 16:05:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-25 16:14:25]

Description was changed from

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=webrtc:6603
==========

to

==========
Refactoring: Hide VideoCodec.codecSpecific as "private"

This refactoring allows runtime checks that functions that access
codec specific information are using the correct union member.
The API also allows replacing the union with another implementation
without changes at calling sites.

BUG=webrtc:6603

Committed: https://crrev.com/257dc39841c4d29e6a360f2dd10af336a45072a3
Cr-Commit-Position: refs/heads/master@{#14775}
==========

[commit-bot: I haz the power, 2016-10-25 16:14:27]

Patchset 11 (id:??) landed as
https://crrev.com/257dc39841c4d29e6a360f2dd10af336a45072a3
Cr-Commit-Position: refs/heads/master@{#14775}