Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8177008:
Adding VideoCaptureDevice for Mac. (Closed)
|
Created:
9 years, 2 months ago by mflodman_chromium_OOO Modified:
9 years, 2 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), wjia(left Chromium) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdding VideoCaptureDevice for Mac.
Adding dependency on QTKit and CoreVideo.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106036
Patch Set 1 #
Total comments: 60
Patch Set 2 : Changes based on review by dmac. #
Total comments: 43
Patch Set 3 : Changes based on comments by dmac and scherkus. #
Total comments: 10
Patch Set 4 : Changes based on review by dmac. #Patch Set 5 : Unittest changes to run on Mac OS X. #
Total comments: 20
Patch Set 6 : Added PostQuitTask to unittest. #
Total comments: 20
Patch Set 7 : "Changes based no review by dmac." #Patch Set 8 : Test change: PostTask instead of PostDelayedTask. #
Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac.h (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.h:18: // Called by Video can we give it a slightly better description? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.h:25: virtual void Allocate(int width, Mark the overridden virtuals with OVERRIDE http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:18: NSArray* device_list = [[NSArray alloc] couple of things here: a) no space between : and args b) Why are you creating a new array from an already existing array that is the exact same? usually if you want to copy an object in objc you would do it with blah = [myArray copy]. In this case I don't think you even want a copy though. c) the way you have it, device_list is being leaked. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:22: for (unsigned int i = 0; i < device_list.count; ++i) { for (QTCaptureDevice *device in [QTCaptureDevice inputDevicesWithMediaType:QTMediaTypeVideo]) { blah blah blah is probably a better way to do this. } http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:25: if (capture_device) { capture_device won't be null. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:40: return NULL; do you not want to at least LOG in either of these cases? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:58: [capture_device_ release]; no need to check capture_device_. Just call [capture_device_ release] http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:87: return; no need for return at end http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:125: capture_device_ = [[VideoCaptureDeviceMacQTKit alloc] init]; if folks call init twice we are going to leak a capture_device_. Should we assert on that case? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:13: pool_ = [[NSAutoreleasePool alloc]init]; space between ] and init http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:13: pool_ = [[NSAutoreleasePool alloc]init]; do you really need an autoreleasepool? please explain why http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:15: frameWidth_ = 0; no need to set things to zero/nil. This happens automagically in objc http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:25: if(captureSession_) { no need to check if these are nil http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:43: + (void) deviceNames:(NSArray *) deviceList { no space between ) and device (in both cases) http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:44: [deviceList initWithArray:[QTCaptureDevice This does not do what you think it does. My guess it you just want to return [QTCaptureDevice inputDevicesWithMediaType:QTMediaTypeVideo] http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:50: - (void)registerReceiver:(media::VideoCaptureDeviceMac *)frameReceiver { should this be "setFrameReceiver" since you can only have one of them? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:54: - (BOOL)setCaptureDevice:(const char *)name { so on one side of the interface you take an NSString and convert it to a char *, and then you rebuild it into an NSString? how about just passing an NSString... http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:62: captureDevices = [[[NSArray alloc] initWithArray: There's a couple of bugs here, but can I suggest an alternative? NSArray *captureDevices = [QTCaptureDeviceinputDevicesWithMediaType:QTMediaTypeVideo]; NSArray *captureDevicesNames = [captureDevices valueForKey:@"localizedDisplayName"]; NSUInteger index = [captureDevicesNames indexOfObject:name]; // This assumes name is passed in as a NSString * as suggested above if (index == NSNotFound) { // Possibly log here? return NO; } QTCaptureDevice *device = [captureDevices objectAtIndex:index]; NSError *error = nil; if (![captureDevice open:&error]) { // Log error return NO; } captureDeviceInput_ = [QTCaptureDeviceInput alloc] initWithDevice:device]; .... http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:100: if (captureDecompressedOutput_) { no need to check. Just call release on it. Same with captrueSession_ and captureDeviceInput_ etc. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:141: return NO; log the error? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:154: return NO; log the error? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:156: [[NSRunLoop currentRunLoop] runUntilDate:[NSDate distantFuture]]; what breaks out of this runloop? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:182: if(frameReceiver_) { do you want to check frameReceiver_ before doing any of the above? http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:184: CVBufferRetain(videoFrame); why do feel you need to retain and release the videoFrame in here? http://codereview.chromium.org/8177008/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:65: #if defined(OS_MACOSX) why can't any of these tests be enabled?
Missed some stuff first pass http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:10: do you need all these imports, or can you forward declare everything? I think the only import you should need is Foundation/Foundation.h http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:16: @interface VideoCaptureDeviceMacQTKit : NSObject{ space between NSObject and { http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:33: // Gets available devices. Normally you would return an autoreleased NSArray. I'm not quite sure what you expect to happen here... http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:40: - (BOOL)setCaptureDevice:(const char*)name; why a const char* instaed of an NSString? Also, I would only have one interface - (BOOL)setCaptureDevice:(NSString *) instead of set and remove. If you pass in nil it would be equivalent to your "remove" at this point. add and remove imply that you allow more than one capture device to be attached. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:47: AndWidth:(int)width lowercase in objc param names, and we don't use "and" in objc methods. so setCaptureHeight:width:framerate: are you sure you don't want fractional framerates?
Thanks for the review, lots of good comments and suggestions. I've addressed them ans I'll have another look at the test and one more change in the QTKit code, listed among the comments. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac.h (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.h:18: // Called by Video I got interrupted when writing that comment and missed that, sorry... http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.h:25: virtual void Allocate(int width, On 2011/10/06 23:36:22, dmac wrote: > Mark the overridden virtuals with OVERRIDE Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:18: NSArray* device_list = [[NSArray alloc] Removed. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:22: for (unsigned int i = 0; i < device_list.count; ++i) { On 2011/10/06 23:36:22, dmac wrote: > for (QTCaptureDevice *device in [QTCaptureDevice > inputDevicesWithMediaType:QTMediaTypeVideo]) { > > blah blah blah is probably a better way to do this. > } Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:25: if (capture_device) { Removed. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:40: return NULL; Done, added LOG(ERROR) calls. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:58: [capture_device_ release]; On 2011/10/06 23:36:22, dmac wrote: > no need to check capture_device_. > Just call [capture_device_ release] Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:87: return; On 2011/10/06 23:36:22, dmac wrote: > no need for return at end Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac.mm:125: capture_device_ = [[VideoCaptureDeviceMacQTKit alloc] init]; Done, added new state + DCHECK. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:10: On 2011/10/06 23:42:04, dmac wrote: > do you need all these imports, or can you forward declare everything? I think > the only import you should need is Foundation/Foundation.h Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:16: @interface VideoCaptureDeviceMacQTKit : NSObject{ On 2011/10/06 23:42:04, dmac wrote: > space between NSObject and { Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:33: // Gets available devices. Done. I'll also look at changing this one to remove all QT* code outside of this class. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:40: - (BOOL)setCaptureDevice:(const char*)name; Changed to NSString*, nil will remove the device and removeCaptureDevice is removed. I also changed to use the device id instead of name, to be able to set different devices with the same name. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.h:47: AndWidth:(int)width Changed. Not used to this, so I hope the change is correct... We are using int elsewhere Chromium video capture, so I'll keep it as an int. I'll bring that up for discussion with the others though. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:13: pool_ = [[NSAutoreleasePool alloc]init]; On 2011/10/06 23:36:22, dmac wrote: > space between ] and init Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:13: pool_ = [[NSAutoreleasePool alloc]init]; > do you really need an autoreleasepool? please explain why Removed. One of my object-c misunderstandings. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:15: frameWidth_ = 0; On 2011/10/06 23:36:22, dmac wrote: > no need to set things to zero/nil. This happens automagically in objc Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:25: if(captureSession_) { On 2011/10/06 23:36:22, dmac wrote: > no need to check if these are nil Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:43: + (void) deviceNames:(NSArray *) deviceList { On 2011/10/06 23:36:22, dmac wrote: > no space between ) and device (in both cases) Modified. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:44: [deviceList initWithArray:[QTCaptureDevice On 2011/10/06 23:36:22, dmac wrote: > This does not do what you think it does. My guess it you just want to return > [QTCaptureDevice inputDevicesWithMediaType:QTMediaTypeVideo] Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:50: - (void)registerReceiver:(media::VideoCaptureDeviceMac *)frameReceiver { Good point, changed. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:54: - (BOOL)setCaptureDevice:(const char *)name { On 2011/10/06 23:36:22, dmac wrote: > so on one side of the interface you take an NSString and convert it to a char *, > and then you rebuild it into an NSString? how about just passing an NSString... Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:62: captureDevices = [[[NSArray alloc] initWithArray: Changed to the suggested code, almost... I changed to use the uniqueId instead of name to distinguish between different devices with the same name. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:100: if (captureDecompressedOutput_) { On 2011/10/06 23:36:22, dmac wrote: > no need to check. Just call release on it. > > Same with captrueSession_ and captureDeviceInput_ etc. Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:141: return NO; On 2011/10/06 23:36:22, dmac wrote: > log the error? Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:154: return NO; On 2011/10/06 23:36:22, dmac wrote: > log the error? Done. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:156: [[NSRunLoop currentRunLoop] runUntilDate:[NSDate distantFuture]]; > what breaks out of this runloop? The runloop was originally from sample, which I probably had misunderstood. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:182: if(frameReceiver_) { Good point, changed. http://codereview.chromium.org/8177008/diff/1/media/video/capture/mac/video_c... media/video/capture/mac/video_capture_device_mac_qtkit.mm:184: CVBufferRetain(videoFrame); > why do feel you need to retain and release the videoFrame in here? It was to make sure videoFrame was valid during the entire call and that is the way it is done in the samples I've seen. I couldn't find any documentation to back this up and it seems reasonable that CVPixelBufferLockBaseAddress should be enough. Removed. http://codereview.chromium.org/8177008/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:65: #if defined(OS_MACOSX) On 2011/10/06 23:36:22, dmac wrote: > why can't any of these tests be enabled? I'll look at this after this upload, but my understanding is that a runloop is needed to take care of events, resulting in we're not getting any frames from the camera. But I'll have another look at this.
Added comment for one thing to change in next upload. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:63: [NSString stringWithUTF8String:device_name_.unique_id.c_str()]; I'll change this to make it autorelease instead.
didn't really look at ObjC code but had a few nits/questions http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.h (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.h:12: #include <string> include order here shouuld be: #include <stl headers> #include "base/compiler_specific.h" #include "media/video/..." http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.h:40: int IncomingFrame(void* video_frame, int video_frame_length, nit: this doesn't read as a verb given you refer to this object as a frame_receiver, how about ReceiveFrame()? :) also what does the return value mean? why the opaque pointer? is there a stronger type we can use? http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:31: new VideoCaptureDeviceMac(device_name); indent by 2 more spaces http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:32: if (!capture_device) { is this checking for a failed memory allocation? these types of checks aren't typically useful, especially since Chrome uses base::EnableTerminationOnOutOfMemory() (i.e., this code will never execute) http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:44: VideoCaptureDeviceMac::VideoCaptureDeviceMac(const Name& device_name) could you re-order the functions to match header file order? http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:52: [capture_device_ release]; I'm an ObjC noob, but is it OK to call release on a NULL object? http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:58: return; is it an error to call Allocate if we're not idle? should we add a DCHECK? http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:85: if (state_ != kAllocated) { ditto for error / DCHECK-worthiness http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:96: if (state_ != kCapturing) { ditto for error / DCHECK-worthiness http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:104: if (state_ != kAllocated && state_ != kCapturing) { ditto for error / DCHECK-worthiness http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:119: DCHECK(state_ == kNotInitialized); DCHECK_EQ http://codereview.chromium.org/8177008/diff/7001/media/video/capture/video_ca... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/video_ca... media/video/capture/video_capture_device_unittest.cc:65: #if defined(OS_MACOSX) will these tests ever be supported? if so you should consider using DISABLED instead the convention we use for disabling certain tests on certain platforms is to prefix all tests with MAYBE_ and the put and #if/#end block at the top of the file i.e., #if defined(OS_MACOSX) #define MAYBE_OpenInvalidDevice DISABLED_OpenInvalidDevice ... #else #define MAYBE_OpenInvalidDevice OpenInvalidDevice #endif Check out chrome/browser/extensions/extension_tabs_apitest.cc for an example The advantages of using DISABLED is that it still lets developers run the tests by overriding via command line flag
My comments totally mess with your interface, but I think it will really simplify this code. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.h (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.h:5: // OS X implementaion of VideoCaptureDevice, using QTKit as native capture API. s/implemmentaion/implementation/ http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:52: [capture_device_ release]; On 2011/10/07 17:17:55, scherkus wrote: > I'm an ObjC noob, but is it OK to call release on a NULL object? Technically it's a "nil" object, and yes it is ;-) http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:63: [NSString stringWithUTF8String:device_name_.unique_id.c_str()]; On 2011/10/07 15:15:29, mflodman wrote: > I'll change this to make it autorelease instead. I'm confused, as this is already autoreleased http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:118: bool VideoCaptureDeviceMac::Init() { From an API point of view, does it make sense to have this init function? The only thing that is really going to fail here is the allocation of the capture_device_. Should this not be done in Allocate? Then DeAllocate could release the capture_device potentially freeing up system resources? This would also allow you to simplify the interface for VideoCaptureDeviceMatQTKit by passing in the init all of the values you are currently calling set on which would make VideoCaptureDeviceMatQTKit basically an immutable object. I would have it something like -(id)initWithDeviceID:(NSString*)id height:(int)height width:(int)width frameRate:(int)frameRate frameReceiver:(media::VideoCaptureDeviceMac *)frameReceiver. It would certainly cut down on a lot of code. Also I wonder if this would let you drop the state_ member var here, and keep track of the state by querying the capture_device_. It doesn't seem right to have state external to the capture_device_ to me. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:14: #include "media/video/capture/video_capture_device.h" do you need these includes? All I can see needing is a declaration of namespace media { class VideoCaptureDeviceMac; } http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:31: QTCaptureDeviceInput *captureDeviceInput_; any reason for retaining the input and the output instead of querying the session for them when you need them? http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:36: + (NSArray *)deviceNames; what is this returning in the NSArray? Names? or Strings? Might be worthwhile putting in the comment. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:45: - (BOOL)setCaptureCapabilities:(int)height:(int)width:(int)frameRate; actually, this should be: - (BOOL)setCaptureHeight:(int)height width:(int)width frameRate:(int)frameRate;
Thanks for the comments! http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.h (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.h:5: // OS X implementaion of VideoCaptureDevice, using QTKit as native capture API. On 2011/10/07 17:38:55, dmac wrote: > s/implemmentaion/implementation/ Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.h:12: #include <string> On 2011/10/07 17:17:55, scherkus wrote: > include order here shouuld be: > > #include <stl headers> > > #include "base/compiler_specific.h" > #include "media/video/..." Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.h:40: int IncomingFrame(void* video_frame, int video_frame_length, > given you refer to this object as a frame_receiver, how about ReceiveFrame()? :) Changed to ReceiveFrame. > also what does the return value mean? why the opaque pointer? is there a > stronger type we can use? QTKit gives a void* to us, but I'm changing to cast it as soon as we get it instead of in "the second step" as now. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:31: new VideoCaptureDeviceMac(device_name); On 2011/10/07 17:17:55, scherkus wrote: > indent by 2 more spaces Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:32: if (!capture_device) { Ok, thanks for the info. Removed. It was done like this in the other VideoCaptureDevice implementations, so we should remove them there as well. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:44: VideoCaptureDeviceMac::VideoCaptureDeviceMac(const Name& device_name) On 2011/10/07 17:17:55, scherkus wrote: > could you re-order the functions to match header file order? The only disorder I see is the static VideoCaptureDevice:: functions I put at top, since they are not part of the header file. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:58: return; On 2011/10/07 17:17:55, scherkus wrote: > is it an error to call Allocate if we're not idle? should we add a DCHECK? Linux and Win returns, instead of DCHECK, and it is tested that observer_->OnFrameInfo is not called for the second call. That test is disabled for Mac right now, but I'll keep the code to get the same behavior. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:63: [NSString stringWithUTF8String:device_name_.unique_id.c_str()]; On 2011/10/07 17:38:55, dmac wrote: > On 2011/10/07 15:15:29, mflodman wrote: > > I'll change this to make it autorelease instead. > > I'm confused, as this is already autoreleased And you're probably confused since I was confused about the objective-c memory terms. Got it now, I hope... http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:85: if (state_ != kAllocated) { On 2011/10/07 17:17:55, scherkus wrote: > ditto for error / DCHECK-worthiness Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:96: if (state_ != kCapturing) { On 2011/10/07 17:17:55, scherkus wrote: > ditto for error / DCHECK-worthiness Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:104: if (state_ != kAllocated && state_ != kCapturing) { On 2011/10/07 17:17:55, scherkus wrote: > ditto for error / DCHECK-worthiness DeAllocate will actually be called twice in most cases, so it's a bit different than the other API calls. (Won't go into details, but this is since a device can be closed directly by MediaStreamManager code). Maybe we can change this in some way, but then we need to change for all three platforms in the same time. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:118: bool VideoCaptureDeviceMac::Init() { The QTKit code is written right now to match Linux and Win in regards to when things are done. I agree the code would be a lot nicer if everything would be done as you suggest, but it wouldn't match Win and Linux. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac.mm:119: DCHECK(state_ == kNotInitialized); On 2011/10/07 17:17:55, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:14: #include "media/video/capture/video_capture_device.h" On 2011/10/07 17:38:55, dmac wrote: > do you need these includes? All I can see needing is a declaration of > > namespace media { > class VideoCaptureDeviceMac; > } Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:31: QTCaptureDeviceInput *captureDeviceInput_; input gets disconnected in 'Stop', to avoid a possible hang, hence kept as member so we can connect if calling start again for the same device. The main reason for having output as a member was to simplify the code. Changed now according to your suggestion + autorelease to simplify dealloc and setCaptureDevice(nil). http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:36: + (NSArray *)deviceNames; Added a comment, but I'll change this to return two arrays of device names and device ids instead. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:45: - (BOOL)setCaptureCapabilities:(int)height:(int)width:(int)frameRate; On 2011/10/07 17:38:55, dmac wrote: > actually, this should be: > > - (BOOL)setCaptureHeight:(int)height width:(int)width frameRate:(int)frameRate; Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/video_ca... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/video_ca... media/video/capture/video_capture_device_unittest.cc:65: #if defined(OS_MACOSX) Thanks, changed! Yes, the plan is to support them, but it will require some rewriting of the tests.
Added comment while waiting for next upload. http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.h:44: // Configures the capture properties. I'll correct this in the next upload, I missed to commit this part locally before uploading.
http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:31: QTCaptureDeviceInput *captureDeviceInput_; On 2011/10/10 12:56:52, mflodman wrote: > input gets disconnected in 'Stop', to avoid a possible hang, hence kept as > member so we can connect if calling start again for the same device. > > The main reason for having output as a member was to simplify the code. Changed > now according to your suggestion + autorelease to simplify dealloc and > setCaptureDevice(nil). Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:31: QTCaptureDeviceInput *captureDeviceInput_; On 2011/10/10 12:56:52, mflodman wrote: > input gets disconnected in 'Stop', to avoid a possible hang, hence kept as > member so we can connect if calling start again for the same device. What hang are you referring to? > > The main reason for having output as a member was to simplify the code. Changed > now according to your suggestion + autorelease to simplify dealloc and > setCaptureDevice(nil). http://codereview.chromium.org/8177008/diff/7001/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_mac_qtkit.h:36: + (NSArray *)deviceNames; On 2011/10/10 12:56:52, mflodman wrote: > Added a comment, but I'll change this to return two arrays of device names and > device ids instead. Done. http://codereview.chromium.org/8177008/diff/7001/media/video/capture/video_ca... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/7001/media/video/capture/video_ca... media/video/capture/video_capture_device_unittest.cc:65: #if defined(OS_MACOSX) On 2011/10/10 12:56:52, mflodman wrote: > Thanks, changed! > > Yes, the plan is to support them, but it will require some rewriting of the > tests. Not to be annoying, but wouldn't it be better to write the tests now while things are fresh in your mind? I think it's a fari assumption to say if they aren't done now, that they are probably never going to get done.... http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:118: capture_device_ = [[VideoCaptureDeviceMacQTKit alloc] init]; ok, how about at the very least then: initWithFrameReceiver: http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:119: if (capture_device_ == NULL) { if (!capture_device_) or if (capture_device_ == nil) http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:16: return [super init]; no need to have an empty init like this. http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:136: if ([[captureSession_ inputs] count] > 0) { are you ever expecting the inputs to be anything other than 1 or 0 here?
More changes based on review by dmac. Still looking at the test... http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:118: capture_device_ = [[VideoCaptureDeviceMacQTKit alloc] init]; Sounds good, changed. http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:119: if (capture_device_ == NULL) { On 2011/10/11 00:25:47, dmac wrote: > if (!capture_device_) > > or > > if (capture_device_ == nil) Done. http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.h (right): http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.h:44: // Configures the capture properties. On 2011/10/10 17:23:28, mflodman wrote: > I'll correct this in the next upload, I missed to commit this part locally > before uploading. Done. http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:16: return [super init]; Changed to include frameObserver registration. http://codereview.chromium.org/8177008/diff/11001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:136: if ([[captureSession_ inputs] count] > 0) { Changed, good comment.
Modified unittest and added test helper to be able to run on Mac.
http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:15: - (void)dealloc { please move dealloc down under init http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:124: DLOG(ERROR) << "Could not connect video capture device."; worth logging the error as well? [[error localizedDescription] UTF8String] http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:144: get rid of blank line http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:150: const int LOCK_FLAGS = 0; we tend to use kLockFlags for constants http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:164: frameReceiver_->ReceiveFrame((UInt8 *)baseAddress, frameSize, static_cast<UInt8*> http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_unittest_helper_mac.mm (right): http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_unittest_helper_mac.mm:12: const int run_loop_timeout = 1; are you sure 1 second is enough? See comment about using a MessageLoop in the test file. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_unittest_helper_mac.mm:14: [[NSRunLoop mainRunLoop] runMode: NSDefaultRunLoopMode no space between : and NSDefaultRunLoopMode http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:16: // The camera is 'locked' by the application once started on MAC OS X, not when s/MAC/Mac/ http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:37: class MockFrameObserver: public media::VideoCaptureDevice::EventHandler { space between r and : http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:109: ExecuteRunLoop(); Have you considered using a MessageLoop (new MessageLoopForUI()) and a loop_->PostDelayedTask(FROM_HERE, new MessageLoop::QuitTask, TestTimeouts::action_max_timeout_ms()); ?
Smaller changes + changed autotest to post quit task. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:15: - (void)dealloc { On 2011/10/14 23:31:35, dmac wrote: > please move dealloc down under init Done. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:124: DLOG(ERROR) << "Could not connect video capture device."; On 2011/10/14 23:31:35, dmac wrote: > worth logging the error as well? [[error localizedDescription] UTF8String] Done, here and two other places. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:144: On 2011/10/14 23:31:35, dmac wrote: > get rid of blank line Done. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:150: const int LOCK_FLAGS = 0; On 2011/10/14 23:31:35, dmac wrote: > we tend to use kLockFlags for constants Done. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:164: frameReceiver_->ReceiveFrame((UInt8 *)baseAddress, frameSize, On 2011/10/14 23:31:35, dmac wrote: > static_cast<UInt8*> Done. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_unittest_helper_mac.mm (right): http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_unittest_helper_mac.mm:12: const int run_loop_timeout = 1; File removed. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_unittest_helper_mac.mm:14: [[NSRunLoop mainRunLoop] runMode: NSDefaultRunLoopMode File removed. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:16: // The camera is 'locked' by the application once started on MAC OS X, not when On 2011/10/14 23:31:35, dmac wrote: > s/MAC/Mac/ Done. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:37: class MockFrameObserver: public media::VideoCaptureDevice::EventHandler { On 2011/10/14 23:31:35, dmac wrote: > space between r and : Done. http://codereview.chromium.org/8177008/diff/22001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:109: ExecuteRunLoop(); I wasn't aware of the MessageLoopForUI, thanks for pointing this out. I changed to using 5 sec as delay, which should be enough and a big margin. 'TestTimeouts::action_max_timeout_ms()' gave a delay of 45s for each test case, which seems a bit long.
You can ignore the things marked with NIT if you want. Just me being pedantic. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:18: // TODO(mflodman) Return name and id as NSArray* instead of QTCaptureDevice*. NIT: I usually expect a blank line before a comment that isn't indented. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:20: Name name; NIT: would be nice if name was a class that just had a c'tor and two getters to keep it immutable, but I know that's not your fault ;-) http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:35: return NULL; NIT: I would assign capture_device to NULL here and just have a single exit point for the method. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:44: capture_device_(NULL) { capture_device_ should be initiailized to nil not NULL. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:64: if ([capture_device_ setCaptureHeight:height NIT: normally we would say if (![capture... instead of compating to NO) http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:72: Capability current_settings; NIT: again, if you are playing in the future, it would be great if this struct was turned into a class with a c'tor and setters so that it was immutable. Immutables are just significantly safer for passing around between threads and such. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:108: return device_name_; NIT: worth inlining? http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:24: frameReceiver_ = frameReceiver; do self = [super init]; if (self) { frameReceiver_ = frameReceiver; } instead. Your first step in an init should be to set self to super init… where init… is the designated initializer for the super class. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/video_c... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/30001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:69: const int quit_task_wait_time_ms = 5000; don't use a fixed constant here. Use one of the values from TestTimeouts because when running under valgrind things can run WAAAY slower than you expect. Is there anyway to get a callback when the camera is ready? http://codereview.chromium.org/8177008/diff/30001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:231: EXPECT_TRUE(wait_event_.TimedWait(base::TimeDelta::FromMilliseconds( do you need the TimedWait and the PostQuitTask?
Changes to objective-c init and some nits. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac.mm (right): http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:18: // TODO(mflodman) Return name and id as NSArray* instead of QTCaptureDevice*. On 2011/10/17 17:02:29, dmac wrote: > NIT: I usually expect a blank line before a comment that isn't indented. Done. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:20: Name name; On 2011/10/17 17:02:29, dmac wrote: > NIT: would be nice if name was a class that just had a c'tor and two getters to > keep it immutable, but I know that's not your fault ;-) That is a really good point. I won't do anything about that in this CL, but I'll make a note about it and implement that later for all platforms in the same CL. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:35: return NULL; On 2011/10/17 17:02:29, dmac wrote: > NIT: I would assign capture_device to NULL here and just have a single exit > point for the method. Done. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:44: capture_device_(NULL) { On 2011/10/17 17:02:29, dmac wrote: > capture_device_ should be initiailized to nil not NULL. Done. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:64: if ([capture_device_ setCaptureHeight:height On 2011/10/17 17:02:29, dmac wrote: > NIT: normally we would say if (![capture... instead of compating to NO) Done. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:72: Capability current_settings; On 2011/10/17 17:02:29, dmac wrote: > NIT: again, if you are playing in the future, it would be great if this struct > was turned into a class with a c'tor and setters so that it was immutable. > Immutables are just significantly safer for passing around between threads and > such. Will do. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac.mm:108: return device_name_; On 2011/10/17 17:02:29, dmac wrote: > NIT: worth inlining? After this CL, I'll check if this can be removed completely instead, but I'd like to follow up with another patch doing this for all three platforms. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... File media/video/capture/mac/video_capture_device_mac_qtkit.mm (right): http://codereview.chromium.org/8177008/diff/30001/media/video/capture/mac/vid... media/video/capture/mac/video_capture_device_mac_qtkit.mm:24: frameReceiver_ = frameReceiver; On 2011/10/17 17:02:29, dmac wrote: > do > > self = [super init]; > if (self) { > frameReceiver_ = frameReceiver; > } > > instead. Your first step in an init should be to set self to super init… > > where init… is the designated initializer for the super class. Done. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/video_c... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/8177008/diff/30001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:69: const int quit_task_wait_time_ms = 5000; Ok, changed to your original proposal. It will take quite some time to run the test, but I guess it's the only way to make sure, for example, Valgrind has enough time. As you said. I'll keep searching for better ways, but I have none right now. http://codereview.chromium.org/8177008/diff/30001/media/video/capture/video_c... media/video/capture/video_capture_device_unittest.cc:231: EXPECT_TRUE(wait_event_.TimedWait(base::TimeDelta::FromMilliseconds( On 2011/10/17 17:02:29, dmac wrote: > do you need the TimedWait and the PostQuitTask? PostQuitTask is needed since it takes care of the RunLoop and the event is used to make sure a frame has been received and the test passed. After PostQuitTask, the event should fire right away and it won't introduce any more wait time. If the test passes as it should that is. I can set the event timeout to a smaller value, like action_timeout_value().
LGTM
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
