Mac VideoCapture: Support for Blackmagic DeckLink device & capabilities enumeration, using SDK.

media/video/capture/mac/video_capture_device_decklink_mac.mm

Index: media/video/capture/mac/video_capture_device_decklink_mac.mm
diff --git a/media/video/capture/mac/video_capture_device_decklink_mac.mm b/media/video/capture/mac/video_capture_device_decklink_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..c58995fd550cb22e3ba4b6d6aaccde9bde27d8c0
--- /dev/null
+++ b/media/video/capture/mac/video_capture_device_decklink_mac.mm
@@ -0,0 +1,138 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/video/capture/mac/video_capture_device_decklink_mac.h"
+
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "third_party/decklink/mac/include/DeckLinkAPI.h"
+
+namespace {
+
+// DeckLink SDK uses ScopedComPtr-style APIs. Chrome ScopedComPtr is only
+// available for Windows builds. This is a knock-off of the needed parts of
+// base::win::ScopedComPtr<> for ref counting.
+template <class T>
+class ScopedDeckLinkPtr : public scoped_refptr<T> {
+ public:
+  using scoped_refptr<T>::ptr_;
+
+  T** Receive() {
+    DCHECK(!ptr_) << "Object leak. Pointer must be NULL";
+    return &ptr_;
+  }
+
+  void** ReceiveVoid() {
+    return reinterpret_cast<void**>(Receive());
+  }
+
+  void Release() {
+    if (ptr_ != NULL) {

[magjed_chromium, 2014/09/01 10:41:13]

Sometimes you compare with NULL, sometimes you don't. I'm not sure what the
style guide dictates, but I guess you should be consistent?

[mcasas, 2014/09/01 14:04:49]

This is copy-paste from [1] and I wanted to leave it as close to the 
original as possible. Makes sense?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_co...

[magjed_chromium, 2014/09/01 14:44:37]

Ok, but you do it in the rest of the code as well, e.g.
DLOG_IF(ERROR, decklink_iter != NULL)
and
if (!decklink_iter)

+      ptr_->Release();
+      ptr_ = NULL;
+    }
+  }
+};
+
+}  // namespace
+
+namespace media {
+
+//static
+void VideoCaptureDeviceDeckLinkMac::EnumerateDevices(
+    VideoCaptureDevice::Names* device_names) {
+  scoped_refptr<IDeckLinkIterator> decklink_iter(
+      CreateDeckLinkIteratorInstance());
+  DLOG_IF(ERROR, decklink_iter != NULL) << "Error creating DeckLink iterator";

[magjed_chromium, 2014/09/01 14:44:37]

This looks wrong, shouldn't it be "decklink_iter == NULL" instead of
"decklink_iter != NULL"?
Also, you might want to change to "!decklink_iter" to be consistent with the
next line.

+  if (!decklink_iter)
+    return;
+
+  ScopedDeckLinkPtr<IDeckLink> decklink;
+  while (decklink_iter->Next(decklink.Receive()) == S_OK) {
+    CFStringRef device_model_name = NULL;
+    HRESULT hr = decklink->GetModelName(&device_model_name);
+    DVLOG_IF(1, hr != S_OK) << "Error reading Blackmagic device model name";
+    CFStringRef device_display_name = NULL;
+    hr = decklink->GetDisplayName(&device_display_name);
+    DVLOG_IF(1, hr != S_OK) << "Error reading Blackmagic device display name";
+    DVLOG_IF(1, hr == S_OK) << "Blackmagic camera found, name: " <<
+        [(NSString*)device_display_name UTF8String];
+
+    VideoCaptureDevice::Name name([(NSString*)device_model_name UTF8String],
+                                  [(NSString*)device_display_name UTF8String],
+                                  VideoCaptureDevice::Name::DECKLINK,
+                                  VideoCaptureDevice::Name::OTHER_TRANSPORT);
+    device_names->push_back(name);

[magjed_chromium, 2014/09/01 10:41:13]

Should we really add this device even if GetModelName and/or GetDisplayName
failed?

[mcasas, 2014/09/01 14:04:50]

Done.

+    decklink.Release();
+  }
+}
+
+// static
+void VideoCaptureDeviceDeckLinkMac::EnumerateDeviceCapabilities(
+    const VideoCaptureDevice::Name& device,
+    VideoCaptureFormats* supported_formats) {
+  ScopedDeckLinkPtr<IDeckLink> decklink;
+  scoped_refptr<IDeckLinkIterator> decklink_iter(
+      CreateDeckLinkIteratorInstance());
+  DLOG_IF(ERROR, decklink_iter != NULL) << "Error creating DeckLink iterator";

[magjed_chromium, 2014/09/01 14:44:37]

Same as above.

+  if (!decklink_iter)
+    return;
+
+  while (decklink_iter->Next(decklink.Receive()) == S_OK) {
+    CFStringRef device_model_name = NULL;
+    if (decklink->GetModelName(&device_model_name) != S_OK)
+      continue;
+    if (device.id().compare([(NSString*)device_model_name UTF8String]) != 0)
+      continue;
+
+    ScopedDeckLinkPtr<IDeckLinkInput> decklink_input;
+    if (decklink->QueryInterface(IID_IDeckLinkInput,
+            decklink_input.ReceiveVoid()) != S_OK) {
+      DLOG(ERROR) << "Error Blackmagic querying input interface.";
+      return;
+    }
+
+    ScopedDeckLinkPtr<IDeckLinkDisplayModeIterator> display_mode_iter;
+    if (decklink_input->GetDisplayModeIterator(display_mode_iter.Receive())
+        != S_OK) {

[perkj_chrome, 2014/09/01 09:50:41]

nit try to keep  != on the line with the first argument you compare with. 

if (decklink_input->GetDisplayModeIterator(
    display_mode_iter.Receive()) != S_OK) {

[mcasas, 2014/09/01 14:04:50]

Done.

+      continue;
+    }
+
+    ScopedDeckLinkPtr<IDeckLinkDisplayMode> displayMode;
+    while (display_mode_iter->Next(displayMode.Receive()) == S_OK) {
+      // IDeckLinkDisplayMode does not have information on pixel format, it
+      // is only available on capture.
+      media::VideoPixelFormat pixelFormat = media::PIXEL_FORMAT_UNKNOWN;
+      BMDTimeValue time_value, time_scale;
+      float frame_rate = 0.0f;
+      if (displayMode->GetFrameRate(&time_value, &time_scale) == S_OK &&
+          time_value > 0) {
+        frame_rate = static_cast<float>(time_scale) / time_value;
+      }
+      media::VideoCaptureFormat format(
+          gfx::Size(displayMode->GetWidth(), displayMode->GetHeight()),
+          frame_rate,
+          pixelFormat);
+      supported_formats->push_back(format);
+      DVLOG(2) << device.name() << " resolution: "

[magjed_chromium, 2014/09/01 10:41:13]

I have seen similar code like this in at least four other places. Maybe we
should add a convenience function
std::ostream& operator<<(std::ostream&, const media::VideoCaptureFormat&)
and replace the duplicated code?

[mcasas, 2014/09/01 14:04:49]

Probably. http://crbug.com/409636

+               << format.frame_size.ToString() << "@: " << format.frame_rate
+               << ", pixel format: " << format.pixel_format;
+      displayMode.Release();
+    }
+    decklink.Release();
+  }
+}
+
+VideoCaptureDeviceDeckLinkMac::VideoCaptureDeviceDeckLinkMac(
+    const Name& device_name) {}
+
+VideoCaptureDeviceDeckLinkMac::~VideoCaptureDeviceDeckLinkMac() {}
+
+void VideoCaptureDeviceDeckLinkMac::AllocateAndStart(
+    const VideoCaptureParams& params,
+    scoped_ptr<VideoCaptureDevice::Client> client) {}

[perkj_chrome, 2014/09/01 09:50:41]

add NOTIMPLEMENTED

[mcasas, 2014/09/01 14:04:50]

Done.

+
+void VideoCaptureDeviceDeckLinkMac::StopAndDeAllocate() {}

[perkj_chrome, 2014/09/01 09:50:41]

add NOTIMPLEMENTED

[mcasas, 2014/09/01 14:04:49]

Done.

+
+} // namespace media