Mac QTKit Video Capture: Force BlackMagic cameras to be opened in HD

Mac QTKit Video Capture: Force BlackMagic cameras to be opened in HD

This is a refry of http://crrev.com/410363002, original description:
>Force BlackMagic cameras to be opened in HD
>
>The Blackmagic driver causes a crash in QTKit when opened in VGA
>when NTSC/PAL 10-bit is selected.  QTKit does not allow us to query a
>device's supported resolution, so this change adds BlackMagic to another
>blacklist so that it is always opened in HD.

In this continuation CL, Blacklisted QTKit devices are found during
enumeration and flagged, so
a) a special capability list is cooked for them, only including HD,
b) they are not reconfigured on opening/capturing.

BUG=396812, 347371
COLLABORATOR= vrk@chromium.org
R=tommi@chromium.org, vrk@chromium.org

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

[mcasas, 2014-08-13 11:01:20]

vrk@ PTAL
tommi@ Owners RS / PTAL.

rsesek@: CC'd since this doesn't touch Obj-C++ code but would be happy if you
want to take a look.

[tommi (sloooow) - chröme, 2014-08-13 12:42:29]

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:28: const gfx::Size
kBlackmagicQTKitCaptureResolution = gfx::Size(1280, 720);
is this a global that requires construction? (if so, your cl will be reverted by
perf sheriffs)

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:29: const float
kBlackmagicQTKitCaptureFrameRate = 60.0f;
These two constants seem specific to blackmagic cameras.  Should these perhaps
rather be properties of the NameAndVid struct above and therefore be settable
for other devices in the future if we need to?

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:194:
kBlackmagicQTKitCaptureResolution, kBlackmagicQTKitCaptureFrameRate,
here, there's an assumption that blacklisted==blackmagic which defeats having a
list

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_mac.mm:46: const float
kMinFrameRate = 1.0f;
can you add documentation for these constants?  I'm assuming they only apply in
specific situations (since 60.0f is being used elsewhere)

[mcasas, 2014-08-13 14:04:53]

vrk@, tommi@ PTAL

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:28: const gfx::Size
kBlackmagicQTKitCaptureResolution = gfx::Size(1280, 720);
On 2014/08/13 12:42:28, tommi wrote:
> is this a global that requires construction? (if so, your cl will be reverted
by
> perf sheriffs)

Done.

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:29: const float
kBlackmagicQTKitCaptureFrameRate = 60.0f;
On 2014/08/13 12:42:28, tommi wrote:
> These two constants seem specific to blackmagic cameras.  Should these perhaps
> rather be properties of the NameAndVid struct above and therefore be settable
> for other devices in the future if we need to?

Done.

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:194:
kBlackmagicQTKitCaptureResolution, kBlackmagicQTKitCaptureFrameRate,
On 2014/08/13 12:42:29, tommi wrote:
> here, there's an assumption that blacklisted==blackmagic which defeats having
a
> list

Done.

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/468833002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_mac.mm:46: const float
kMinFrameRate = 1.0f;
On 2014/08/13 12:42:29, tommi wrote:
> can you add documentation for these constants?  I'm assuming they only apply
in
> specific situations (since 60.0f is being used elsewhere)

Done.

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

lgtm

[vrk (LEFT CHROMIUM), 2014-08-14 02:50:54]

lgtm

https://codereview.chromium.org/468833002/diff/20001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/468833002/diff/20001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:46: if
(name.id().find(kBlacklistedCameras[i].name) != std::string::npos) {
Does case sensitivity matter?

[mcasas, 2014-08-14 07:35:59]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2014-08-14 07:36:06]

https://codereview.chromium.org/468833002/diff/20001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/468833002/diff/20001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:46: if
(name.id().find(kBlacklistedCameras[i].name) != std::string::npos) {
On 2014/08/14 02:50:54, Victoria Kirst wrote:
> Does case sensitivity matter?

ATM I've have always seen "Blackmagic" as part of the name, so nothing
hints to the need for case insensitivity. 

As a side note on Blackmagic, if using AVFoundation (beyond the scope
of this CL), name.id() would always return :
 21881B92-C9F3-42BD-B238-01FDA82C8A9C
for all and any Blackmagics plugged in the system, which prevents from
using more than one device from that brand simultaneously! [1] :(


[1] http://forum.blackmagicdesign.com/viewtopic.php?f=11&t=16092

[commit-bot: I haz the power, 2014-08-14 07:38:20]

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

[mcasas, 2014-08-14 14:11:50]

Committed patchset #2 manually as 289552 (presubmit successful).