Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
229063003:
Mac AVFoundation: use QTKit for cameras not working with AVF (Blackmagic). (Closed)
|
Created:
6 years, 8 months ago by mcasas Modified:
6 years, 8 months ago CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, miu+watch_chromium.org, vrk (LEFT CHROMIUM) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMac AVFoundation: use QTKit for devices not working with AVF (Blackmagic).
Some video capture devices are not correctly supported
in Mac when using AVFoundation. F.e. the Blackmagic
UltraStudio Mini Recorder only works correctly when
opened in 480p or less.
A blacklist is added to video_capture_device_mac.mm to
identify those devices. The blacklisted devices are
identified by USB Vendor ID. This list is checked while
enumerating the devices using AVFoundation in
VideoCaptureDevice::GetDeviceNames(). If a singled out
device is found, the QTKit list of devices is parsed
as well and those with a similar name are added
to the "final" list of devices, with a "QTKit" prefix.
The VideoCaptureDevice::Name CaptureApiType, only
used for Windows devices, is extended to identify
if the device is a QTKit or an AVFoundation.
Tested via adding an artificial devices to the
blacklist, a "Hercules" camera, otherwise innocent.
BUG=288562, 347371
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263235
Patch Set 1 : #
Total comments: 12
Patch Set 2 : rsesek@ comments #
Total comments: 2
Patch Set 3 : //static #
Total comments: 12
Patch Set 4 : rsesek@ comments #
Total comments: 5
Patch Set 5 : rsesek@ nits #
Messages
Total messages: 16 (0 generated)
rsesek@, tommi@, PTAL.
This seems like the kind of thing you'd want a unittest for. https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:41: &kQVGA, nit: should only be indented 2 spaces https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:77: bool is_blacklisted_device = FALSE; nit: lowercase false https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:84: [key UTF8String], Name::AVFOUNDATION); nit: only indent 4 spaces from the previous line https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:85: device_names->push_back(name); Why include blacklisted devices in the list? https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:99: [key UTF8String], Name::QTKIT); nit: only indent 4 spaces from the previous line https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:117: [key UTF8String], Name::QTKIT); nit: indent
rsesek@ PTAL. Unittest: I thought about it too, scope seems a bit larger than this CL, since video_capture_device_unittest.cc is C++ code only, while this code lives in Objective-C. Perhaps I could add a TODO for a video_capture_device_mac_unittests.mm, WDYT? https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:41: &kQVGA, On 2014/04/08 20:16:05, rsesek wrote: > nit: should only be indented 2 spaces Done. https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:77: bool is_blacklisted_device = FALSE; On 2014/04/08 20:16:05, rsesek wrote: > nit: lowercase false Done. https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:84: [key UTF8String], Name::AVFOUNDATION); On 2014/04/08 20:16:05, rsesek wrote: > nit: only indent 4 spaces from the previous line Done. https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:85: device_names->push_back(name); On 2014/04/08 20:16:05, rsesek wrote: > Why include blacklisted devices in the list? They might still work with AVFoundation in some cases, i.e. Blackmagic works in 480p, 240p, but not in 720p. Do you think it'd cause confusion? The QTKit ones have "QTKit" prefix. https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:99: [key UTF8String], Name::QTKIT); On 2014/04/08 20:16:05, rsesek wrote: > nit: only indent 4 spaces from the previous line Done. https://codereview.chromium.org/229063003/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:117: [key UTF8String], Name::QTKIT); On 2014/04/08 20:16:05, rsesek wrote: > nit: indent Done.
I am traveling today but can rs lgtm the change fwiw. Can you let vrk@ know about it? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/09 10:41:29, mcasas wrote: > Unittest: I thought about it too, scope seems a bit larger > than this CL, since video_capture_device_unittest.cc is > C++ code only, while this code lives in Objective-C. > Perhaps I could add a TODO for a > video_capture_device_mac_unittests.mm, WDYT? Is it more complex than just moving it to a .mm file and adding the test?
https://codereview.chromium.org/229063003/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:71: // static
https://codereview.chromium.org/229063003/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:71: On 2014/04/10 16:19:38, rsesek wrote: > // static Done.
https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:83: for (NSString* key in capture_devices) { I'd maybe add some comments: // Enumerate all the devices found by AVFoundation. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:88: // Compare the USB VendorID of the device to the blacklisted ones. You're still adding blacklisted devices, though, right? So for a blacklisted device, there will also be one with the same name prefixed with QTKit? https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:89: for (size_t i = 0; i < arraysize(kBlacklistedCameras); ++i) { // Find any blacklisted devices, and if there are some <answer to the question above>. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:91: if (is_blacklisted_device) Doesn't this mean if your blacklist has more than one device, and the user has several blacklisted devices, only one will be blacklisted in practice? https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:105: if (is_blacklisted_device) { Since this only happens for AVFoundation, maybe fold it into that branch above? https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:117: Name name("QTKit " + std::string([device_name UTF8String]), This string isn't visible anywhere, is it?
rsesek@ PTAL. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:83: for (NSString* key in capture_devices) { On 2014/04/10 17:49:10, rsesek wrote: > I'd maybe add some comments: > > // Enumerate all the devices found by AVFoundation. Done. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:88: // Compare the USB VendorID of the device to the blacklisted ones. On 2014/04/10 17:49:10, rsesek wrote: > You're still adding blacklisted devices, though, right? So for a blacklisted > device, there will also be one with the same name prefixed with QTKit? Yes, exactly, point being that some resolutions of the blacklisted devices work with AVFoundation. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:89: for (size_t i = 0; i < arraysize(kBlacklistedCameras); ++i) { On 2014/04/10 17:49:10, rsesek wrote: > // Find any blacklisted devices, and if there are some <answer to the question > above>. Done. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:91: if (is_blacklisted_device) On 2014/04/10 17:49:10, rsesek wrote: > Doesn't this mean if your blacklist has more than one device, and the user has > several blacklisted devices, only one will be blacklisted in practice? Wrong variable name, corrected to |is_any_device_blacklisted| . The continue stops the comparison between the enumerated device and the blacklisted devices, but does not prevent from posterior device name comparisons. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:105: if (is_blacklisted_device) { On 2014/04/10 17:49:10, rsesek wrote: > Since this only happens for AVFoundation, maybe fold it into that branch above? Done. https://codereview.chromium.org/229063003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:117: Name name("QTKit " + std::string([device_name UTF8String]), On 2014/04/10 17:49:10, rsesek wrote: > This string isn't visible anywhere, is it? This is the string that shows when clicking on the camera icon inside the omnibox after a successful getUserMedia(), so is shown to the user. The blacklisted device is a fairly specialised and high end one, needing external configuration and having all kinds of whistles and bells. Plus, the Blackmagic device is multiplexed into a number of "virtual" devices (see the bug for the listing), so the potential target user of this code is definitely camera savvy, hence I thought it fair to show both AVFoundation naming and QTKit- preceded ones and let the user decide. https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:74: // Loop through all available devices and add to |device_names|. Removed device_names->clear() to align with the other implementations.
LGTM w/ nits. https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:91: if (is_any_device_blacklisted) This if isn't necessary, is it? You'll continue the loop regardless. https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:101: NSString* device_name; You can declare this within the loop.
https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:91: if (is_any_device_blacklisted) On 2014/04/10 21:18:31, rsesek wrote: > This if isn't necessary, is it? You'll continue the loop regardless. Oops, this was supposed to be a "break" iso a continue, corrected. https://codereview.chromium.org/229063003/diff/90001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:101: NSString* device_name; On 2014/04/10 21:18:31, rsesek wrote: > You can declare this within the loop. Done.
lgtm
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/229063003/100001
Message was sent while issue was closed.
Change committed as 263235 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
