Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(184)

Issue 535983002: Mac Video Capture: Support for Blackmagic DeckLink SDK video capture. (Closed)

Created:
6 years, 3 months ago by mcasas
Modified:
6 years, 2 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@crbug408493__1__Enumerate_blackmagic_devices__2__branched_from_master
Project:
chromium
Visibility:
Public.

Description

Mac Video Capture: Support for Blackmagic DeckLink SDK video capture. Implementation of necessario VideoCaptureDevice methods to capture video using BlackMagic DeckLink SDK. Internal class DeckLinkCaptureDelegate does all the hard work of configuring and starting/stopping the capture by implementing IDeckLinkInputCallback and IUnknown interfaces as required by the SDK. This class is reference counted by both the SDK and VCDDeckLinkMac. VideoCaptureDeviceDeckLinkMac is plugged by VCDFactoryMac instead of VCDMac for the appropriate devices. This is a follow up to http://crrev.com/509313002 and http://crrev.com/518073002 BUG=408493, 408534 Committed: https://crrev.com/4729b02c56fc13081c742b9113117e7291537fbc Cr-Commit-Position: refs/heads/master@{#297668}

Patch Set 1 : #

Total comments: 14

Patch Set 2 : magjed@s comments #

Patch Set 3 : Rebase. #

Total comments: 47

Patch Set 4 : tommi@s comments. Extracted DeckLinkCaptureDelegate. #

Total comments: 2

Patch Set 5 : Rebased to latest predecessor CL. Bug fix in DeckLinkCaptureDelegate::AllocateAndStart() #

Patch Set 6 : Rebased against master #

Total comments: 27

Patch Set 7 : tommi@s comments #

Total comments: 5

Patch Set 8 : tommi@s comments++ #

Patch Set 9 : Explicit cast in QueryInterface(); using labs() iso abs(). #

Total comments: 1

Patch Set 10 : Nit. #

Total comments: 24

Patch Set 11 : rsesek@ comments #

Total comments: 5

Patch Set 12 : rsesek@ comments: Made AddRef()/Release() depend on base::RefCountedThreadSafe<> impl. #

Total comments: 1

Patch Set 13 : Public base::RefCountedThreadSafe<DeckLinkCaptureDelegate> inheritance. #

Total comments: 9

Patch Set 14 : tommi@s comments #

Total comments: 2

Patch Set 15 : tommi@s nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+371 lines, -31 lines) Patch
M media/video/capture/mac/video_capture_device_decklink_mac.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +41 lines, -4 lines 0 comments Download
M media/video/capture/mac/video_capture_device_decklink_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +319 lines, -19 lines 0 comments Download
M media/video/capture/mac/video_capture_device_factory_mac.mm View 1 2 3 4 5 6 2 chunks +11 lines, -8 lines 0 comments Download

Messages

