Refactoring: Hide VideoCodec.codecSpecific as "private"

webrtc/common_types.h

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_COMMON_TYPES_H_
 #define WEBRTC_COMMON_TYPES_H_
 
 #include <assert.h>
 #include <stddef.h>
 #include <string.h>
 
 #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)
 // Disable "new behavior: elements of array will be default initialized"
 // warning. Affects OverUseDetectorOptions.
 #pragma warning(disable:4351)
 #endif
 
 #ifdef WEBRTC_EXPORT
 #define WEBRTC_DLLEXPORT _declspec(dllexport)
@@ skipping 644 matching lines @@
   int target_bitrate_bps;
   // TODO(ivica): Add max_quantizer and min_quantizer?
 };
 
 enum VideoCodecMode {
   kRealtimeVideo,
   kScreensharing
 };
 
 // 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;
 
   unsigned short      width;
   unsigned short      height;
 
   unsigned int        startBitrate;  // kilobits/sec.
   unsigned int        maxBitrate;  // kilobits/sec.
   unsigned int        minBitrate;  // kilobits/sec.
   unsigned int        targetBitrate;  // kilobits/sec.
 
   unsigned char       maxFramerate;
 
-  VideoCodecUnion     codecSpecific;
-
   unsigned int        qpMax;
   unsigned char       numberOfSimulcastStreams;
   SimulcastStream     simulcastStream[kMaxSimulcastStreams];
   SpatialLayer spatialLayers[kMaxSpatialLayers];
 
   VideoCodecMode      mode;
 
   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
 // experimentation with bandwidth estimation parameters.
 // See modules/remote_bitrate_estimator/overuse_detector.h
 struct OverUseDetectorOptions {
   OverUseDetectorOptions()
       : initial_slope(8.0/512.0),
         initial_offset(0),
         initial_e(),
@@ skipping 172 matching lines @@
 enum class RtcpMode { kOff, kCompound, kReducedSize };
 
 enum NetworkState {
   kNetworkUp,
   kNetworkDown,
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_COMMON_TYPES_H_