Added supported formats caching to VideoCaptureManager.

Added supported formats caching to VideoCaptureManager, and
adapted code to cideo_capture_types.{h,cc} reorganization.

Rewritten http://crrev.com/29423003 for the new structure of
video_capture_types.h: The device's capabilities are turned into
a vector of supported formats. No new code is added, the
previously reviewed one is adapted.

VideoCaptureCapability class is not used anymore, so removed (I
tried to make very evident every time I use VideoCaptureFormat
as a supported format).

VideoCaptureLinux and FakeVCD are also adapted, originally the
code for retrieving their caps comes from http://crrev.com/24079003

1 VCM unittest disabled for Mac Valgrind: Underlying FakeVCD
hits a SIGSEGV at the end of this test in Mac Valgrind bot,
see http://crbug.com/319955. Possibly associated to FakeVCD
races, re-enable this test after http://crbug.com/323893, 
ongoing http://crrev.com/91523002.


NOTE: Time ago a bug was filed against this code due to chrome OS
new user's take-a-picture would not show anything. I just verified
this seems to work now.

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

[mcasas, 2013-11-27 16:44:45]

perkj@, following our conversation, could you PTAL?

+tommi@ cc FYI.

[perkj_chrome, 2013-11-27 17:53:12]

lg but we need to fix the fake device first.

What was the Chrome os problem?

