Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionMac 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 #
Messages
Total messages: 39 (10 generated)
Patchset #1 (id:1) has been deleted
mcasas@chromium.org changed reviewers: + magjed@chromium.org
magjed@ PTAL.
https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:81: media::VideoPixelFormat pixel_format = media::PIXEL_FORMAT_UNKNOWN; Why do you need to declare this variable? Why not use media::PIXEL_FORMAT_UNKNOWN directly when constructing format instead? https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:93: media::VideoCaptureFormat format( format can be made const https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:156: display_mode_frame_rate = static_cast<float>(time_scale) / time_value; Don't you need to check time_value > 0 here? Anyway, this is duplicated code from EnumerateDevices. Maybe introduce a common function that returns the frame rate in the format we want? Something like this: float GetDisplayModeFrameRate(type? display_mode) { BMDTimeValue time_value, time_scale; float display_mode_frame_rate = 0.0f; if (display_mode->GetFrameRate(&time_value, &time_scale) == S_OK && time_value > 0) { display_mode_frame_rate = static_cast<float>(time_scale) / time_value; } // Interlaced formats are going to be marked as double the frame rate, // which follows the general naming conventions. if (display_mode->GetFieldDominance() == bmdLowerFieldFirst || display_mode->GetFieldDominance() == bmdUpperFieldFirst) display_mode_frame_rate *= 2.0f; return display_mode_frame_rate; } https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:163: int diff = abs(display_mode->GetWidth() - diff can be made const https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:230: case bmdFormat8BitBGRA: Why have you included this case if you don't do anything with it? https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:235: media::VideoCaptureFormat captureFormat( captureFormat can be made const https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:260: == 0) { you should align == 0 to memcmp
mcasas@chromium.org changed reviewers: + tommi@chromium.org
magjed@, tommi@ PTAL https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:81: media::VideoPixelFormat pixel_format = media::PIXEL_FORMAT_UNKNOWN; On 2014/09/04 13:39:18, magjed wrote: > Why do you need to declare this variable? Why not use > media::PIXEL_FORMAT_UNKNOWN directly when constructing format instead? Done. https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:93: media::VideoCaptureFormat format( On 2014/09/04 13:39:18, magjed wrote: > format can be made const Done. https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:156: display_mode_frame_rate = static_cast<float>(time_scale) / time_value; On 2014/09/04 13:39:18, magjed wrote: > Don't you need to check time_value > 0 here? Anyway, this is duplicated code > from EnumerateDevices. Maybe introduce a common function that returns the frame > rate in the format we want? Something like this: > > float GetDisplayModeFrameRate(type? display_mode) { > BMDTimeValue time_value, time_scale; > float display_mode_frame_rate = 0.0f; > if (display_mode->GetFrameRate(&time_value, &time_scale) == S_OK && > time_value > 0) { > display_mode_frame_rate = static_cast<float>(time_scale) / time_value; > } > // Interlaced formats are going to be marked as double the frame rate, > // which follows the general naming conventions. > if (display_mode->GetFieldDominance() == bmdLowerFieldFirst || > display_mode->GetFieldDominance() == bmdUpperFieldFirst) > display_mode_frame_rate *= 2.0f; > return display_mode_frame_rate; > } Done. https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:163: int diff = abs(display_mode->GetWidth() - On 2014/09/04 13:39:18, magjed wrote: > diff can be made const Done. https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:230: case bmdFormat8BitBGRA: On 2014/09/04 13:39:18, magjed wrote: > Why have you included this case if you don't do anything with it? I guess eventually I wanted to do something but then I became blind to it :) Removed. https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:235: media::VideoCaptureFormat captureFormat( On 2014/09/04 13:39:18, magjed wrote: > captureFormat can be made const Done. https://codereview.chromium.org/535983002/diff/20001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:260: == 0) { On 2014/09/04 13:39:18, magjed wrote: > you should align == 0 to memcmp Done.
https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... 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/... media/video/capture/mac/video_capture_device_decklink_mac.h:74: // IDeckLinkInputCallback interface implementation. should this be private/protected? (same for the VideoCaptureDevice implementation) https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:84: virtual HRESULT QueryInterface (REFIID iid, LPVOID *ppv) OVERRIDE; Since this implements reference counting and VideoCaptureDeviceDeckLinkMac is not really reference counted, I think we should implement this interface in a separate class and hold on to an instance of that class in VideoCaptureDeviceDeckLinkMac. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:98: ScopedDeckLinkPtr<IDeckLinkInput> decklink_input_; is this a circular dependency to an object that also has a pointer to |this|? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:103: int32_t ref_count_; see comment above - I think this should be in a separate class. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.mm (left): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:18: class ScopedDeckLinkPtr : public scoped_refptr<T> { if you create a special subclass that implements the IUnknown things, then we could still have that class in this source file and keep this helper here. I'd like to limit the scope of this class as much as possible. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:25: display_mode->GetFieldDominance() == bmdUpperFieldFirst) {} https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:119: void VideoCaptureDeviceDeckLinkMac::AllocateAndStart( add thread check https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:143: SetErrorState("Error querying input interface."); should we release declink_ here in this case? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:151: return; same here for decklink_ and decklink_input_. (same for all other error returns) https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:191: return; nit: return statement not necessary - but we should probably release all resources here that we're holding on to. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:196: if (decklink_input_->StopStreams() != S_OK) add thread check first and also check these pointers before dereferencing https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:206: DVLOG(1) << __FUNCTION__; thread check? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:213: // Capture frames are manipulated as an IDeckLinkVideoFrame. thread check? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:214: void *videoData; void* video_data = NULL; https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:226: SetErrorState("Unsupported pixel format"); add break; for posterity https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:229: const VideoCaptureFormat captureFormat( why captureFormat here and not capture_format? I see videoData above as well but then there's pixel_format - is there some style here that I'm perhaps missing? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:245: *ppv = NULL; no need to do this https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:246: // Obtain the IUnknown interface and compare it the provided REFIID this isn't obtaining the interface itself, it's fetching the id of the interface. I don't think you need a variable for this though, you should be able to just call CFUUIDGetUUIDBytes() inline in the memcmp call, right? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:254: *ppv = (IDeckLinkNotificationCallback*)this; static_cast https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:258: return result; this implementation can be simplified: if (memcmp(<IUnknownUUID cmp>) == 0 || memcmp(<IDeckLinkNotificationCallback cmp>) == 0) { *ppv = static_cast<IDeckLinkNotificationCallback*>(this); AddRef(); return S_OK; } return E_NOINTERFACE; https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:262: return OSAtomicIncrement32(&ref_count_); why atomic? Do we expect multithreaded access? (if not, add thread check?) https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:269: return 0; nit: this return statement isn't necessary
Patchset #4 (id:80001) has been deleted
tommi@ PTAL. Big change: splitted VCDDeckLinkMac into two classes, one called like the original and another doing all the hard work and ref counted by both Cr and the SDK. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:25: using scoped_refptr<T>::ptr_; On 2014/09/05 09:24:21, tommi wrote: > why do you need this? Subtemplating a template is a bit special, meaning that I need to explicitly state which |ptr_| I'm manipulating, because it depends on the template parameter. Explained in all its glory in [1]. [1] http://www.parashift.com/c++-faq-lite/nondependent-name-lookup-members.html https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:74: // IDeckLinkInputCallback interface implementation. On 2014/09/05 09:24:20, tommi wrote: > should this be private/protected? (same for the VideoCaptureDevice > implementation) Had a second look at the inheritance qualifiers and at the methods and privatised/protectesised as much as possible. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:84: virtual HRESULT QueryInterface (REFIID iid, LPVOID *ppv) OVERRIDE; On 2014/09/05 09:24:21, tommi wrote: > Since this implements reference counting and VideoCaptureDeviceDeckLinkMac is > not really reference counted, I think we should implement this interface in a > separate class and hold on to an instance of that class in > VideoCaptureDeviceDeckLinkMac. I created a DeckLinkCaptureDelegate class that holds all DeckLink specifics and moved it to the .mm file. Capture callbacks happen in a DeckLink thread, so had to add an AutoLock here and there. DeckLinkCaptureDelegate holds a weak reference to VideoCaptureDeviceDeckLinkMac to send frames to it. DLCaptureDelegate is reference-counted by the DeckLink SDK via its IUnknown interface, and VCDDeckLinkMac ref counts it. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:98: ScopedDeckLinkPtr<IDeckLinkInput> decklink_input_; On 2014/09/05 09:24:20, tommi wrote: > is this a circular dependency to an object that also has a pointer to |this|? Yes, should I turn it into a weak pointer? https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.h:103: int32_t ref_count_; On 2014/09/05 09:24:20, tommi wrote: > see comment above - I think this should be in a separate class. Acknowledged. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.mm (left): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:18: class ScopedDeckLinkPtr : public scoped_refptr<T> { On 2014/09/05 09:24:22, tommi wrote: > if you create a special subclass that implements the IUnknown things, then we > could still have that class in this source file and keep this helper here. I'd > like to limit the scope of this class as much as possible. Acknowledged. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:25: display_mode->GetFieldDominance() == bmdUpperFieldFirst) On 2014/09/05 09:24:21, tommi wrote: > {} Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:119: void VideoCaptureDeviceDeckLinkMac::AllocateAndStart( On 2014/09/05 09:24:21, tommi wrote: > add thread check Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:143: SetErrorState("Error querying input interface."); On 2014/09/05 09:24:21, tommi wrote: > should we release declink_ here in this case? SetErrorState() will ping VCController->VideoCaptureHost ->MediaStreamManager->VideoCaptureManager which will in turn StopAndDeallocate() and destroy VCDDeckLinkMac, and until then we need to hold to these two objects. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:151: return; On 2014/09/05 09:24:21, tommi wrote: > same here for decklink_ and decklink_input_. > (same for all other error returns) See above. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:191: return; On 2014/09/05 09:24:21, tommi wrote: > nit: return statement not necessary - but we should probably release all > resources here that we're holding on to. See above. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:196: if (decklink_input_->StopStreams() != S_OK) On 2014/09/05 09:24:21, tommi wrote: > add thread check first and also check these pointers before dereferencing Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:206: DVLOG(1) << __FUNCTION__; On 2014/09/05 09:24:21, tommi wrote: > thread check? Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:213: // Capture frames are manipulated as an IDeckLinkVideoFrame. On 2014/09/05 09:24:22, tommi wrote: > thread check? Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:214: void *videoData; On 2014/09/05 09:24:21, tommi wrote: > void* video_data = NULL; Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:226: SetErrorState("Unsupported pixel format"); On 2014/09/05 09:24:21, tommi wrote: > add break; for posterity Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:229: const VideoCaptureFormat captureFormat( On 2014/09/05 09:24:21, tommi wrote: > why captureFormat here and not capture_format? I see videoData above as well > but then there's pixel_format - is there some style here that I'm perhaps > missing? No, you're all right. The sample code(s) I followed was written in Windows CamelCase and I became eventually blind to these vars. Corrected. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:245: *ppv = NULL; On 2014/09/05 09:24:22, tommi wrote: > no need to do this Acknowledged. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:246: // Obtain the IUnknown interface and compare it the provided REFIID On 2014/09/05 09:24:21, tommi wrote: > this isn't obtaining the interface itself, it's fetching the id of the > interface. > > I don't think you need a variable for this though, you should be able to just > call CFUUIDGetUUIDBytes() inline in the memcmp call, right? Acknowledged. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:254: *ppv = (IDeckLinkNotificationCallback*)this; On 2014/09/05 09:24:21, tommi wrote: > static_cast Acknowledged. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:258: return result; On 2014/09/05 09:24:21, tommi wrote: > this implementation can be simplified: > > if (memcmp(<IUnknownUUID cmp>) == 0 || > memcmp(<IDeckLinkNotificationCallback cmp>) == 0) { > *ppv = static_cast<IDeckLinkNotificationCallback*>(this); > AddRef(); > return S_OK; > } > > return E_NOINTERFACE; Done. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:262: return OSAtomicIncrement32(&ref_count_); On 2014/09/05 09:24:21, tommi wrote: > why atomic? Do we expect multithreaded access? (if not, add thread check?) Reference/Example implementation says so :) Added thread check anyway. https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_decklink_mac.mm:269: return 0; On 2014/09/05 09:24:21, tommi wrote: > nit: this return statement isn't necessary Done.
https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac... 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 the most closely matched format is already implemented in Windows: GetBestMatchedFormat and also in Mac: GetBestMatchSupportedResolution. There is an implementation in third_party/webrtc/modules/video_capture/device_info_impl.cc as well, but that file is deprecated? Anyway, I made a CL that moves the functionality to media/video/capture/video_capture_types.cc. Please take a look: https://codereview.chromium.org/558623002/
Patchset #5 (id:120001) has been deleted
Patchset #6 (id:160001) has been deleted
guys PTAL. https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/100001/media/video/capture/mac... 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 14:36:24, magjed wrote: > This code for finding the most closely matched format is already implemented in > Windows: GetBestMatchedFormat and also in Mac: GetBestMatchSupportedResolution. > There is an implementation in > third_party/webrtc/modules/video_capture/device_info_impl.cc as well, but that > file is deprecated? > > Anyway, I made a CL that moves the functionality to > media/video/capture/video_capture_types.cc. Please take a look: > https://codereview.chromium.org/558623002/ Acknowledged.
https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/60001/media/video/capture/mac/... 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 2014/09/05 09:24:21, tommi wrote: > > why do you need this? > > Subtemplating a template is a bit special, meaning that > I need to explicitly state which |ptr_| I'm manipulating, > because it depends on the template parameter. > > Explained in all its glory in [1]. > > [1] http://www.parashift.com/c++-faq-lite/nondependent-name-lookup-members.html Ah, sorry, I was actually more thinking why you're making it public :) However, even so, I don't think you need this since it's clear that it comes from scoped_ptr (I didn't need this for example when I wrote ScopedComPtr). https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:55: void sendErrorString(const std::string& reason); SendErrorString? (fix capitalization) https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:58: void sendLogString(const std::string& message); ditto. also, do these need to be public? https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:21: using scoped_refptr<T>::ptr_; see comment about this from a previous patch set https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:51: virtual HRESULT VideoInputFormatChanged ( make all of these private? https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:72: void sendErrorString(const std::string& reason); why the lower case 's'? https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:81: media::VideoCaptureDeviceDeckLinkMac* frame_receiver_; // Weak. explain lifetime? https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:144: decklink_input_.ReceiveVoid()) != S_OK) { indent https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:185: bmdFormat8BitYUV, bmdVideoInputFlagDefault) != S_OK) { indent https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:197: if (!decklink_input_.get()) { no {} (see next 'if') https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:219: void *video_data = NULL; void* video_data = NULL; should this perhaps be uint8* to be in line with other buffer pointers? https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:256: *ppv = reinterpret_cast<IDeckLinkNotificationCallback*>(this); static_cast
Patchset #7 (id:200001) has been deleted
tommi@, magjed@ PTAL. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:55: void sendErrorString(const std::string& reason); On 2014/09/22 20:26:38, tommi wrote: > SendErrorString? (fix capitalization) Done. Was confused with Obj-C. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:58: void sendLogString(const std::string& message); On 2014/09/22 20:26:38, tommi wrote: > ditto. > Done. > also, do these need to be public? Afraid so, they are called from DeckLinkCaptureDelegate. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:21: using scoped_refptr<T>::ptr_; On 2014/09/22 20:26:38, tommi wrote: > see comment about this from a previous patch set Made private, but I need it there otherwise clang complains like: use of undeclared identifier 'ptr_' Which is kind of silly, that such a simple template specialization confuses the compiler :? :( https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:51: virtual HRESULT VideoInputFormatChanged ( On 2014/09/22 20:26:38, tommi wrote: > make all of these private? Made IDeckLinkInputCallback interface methods private, but those from IUnknown cannot be done so, they are referenced by VideoCaptureDeviceDeckLinkMac's ScopedDeckLinkPtr of this class. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:72: void sendErrorString(const std::string& reason); On 2014/09/22 20:26:38, tommi wrote: > why the lower case 's'? My bad, that would be Obj-C. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:81: media::VideoCaptureDeviceDeckLinkMac* frame_receiver_; // Weak. On 2014/09/22 20:26:38, tommi wrote: > explain lifetime? Done. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:144: decklink_input_.ReceiveVoid()) != S_OK) { On 2014/09/22 20:26:38, tommi wrote: > indent Done. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:185: bmdFormat8BitYUV, bmdVideoInputFlagDefault) != S_OK) { On 2014/09/22 20:26:38, tommi wrote: > indent Done. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:197: if (!decklink_input_.get()) { On 2014/09/22 20:26:38, tommi wrote: > no {} (see next 'if') Done. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:219: void *video_data = NULL; On 2014/09/22 20:26:38, tommi wrote: > void* video_data = NULL; > > should this perhaps be uint8* to be in line with other buffer pointers? Done. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:256: *ppv = reinterpret_cast<IDeckLinkNotificationCallback*>(this); On 2014/09/22 20:26:38, tommi wrote: > static_cast According to clang, static_cast from 'DeckLinkCaptureDelegate *' to 'IDeckLinkNotificationCallback *' is not allowed. This is boilerplate code anyway.
https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:51: virtual HRESULT VideoInputFormatChanged ( On 2014/09/23 08:54:54, mcasas wrote: > On 2014/09/22 20:26:38, tommi wrote: > > make all of these private? > > Made IDeckLinkInputCallback interface methods private, but > those from IUnknown cannot be done so, they are referenced > by VideoCaptureDeviceDeckLinkMac's ScopedDeckLinkPtr of > this class. ah, makes sense. (ultra nit could we add scoped_refptr as a friend to DeckLinkCaptureDelegate?) Totally up to you though. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:256: *ppv = reinterpret_cast<IDeckLinkNotificationCallback*>(this); On 2014/09/23 08:54:54, mcasas wrote: > On 2014/09/22 20:26:38, tommi wrote: > > static_cast > > According to clang, > static_cast from 'DeckLinkCaptureDelegate *' to 'IDeckLinkNotificationCallback > *' is not allowed. > > This is boilerplate code anyway. yeah, that's because the inheritance was set to 'private'. reinterpret_cast is going to do the wrong thing (by coincidence it works now). https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:45: class DeckLinkCaptureDelegate : private IDeckLinkInputCallback { nit: it's very unusual to have a private inheritance from an interface like this. Is it needed? https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:52: virtual HRESULT QueryInterface (REFIID iid, LPVOID *ppv) OVERRIDE; no space after QI LPVOID *ppv -> void** ppv https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_factory_mac.mm (right): https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... media/video/capture/mac/video_capture_device_factory_mac.mm:107: if (!device->Init(device_name.capture_api_type())) { nice :)
mcasas@chromium.org changed reviewers: + rsesek@chromium.org
tommi@ PTAL. rsesek@ FYI/PTAL. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:51: virtual HRESULT VideoInputFormatChanged ( On 2014/09/23 12:59:53, tommi wrote: > On 2014/09/23 08:54:54, mcasas wrote: > > On 2014/09/22 20:26:38, tommi wrote: > > > make all of these private? > > > > Made IDeckLinkInputCallback interface methods private, but > > those from IUnknown cannot be done so, they are referenced > > by VideoCaptureDeviceDeckLinkMac's ScopedDeckLinkPtr of > > this class. > > ah, makes sense. > > (ultra nit could we add scoped_refptr as a friend to DeckLinkCaptureDelegate?) > Totally up to you though. Done. https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:256: *ppv = reinterpret_cast<IDeckLinkNotificationCallback*>(this); On 2014/09/23 12:59:53, tommi wrote: > On 2014/09/23 08:54:54, mcasas wrote: > > On 2014/09/22 20:26:38, tommi wrote: > > > static_cast > > > > According to clang, > > static_cast from 'DeckLinkCaptureDelegate *' to 'IDeckLinkNotificationCallback > > *' is not allowed. > > > > This is boilerplate code anyway. > > yeah, that's because the inheritance was set to 'private'. reinterpret_cast is > going to do the wrong thing (by coincidence it works now). Actually, thanks for insisting here. The code is wrong, the IID to use is the same as the actual class: IID_IDeckLinkInputCallback. Corrected now, and consequently we don't need the cast. https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:45: class DeckLinkCaptureDelegate : private IDeckLinkInputCallback { On 2014/09/23 12:59:54, tommi wrote: > nit: it's very unusual to have a private inheritance from an interface like > this. Is it needed? Changed to public inheritance. https://codereview.chromium.org/535983002/diff/220001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:52: virtual HRESULT QueryInterface (REFIID iid, LPVOID *ppv) OVERRIDE; On 2014/09/23 12:59:54, tommi wrote: > no space after QI > LPVOID *ppv -> void** ppv Done.
https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... 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: > On 2014/09/23 12:59:53, tommi wrote: > > On 2014/09/23 08:54:54, mcasas wrote: > > > On 2014/09/22 20:26:38, tommi wrote: > > > > static_cast > > > > > > According to clang, > > > static_cast from 'DeckLinkCaptureDelegate *' to > 'IDeckLinkNotificationCallback > > > *' is not allowed. > > > > > > This is boilerplate code anyway. > > > > yeah, that's because the inheritance was set to 'private'. reinterpret_cast > is > > going to do the wrong thing (by coincidence it works now). > > Actually, thanks for insisting here. The code is wrong, the IID to use is the > same as the actual class: IID_IDeckLinkInputCallback. Corrected now, and > consequently we don't need the cast. Actually, we still need the cast in there to be safe. The class may have several vtables - well at least two. One for the class itself (e.g. containing a pointer to the dtor), and another for the IDeckLinkInputCallback (and IUnknown by inheritance) interface. If you just assign *ppv to |this|, how do you know which representation you're handing out? To be safe, I'd like to do this: *ppv = static_cast<IDeckLinkInputCallback*>(this); This guarantees that the pointer you hand back, will support IUnknown and IDeckLinkInputCallback - and nothing else.
Patchset #9 (id:260001) has been deleted
On 2014/09/23 17:15:28, tommi wrote: > https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... > File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): > > https://codereview.chromium.org/535983002/diff/180001/media/video/capture/mac... > 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: > > On 2014/09/23 12:59:53, tommi wrote: > > > On 2014/09/23 08:54:54, mcasas wrote: > > > > On 2014/09/22 20:26:38, tommi wrote: > > > > > static_cast > > > > > > > > According to clang, > > > > static_cast from 'DeckLinkCaptureDelegate *' to > > 'IDeckLinkNotificationCallback > > > > *' is not allowed. > > > > > > > > This is boilerplate code anyway. > > > > > > yeah, that's because the inheritance was set to 'private'. reinterpret_cast > > is > > > going to do the wrong thing (by coincidence it works now). > > > > Actually, thanks for insisting here. The code is wrong, the IID to use is the > > same as the actual class: IID_IDeckLinkInputCallback. Corrected now, and > > consequently we don't need the cast. > > Actually, we still need the cast in there to be safe. > The class may have several vtables - well at least two. One for the class > itself (e.g. containing a pointer to the dtor), and another for the > IDeckLinkInputCallback (and IUnknown by inheritance) interface. > > If you just assign *ppv to |this|, how do you know which representation you're > handing out? > > To be safe, I'd like to do this: > > *ppv = static_cast<IDeckLinkInputCallback*>(this); > > This guarantees that the pointer you hand back, will support IUnknown and > IDeckLinkInputCallback - and nothing else. Done. PTAL.
lgtm https://codereview.chromium.org/535983002/diff/280001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/280001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:261: *ppv = static_cast<IDeckLinkInputCallback*>(this);; nit: one semicolon
rsesek@ PTAL.
https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:49: int length, Lengths of buffers should really be measured in size_t. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:70: // Reference counted handle to the DeckLink capture delegate, ref counted by Is this also protected by |lock_|? If not, put a blank line before this. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:59: virtual HRESULT VideoInputFormatChanged ( nit: extra space before ( https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:63: virtual HRESULT VideoInputFrameArrived ( same https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:68: virtual HRESULT QueryInterface (REFIID iid, void** ppv) OVERRIDE; same https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:91: // Internal counter for IUnknown interface implementation. Is it really necessary to roll your own reference counting? https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:212: HRESULT DeckLinkCaptureDelegate::VideoInputFormatChanged ( nit: space before ( https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:220: HRESULT DeckLinkCaptureDelegate::VideoInputFrameArrived ( same https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:268: ULONG DeckLinkCaptureDelegate::AddRef(void) { no (void) https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:273: ULONG DeckLinkCaptureDelegate::Release(void) { no (void) https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:275: int32_t newRefValue = OSAtomicDecrement32(&ref_count_); naming: under_score
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... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:49: int length, On 2014/09/24 17:06:14, rsesek wrote: > Lengths of buffers should really be measured in size_t. Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:70: // Reference counted handle to the DeckLink capture delegate, ref counted by On 2014/09/24 17:06:14, rsesek wrote: > Is this also protected by |lock_|? If not, put a blank line before this. Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:59: virtual HRESULT VideoInputFormatChanged ( On 2014/09/24 17:06:14, rsesek wrote: > nit: extra space before ( Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:63: virtual HRESULT VideoInputFrameArrived ( On 2014/09/24 17:06:14, rsesek wrote: > same Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:68: virtual HRESULT QueryInterface (REFIID iid, void** ppv) OVERRIDE; On 2014/09/24 17:06:14, rsesek wrote: > same Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:91: // Internal counter for IUnknown interface implementation. On 2014/09/24 17:06:14, rsesek wrote: > Is it really necessary to roll your own reference counting? The DeckLink SDK forces whoever implements IDeckLinkInputCallback to be referenced counted using a Com-like syntax, so I'm afraid this is needed. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:212: HRESULT DeckLinkCaptureDelegate::VideoInputFormatChanged ( On 2014/09/24 17:06:14, rsesek wrote: > nit: space before ( Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:220: HRESULT DeckLinkCaptureDelegate::VideoInputFrameArrived ( On 2014/09/24 17:06:14, rsesek wrote: > same Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:268: ULONG DeckLinkCaptureDelegate::AddRef(void) { On 2014/09/24 17:06:14, rsesek wrote: > no (void) Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:273: ULONG DeckLinkCaptureDelegate::Release(void) { On 2014/09/24 17:06:14, rsesek wrote: > no (void) Done. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:275: int32_t newRefValue = OSAtomicDecrement32(&ref_count_); On 2014/09/24 17:06:14, rsesek wrote: > naming: under_score Done.
rsesek@ ping.
https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:91: // Internal counter for IUnknown interface implementation. On 2014/09/24 19:39:56, mcasas wrote: > On 2014/09/24 17:06:14, rsesek wrote: > > Is it really necessary to roll your own reference counting? > > The DeckLink SDK forces whoever implements IDeckLinkInputCallback > to be referenced counted using a Com-like syntax, so I'm afraid this > is needed. Can you really not find any other way to not use parts of //base to do this? There's a reason that base::'s implementation is largely in the "subtle" namespace. AIUI, the return values of AddRef() and Release() are meant only for testing purposes. So, for example, they could merely return the result of HasOneRef() and then you could use the the base refcounting implementation. https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.h (right): https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.h:75: base::ThreadChecker thread_checker_; Document on what thread this is supposed to be bound. https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:94: base::ThreadChecker thread_checker_; Same, what thread does this run on? https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:270: return OSAtomicIncrement32(&ref_count_); ref_count_ is an int32_t yet it's being returned here as an unsigned 32-bit quantity. Same with below.
rsesek@, tommi@ please have a look at the new AddRef()/Release() impl. https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/300001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:91: // Internal counter for IUnknown interface implementation. On 2014/09/29 21:08:51, Robert Sesek wrote: > On 2014/09/24 19:39:56, mcasas wrote: > > On 2014/09/24 17:06:14, rsesek wrote: > > > Is it really necessary to roll your own reference counting? > > > > The DeckLink SDK forces whoever implements IDeckLinkInputCallback > > to be referenced counted using a Com-like syntax, so I'm afraid this > > is needed. > > Can you really not find any other way to not use parts of //base to do this? > There's a reason that base::'s implementation is largely in the "subtle" > namespace. > > AIUI, the return values of AddRef() and Release() are meant only for testing > purposes. So, for example, they could merely return the result of HasOneRef() > and then you could use the the base refcounting implementation. I made DeckLinkCaptureDelegate derive from base::RefCountedThreadSafe<> and use its methods to implement the IUnknown AddRef()/Release(). Please double check if that's an improvement. https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:94: base::ThreadChecker thread_checker_; On 2014/09/29 21:08:51, Robert Sesek wrote: > Same, what thread does this run on? Done. https://codereview.chromium.org/535983002/diff/320001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:270: return OSAtomicIncrement32(&ref_count_); On 2014/09/29 21:08:51, Robert Sesek wrote: > ref_count_ is an int32_t yet it's being returned here as an unsigned 32-bit > quantity. Same with below. Done.
Yes, I think this is an improvement. LGTM https://codereview.chromium.org/535983002/diff/340001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/340001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:47: base::RefCountedThreadSafe<DeckLinkCaptureDelegate> { Missing "public" inheritance.
https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... 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... media/video/capture/mac/video_capture_device_decklink_mac.mm:162: return; release decklink_ and decklink_input_? https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:182: return; free so-far-obtained objects? same below What you could do to make this simpler is to use local variables to do all the work and only assign to members when you get to the end (successfully). https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:204: void DeckLinkCaptureDelegate::StopAndDeAllocate() { Should this method also release all outstanding references that were obtained in AllocateAndStart()? https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:471: void VideoCaptureDeviceDeckLinkMac::StopAndDeAllocate() { thread check?
https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:155: return; On 2014/09/30 18:21:39, tommi wrote: > release decklink_? See below. https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:162: return; On 2014/09/30 18:21:39, tommi wrote: > release decklink_ and decklink_input_? See below. https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:182: return; On 2014/09/30 18:21:38, tommi wrote: > free so-far-obtained objects? same below > > What you could do to make this simpler is to use local variables to do all the > work and only assign to members when you get to the end (successfully). Done. https://codereview.chromium.org/535983002/diff/360001/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:204: void DeckLinkCaptureDelegate::StopAndDeAllocate() { On 2014/09/30 18:21:38, tommi wrote: > Should this method also release all outstanding references that were obtained in > AllocateAndStart()? Done.
(not lgtm, just to be sure) https://codereview.chromium.org/535983002/diff/350004/media/video/capture/mac... File media/video/capture/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/535983002/diff/350004/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:205: decklink_ = decklink_local; nit: could maybe use swap()? https://codereview.chromium.org/535983002/diff/350004/media/video/capture/mac... media/video/capture/mac/video_capture_device_decklink_mac.mm:217: decklink_input_->Release(); should be .Release() (and next line)
PTAL
to be fair, the .Release() comment was not a nit :) lgtm!
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/535983002/390001
Message was sent while issue was closed.
Committed patchset #15 (id:390001) as bfe5041bf4d6c8535dd42d6e7222e7f04d50729c
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/4729b02c56fc13081c742b9113117e7291537fbc Cr-Commit-Position: refs/heads/master@{#297668} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