Total messages: 39 (10 generated)
mcasas
magjed@ PTAL.
6 years, 3 months ago (2014-09-04 10:48:00 UTC) #3
magjed_chromium
https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode81 media/video/capture/mac/video_capture_device_decklink_mac.mm:81: media::VideoPixelFormat pixel_format = media::PIXEL_FORMAT_UNKNOWN; Why do you need to ...
6 years, 3 months ago (2014-09-04 13:39:19 UTC) #4
mcasas
magjed@, tommi@ PTAL https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode81 media/video/capture/mac/video_capture_device_decklink_mac.mm:81: media::VideoPixelFormat pixel_format = media::PIXEL_FORMAT_UNKNOWN; On 2014/09/04 ...
6 years, 3 months ago (2014-09-05 09:00:40 UTC) #6
tommi (sloooow) - chröme
https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/video_capture_device_decklink_mac.h File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/video_capture_device_decklink_mac.h#newcode25 media/video/capture/mac/video_capture_device_decklink_mac.h:25: using scoped_refptr<T>::ptr_; why do you need this? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/video_capture_device_decklink_mac.h#newcode74 media/video/capture/mac/video_capture_device_decklink_mac.h:74: ...
6 years, 3 months ago (2014-09-05 09:24:22 UTC) #7
mcasas
tommi@ PTAL. Big change: splitted VCDDeckLinkMac into two classes, one called like the original and ...
6 years, 3 months ago (2014-09-09 11:05:01 UTC) #9
magjed_chromium
https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode159 media/video/capture/mac/video_capture_device_decklink_mac.mm:159: while (display_mode_iter->Next(display_mode.Receive()) == S_OK) { This code for finding ...
6 years, 3 months ago (2014-09-09 14:36:24 UTC) #10
mcasas
guys PTAL. https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode159 media/video/capture/mac/video_capture_device_decklink_mac.mm:159: while (display_mode_iter->Next(display_mode.Receive()) == S_OK) { On 2014/09/09 ...
6 years, 3 months ago (2014-09-22 14:42:36 UTC) #13
tommi (sloooow) - chröme
https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/video_capture_device_decklink_mac.h File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/video_capture_device_decklink_mac.h#newcode25 media/video/capture/mac/video_capture_device_decklink_mac.h:25: using scoped_refptr<T>::ptr_; On 2014/09/09 11:05:00, mcasas wrote: > On ...
6 years, 3 months ago (2014-09-22 20:26:38 UTC) #14
mcasas
tommi@, magjed@ PTAL. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.h File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.h#newcode55 media/video/capture/mac/video_capture_device_decklink_mac.h:55: void sendErrorString(const std::string& reason); On 2014/09/22 ...
6 years, 3 months ago (2014-09-23 08:54:54 UTC) #16
tommi (sloooow) - chröme
https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode51 media/video/capture/mac/video_capture_device_decklink_mac.mm:51: virtual HRESULT VideoInputFormatChanged ( On 2014/09/23 08:54:54, mcasas wrote: ...
6 years, 3 months ago (2014-09-23 12:59:54 UTC) #17
mcasas
tommi@ PTAL. rsesek@ FYI/PTAL. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode51 media/video/capture/mac/video_capture_device_decklink_mac.mm:51: virtual HRESULT VideoInputFormatChanged ( On ...
6 years, 3 months ago (2014-09-23 15:03:47 UTC) #19
tommi (sloooow) - chröme
https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode256 media/video/capture/mac/video_capture_device_decklink_mac.mm:256: *ppv = reinterpret_cast<IDeckLinkNotificationCallback*>(this); On 2014/09/23 15:03:46, mcasas wrote: > ...
6 years, 3 months ago (2014-09-23 17:15:28 UTC) #20
mcasas
On 2014/09/23 17:15:28, tommi wrote: > https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm > File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): > > https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode256 > ...
6 years, 3 months ago (2014-09-24 06:49:01 UTC) #22
tommi (sloooow) - chröme
lgtm https://codereview.chromium.org/535983002/diff/280001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/280001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode261 media/video/capture/mac/video_capture_device_decklink_mac.mm:261: *ppv = static_cast<IDeckLinkInputCallback*>(this);; nit: one semicolon
6 years, 3 months ago (2014-09-24 13:55:56 UTC) #23
mcasas
rsesek@ PTAL.
6 years, 3 months ago (2014-09-24 15:19:02 UTC) #24
Robert Sesek
https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac/video_capture_device_decklink_mac.h File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac/video_capture_device_decklink_mac.h#newcode49 media/video/capture/mac/video_capture_device_decklink_mac.h:49: int length, Lengths of buffers should really be measured ...
6 years, 3 months ago (2014-09-24 17:06:14 UTC) #25
mcasas
rsesek@ PTAL. FI: I won't be landing this before the cut in any case. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac/video_capture_device_decklink_mac.h ...
6 years, 3 months ago (2014-09-24 19:39:57 UTC) #26
mcasas
rsesek@ ping.
6 years, 2 months ago (2014-09-29 09:41:06 UTC) #27
Robert Sesek
https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode91 media/video/capture/mac/video_capture_device_decklink_mac.mm:91: // Internal counter for IUnknown interface implementation. On 2014/09/24 ...
6 years, 2 months ago (2014-09-29 21:08:51 UTC) #28
mcasas
rsesek@, tommi@ please have a look at the new AddRef()/Release() impl. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): ...
6 years, 2 months ago (2014-09-30 14:18:49 UTC) #29
Robert Sesek
Yes, I think this is an improvement. LGTM https://codereview.chromium.org/535983002/diff/340001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/340001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode47 media/video/capture/mac/video_capture_device_decklink_mac.mm:47: base::RefCountedThreadSafe<DeckLinkCaptureDelegate> ...
6 years, 2 months ago (2014-09-30 14:41:12 UTC) #30
tommi (sloooow) - chröme
https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode155 media/video/capture/mac/video_capture_device_decklink_mac.mm:155: return; release decklink_? https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode162 media/video/capture/mac/video_capture_device_decklink_mac.mm:162: return; release decklink_ and ...
6 years, 2 months ago (2014-09-30 18:21:40 UTC) #31
mcasas
https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode155 media/video/capture/mac/video_capture_device_decklink_mac.mm:155: return; On 2014/09/30 18:21:39, tommi wrote: > release decklink_? ...
6 years, 2 months ago (2014-10-01 16:08:55 UTC) #32
tommi (sloooow) - chröme
(not lgtm, just to be sure) https://codereview.chromium.org/535983002/diff/350004/media/video/capture/mac/video_capture_device_decklink_mac.mm File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/350004/media/video/capture/mac/video_capture_device_decklink_mac.mm#newcode205 media/video/capture/mac/video_capture_device_decklink_mac.mm:205: decklink_ = decklink_local; ...
6 years, 2 months ago (2014-10-01 16:13:42 UTC) #33
mcasas
PTAL
6 years, 2 months ago (2014-10-01 16:16:37 UTC) #34
tommi (sloooow) - chröme
to be fair, the .Release() comment was not a nit :) lgtm!
6 years, 2 months ago (2014-10-01 16:36:19 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/535983002/390001
6 years, 2 months ago (2014-10-01 16:38:55 UTC) #37
commit-bot: I haz the power
Committed patchset #15 (id:390001) as bfe5041bf4d6c8535dd42d6e7222e7f04d50729c
6 years, 2 months ago (2014-10-01 17:19:59 UTC) #38
commit-bot: I haz the power
6 years, 2 months ago (2014-10-01 17:20:47 UTC) #39
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/4729b02c56fc13081c742b9113117e7291537fbc
Cr-Commit-Position: refs/heads/master@{#297668}

Powered by Google App Engine
This is Rietveld 408576698