Mac AVFoundation: Blackmagic camera blacklisting update

Mac AVFoundation: Blackmagic camera blacklisting update

crrev.com/255933006 landed blacklisting for BlackMagic
cameras in Mac: QTKit would be used instead of AVFoundation
for these devices.

The code was based on the assumption that these cameras
have a USB vendor ID but this is not correct since they
use a Thunderbolt connection, i.e. PCIe, and another
crrev.com/366593003 is going to prevent GetModel().

This CL changes to using a characteristic substring of
the uniqueId.

TEST: Plug in a Blackmagic, f.i. an UltraStudio Mini
Recorder (doesn't need any camera connected to it) and
check enumerate devices, there should be a multiplicity
of devices with a "QTKit..." prefix, indicating there was
a blacklisting process.

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

[mcasas, 2014-07-03 08:11:57]

tommi@ PTAL.

[tommi (sloooow) - chröme, 2014-07-03 09:51:39]

On 2014/07/03 08:11:57, mcasas wrote:
> tommi@ PTAL.

FYI on the description.  Chromium uses "TEST=<what QA should do to verify
correctness>" and not "TESTED=<what I did to check if this works>".

[tommi (sloooow) - chröme, 2014-07-03 09:56:56]

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

https://codereview.chromium.org/362323003/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:110:
is_any_device_blacklisted |= (strcasestr(name.id().c_str(),
I don't recall seeing strcasestr being used before (I don't think it's
standard). Can you use strcasecmp() instead? (see base/strings/string_util.h)

https://codereview.chromium.org/362323003/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:110:
is_any_device_blacklisted |= (strcasestr(name.id().c_str(),
no need for |=.  Just use = since you'll break as soon as it becomes true.

https://codereview.chromium.org/362323003/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:111:
kBlacklistedCameras[i].unique_id_signature) != NULL);
!= 0
(since the function returns int, not a pointer)

[mcasas, 2014-07-03 12:52:56]

tommi@ thanks for the TEST/TESTED tip :) , PTAL

The |unique_id_signature| is a substring of the device's unique_id,
see f.i. [1].

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

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

https://codereview.chromium.org/362323003/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:110:
is_any_device_blacklisted |= (strcasestr(name.id().c_str(),
On 2014/07/03 09:56:56, tommi wrote:
> I don't recall seeing strcasestr being used before (I don't think it's
> standard). Can you use strcasecmp() instead? (see base/strings/string_util.h)

Oops, you're right, seems to be BSD-only.

I refactored a bit the code and now it uses std::string::find.

https://codereview.chromium.org/362323003/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:110:
is_any_device_blacklisted |= (strcasestr(name.id().c_str(),
On 2014/07/03 09:56:56, tommi wrote:
> no need for |=.  Just use = since you'll break as soon as it becomes true.

Done.

https://codereview.chromium.org/362323003/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_factory_mac.mm:111:
kBlacklistedCameras[i].unique_id_signature) != NULL);
On 2014/07/03 09:56:56, tommi wrote:
> != 0
> (since the function returns int, not a pointer)

Done.

[tommi (sloooow) - chröme, 2014-07-03 13:35:14]

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

https://codereview.chromium.org/362323003/diff/20001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:23: }
kBlacklistedCameras[] = { { "01FDA82C8A9C", "Blackmagic" } };
Should this be "-01FDA82C8A9C" to be on the safe side?

https://codereview.chromium.org/362323003/diff/20001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:111:
name.id().find(kBlacklistedCameras[i].unique_id_signature)
if I understand correctly, you want to make sure the string ends with the
signature, correct?  If so, you could use EndsWith() (see string_util.h)

[mcasas, 2014-07-03 13:50:26]

PTAL.

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

https://codereview.chromium.org/362323003/diff/20001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:23: }
kBlacklistedCameras[] = { { "01FDA82C8A9C", "Blackmagic" } };
On 2014/07/03 13:35:14, tommi wrote:
> Should this be "-01FDA82C8A9C" to be on the safe side?

Done.

https://codereview.chromium.org/362323003/diff/20001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:111:
name.id().find(kBlacklistedCameras[i].unique_id_signature)
On 2014/07/03 13:35:14, tommi wrote:
> if I understand correctly, you want to make sure the string ends with the
> signature, correct?  If so, you could use EndsWith() (see string_util.h)

Nice! Done.

[tommi (sloooow) - chröme, 2014-07-03 14:06:34]

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

https://codereview.chromium.org/362323003/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:20: // substrings of
the uniqueId and name, the latter usually the vendor's name.
nit: change 'characteristic substrings' to ... hmm... 'the last part of the
identifier string' (? I'm sure you can come up with something better)

https://codereview.chromium.org/362323003/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:111: const bool
kCaseInsensitive = false;
uhm... kCaseInsensitive = false, meanse case sensitive, but the argument is
actually case_sensitive, so although this was meant to clarify, I'm afraid it's
confusing :)  What about just passing |false| to the function? :)

[mcasas, 2014-07-03 16:07:38]

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

https://codereview.chromium.org/362323003/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:20: // substrings of
the uniqueId and name, the latter usually the vendor's name.
On 2014/07/03 14:06:33, tommi wrote:
> nit: change 'characteristic substrings' to ... hmm... 'the last part of the
> identifier string' (? I'm sure you can come up with something better)

Done.

https://codereview.chromium.org/362323003/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:111: const bool
kCaseInsensitive = false;
On 2014/07/03 14:06:34, tommi wrote:
> uhm... kCaseInsensitive = false, meanse case sensitive, but the argument is
> actually case_sensitive, so although this was meant to clarify, I'm afraid
it's
> confusing :)  What about just passing |false| to the function? :)

Yeah, the clarification didn't clarify much :) Done.

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

ping.

[tommi (sloooow) - chröme, 2014-07-08 07:59:12]

oops sorry. lgtm.

[mcasas, 2014-07-08 08:19:55]

The CQ bit was checked by mcasas@chromium.org

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

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

[commit-bot: I haz the power, 2014-07-08 10:27:50]

Change committed as 281707