VideoCapturerDelegate: Retrieve supported/in-use format(s) for constraint negotiation

VideoCapturerDelegate: Retrieve supported/in-use format(s) for constraint negotiation.

VideoCapturerDelegate uses currently a set of fixed 
resolutions and frame rates for constraint negotiation.
Instead, the associated VideoCaptureImplManager is
queried first for the device format in-use; if that
in non-null, is used, otherwise the whole list of 
supported formats is retrieved and used. If that last
list is also empty (platform does not support it,
such as Mac QTKit or Win DirectShow), then the by-
default set of resolutions are used.

This CL would change behaviour of scenarios in
which requested capture resolution or frame rate is
larger than the one provided by hardware, for example
Fake Video Capture Device supports only 
20fps, if the Mandatory MinFps is larger than that,
the resolution algorithm will fail.

No unittests added to WebRtcGetUserMediaBrowserTest:  
the unsupported format is added in 
https://codereview.chromium.org/197213004/ . Rest of
the tests in the file change the gUM to request fps
between 10 and 30, since the FakeVCD supports 20, and
it would fail with a [30-30] fps range requested.

BUG=309554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261717

[mcasas, 2014-03-12 14:18:24]

perkj@ PTAL

[perkj_chrome, 2014-03-12 19:55:47]

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
content/browser/media/webrtc_getusermedia_browsertest.cc:339:
ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
Oh, I did this in https://codereview.chromium.org/197213004/

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
content/browser/media/webrtc_getusermedia_browsertest.cc:351:
ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
Do we need a special test for this? I think this is the same as the one above
except for the frame rate?

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
content/browser/media/webrtc_getusermedia_browsertest.cc:391:
kGetUserMediaAndAnalyseAndStop, 640, 640, 480, 480, 20, 20);
Can we skip changing the maxFramerate and only change the minFramerate. Ie- Sett
it to max 30 min 10.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:45:
source_formats_callback_() {
no need to initialize this here.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:80:
RenderThreadImpl::current()->video_capture_impl_manager()
No... Do
capture_engine_->GetDeviceFormatInUse(base::Bind(&VideoCapturerDelegate::OnDeviceFormatsInUseReceived,
this);

Ie, do not use unretained, unretained is a promise that VideoCapturerDelegate
will not be deleted while waiting for the callback. If you only use this,
base::Bind will add a ref to VideoCaptureDelegate and thus ensure that the
object will not be deleted.

Since we already have capture_engine_, use it.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:108:
started_callback_.Reset();
Do source_formats_callback_.Reset() here to make sure source_formats_callback_
is not triggered after StopDeliver is called. Please see the comment in
MediaStreamVideoSource.h on StopSourceImpl.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:115:
source_formats_callback_.Run(formats_in_use);
check for isnull.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:118:
RenderThreadImpl::current()->video_capture_impl_manager()
Same as above.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:130:
source_formats_callback_.Run(formats);
check for isnull.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:56: void
OnDeviceFormatsInUseReceived(const media::VideoCaptureFormats& formats);
make these functions private

[mcasas, 2014-03-13 08:04:56]

perkj@ PTAL.

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
content/browser/media/webrtc_getusermedia_browsertest.cc:339:
ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
On 2014/03/12 19:55:48, perkj wrote:
> Oh, I did this in https://codereview.chromium.org/197213004/

I'll remove these two tests then, but I'd recommend adding
a failing gUM with valid values, not 99999.

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
content/browser/media/webrtc_getusermedia_browsertest.cc:351:
ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
On 2014/03/12 19:55:48, perkj wrote:
> Do we need a special test for this? I think this is the same as the one above
> except for the frame rate? 

See above.

https://codereview.chromium.org/195363002/diff/80001/content/browser/media/we...
content/browser/media/webrtc_getusermedia_browsertest.cc:391:
kGetUserMediaAndAnalyseAndStop, 640, 640, 480, 480, 20, 20);
On 2014/03/12 19:55:48, perkj wrote:
> Can we skip changing the maxFramerate and only change the minFramerate. Ie-
Sett
> it to max 30 min 10.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:45:
source_formats_callback_() {
On 2014/03/12 19:55:48, perkj wrote:
> no need to initialize this here.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:80:
RenderThreadImpl::current()->video_capture_impl_manager()
On 2014/03/12 19:55:48, perkj wrote:
> No... Do
>
capture_engine_->GetDeviceFormatInUse(base::Bind(&VideoCapturerDelegate::OnDeviceFormatsInUseReceived,
> this);
> 
> Ie, do not use unretained, unretained is a promise that VideoCapturerDelegate
> will not be deleted while waiting for the callback. If you only use this,
> base::Bind will add a ref to VideoCaptureDelegate and thus ensure that the
> object will not be deleted.
> 
> Since we already have capture_engine_, use it.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:108:
started_callback_.Reset();
On 2014/03/12 19:55:48, perkj wrote:
> Do source_formats_callback_.Reset() here to make sure source_formats_callback_
> is not triggered after StopDeliver is called. Please see the comment in
> MediaStreamVideoSource.h on StopSourceImpl.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:115:
source_formats_callback_.Run(formats_in_use);
On 2014/03/12 19:55:48, perkj wrote:
> check for isnull.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:118:
RenderThreadImpl::current()->video_capture_impl_manager()
On 2014/03/12 19:55:48, perkj wrote:
> Same as above.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:130:
source_formats_callback_.Run(formats);
On 2014/03/12 19:55:48, perkj wrote:
> check for isnull.

Done.

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/195363002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.h:56: void
OnDeviceFormatsInUseReceived(const media::VideoCaptureFormats& formats);
On 2014/03/12 19:55:48, perkj wrote:
> make these functions private

Done.

[perkj_chrome, 2014-03-13 08:38:02]

Looks good.

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:172:
DCHECK(!source_formats_callback_.is_null());
That is not what I meant. This will now crash is StopDeliver is called. So I
meant you should do 
if !source_formats_callback_.is_null()
 source_formats_callback_.Run(formats_in_use);

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:187:
DCHECK(!source_formats_callback_.is_null());
dito

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:199:
source_formats_callback_.Run(default_formats);
and here

[mcasas, 2014-03-13 09:01:22]

perkj@ PTAL

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:172:
DCHECK(!source_formats_callback_.is_null());
On 2014/03/13 08:38:03, perkj wrote:
> That is not what I meant. This will now crash is StopDeliver is called. So I
> meant you should do 
> if !source_formats_callback_.is_null()
>  source_formats_callback_.Run(formats_in_use); 
> 

Done.

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:187:
DCHECK(!source_formats_callback_.is_null());
On 2014/03/13 08:38:03, perkj wrote:
> dito

Done.

https://codereview.chromium.org/195363002/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:199:
source_formats_callback_.Run(default_formats);
On 2014/03/13 08:38:03, perkj wrote:
> and here

Done.

[mcasas, 2014-03-13 16:14:10]

perkj@, tommi@ PTAL.

[tommi (sloooow) - chröme, 2014-03-13 17:19:37]

https://codereview.chromium.org/195363002/diff/160001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:23: {1920, 1080,
30},
nit: can you un-indent these by 2 spaces?

https://codereview.chromium.org/195363002/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:200:
source_formats_callback_.Run(default_formats);
whenever we call source_formats_callback_.Run(), should we call
source_formats_callback_.Reset() or is it OK to call
source_formats_callback_.Run() more than once in some situation?

[mcasas, 2014-03-14 08:06:38]

tommi@, perkj@ PTAL.

https://codereview.chromium.org/195363002/diff/160001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:23: {1920, 1080,
30},
On 2014/03/13 17:19:38, tommi wrote:
> nit: can you un-indent these by 2 spaces?

Done.

https://codereview.chromium.org/195363002/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:200:
source_formats_callback_.Run(default_formats);
On 2014/03/13 17:19:38, tommi wrote:
> whenever we call source_formats_callback_.Run(), should we call
> source_formats_callback_.Reset() or is it OK to call
> source_formats_callback_.Run() more than once in some situation?

There should only be one call back, so Reset() is fine.

[perkj_chrome, 2014-03-14 14:29:32]

I am not sure I understand the cl description in the following paragraphs? Are
you saying this is a bad cl that should not be landed? Can you update the
description? 

This CL would break compatibility with scenarios in 
which requested capture resolution or frame rate does 
not coincide with the one provided by hardware, 
notably Fake Video Capture Device supports only 
320x240 and 640x480 at 20fps.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:23: {1920, 1080,
30},
I think the previous indentation was correct except. 4 space.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:30: };
}; should probably be on the previous line.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:170: DVLOG(3) <<
"OnDeviceFormatsInUseReceived: " << formats_in_use.size();
if (source_formats_callback_.is_null())
  return;

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:177:
DCHECK(!source_formats_callback_.is_null());
remove this DCHECK

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:187: << "
received";
use early exist
if (source_formats_callback_.is_null())
  return;

