Consolidate duplicated frame / capability structures in video_capture_types.h

Consolidate duplicated frame / capability structures in video capture types

Consolidate common parts of VideoCaptureCapability and VideoCaptureParams into VideoCaptureFormat, renamed frames_per_second->frame_rate for all; 
VideoCaptureParams and VideoCaptureCapability changed to classes, since they have constructors.

Format extracted from VideoCaptureCapability and renamed to VideoPixelFormat (otherwise simply called "media::Format" ). Individual color "formats" renamed like kI420 -> PIXEL_FORMAT_I420, etc.


VideoCaptureFormat::isValid and VideoCaptureParams::isValid implementation.


BUG=260572
BUG=260220
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221945

[mcasas, 2013-08-23 15:09:50]

Hi tommi, could you please have a go at this clean-up CL ? 

Most of the modifications are concentrated in
media/video/capture/video_capture_types.h , see description. I think most of the
other changes are simply cosmetic...

As usual, feel free to pull other reviewers as you see fit :)

Cheers!

[mcasas, 2013-08-23 15:18:02]

Hi miu, tommi suggested you could take a look at
media/video/capture/video_capture_types.h and review the clean up there? Thanks
a bunch!

[miu, 2013-08-26 21:48:42]

On 2013/08/23 15:18:02, miguelao wrote:
> Hi miu, tommi suggested you could take a look at
> media/video/capture/video_capture_types.h and review the clean up there?
Thanks
> a bunch!

BTW--I removed myself and put nick@ on the review since he's actively working in
this area.

[mcasas, 2013-09-03 08:38:38]

friendly ping ncarter :)

[mcasas, 2013-09-04 14:54:16]

On 2013/09/03 08:38:38, miguelao wrote:
> friendly ping ncarter :)

ping ping

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

lgtm

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:23: enum VideoPixelFormat {
I really like this type name. It packs a lot of punch.

[commit-bot: I haz the power, 2013-09-04 20:45:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/22876027/1

[commit-bot: I haz the power, 2013-09-04 21:01:38]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tommi (sloooow) - chröme, 2013-09-05 08:14:23]

lgtm

[commit-bot: I haz the power, 2013-09-05 08:15:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/22876027/1

[commit-bot: I haz the power, 2013-09-05 08:26:35]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mcasas, 2013-09-05 08:36:57]

@dmichael: could you please review and LGTM
content/renderer/pepper/pepper_video_capture_host.cc ? 
(Some minor renaming).

@scherkus: could you please review and LGTM media/media.gyp ? (Added one .cc
file to compilation)

both are a tiny changes but need owner's approval ;)

[dmichael (off chromium), 2013-09-05 15:09:34]

lgtm. You can TBR me on stuff this simple in the future, if you prefer.

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

https://codereview.chromium.org/22876027/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/22876027/diff/1/media/media.gyp#newcode404
media/media.gyp:404: 'video/capture/video_capture_types.cc',
should be in alphabetical order (.cc < .h)

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:24: kColorUnknown,  // Color format
not set.
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.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:38: VideoCaptureFormat()
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...

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:55: public:
nit: remove as it's redundant

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:69: bool IsValid() const;
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

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:71: public:
nit: remove as it's redundant

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:96: public:
nit: remove as it's redundant

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

Addressed scherkus@'s comments, thanks for the quick reply!

https://codereview.chromium.org/22876027/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/22876027/diff/1/media/media.gyp#newcode404
media/media.gyp:404: 'video/capture/video_capture_types.cc',
On 2013/09/06 00:55:01, scherkus wrote:
> should be in alphabetical order (.cc < .h)

Done.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:23: enum VideoPixelFormat {
Kick Arses!

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:24: kColorUnknown,  // Color format
not set.
For (my) future reference:

>> Though the Google C++ Style Guide now says to use kConstantNaming for enums,
Chromium was written using MACRO_STYLE naming.  Continue to use this style for
consistency.

Done.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:38: VideoCaptureFormat()
On 2013/09/06 00:55:01, scherkus wrote:
> 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...

Done.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:55: public:
On 2013/09/06 00:55:01, scherkus wrote:
> nit: remove as it's redundant

Done.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:69: bool IsValid() const;
My miss: is just a copy-paste!
Removed VideoCaptureFormat::isValid, in favour of the inherited.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:71: public:
On 2013/09/06 00:55:01, scherkus wrote:
> nit: remove as it's redundant

Done.

https://codereview.chromium.org/22876027/diff/1/media/video/capture/video_cap...
media/video/capture/video_capture_types.h:96: public:
On 2013/09/06 00:55:01, scherkus wrote:
> nit: remove as it's redundant

Done.

[scherkus (not reviewing), 2013-09-06 18:04:51]

few more nits

also please update your CL description to reflect the enum change

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_types.cc (right):

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:15:
frame_size_type(ConstantResolutionVideoCaptureDevice) {};
trailing ; not needed

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:25: frame_size_type(frame_size_type)
{};
trailing ; not needed

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:41: session_id(0) {};
trailing ; not needed

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:55: session_id(0) {};
trailing ; not needed

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.h:43: // Checks that all values are in
the expected range. All limits are specified
nit: add blank line before this

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.h:56: VideoCaptureParams() :
session_id(0) {};
move to .cc

don't forget to trim off trailing ; from } (it's not needed in the .cc)

[mcasas, 2013-09-06 18:27:58]

Updated CL description with colour space conversion change. Done adding
scherkus@'s comments, cheers!

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_types.cc (right):

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:15:
frame_size_type(ConstantResolutionVideoCaptureDevice) {};
On 2013/09/06 18:04:51, scherkus wrote:
> trailing ; not needed

Done.

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:25: frame_size_type(frame_size_type)
{};
On 2013/09/06 18:04:51, scherkus wrote:
> trailing ; not needed

Done.

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:41: session_id(0) {};
On 2013/09/06 18:04:51, scherkus wrote:
> trailing ; not needed

Done.

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.cc:55: session_id(0) {};
On 2013/09/06 18:04:51, scherkus wrote:
> trailing ; not needed

Done.

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.h:43: // Checks that all values are in
the expected range. All limits are specified
On 2013/09/06 18:04:51, scherkus wrote:
> nit: add blank line before this

Done.

https://codereview.chromium.org/22876027/diff/35001/media/video/capture/video...
media/video/capture/video_capture_types.h:56: VideoCaptureParams() :
session_id(0) {};
On 2013/09/06 18:04:51, scherkus wrote:
> move to .cc
> 
> don't forget to trim off trailing ; from } (it's not needed in the .cc)

Done.

[scherkus (not reviewing), 2013-09-06 21:45:21]

lgtm

[commit-bot: I haz the power, 2013-09-07 18:28:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/22876027/84001

[commit-bot: I haz the power, 2013-09-08 03:38:00]

Change committed as 221945