Merge VideoCaptureDevice::Capability with media::VideoCaptureCapability

Merge VideoCaptureDevice::Capability with media::VideoCaptureCapability
This is a second patch as the follow up of http://codereview.chromium.org/10108009/, just to facilitate code review
1. Move VideoCaptureDevice::Format to VideoCaptureCapability::Format.
2. Fix a code style issue in media_stream_device_settings.h.
3. Remove the TODO comments in media_stream_device_settings.h, since the media stream infobar is already implemented in linux and windows.

BUG=123738
TEST=content_unittests, media_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134415

[Jeremy Mao, 2012-04-19 05:36:37]

Hi wjia, scherkus,

Could please kindly take a look at this when you are free?

BTW, I just roughly merge VideoCaptureDevice::Format with VideoFrame::Format in
this patch, I will do another merge with the color formats.

Thanks
Jeremy

[scherkus (not reviewing), 2012-04-19 19:09:35]

http://codereview.chromium.org/10035054/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/10035054/diff/1/media/base/video_frame.h#newco...
media/base/video_frame.h:40: kColorUnknown,  // Color format not set.
does the capture code actually use VideoFrame or just the constants?

If the latter I'd prefer a separate enum for that is specific for capturing,
otherwise we give the false impression that VideoFrame (the class) supports
these formats

[wjia(left Chromium), 2012-04-19 22:02:28]