https://codereview.chromium.org/91343002/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/91343002/diff/40001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:250:
TEST_F(VideoCaptureManagerTest, ManipulateDeviceAndCheckCapabilities) {
Right- But as I said- the same code is used on all platforms so this should
sooner or later cause flakyness on all platforms.  So you can not land this
before the problem is solved.

https://codereview.chromium.org/91343002/diff/40001/content/common/media/vide...
File content/common/media/video_capture_messages.h (left):

https://codereview.chromium.org/91343002/diff/40001/content/common/media/vide...
content/common/media/video_capture_messages.h:23:
IPC_STRUCT_TRAITS_BEGIN(media::VideoCaptureCapability)
I am a bit confused. Have you added
IPC_STRUCT_TRAITS_BEGIN(media::VideoCaptureCapability or has someone just forgot
to remove this and decided to use VideoCaptureFormat?

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.h (left):

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.h:49: // EXPERIMENTAL, similar to
allocate, but changes resolution and calls
And this - is this intentional?

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.h:27: static int
NumberOfFakeDevices();
Fyi- I am trying to do the same thing in
https://codereview.chromium.org/88283002

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.h:65: static int
number_of_fake_devices_;
atomic32 if you touch this from several threads like I do in the cl mentioned
above.

[mcasas, 2013-12-03 14:28:40]

scherkus@ could you have a look at content/browser/renderer_host/media/* ?

cevans@ could you have a look at 
content/common/media/video_capture_messages.h ?
 -> Rationale: removed IPC message unused and associated
 to removed class VideoCaptureParams.

https://codereview.chromium.org/91343002/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/91343002/diff/40001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:250:
TEST_F(VideoCaptureManagerTest, ManipulateDeviceAndCheckCapabilities) {
We'll review that first then.

https://codereview.chromium.org/91343002/diff/40001/content/common/media/vide...
File content/common/media/video_capture_messages.h (left):

https://codereview.chromium.org/91343002/diff/40001/content/common/media/vide...
content/common/media/video_capture_messages.h:23:
IPC_STRUCT_TRAITS_BEGIN(media::VideoCaptureCapability)
On 2013/11/27 17:53:12, perkj wrote:
> I am a bit confused. Have you added
> IPC_STRUCT_TRAITS_BEGIN(media::VideoCaptureCapability or has someone just
forgot
> to remove this and decided to use VideoCaptureFormat?

sheu@ added it during the review, diff can be seen here:
https://codereview.chromium.org/83633008/diff/1/content/common/media/video_ca...

but AFAIK, is not used at all -- only use case is actually
the following CL to this. So is safe to remove.

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.h (left):

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.h:49: // EXPERIMENTAL, similar to
allocate, but changes resolution and calls
On 2013/11/27 17:53:12, perkj wrote:
> And this - is this intentional? 

Well, this code is not experimental anymore (I added this
comment). Besides, the OnFrameInfoChanged is deprecated.

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.h:27: static int
NumberOfFakeDevices();
On 2013/11/27 17:53:12, perkj wrote:
> Fyi- I am trying to do the same thing in
> https://codereview.chromium.org/88283002

First one wins! :)
Kidding: I'll rebase against you.

https://codereview.chromium.org/91343002/diff/40001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.h:65: static int
number_of_fake_devices_;
On 2013/11/27 17:53:12, perkj wrote:
> atomic32 if you touch this from several threads like I do in the cl mentioned
> above.

If you only have one setter and multiple getters, is there
a race?

[scherkus (not reviewing), 2013-12-03 22:04:47]

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager.cc:284:
DCHECK(device_in_use);
if you're DCHECK'ing this is non-null ... but then using an if statement on the
next line it's unclear whether you expect this to be null or non-null

if this should never be non-null, then remove the if statement

if this can be null, then remove the DCHECK

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:176:
.Times(1)
nit: Times(1) is implied when using EXPECT_CALL()
https://code.google.com/p/googlemock/wiki/CheatSheet#Setting_Expectations

I'd recommend not using it in your new test below and save yourselve some lines
of code

(you may want to send out a separate follow up CL that cleans up Times(1) usage)

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.cc:11: #include
"base/strings/string_number_conversions.h"
I think you can remove this header file now that you're using StringPrintf()

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
media/video/capture/linux/video_capture_device_linux.cc:150: 
nit: remove extra blank line

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
media/video/capture/linux/video_capture_device_linux.cc:153: int fd;
it would be nice to use ScopedFD from base/

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
media/video/capture/linux/video_capture_device_linux.cc:154: if ((fd =
open(device.id().c_str(), O_RDONLY)) < 0)
shouldn't these calls be using HANDLE_EINTR()?

can you file a bug and add a TODO to audit this whole file for proper
HANDLE_EINTR() and other POSIX open/ioctl/close/etc... usage?

[mcasas, 2013-12-04 15:23:32]

Hi scherkus@, ptal.

cevans@: ping.

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager.cc:284:
DCHECK(device_in_use);
On 2013/12/03 22:04:48, scherkus wrote:
> if you're DCHECK'ing this is non-null ... but then using an if statement on
the
> next line it's unclear whether you expect this to be null or non-null
> 
> if this should never be non-null, then remove the if statement
> 
> if this can be null, then remove the DCHECK

Done.

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/91343002/diff/60001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:176:
.Times(1)
Thanks, see http://crbug.com/325740.

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/fake_...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/fake_...
media/video/capture/fake_video_capture_device.cc:11: #include
"base/strings/string_number_conversions.h"
On 2013/12/03 22:04:48, scherkus wrote:
> I think you can remove this header file now that you're using StringPrintf()

Done.

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
media/video/capture/linux/video_capture_device_linux.cc:150: 
On 2013/12/03 22:04:48, scherkus wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
media/video/capture/linux/video_capture_device_linux.cc:153: int fd;
On 2013/12/03 22:04:48, scherkus wrote:
> it would be nice to use ScopedFD from base/

Same as below, http://crbug.com/324428 for this file's
cleanup and chromium-magic use.

https://codereview.chromium.org/91343002/diff/60001/media/video/capture/linux...
media/video/capture/linux/video_capture_device_linux.cc:154: if ((fd =
open(device.id().c_str(), O_RDONLY)) < 0)
On 2013/12/03 22:04:48, scherkus wrote:
> shouldn't these calls be using HANDLE_EINTR()?
> 
> can you file a bug and add a TODO to audit this whole file for proper
> HANDLE_EINTR() and other POSIX open/ioctl/close/etc... usage?

I reused http://crbug.com/324428 which already points out that the
|v4l2_thread_| might be not needed, to add this point, thanks!

[mcasas, 2013-12-09 15:05:40]

Actually I didn't need to bother cevans@, scherkus@ can
give LGTM's for:

content/common/media/video_capture_messages.h

 -> Rationale: removed IPC message unused and associated
 to removed class VideoCaptureParams.

[scherkus (not reviewing), 2013-12-09 19:01:51]

lgtm

[scherkus (not reviewing), 2013-12-09 19:01:52]

lgtm

[mcasas, 2013-12-10 11:20:32]

perkj@: it seems that "Rebase" made a big mess, PTAL now.

inferno@: content/common/media/video_capture_messages.h

[perkj_chrome, 2013-12-10 12:27:13]

lgtm if the below is addressed.

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:410:
media::VideoCaptureDevice::Names::iterator it;
nit: prefer to minimize the scope and put this in the for loop.

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:241: //
check this test after http://crbug.com/323893.
looks like this is fixed. Remove the comments and also remove comments in the cl
description. http://crbug.com/323893

Run Mac ValGrind try server with this patch?

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:272:
ASSERT_EQ(supported_formats.size(), 1u);
nit: This can be EXPECT_EQ

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:40: Name
name(base::StringPrintf("fake_device_%d", n),
Seems like we keep swithing StringPrintF or and IntToString back and forth. But
ok...

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.h:29: static int
NumberOfFakeDevices();
size_t to match SetNumberOfFakeDevices

[mcasas, 2013-12-12 10:38:04]

Hi tsepez@: content/common/media/video_capture_messages.h OWNERS stamp please.

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:410:
media::VideoCaptureDevice::Names::iterator it;
On 2013/12/10 12:27:14, perkj wrote:
> nit: prefer to minimize the scope and put this in the for loop.

Done.

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:241: //
check this test after http://crbug.com/323893.
On 2013/12/10 12:27:14, perkj wrote:
> looks like this is fixed. Remove the comments and also remove comments in the
cl
> description. http://crbug.com/323893
> 
> Run Mac ValGrind try server with this patch?

Done.

https://codereview.chromium.org/91343002/diff/210001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:272:
ASSERT_EQ(supported_formats.size(), 1u);
On 2013/12/10 12:27:14, perkj wrote:
> nit: This can be EXPECT_EQ

I prefer the test to fail and not continue if the list
is not of size 1 exactly. IOW it doesn't make sense to
continue this test case. Sense?

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.cc:40: Name
name(base::StringPrintf("fake_device_%d", n),
On 2013/12/10 12:27:14, perkj wrote:
> Seems like we keep swithing StringPrintF or and IntToString back and forth.
But
> ok...

:D I thought this is more performant :D

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/91343002/diff/210001/media/video/capture/fake...
media/video/capture/fake_video_capture_device.h:29: static int
NumberOfFakeDevices();
On 2013/12/10 12:27:14, perkj wrote:
> size_t to match SetNumberOfFakeDevices

Done.

[Tom Sepez, 2013-12-12 17:34:14]

Rubberstamp LGTM on deleting code from video_capture_messages.h

[commit-bot: I haz the power, 2013-12-12 17:36:25]

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

[commit-bot: I haz the power, 2013-12-12 20:40:28]

Change committed as 240395