Refactoring: Hide VideoCodec.codecSpecific as "private"

webrtc/common_types.h

Index: webrtc/common_types.h
diff --git a/webrtc/common_types.h b/webrtc/common_types.h
index 587b90d753ce3f3eb092dfd01edb8b1b68303719..0cb0f3ed780fa5cd3282e54ff78cac3ad291182e 100644
--- a/webrtc/common_types.h
+++ b/webrtc/common_types.h
@@ -18,6 +18,7 @@
 #include <string>
 #include <vector>
 
+#include "webrtc/base/checks.h"

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

Let's avoid including this header here.  DCHECKs are for implementation not
declarations.

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

Done.

 #include "webrtc/typedefs.h"
 
 #if defined(_MSC_VER)
@@ -682,7 +683,11 @@ enum VideoCodecMode {
 };
 
 // Common video codec properties
-struct VideoCodec {
+class VideoCodec {
+ public:
+  VideoCodec();
+
+  // Public variables. TODO(hta): Make them private with accessors.
   VideoCodecType      codecType;
   char                plName[kPayloadNameSize];
   unsigned char       plType;
@@ -697,8 +702,6 @@ struct VideoCodec {
 
   unsigned char       maxFramerate;
 
-  VideoCodecUnion     codecSpecific;
-
   unsigned int        qpMax;
   unsigned char       numberOfSimulcastStreams;
   SimulcastStream     simulcastStream[kMaxSimulcastStreams];
@@ -708,6 +711,40 @@ struct VideoCodec {
 
   bool operator==(const VideoCodec& other) const = delete;
   bool operator!=(const VideoCodec& other) const = delete;
+
+  // Accessors for codec specific information.
+  // There is a const version of each that returns a reference,
+  // and a non-const version that returns a pointer, in order
+  // to allow modification of the parameters.
+  VideoCodecVP8* VP8() {

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

move all of these to a source file

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

Done.

+    RTC_CHECK(codecType == kVideoCodecVP8);

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

I notice that some of these are CHECK while others are DCHECK.  I think they
should all be RTC_DCHECK_EQ

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

Done.

+    return &codecSpecific_.VP8;
+  }
+  const VideoCodecVP8& VP8() const {
+    RTC_CHECK(codecType == kVideoCodecVP8);
+    return codecSpecific_.VP8;
+  }
+  VideoCodecVP9* VP9() {
+    RTC_DCHECK(codecType == kVideoCodecVP9);
+    return &codecSpecific_.VP9;
+  }
+  const VideoCodecVP9& VP9() const {
+    RTC_DCHECK(codecType == kVideoCodecVP9);
+    return codecSpecific_.VP9;
+  }
+  VideoCodecH264* H264() {
+    RTC_DCHECK(codecType == kVideoCodecH264);
+    return &codecSpecific_.H264;
+  }
+  const VideoCodecH264& H264() const {
+    RTC_DCHECK(codecType == kVideoCodecH264);
+    return codecSpecific_.H264;
+  }
+
+ private:
+  // TODO(hta): Consider replacing the union with a pointer type.

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

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.

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

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.

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

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. :)

+  // This will allow removing the VideoCodec* types from this file.
+  VideoCodecUnion codecSpecific_;

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

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:11:31]

Done.

 };
 
 // Bandwidth over-use detector options.  These are used to drive