On Thu, Apr 19, 2012 at 12:09 PM, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.**org/10035054/diff/1/media/**
>
base/video_frame.h<http://codereview.chromium.org/10035054/diff/1/media/base/video_frame.h>
> File media/base/video_frame.h (right):
>
> http://codereview.chromium.**org/10035054/diff/1/media/**
>
base/video_frame.h#newcode40<http://codereview.chromium.org/10035054/diff/1/media/base/video_frame.h#newcode40>
> media/base/video_frame.h:40: kColorUnknown,  // Color format not set.
> does the capture code actually use VideoFrame or just the constants?
>
> If the latter I'd prefer a separate enum for that is specific for
> capturing, otherwise we give the false impression that VideoFrame (the
> class) supports these formats
>
>
video capture use VideoFrame only when it tries to deliver frames to
VideoRenderer.

I agree with scherkus@ that capture code should have its own color format
enums.



>
http://codereview.chromium.**org/10035054/<http://codereview.chromium.org/100...
>

[Jeremy Mao, 2012-04-20 02:23:32]

On 2012/04/19 19:09:35, scherkus wrote:
> http://codereview.chromium.org/10035054/diff/1/media/base/video_frame.h
> File media/base/video_frame.h (right):
> 
>
http://codereview.chromium.org/10035054/diff/1/media/base/video_frame.h#newco...
> media/base/video_frame.h:40: kColorUnknown,  // Color format not set.
> does the capture code actually use VideoFrame or just the constants?
> 
> If the latter I'd prefer a separate enum for that is specific for capturing,
> otherwise we give the false impression that VideoFrame (the class) supports
> these formats
Reasonable explanation, I will refine this patch.

Thanks
Jeremy

[Jeremy Mao, 2012-04-25 07:50:26]

Hi wjia, scherkus,

Do you think it's the best way to keep "VideoColorFormat" info in
VideoFrame(class)?

I think maybe we can move the format info out of VideoFrame and put it in a
common place, and the code will looks more clean.

Any thoughts on this?

Thanks & Best regards
Jeremy

[scherkus (not reviewing), 2012-04-25 16:49:08]

On 2012/04/25 07:50:26, Jeremy Mao wrote:
> Hi wjia, scherkus,
> 
> Do you think it's the best way to keep "VideoColorFormat" info in
> VideoFrame(class)?
> 
> I think maybe we can move the format info out of VideoFrame and put it in a
> common place, and the code will looks more
clean.http://codereview.chromium.org/10035054/
> 
> Any thoughts on this?
> 
> Thanks & Best regards
> Jeremy

VideoFrame has an enum of formats that it supports. Re-using those enums for
VideoCaptureCapability and merging the set doesn't seem like a good idea.

Instead, VideoCaptureCapability should define its own enum inside of the class
and stop using VideoFrame::Format altogether.

wjia would you be OK with that or am I missing something?

[Jeremy Mao, 2012-04-26 07:32:01]

Sorry for the inconvenience, the changed files seem a little bit more. :)

Hi wjia, sherkus,

Could you please take a look at all changes?

Hi mflodman,

Could you please take a look at the changes in media_stream_device_settings.h?

Hi piman,

Could you please take a look at the changes in ppb_video_capture_impl.cc?

Thanks & Best regards
Jeremy

[piman, 2012-04-26 09:09:17]

On 2012/04/26 07:32:01, Jeremy Mao wrote:
> Sorry for the inconvenience, the changed files seem a little bit more. :)
> 
> Hi wjia, sherkus,
> 
> Could you please take a look at all changes?
> 
> Hi mflodman,
> 
> Could you please take a look at the changes in media_stream_device_settings.h?
> 
> Hi piman,
> 
> Could you please take a look at the changes in ppb_video_capture_impl.cc?

Still LGTM for that file.

> 
> Thanks & Best regards
> Jeremy

[wjia(left Chromium), 2012-04-26 18:19:40]

On 2012/04/25 16:49:08, scherkus wrote:
> On 2012/04/25 07:50:26, Jeremy Mao wrote:
> > Hi wjia, scherkus,
> > 
> > Do you think it's the best way to keep "VideoColorFormat" info in
> > VideoFrame(class)?
> > 
> > I think maybe we can move the format info out of VideoFrame and put it in a
> > common place, and the code will looks more
> clean.http://codereview.chromium.org/10035054/
> > 
> > Any thoughts on this?
> > 
> > Thanks & Best regards
> > Jeremy
> 
> VideoFrame has an enum of formats that it supports. Re-using those enums for
> VideoCaptureCapability and merging the set doesn't seem like a good idea.
> 
> Instead, VideoCaptureCapability should define its own enum inside of the class
> and stop using VideoFrame::Format altogether.
> 
> wjia would you be OK with that or am I missing something?

Sorry for the delay. I was OOO yesterday. As I stated in previous reply, I agree
with scherkus@ that capture code should have its own color format
enums.

[wjia(left Chromium), 2012-04-26 20:06:32]

LGTM. Thanks!

One nit: it might be better to have media::VideoCaptureFormat or
media::VideoCapture::Format, instead of media::Format. But we can do it later.

[scherkus (not reviewing), 2012-04-26 20:28:38]

http://codereview.chromium.org/10035054/diff/9001/media/video/capture/video_c...
File media/video/capture/video_capture_types.h (right):

http://codereview.chromium.org/10035054/diff/9001/media/video/capture/video_c...
media/video/capture/video_capture_types.h:25: enum Format {
this is not good and pollutes the media namespace

as I suggested originally please move this enum into VideoCaptureCapability

[Jeremy Mao, 2012-04-27 02:58:13]

On 2012/04/26 20:28:38, scherkus wrote:
>
http://codereview.chromium.org/10035054/diff/9001/media/video/capture/video_c...
> File media/video/capture/video_capture_types.h (right):
> 
>
http://codereview.chromium.org/10035054/diff/9001/media/video/capture/video_c...
> media/video/capture/video_capture_types.h:25: enum Format {
> this is not good and pollutes the media namespace
> 
> as I suggested originally please move this enum into VideoCaptureCapability
Fixed.

[mflodman_chromium_OOO, 2012-04-27 14:39:17]

One small comment regarding the comment in media_stream_device_settings.h.

http://codereview.chromium.org/10035054/diff/20001/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_device_settings.h (right):

http://codereview.chromium.org/10035054/diff/20001/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_device_settings.h:11: // 1.
MediaStreamProvider::EnumrateDevices() is called to enumerate a list of
I'd prefer to remove 1 since that is not done in this class.

[scherkus (not reviewing), 2012-04-27 17:42:40]

LGTM w/ one nit

thanks!!!

http://codereview.chromium.org/10035054/diff/20001/media/video/capture/win/si...
File media/video/capture/win/sink_input_pin_win.h (right):

http://codereview.chromium.org/10035054/diff/20001/media/video/capture/win/si...
media/video/capture/win/sink_input_pin_win.h:28: void
SetRequestedMediaCapability(
nit: this can fit on one line

[mflodman_chromium_OOO, 2012-04-27 20:16:29]

On 2012/04/27 14:39:17, mflodman wrote:
> One small comment regarding the comment in media_stream_device_settings.h.
> 
>
http://codereview.chromium.org/10035054/diff/20001/content/browser/renderer_h...
> File content/browser/renderer_host/media/media_stream_device_settings.h
(right):
> 
>
http://codereview.chromium.org/10035054/diff/20001/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_device_settings.h:11: // 1.
> MediaStreamProvider::EnumrateDevices() is called to enumerate a list of
> I'd prefer to remove 1 since that is not done in this class.

Please look at the comment, but LGTM for media_stream_device_settings.h.

[Jeremy Mao, 2012-04-27 22:24:15]

On 2012/04/27 17:42:40, scherkus wrote:
> LGTM w/ one nit
> 
> thanks!!!
> 
>
http://codereview.chromium.org/10035054/diff/20001/media/video/capture/win/si...
> File media/video/capture/win/sink_input_pin_win.h (right):
> 
>
http://codereview.chromium.org/10035054/diff/20001/media/video/capture/win/si...
> media/video/capture/win/sink_input_pin_win.h:28: void
> SetRequestedMediaCapability(
> nit: this can fit on one line
fixed

[Jeremy Mao, 2012-04-27 22:24:46]

On 2012/04/27 14:39:17, mflodman wrote:
> One small comment regarding the comment in media_stream_device_settings.h.
> 
>
http://codereview.chromium.org/10035054/diff/20001/content/browser/renderer_h...
> File content/browser/renderer_host/media/media_stream_device_settings.h
(right):
> 
>
http://codereview.chromium.org/10035054/diff/20001/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_device_settings.h:11: // 1.
> MediaStreamProvider::EnumrateDevices() is called to enumerate a list of
> I'd prefer to remove 1 since that is not done in this class.
fixed

[commit-bot: I haz the power, 2012-04-27 22:25:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yujie.mao@intel.com/10035054/27002

[commit-bot: I haz the power, 2012-04-27 22:50:10]

Try job failure for 10035054-27002 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-27 22:53:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yujie.mao@intel.com/10035054/32001

[commit-bot: I haz the power, 2012-04-28 00:56:08]

Change committed as 134415