[mcasas, 2014-03-16 10:18:02]

perkj@ PTAL.

Tried my best to rephrase the CL description, please check it out as well.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:23: {1920, 1080,
30},
On 2014/03/14 14:29:33, perkj wrote:
> I think the previous indentation was correct except. 4 space.

Reformatted according to clang-format chromium.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:30: };
On 2014/03/14 14:29:33, perkj wrote:
> }; should probably be on the previous line.

See above.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:170: DVLOG(3) <<
"OnDeviceFormatsInUseReceived: " << formats_in_use.size();
On 2014/03/14 14:29:33, perkj wrote:
> if (source_formats_callback_.is_null())
>   return;

Done.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:177:
DCHECK(!source_formats_callback_.is_null());
On 2014/03/14 14:29:33, perkj wrote:
> remove this DCHECK

Done.

https://codereview.chromium.org/195363002/diff/180001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:187: << "
received";
On 2014/03/14 14:29:33, perkj wrote:
> use early exist
> if (source_formats_callback_.is_null())
>   return;

Done.

[perkj_chrome, 2014-03-17 11:05:39]

lgtm with a nit.

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:175: // If there
are no formats in use, try to retrieve the whole list of them.
nit: of supported formats

[tommi (sloooow) - chröme, 2014-03-17 11:56:22]

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:185: if
(!source_formats_callback_.is_null())
if we get here and source_formats_callback_ is null, is that programmer error?
If so, we should remove this check and replace it with a DCHECK().  If not, can
you document here what could have happened that caused the callback to be set to
null during the operation.

