Consolidate duplicated frame / capability structures in video_capture_types.h

media/video/capture/video_capture_types.h

Index: media/video/capture/video_capture_types.h
diff --git a/media/video/capture/video_capture_types.h b/media/video/capture/video_capture_types.h
index 57712727ef0f25046c16bf1c2ac7daca632a2d67..c13193975873021753ba38d424580c37b282d88d 100644
--- a/media/video/capture/video_capture_types.h
+++ b/media/video/capture/video_capture_types.h
@@ -19,68 +19,84 @@ enum VideoCaptureResolutionType {
   MaxVideoCaptureResolutionType,  // Must be last.
 };
 
-// Parameters for starting video capture and device information.
-struct VideoCaptureParams {
-  VideoCaptureParams()
+// Color formats from camera.
+enum VideoPixelFormat {

[ncarter (slow), 2013/09/04 18:05:05]

I really like this type name. It packs a lot of punch.

[mcasas, 2013/09/06 09:13:13]

Kick Arses!

+  kColorUnknown,  // Color format not set.

[scherkus (not reviewing), 2013/09/06 00:55:01]

now that these are in the global namespace they need a common prefix, otherwise
they can be easily confused with other similar enums in the media namespace
(e.g., VideoFrame::Format)

finally... if you're renaming all the variables, now might be a good time to
switch to SHOUTING_STYLE as per Chromium style -- I didn't catch this on my
review of the original code ~2 years ago :(

For example:
enum VideoPixelFormat {
  PIXEL_FORMAT_UNKNOWN,
  PIXEL_FORMAT_I420,
  // etc...
};

Feel free to use a shorter-yet-still-descriptive prefix if you can think of one.

[mcasas, 2013/09/06 09:13:13]

For (my) future reference:
Done.

+  kI420,
+  kYUY2,
+  kUYVY,
+  kRGB24,
+  kARGB,
+  kMJPEG,
+  kNV21,
+  kYV12,
+};
+
+// Video capture format specification.
+class MEDIA_EXPORT VideoCaptureFormat {
+ public:
+  VideoCaptureFormat()

[scherkus (not reviewing), 2013/09/06 00:55:01]

none of these constructors need to be inlined -- please move to .cc

details:
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

[mcasas, 2013/09/06 09:13:13]

Done.

       : width(0),
         height(0),
-        frame_per_second(0),
-        session_id(0),
+        frame_rate(0),
         frame_size_type(ConstantResolutionVideoCaptureDevice) {};
+  VideoCaptureFormat(int width,
+                     int height,
+                     int frame_rate,
+                     VideoCaptureResolutionType frame_size_type)
+      : width(width),
+        height(height),
+        frame_rate(frame_rate),
+        frame_size_type(frame_size_type) {};
+  // Checks that all values are in the expected range. All limits are specified
+  // in media::Limits.
+  bool IsValid() const;
+
+ public:

[scherkus (not reviewing), 2013/09/06 00:55:01]

nit: remove as it's redundant

[mcasas, 2013/09/06 09:13:13]

Done.

   int width;
   int height;
-  int frame_per_second;
-  VideoCaptureSessionId session_id;
+  int frame_rate;
   VideoCaptureResolutionType frame_size_type;
 };
 
-// Capabilities describe the format a camera capture video in.
-struct VideoCaptureCapability {
-  // Color formats from camera.
-  enum Format {
-    kColorUnknown,  // Color format not set.
-    kI420,
-    kYUY2,
-    kUYVY,
-    kRGB24,
-    kARGB,
-    kMJPEG,
-    kNV21,
-    kYV12,
-  };
+// Parameters for starting video capture and device information.
+class MEDIA_EXPORT VideoCaptureParams : public VideoCaptureFormat {
+ public:
+  VideoCaptureParams()
+      : session_id(0) {};
+  // Checks that all values are in the expected range. All limits are specified
+  // in media::Limits.
+  bool IsValid() const;

[scherkus (not reviewing), 2013/09/06 00:55:01]

if VideoCaptureParams inherits from VideoCaptureFormat, does it need its own
IsValid method?

The .cc code looks identical and while I may have missed something, I didn't see
anything in this CL that would require a separate method

[mcasas, 2013/09/06 09:13:13]

My miss: is just a copy-paste!
Removed VideoCaptureFormat::isValid, in favour of the inherited.

+
+ public:

[scherkus (not reviewing), 2013/09/06 00:55:01]

nit: remove as it's redundant

[mcasas, 2013/09/06 09:13:13]

Done.

+  VideoCaptureSessionId session_id;
+};
 
+// Capabilities describe the format a camera capture video in.
+class VideoCaptureCapability : public VideoCaptureFormat {
+ public:
   VideoCaptureCapability()
-      : width(0),
-        height(0),
-        frame_rate(0),
-        color(kColorUnknown),
+      : color(kColorUnknown),
         expected_capture_delay(0),
         interlaced(false),
-        frame_size_type(ConstantResolutionVideoCaptureDevice),
         session_id(0) {};
   VideoCaptureCapability(int width,
                          int height,
                          int frame_rate,
-                         Format color,
+                         VideoPixelFormat color,
                          int delay,
                          bool interlaced,
                          VideoCaptureResolutionType frame_size_type)
-      : width(width),
-        height(height),
-        frame_rate(frame_rate),
+      : VideoCaptureFormat(width, height, frame_rate, frame_size_type),
         color(color),
         expected_capture_delay(delay),
         interlaced(interlaced),
-        frame_size_type(frame_size_type),
         session_id(0) {};
 
-  int width;                   // Desired width.
-  int height;                  // Desired height.
-  int frame_rate;              // Desired frame rate.
-  Format color;                // Desired video type.
+ public:

[scherkus (not reviewing), 2013/09/06 00:55:01]

nit: remove as it's redundant

[mcasas, 2013/09/06 09:13:13]

Done.

+  VideoPixelFormat color;      // Desired video type.
   int expected_capture_delay;  // Expected delay in millisecond.
   bool interlaced;             // Need interlace format.
-  VideoCaptureResolutionType frame_size_type;
   VideoCaptureSessionId session_id;
 };