Piggybacking simulcast id and ALR experiment id into video content type extension.

webrtc/api/video/video_content_type.h

Index: webrtc/api/video/video_content_type.h
diff --git a/webrtc/api/video/video_content_type.h b/webrtc/api/video/video_content_type.h
index 5c468c079d35ea2d20c115234cf38d7e7fcd42bd..4afdd279478ec3f3f7fb10de6bcd68d8c2733b94 100644
--- a/webrtc/api/video/video_content_type.h
+++ b/webrtc/api/video/video_content_type.h
@@ -15,10 +15,33 @@
 
 namespace webrtc {
 
-enum class VideoContentType : uint8_t {
-  UNSPECIFIED = 0,
-  SCREENSHARE = 1,
-  TOTAL_CONTENT_TYPES  // Must be the last value in the enum.
+// Class designed to behave like enum to be included in RTP header structs.
+struct VideoContentType {
+  // Common content types used outside of experiments.
+  static const VideoContentType UNSPECIFIED;

[tommi, 2017/08/29 09:25:45]

this isn't an enum, so should be kUnspecified.

actually, why isn't it an enum? (looks like it was before)

[ilnik, 2017/08/29 10:26:14]

It's not enum, because there're 64 different combinations of content type,
simulcast id and experiment id now mixed together. So this is now a class
mimicking an enum. Extra-options agnostic clients could just use it as an enum
class.

[kwiberg-webrtc, 2017/08/29 11:44:28]

Even with many combinations, it should be convenient to make it an enum class,
if you overload | and & and/or provide convenience functions:

  enum class VideoContentType : uint8_t {
    kSomeFlag = 1,
    kSomeOtherFlag = 2,
    kYetAnotherFlag = 4,
  };

  VideoContentType operator|(VideoContentType a,
                             VideoContentType b) {
    return static_cast<VideoContentType>(
      static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
  }

  VideoContentType SetSomeOtherFlag(VideoContentType a) {
    return a | VideoContentType::kSomeOtherFlag;
  }

You get free functions rather than methods, but I'd say that's an OK tradeoff.

[ilnik, 2017/08/29 12:01:57]

Can you elaborate more, how do I, for example, combine two 4-bit values in enum?
Do I define some dummy enum value equal to 255 to fix uint8_t as underlying type
and then simply use bit arithmetic to set values as I want? Is it safe to save
non-listed values in the enum?

[kwiberg-webrtc, 2017/08/29 12:12:45]

http://en.cppreference.com/w/cpp/language/enum says:

  "Values of integer, floating-point, and other enumeration types can be
converted, such as by static_cast, to any enumeration type. The result is
unspecified if the value, converted to the enumeration's underlying type, is out
of this enumeration's range. If the underlying type is fixed, the range is the
range of the underlying type. If the underlying type is not fixed, the range is
all values possible for the smallest bit field large enough to hold all
enumerators of the target enumeration. Note that the value after such conversion
may not necessarily equal any of the named enumerators defined for the type."

So by fixing the range to uint8_t, as I did in the example in my previous
comment, we ensure that any integer in the range 0-255 is valid.

[ilnik, 2017/08/29 12:41:21]

Thank you! That was misconception on my side. Please take a look at updated
version.

+  static const VideoContentType SCREENSHARE;
+
+  operator uint8_t() const;
+
+  // No conversion constructor because otherwise this struct and many structs
+  // it's included into will be considered 'complex' and many chromium style
+  // errors will be generated.
+
+  uint8_t operator=(uint8_t value);
+  bool operator==(const VideoContentType& other);
+  bool operator==(uint8_t value);
+
+  bool SetExperimentId(uint8_t experiment_id);
+  bool SetSimulcastId(uint8_t simulcast_id);
+
+  uint8_t GetExperimentId() const;
+  uint8_t GetSimulcastId() const;
+
+  bool IsScreenshare() const;
+
+  static bool IsValidContentType(uint8_t content_type);
+
+  uint8_t content_type;

[tommi, 2017/08/29 09:25:45]

make this private?  (why a struct and not class?)

[ilnik, 2017/08/29 10:26:15]

Can't make it private and class. It will make this class 'complex', which will
make everything with it be 'complex' too (RTPVideoHeader and bunch of other
header related structs). Then there will be a lot of compile errors like this:
"Chromium/Style: complex class should have out of line constructor". This will
require broad refactoring of a lot of API and core structs just for making one
member private. 

Then I am a not sure if it will not break everything. There's a lot of code
using these header structs as POD, memseting them and so on.

 };
 
 }  // namespace webrtc