[mcasas, 2014-03-17 15:12:58]

tommi@ PTAL.

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:175: // If there
are no formats in use, try to retrieve the whole list of them.
On 2014/03/17 11:05:40, perkj wrote:
> nit: of supported formats

Done.

https://codereview.chromium.org/195363002/diff/200001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:185: if
(!source_formats_callback_.is_null())
On 2014/03/17 11:56:22, tommi wrote:
> if we get here and source_formats_callback_ is null, is that programmer error?
> If so, we should remove this check and replace it with a DCHECK().  If not,
can
> you document here what could have happened that caused the callback to be set
to
> null during the operation.

Done.

[mcasas, 2014-03-19 13:43:53]

ping?

[tommi (sloooow) - chröme, 2014-03-19 13:54:41]

lgtm

[mcasas, 2014-03-31 10:06:57]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-03-31 10:07:06]

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

[commit-bot: I haz the power, 2014-03-31 11:22:01]

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

[commit-bot: I haz the power, 2014-03-31 11:22:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[mcasas, 2014-03-31 15:26:14]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:26:30]

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

[commit-bot: I haz the power, 2014-03-31 16:48:37]

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

[commit-bot: I haz the power, 2014-03-31 16:48:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[mcasas, 2014-04-01 08:37:45]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-01 08:37:52]

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

[commit-bot: I haz the power, 2014-04-01 10:04:36]

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

[commit-bot: I haz the power, 2014-04-01 10:04:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mcasas, 2014-04-03 12:37:00]

On 2014/04/01 10:04:37, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on linux_chromium_chromeos_rel

Waiting for perkj@ commit r261364 to roll through to the servers.

[mcasas, 2014-04-04 07:18:59]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-04 07:19:11]

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

[commit-bot: I haz the power, 2014-04-04 09:27:10]

Change committed as 261717