Clarify resolution change behaviors of video capture devices

Clarify resolution change behaviors of video capture devices

There's no functional change in this CL.

I clarified the behavior of resolution changes for video capture devices
by using an enum to describe resolution change policy instead of a
simple boolean.

BUG=388355
Committed: https://crrev.com/67edf766aa17c66b10bcb7e65125bc4652c90e5b
Cr-Commit-Position: refs/heads/master@{#294714}

[Alpha Left Google, 2014-09-04 22:56:59]

hclam@chromium.org changed reviewers:
+ perkj@chromium.org

[perkj_chrome, 2014-09-05 06:32:04]

perkj@chromium.org changed reviewers:
+ mcasas@chromium.org

[perkj_chrome, 2014-09-05 06:32:05]

I think Miguel is the best person to review this.

[perkj_chrome, 2014-09-07 14:07:09]

lgtm with the following nits.

https://codereview.chromium.org/541163002/diff/40001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/541163002/diff/40001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:154:
media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT) {
I am not sure if this is the correct intdentation or if
media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT should be indendet 4 more spaces. 

if (!capture_size_updated_ ||
    params_.resolution_change_policy ==
        params_.resolution_change_policy) {

https://codereview.chromium.org/541163002/diff/40001/content/browser/media/ca...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/541163002/diff/40001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device.cc:310:
media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT) {
dito

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:224:
media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT ?
indent 4 more.

[littleone691_gmail.com, 2014-09-07 21:45:20]

On Thursday, September 4, 2014 6:57:00 PM UTC-4, Alpha Lam wrote:
> Reviewers: perkj,
> 
> 
> 
> Description:
> 
> Clarify resolution change behaviors of video capture devices
> 
> 
> 
> There's no functional change in this CL.
> 
> 
> 
> I clarified the behavior of resolution changes for video capture devices
> 
> by using an enum to describe resolution change policy instead of a
> 
> simple boolean.
> 
> 
> 
> BUG=388355
> 
> 
> 
> Please review this at https://codereview.chromium.org/541163002/
> 
> 
> 
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
> 
> 
> 
> Affected files (+42, -35 lines):
> 
>    M content/browser/media/capture/content_video_capture_device_core.cc
> 
>    M content/browser/media/capture/desktop_capture_device.cc
> 
>    M content/browser/media/capture/desktop_capture_device_unittest.cc
> 
>    M  
> 
> content/browser/media/capture/web_contents_video_capture_device_unittest.cc
> 
>    M content/browser/renderer_host/media/video_capture_host.cc
> 
>    M content/common/media/video_capture_messages.h
> 
>    M content/renderer/media/media_stream_video_capture_source_unittest.cc
> 
>    M content/renderer/media/media_stream_video_capturer_source.cc
> 
>    M content/renderer/media/video_capture_impl.cc
> 
>    M content/renderer/pepper/pepper_video_capture_host.cc
> 
>    M media/video/capture/fake_video_capture_device_unittest.cc
> 
>    M media/video/capture/video_capture_device_unittest.cc
> 
>    M media/video/capture/video_capture_types.h
> 
>    M media/video/capture/video_capture_types.cc
> 
> 
> 
> 
> 
> Index: content/browser/media/capture/content_video_capture_device_core.cc
> 
> diff --git  
> 
> a/content/browser/media/capture/content_video_capture_device_core.cc  
> 
> b/content/browser/media/capture/content_video_capture_device_core.cc
> 
> index  
> 
>
8152b3dbe0e9cc19636190fff20137c9555ec88f..87efe021178da7de16dbaead34ed630b073f0b12
 
> 
> 100644
> 
> --- a/content/browser/media/capture/content_video_capture_device_core.cc
> 
> +++ b/content/browser/media/capture/content_video_capture_device_core.cc
> 
> @@ -150,7 +150,8 @@ void ThreadSafeCaptureOracle::UpdateCaptureSize(const  
> 
> gfx::Size& source_size) {
> 
>     // If this is the first call to UpdateCaptureSize(), or the receiver  
> 
> supports
> 
>     // variable resolution, then determine the capture size by treating the
> 
>     // requested width and height as maxima.
> 
> -  if (!capture_size_updated_ || params_.allow_resolution_change) {
> 
> +  if (!capture_size_updated_ || params_.resolution_change_policy ==
> 
> +      media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT) {
> 
>       // The capture resolution should not exceed the source frame size.
> 
>       // In other words it should downscale the image but not upscale it.
> 
>       if (source_size.width() > params_.requested_format.frame_size.width() |

> 
> |
> 
> Index: content/browser/media/capture/desktop_capture_device.cc
> 
> diff --git a/content/browser/media/capture/desktop_capture_device.cc  
> 
> b/content/browser/media/capture/desktop_capture_device.cc
> 
> index  
> 
>
930b752159408de76259fbcd81318ccf0d4b872b..a2258f2c9994bb2371f7f634ed0fe1306de35c3a
 
> 
> 100644
> 
> --- a/content/browser/media/capture/desktop_capture_device.cc
> 
> +++ b/content/browser/media/capture/desktop_capture_device.cc
> 
> @@ -306,7 +306,8 @@ void DesktopCaptureDevice::Core::RefreshCaptureFormat(
> 
>     output_frame_.reset();
> 
> 
> 
>     if (previous_frame_size_.is_empty() ||
> 
> -      requested_params_.allow_resolution_change) {
> 
> +      requested_params_.resolution_change_policy ==
> 
> +      media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT) {
> 
>       // If this is the first frame, or the receiver supports variable  
> 
> resolution
> 
>       // then determine the output size by treating the requested width &  
> 
> height
> 
>       // as maxima.
> 
> Index: content/browser/media/capture/desktop_capture_device_unittest.cc
> 
> diff --git  
> 
> a/content/browser/media/capture/desktop_capture_device_unittest.cc  
> 
> b/content/browser/media/capture/desktop_capture_device_unittest.cc
> 
> index  
> 
>
5d37d117a18068f9488a2e78ed326267ee036cde..e1ce84b894f9ba9f31efb30047928a49b82a969b
 
> 
> 100644
> 
> --- a/content/browser/media/capture/desktop_capture_device_unittest.cc
> 
> +++ b/content/browser/media/capture/desktop_capture_device_unittest.cc
> 
> @@ -172,7 +172,6 @@ TEST_F(DesktopCaptureDeviceTest, MAYBE_Capture) {
> 
>     capture_params.requested_format.frame_size.SetSize(640, 480);
> 
>     capture_params.requested_format.frame_rate = kFrameRate;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     capture_device_->AllocateAndStart(
> 
>         capture_params, client.PassAs<media::VideoCaptureDevice::Client>());
> 
>     EXPECT_TRUE(done_event.TimedWait(TestTimeouts::action_max_timeout()));
> 
> @@ -209,7 +208,6 @@ TEST_F(DesktopCaptureDeviceTest,  
> 
> ScreenResolutionChangeConstantResolution) {
> 
>                                                        kTestFrameHeight1);
> 
>     capture_params.requested_format.frame_rate = kFrameRate;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
> 
> 
>     capture_device_->AllocateAndStart(
> 
>         capture_params, client.PassAs<media::VideoCaptureDevice::Client>());
> 
> @@ -251,7 +249,6 @@ TEST_F(DesktopCaptureDeviceTest,  
> 
> ScreenResolutionChangeVariableResolution) {
> 
>                                                        kTestFrameHeight2);
> 
>     capture_params.requested_format.frame_rate = kFrameRate;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
> 
> 
>     capture_device_->AllocateAndStart(
> 
>         capture_params, client.PassAs<media::VideoCaptureDevice::Client>());
> 
> Index:  
> 
> content/browser/media/capture/web_contents_video_capture_device_unittest.cc
> 
> diff --git  
> 
> a/content/browser/media/capture/web_contents_video_capture_device_unittest.cc 

> 
> b/content/browser/media/capture/web_contents_video_capture_device_unittest.cc
> 
> index  
> 
>
4723c79febed72fff8b054c44f7283921a3a39a1..1f9f281f5ca52b4cb28647396566dd04bc259dd3
 
> 
> 100644
> 
> ---  
> 
> a/content/browser/media/capture/web_contents_video_capture_device_unittest.cc
> 
> +++  
> 
> b/content/browser/media/capture/web_contents_video_capture_device_unittest.cc
> 
> @@ -644,7 +644,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> InvalidInitialWebContentsError) {
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device()->AllocateAndStart(capture_params,  
> 
> client_observer()->PassClient());
> 
>     ASSERT_NO_FATAL_FAILURE(client_observer()->WaitForError());
> 
>     device()->StopAndDeAllocate();
> 
> @@ -662,7 +661,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> WebContentsDestroyed) {
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device()->AllocateAndStart(capture_params,  
> 
> client_observer()->PassClient());
> 
>     // Do one capture to prove
> 
>     source()->SetSolidColor(SK_ColorRED);
> 
> @@ -690,7 +688,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device()->AllocateAndStart(capture_params,  
> 
> client_observer()->PassClient());
> 
> 
> 
>     // Make a point of not running the UI messageloop here.
> 
> @@ -712,7 +709,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> StopWithRendererWorkToDo) {
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device()->AllocateAndStart(capture_params,  
> 
> client_observer()->PassClient());
> 
> 
> 
>     base::RunLoop().RunUntilIdle();
> 
> @@ -732,7 +728,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> DeviceRestart) {
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device()->AllocateAndStart(capture_params,  
> 
> client_observer()->PassClient());
> 
>     base::RunLoop().RunUntilIdle();
> 
>     source()->SetSolidColor(SK_ColorRED);
> 
> @@ -771,7 +766,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> GoesThroughAllTheMotions) {
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device()->AllocateAndStart(capture_params,  
> 
> client_observer()->PassClient());
> 
> 
> 
>     for (int i = 0; i < 6; i++) {
> 
> @@ -822,7 +816,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> RejectsInvalidAllocateParams) {
> 
>     capture_params.requested_format.frame_size.SetSize(1280, 720);
> 
>     capture_params.requested_format.frame_rate = -2;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     BrowserThread::PostTask(
> 
>         BrowserThread::UI,
> 
>         FROM_HERE,
> 
> @@ -844,7 +837,6 @@ TEST_F(WebContentsVideoCaptureDeviceTest,  
> 
> BadFramesGoodFrames) {
> 
>     capture_params.requested_format.frame_size.SetSize(kTestWidth,  
> 
> kTestHeight);
> 
>     capture_params.requested_format.frame_rate = kTestFramesPerSecond;
> 
>     capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     // 1x1 is too small to process; we intend for this to result in an error.
> 
>     source()->SetCopyResultSize(1, 1);
> 
>     source()->SetSolidColor(SK_ColorRED);
> 
> Index: content/browser/renderer_host/media/video_capture_host.cc
> 
> diff --git a/content/browser/renderer_host/media/video_capture_host.cc  
> 
> b/content/browser/renderer_host/media/video_capture_host.cc
> 
> index  
> 
>
604cb323be12741569cdd7cd8fa2541cac4bc02b..036f0cbdc5ca3cacd7000114ae9de6f2bb7523e2
 
> 
> 100644
> 
> --- a/content/browser/renderer_host/media/video_capture_host.cc
> 
> +++ b/content/browser/renderer_host/media/video_capture_host.cc
> 
> @@ -220,7 +220,9 @@ void VideoCaptureHost::OnStartCapture(int device_id,
> 
>              << ", device_id=" << device_id
> 
>              << ", format=" << params.requested_format.frame_size.ToString()
> 
>              << "@" << params.requested_format.frame_rate
> 
> -           << " (" <<  
> 
> (params.allow_resolution_change ? "variable" : "constant")
> 
> +           << " (" << (params.resolution_change_policy ==
> 
> +                       media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT ?
> 
> +                       "variable" : "constant")
> 
>              << ")";
> 
>     VideoCaptureControllerID controller_id(device_id);
> 
>     if (entries_.find(controller_id) != entries_.end()) {
> 
> Index: content/common/media/video_capture_messages.h
> 
> diff --git a/content/common/media/video_capture_messages.h  
> 
> b/content/common/media/video_capture_messages.h
> 
> index  
> 
>
abfc46c17dcbeb8ccd48681d4a5f223cd2049a42..4bf24c1ccdf13a4bf0eff4594297d2039796806b
 
> 
> 100644
> 
> --- a/content/common/media/video_capture_messages.h
> 
> +++ b/content/common/media/video_capture_messages.h
> 
> @@ -17,9 +17,12 @@
> 
>   IPC_ENUM_TRAITS_MAX_VALUE(content::VideoCaptureState,
> 
>                             content::VIDEO_CAPTURE_STATE_LAST)
> 
> 
> 
> +IPC_ENUM_TRAITS_MAX_VALUE(media::ResolutionChangePolicy,
> 
> +                          media::RESOLUTION_POLICY_LAST)
> 
> +
> 
>   IPC_STRUCT_TRAITS_BEGIN(media::VideoCaptureParams)
> 
>     IPC_STRUCT_TRAITS_MEMBER(requested_format)
> 
> -  IPC_STRUCT_TRAITS_MEMBER(allow_resolution_change)
> 
> +  IPC_STRUCT_TRAITS_MEMBER(resolution_change_policy)
> 
>   IPC_STRUCT_TRAITS_END()
> 
> 
> 
>   // TODO(nick): device_id in these messages is basically just a route_id. We
> 
> Index: content/renderer/media/media_stream_video_capture_source_unittest.cc
> 
> diff --git  
> 
> a/content/renderer/media/media_stream_video_capture_source_unittest.cc  
> 
> b/content/renderer/media/media_stream_video_capture_source_unittest.cc
> 
> index  
> 
>
8492039a7bd6a6521965cc19a4f74cd5ed0c7ff1..5bc075d64582f9fa0544e0636fe2d314590cbd1f
 
> 
> 100644
> 
> --- a/content/renderer/media/media_stream_video_capture_source_unittest.cc
> 
> +++ b/content/renderer/media/media_stream_video_capture_source_unittest.cc
> 
> @@ -96,7 +96,8 @@ TEST_F(MediaStreamVideoCapturerSourceTest,  
> 
> TabCaptureAllowResolutionChange) {
> 
>     InitWithDeviceInfo(device_info);
> 
> 
> 
>     EXPECT_CALL(mock_delegate(), StartCapture(
> 
> -      testing::Field(&media::VideoCaptureParams::allow_resolution_change,  
> 
> true),
> 
> +      testing::Field(&media::VideoCaptureParams::resolution_change_policy,
> 
> +                     media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT),
> 
>         testing::_,
> 
>         testing::_)).Times(1);
> 
>     blink::WebMediaStreamTrack track = StartSource();
> 
> @@ -111,7 +112,8 @@ TEST_F(MediaStreamVideoCapturerSourceTest,
> 
>     InitWithDeviceInfo(device_info);
> 
> 
> 
>     EXPECT_CALL(mock_delegate(), StartCapture(
> 
> -      testing::Field(&media::VideoCaptureParams::allow_resolution_change,  
> 
> true),
> 
> +      testing::Field(&media::VideoCaptureParams::resolution_change_policy,
> 
> +                     media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT),
> 
>         testing::_,
> 
>         testing::_)).Times(1);
> 
>     blink::WebMediaStreamTrack track = StartSource();
> 
> Index: content/renderer/media/media_stream_video_capturer_source.cc
> 
> diff --git a/content/renderer/media/media_stream_video_capturer_source.cc  
> 
> b/content/renderer/media/media_stream_video_capturer_source.cc
> 
> index  
> 
>
1d82b12280d3421116f2a86ba38c1239619cdc5a..edb81547b4b2d0c7d6e1c7bd2c763c5437ad26ab
 
> 
> 100644
> 
> --- a/content/renderer/media/media_stream_video_capturer_source.cc
> 
> +++ b/content/renderer/media/media_stream_video_capturer_source.cc
> 
> @@ -236,7 +236,8 @@ void MediaStreamVideoCapturerSource::StartSourceImpl(
> 
>     media::VideoCaptureParams new_params(params);
> 
>     if (device_info().device.type == MEDIA_TAB_VIDEO_CAPTURE ||
> 
>         device_info().device.type == MEDIA_DESKTOP_VIDEO_CAPTURE) {
> 
> -    new_params.allow_resolution_change = true;
> 
> +    new_params.resolution_change_policy =
> 
> +        media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT;
> 
>     }
> 
>     delegate_->StartCapture(
> 
>         new_params,
> 
> Index: content/renderer/media/video_capture_impl.cc
> 
> diff --git a/content/renderer/media/video_capture_impl.cc  
> 
> b/content/renderer/media/video_capture_impl.cc
> 
> index  
> 
>
1edc3d5d9f3168467daae4ca18e57c489aa1d353..8afd5fbd4d12c0386beab61f7290c5ab3aa66968
 
> 
> 100644
> 
> --- a/content/renderer/media/video_capture_impl.cc
> 
> +++ b/content/renderer/media/video_capture_impl.cc
> 
> @@ -103,8 +103,8 @@ void VideoCaptureImpl::StartCapture(
> 
>         clients_[client_id] = client_info;
> 
>         // TODO(sheu): Allowing resolution change will require that all
> 
>         // outstanding clients of a capture session support resolution  
> 
> change.
> 
> -      DCHECK_EQ(params_.allow_resolution_change,
> 
> -                params.allow_resolution_change);
> 
> +      DCHECK_EQ(params_.resolution_change_policy,
> 
> +                params.resolution_change_policy);
> 
>       } else if (state_ == VIDEO_CAPTURE_STATE_STOPPING) {
> 
>         clients_pending_on_restart_[client_id] = client_info;
> 
>         DVLOG(1) << "StartCapture: Got new resolution "
> 
> Index: content/renderer/pepper/pepper_video_capture_host.cc
> 
> diff --git a/content/renderer/pepper/pepper_video_capture_host.cc  
> 
> b/content/renderer/pepper/pepper_video_capture_host.cc
> 
> index  
> 
>
d28813b338e42678cc293958814258fa5d07a606..6e8ea32f8b009904fa3665dafe2850c39ac143e5
 
> 
> 100644
> 
> --- a/content/renderer/pepper/pepper_video_capture_host.cc
> 
> +++ b/content/renderer/pepper/pepper_video_capture_host.cc
> 
> @@ -357,7 +357,6 @@ void PepperVideoCaptureHost::SetRequestedInfo(
> 
>         gfx::Size(device_info.width, device_info.height),
> 
>         frames_per_second,
> 
>         media::PIXEL_FORMAT_I420);
> 
> -  video_capture_params_.allow_resolution_change = false;
> 
>   }
> 
> 
> 
>   void PepperVideoCaptureHost::DetachPlatformVideoCapture() {
> 
> Index: media/video/capture/fake_video_capture_device_unittest.cc
> 
> diff --git a/media/video/capture/fake_video_capture_device_unittest.cc  
> 
> b/media/video/capture/fake_video_capture_device_unittest.cc
> 
> index  
> 
>
a2f0d165c0a9312fc6bf1a1ad08b96b19e323c11..a925dda703397a959e453c809af17e75ac59d1a3
 
> 
> 100644
> 
> --- a/media/video/capture/fake_video_capture_device_unittest.cc
> 
> +++ b/media/video/capture/fake_video_capture_device_unittest.cc
> 
> @@ -134,7 +134,6 @@ TEST_F(FakeVideoCaptureDeviceTest, Capture) {
> 
>     capture_params.requested_format.frame_size.SetSize(640, 480);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>     WaitForCapturedFrame();
> 
>     EXPECT_EQ(last_format().frame_size.width(), 640);
> 
> @@ -176,7 +175,8 @@ TEST_F(FakeVideoCaptureDeviceTest,  
> 
> CaptureVariableResolution) {
> 
>     capture_params.requested_format.frame_size.SetSize(640, 480);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = true;
> 
> +  capture_params.resolution_change_policy =
> 
> +      RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT;
> 
> 
> 
>     ASSERT_GT(static_cast<int>(names->size()), 0);
> 
> 
> 
> Index: media/video/capture/video_capture_device_unittest.cc
> 
> diff --git a/media/video/capture/video_capture_device_unittest.cc  
> 
> b/media/video/capture/video_capture_device_unittest.cc
> 
> index  
> 
>
afc6dff6aa5e9f689df82b830c7a4f876d8aa797..9a67fc048e5718d8e58af10c11b9ca5d9dd2657c
 
> 
> 100644
> 
> --- a/media/video/capture/video_capture_device_unittest.cc
> 
> +++ b/media/video/capture/video_capture_device_unittest.cc
> 
> @@ -232,7 +232,6 @@ TEST_F(VideoCaptureDeviceTest, OpenInvalidDevice) {
> 
>       capture_params.requested_format.frame_size.SetSize(640, 480);
> 
>       capture_params.requested_format.frame_rate = 30;
> 
>       capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -    capture_params.allow_resolution_change = false;
> 
>       device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>       device->StopAndDeAllocate();
> 
>     }
> 
> @@ -258,7 +257,6 @@ TEST_F(VideoCaptureDeviceTest, CaptureVGA) {
> 
>     capture_params.requested_format.frame_size.SetSize(640, 480);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>     // Get captured video frames.
> 
>     WaitForCapturedFrame();
> 
> @@ -285,7 +283,6 @@ TEST_F(VideoCaptureDeviceTest, Capture720p) {
> 
>     capture_params.requested_format.frame_size.SetSize(1280, 720);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>     // Get captured video frames.
> 
>     WaitForCapturedFrame();
> 
> @@ -309,7 +306,6 @@ TEST_F(VideoCaptureDeviceTest, MAYBE_AllocateBadSize) {
> 
>     capture_params.requested_format.frame_size.SetSize(637, 472);
> 
>     capture_params.requested_format.frame_rate = 35;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>     WaitForCapturedFrame();
> 
>     device->StopAndDeAllocate();
> 
> @@ -339,7 +335,6 @@ TEST_F(VideoCaptureDeviceTest, ReAllocateCamera) {
> 
>       capture_params.requested_format.frame_size = resolution;
> 
>       capture_params.requested_format.frame_rate = 30;
> 
>       capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -    capture_params.allow_resolution_change = false;
> 
>       device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>       device->StopAndDeAllocate();
> 
>     }
> 
> @@ -349,7 +344,6 @@ TEST_F(VideoCaptureDeviceTest, ReAllocateCamera) {
> 
>     capture_params.requested_format.frame_size.SetSize(320, 240);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
> 
> 
>     ResetWithNewClient();
> 
>     scoped_ptr<VideoCaptureDevice> device(
> 
> @@ -380,7 +374,6 @@ TEST_F(VideoCaptureDeviceTest,  
> 
> DeAllocateCameraWhileRunning) {
> 
>     capture_params.requested_format.frame_size.SetSize(640, 480);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>     // Get captured video frames.
> 
>     WaitForCapturedFrame();
> 
> @@ -409,7 +402,6 @@ TEST_F(VideoCaptureDeviceTest, MAYBE_CaptureMjpeg) {
> 
>     capture_params.requested_format.frame_size.SetSize(1280, 720);
> 
>     capture_params.requested_format.frame_rate = 30;
> 
>     capture_params.requested_format.pixel_format = PIXEL_FORMAT_MJPEG;
> 
> -  capture_params.allow_resolution_change = false;
> 
>     device->AllocateAndStart(capture_params, client_.PassAs<Client>());
> 
>     // Get captured video frames.
> 
>     WaitForCapturedFrame();
> 
> Index: media/video/capture/video_capture_types.cc
> 
> diff --git a/media/video/capture/video_capture_types.cc  
> 
> b/media/video/capture/video_capture_types.cc
> 
> index  
> 
>
17f2d654c6193e697b8dbc9a188bcaa099d4e74d..8aab6e58669d595ab6c28937e1e27db46dc341a9
 
> 
> 100644
> 
> --- a/media/video/capture/video_capture_types.cc
> 
> +++ b/media/video/capture/video_capture_types.cc
> 
> @@ -59,6 +59,6 @@ std::string  
> 
> VideoCaptureFormat::PixelFormatToString(VideoPixelFormat format) {
> 
>     return "";
> 
>   }
> 
> 
> 
> -VideoCaptureParams::VideoCaptureParams() : allow_resolution_change(false)  
> 
> {}
> 
> -
> 
> +VideoCaptureParams::VideoCaptureParams()
> 
> +    : resolution_change_policy(RESOLUTION_POLICY_FIXED) {}
> 
>   }  // namespace media
> 
> 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  
> 
>
a6a13a81b070cb33224012525bda8b8dc1911c18..9c2f623daa944639239d5da4865706d5c7efe3a8
 
> 
> 100644
> 
> --- a/media/video/capture/video_capture_types.h
> 
> +++ b/media/video/capture/video_capture_types.h
> 
> @@ -31,6 +31,23 @@ enum VideoPixelFormat {
> 
>     PIXEL_FORMAT_MAX,
> 
>   };
> 
> 
> 
> +// Policies for capture devices that has source content with dynamic  
> 
> resolution.
> 
> +enum ResolutionChangePolicy {
> 
> +  // Capture device outputs a fixed resolution all the time. The  
> 
> resolution of
> 
> +  // the first frame is the resolution for all frames.
> 
> +  // It is implementation specific for the capture device to scale,  
> 
> letter-box
> 
> +  // and pillar-box. The only gurantee is that resolution will never  
> 
> change.
> 
> +  RESOLUTION_POLICY_FIXED,
> 
> +
> 
> +  // Capture device outputs frames with dynamic resolution. The width and  
> 
> height
> 
> +  // will not exceed the maximum dimensions specified. The typical  
> 
> scenario is
> 
> +  // the frames will have the same aspect ratio as the original content and
> 
> +  // scaled down to fit inside the limit.
> 
> +  RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT,
> 
> +
> 
> +  RESOLUTION_POLICY_LAST,
> 
> +};
> 
> +
> 
>   // Some drivers use rational time per frame instead of float frame rate,  
> 
> this
> 
>   // constant k is used to convert between both: A fps -> [k/k*A]  
> 
> seconds/frame.
> 
>   const int kFrameRatePrecision = 10000;
> 
> @@ -70,8 +87,8 @@ class MEDIA_EXPORT VideoCaptureParams {
> 
>     // Requests a resolution and format at which the capture will occur.
> 
>     VideoCaptureFormat requested_format;
> 
> 
> 
> -  // Allow mid-capture resolution change.
> 
> -  bool allow_resolution_change;
> 
> +  // Policy for resolution change.
> 
> +  ResolutionChangePolicy resolution_change_policy;
> 
>   };
> 
> 
> 
>   }  // namespace media

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mcasas, 2014-09-08 09:09:53]

hclam@: what's the meaning of this change? This
file went already from an enum to a boolean [1]
and now it would be back to enum. Are you planning
on adding more entries to the enumeration? If so,
please state it in the Issue description.



[1]
https://codereview.chromium.org/68503005/diff/1580001/media/video/capture/vid...

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:221: << ", format=" <<
params.requested_format.frame_size.ToString()
Please use params.requested_format.ToString().

https://codereview.chromium.org/541163002/diff/40001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/541163002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_types.h:39: // and pillar-box. The only
gurantee is that resolution will never change.
s/gurantee/guarantee/

[Alpha Left Google, 2014-09-09 01:00:11]

On 2014/09/08 09:09:53, mcasas wrote:
> hclam@: what's the meaning of this change? This
> file went already from an enum to a boolean [1]
> and now it would be back to enum. Are you planning
> on adding more entries to the enumeration? If so,
> please state it in the Issue description.

I have no idea it was an enum before. :)

The background of this change is crbug.com/388355. The real problem is there's
different behavior in capturers regarding resolution changes. And the boolean
type is not able to capture all the details. Nor does it document well the
behavior well.

For example:
1. video cameras are always fixed.
2. tab capture is always faixed.
3. desktop capture is dynamic, but only changes it within limit. Even if it is
set to fixed it behaves differently than (1) and (2).

This change is to clarify existing behavior and potentially allow new behaviors.

[Alpha Left Google, 2014-09-09 01:04:22]

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:221: << ", format=" <<
params.requested_format.frame_size.ToString()
On 2014/09/08 09:09:53, mcasas wrote:
> Please use params.requested_format.ToString().

Done.

https://codereview.chromium.org/541163002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:224:
media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT ?
On 2014/09/07 14:07:09, perkj wrote:
> indent 4 more.

Done.

[Alpha Left Google, 2014-09-11 20:38:17]

hclam@chromium.org changed reviewers:
+ tsepez@chromium.org

[Alpha Left Google, 2014-09-11 20:38:17]

+tsepez for ipc messages.

[Tom Sepez, 2014-09-11 22:33:10]

Messages LGTM.

[Alpha Left Google, 2014-09-11 22:33:30]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-09-11 22:35:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541163002/60001

[commit-bot: I haz the power, 2014-09-12 00:14:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 00:14:16]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54939)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59866)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alpha Left Google, 2014-09-12 00:44:34]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-09-12 00:45:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541163002/100001

[commit-bot: I haz the power, 2014-09-12 01:09:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 01:09:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alpha Left Google, 2014-09-12 02:57:40]

hclam@chromium.org changed reviewers:
+ dmichael@chromium.org

[Alpha Left Google, 2014-09-12 02:57:41]

+dmichael for pepper.

[Alpha Left Google, 2014-09-12 18:12:10]

hclam@chromium.org changed reviewers:
+ bbudge@chromium.org

[Alpha Left Google, 2014-09-12 18:12:11]

+bbudge for pepper.

[dmichael (off chromium), 2014-09-12 18:14:14]

pepper lgtm

[bbudge, 2014-09-12 18:14:41]

lgtm

[Alpha Left Google, 2014-09-13 00:44:37]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:53:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541163002/100001

[commit-bot: I haz the power, 2014-09-13 01:07:38]

Committed patchset #6 (id:100001) as 33491113c439431e8dc03f5d3e0278121ab7d826

[commit-bot: I haz the power, 2014-09-13 01:17:39]

Patchset 6 (id:??) landed as
https://crrev.com/67edf766aa17c66b10bcb7e65125bc4652c90e5b
Cr-Commit-Position: refs/heads/master@{#294714}