Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24615005:
Added AVFoundation Glue and Device Monitoring for Mac. (Closed)
|
Created:
7 years, 2 months ago by mcasas Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, wjia+watch_chromium.org, feature-media-reviews_chromium.org, DaleCurtis Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded AVFoundation Glue and Device Monitoring for Mac.
This CL adds files called avfoundation_glue.{h,mm} to media/video/capture/mac.
These files encapsulate dynamic loading of AVFoundation libraries and define+
implement a facade to the used AVFoundation classes. The first usage of those
is the DeviceMonitorMac, where the original QTKit event class is transformed
into an interface (MacMonitorInterface), implemented by both QTKitMonitorImpl
and AVFoundationMonitorImpl. the usage of one or the other is decided on
DeviceMonitorMac constructor time.
This CL is part of a larger exercise to add support for Video Capture in
Mac > 10.6 using AVFoundation.
BUG=288562
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230553
Patch Set 1 #
Total comments: 50
Patch Set 2 : OVerhauled following mark@ review. #
Total comments: 21
Patch Set 3 : Comments addressed. #
Total comments: 2
Patch Set 4 : #
Total comments: 25
Patch Set 5 : mark@'s comments addressed. #
Total comments: 25
Patch Set 6 : rsesek@'s comments addressed. #
Total comments: 24
Patch Set 7 : rsesek@'s comments and some nits. #
Total comments: 18
Patch Set 8 : More rsesek@ comments. #
Total comments: 2
Patch Set 9 : Reinterpret_cast and AV prefix in uppercase #Patch Set 10 : Rebase. #
Total comments: 12
Patch Set 11 : MacMonitor->DeviceMonitorMacImpl and nits. #
Total comments: 6
Patch Set 12 : rsesek@ last round of nits taken in. #
Total comments: 8
Patch Set 13 : dalecurtis@ comments #
Total comments: 8
Patch Set 14 : avi@ comments. #
Total comments: 1
Patch Set 15 : Added comments following avi@ ones. #
Messages
Total messages: 42 (0 generated)
Hi mark@, I tried my best to implement a facade for dynamic loading AVFoundation. Could you please take a look?
This is nowhere near ready. It doesn’t even appear to have been tested. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.h:23: class QTKitMonitorImpl; I don’t think you need these here. These classes don’t need to be exposed beyond the .mm file now. You can just move them into an anonymous namespace in the .mm file. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:19: MacMonitorInterface(DeviceMonitorMac* monitor) This constructor should be explicit. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic... https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:23: device_arrival_(nil), nil is an initializer for an Objective-C object pointer, but device_arrival_ and device_removal_ are both ints, so you should initialize them with 0. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:28: virtual void OnDeviceChanged() = 0; Weird indentation in this entire protected section. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:30: DeviceMonitorMac* monitor_; Protected data is almost unseen, even in interface cases like this one. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... and http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access... I recommend making MacMonitorInterface a pure interface class. Remove all of the data and track it all in the subclasses. As it stands now, there is no logic in MacMonitorInterface that actually uses or maintains any of this data. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:38: DeviceMonitorMac::MacMonitorInterface::~MacMonitorInterface() { } In the class declaration above, you made the destructor pure virtual, so what’s this for? https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:42: QTKitMonitorImpl(DeviceMonitorMac* monitor); explicit. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:53: device_arrival_ = (int) No, you definitely can’t cast id to int. I don’t know why you’d even try, since you only ever seem to use it as an id. id is a pointer type. Aside from generally not wanting to mix pointer types with integer types for sanity’s sake, in the 64-bit environment, id will be 64 bits wide, and the cast to int will truncate it. This comment applies throughout the file. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:104: AVFoundationMonitorImpl(DeviceMonitorMac* monitor); explicit https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:111: DeviceMonitorMac* monitor) This line would be a 4-space continuation line indent. (And the next line can stay as-is.) https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:144: for (AVCaptureDeviceGlue* device in devices) { Is it possible for the actual devices to change without the number of devices changing? Even if you’re getting one notification per connection/disconnection, it’s possible that your notification is not processed until after other events happen on the system. The state of your processing is not necessarily synchronized with the state of the system at the time the notification was generated. If one video device is disconnected and a different one is subsequently connected, and you don’t process either notification until after the second one was connected, number_video_devices will never appear to change, but the devices attached to the system were definitely changed. Your processing the first disconnect notification races against the second connect event. The QTKit code above shares this problem. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:166: if ([AVFoundationGlue IsAVFoundationSupported]){ Method names (even class method names, what would be a class-static function in C++) in Objective-C are always named with a leading lowercase letter. http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Objective-C_M... https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:7: You need a comment saying why we have to do this instead of linking against the AVFoundation framework directly. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:13: // AVFoundation Bundle could be loaded correctly, or NO otherwise. bundle uses a lowercase b. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:14: + (BOOL)IsAVFoundationSupported; See previous comment about naming. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:16: + (NSBundle*)getAVFoundationBundle; Normal naming would just be avFoundationBundle. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:23: + (NSArray *)devices; Above, you nestled the * up against the type name. Here, you put a space in between. Be consistent. Chromium code tends to prefer it without the space. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:30: @"AVCaptureDeviceWasConnectedNotification"; You shouldn’t hard-code these 5 strings, you should pull them out of the bundle dynamically. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:17: return [NSBundle This gets a new bundle each time it’s called, which kind of sucks. It should cache it. static NSBundle* bundle = [NSBundle bundleWithPath:…]; return bundle; https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:26: + (NSArray *)devices { Same comment about consistency with spacing around your *s for pointers applies. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:33: Class AVCaptureDeviceClass = [[AVFoundationGlue getAVFoundationBundle] AVCaptureDeviceClass is a local variable, and those are named with leading lowercase letters. http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Variable_Names https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:34: classNamed:@"AVCAptureDevice"]; This class name is miscapitalized. Did you even test this code? https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:35: // See http://goo.gl/hYvBs3 for an explanation of the 2 step process for Apple doc links go stale frequently. Don’t use shortened links for them. If you use the whole link, at least future readers will see which doc you intended to point to easily, and will have less of a hard time finding it. But I think this comment is kind of obvious, so you might just not need it at all. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:37: id AVCaptureDeviceInstance =[[AVCaptureDeviceClass alloc] init]; Space on each side of operators, like =. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:37: id AVCaptureDeviceInstance =[[AVCaptureDeviceClass alloc] init]; You alloc and init this, don’t release it or autorelease it or use a scoper, and then leak it. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:37: id AVCaptureDeviceInstance =[[AVCaptureDeviceClass alloc] init]; It seems very suspicious that you would just allocate your own empty AVCaptureDevice and then ask it if it has a media type. How is it supposed to know? It’s anonymous, and you just created it. It doesn’t have anything to do with a real device.
Hi mark@, first of all again apologies for the misunderstanding in the first code review. I've spent quite some time over the w/e and now I think it's more baked than before. In my defense I should say that not all code was mine. Now I take full responsibility (and blame) for it. Hence I'd like to ask for a coarse review that would not take much of your time, i.e. I would like to get a pointer if I'm going in the right direction or not. Also, ignore syntax and comments, that will get addressed later. Objective C idioms and things I'm doing wrong are welcome of course :) With that said, there are two big things still NOT OK that I know of: - A series of strings are still hardcoded in AVFoundationGlue (AVCaptureDeviceWasConnectedNotification, etc). I could not find a way to get them out of the Bundle, since they are statically allocated on the global AVFoundation namespace, i.e. outside of a class. Couldn't find it. Must have tried 20 different things. Maybe you could have a suggestion for this? - For some reason, registering for a notification based on a name, did not work for me. So I register to all notifications. This is known by me and I intend to address it. I didn't write a crbug.com/... because this needs to be solved before landing. So please don't spend time on this. - Generally I wrote comments freely. I will have a look at them and polish it before CL, please ignore them. Thanks! https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.h:23: class QTKitMonitorImpl; On 2013/09/26 17:09:10, Mark Mentovai wrote: > I don’t think you need these here. These classes don’t need to be exposed beyond > the .mm file now. You can just move them into an anonymous namespace in the .mm > file. Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:19: MacMonitorInterface(DeviceMonitorMac* monitor) On 2013/09/26 17:09:10, Mark Mentovai wrote: > This constructor should be explicit. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic... Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:23: device_arrival_(nil), On 2013/09/26 17:09:10, Mark Mentovai wrote: > nil is an initializer for an Objective-C object pointer, but device_arrival_ and > device_removal_ are both ints, so you should initialize them with 0. Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:28: virtual void OnDeviceChanged() = 0; On 2013/09/26 17:09:10, Mark Mentovai wrote: > Weird indentation in this entire protected section. Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:30: DeviceMonitorMac* monitor_; On 2013/09/26 17:09:10, Mark Mentovai wrote: > Protected data is almost unseen, even in interface cases like this one. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... > and > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access... > > I recommend making MacMonitorInterface a pure interface class. Remove all of the > data and track it all in the subclasses. As it stands now, there is no logic in > MacMonitorInterface that actually uses or maintains any of this data. Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:38: DeviceMonitorMac::MacMonitorInterface::~MacMonitorInterface() { } Gone. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:42: QTKitMonitorImpl(DeviceMonitorMac* monitor); On 2013/09/26 17:09:10, Mark Mentovai wrote: > explicit. Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:53: device_arrival_ = (int) On 2013/09/26 17:09:10, Mark Mentovai wrote: > No, you definitely can’t cast id to int. I don’t know why you’d even try, since > you only ever seem to use it as an id. > > id is a pointer type. Aside from generally not wanting to mix pointer types with > integer types for sanity’s sake, in the 64-bit environment, id will be 64 bits > wide, and the cast to int will truncate it. > > This comment applies throughout the file. Without wanting to escape my public defamation, I'd like to say in my defense that (most of) this code comes from somewhere else... https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:104: AVFoundationMonitorImpl(DeviceMonitorMac* monitor); On 2013/09/26 17:09:10, Mark Mentovai wrote: > explicit Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:111: DeviceMonitorMac* monitor) On 2013/09/26 17:09:10, Mark Mentovai wrote: > This line would be a 4-space continuation line indent. (And the next line can > stay as-is.) Done. https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:144: for (AVCaptureDeviceGlue* device in devices) { I thought this is addressed by the "usingBlock" in the addObserverForName, that would make the OnDeviceChanged() run on a critical section and with no race conditions? https://codereview.chromium.org/24615005/diff/1/content/browser/device_monito... content/browser/device_monitor_mac.mm:166: if ([AVFoundationGlue IsAVFoundationSupported]){ Happy to do so. I was following the style in: IsOSLionOrLater() :( https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:7: On 2013/09/26 17:09:10, Mark Mentovai wrote: > You need a comment saying why we have to do this instead of linking against the > AVFoundation framework directly. Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:13: // AVFoundation Bundle could be loaded correctly, or NO otherwise. On 2013/09/26 17:09:10, Mark Mentovai wrote: > bundle uses a lowercase b. Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:14: + (BOOL)IsAVFoundationSupported; On 2013/09/26 17:09:10, Mark Mentovai wrote: > See previous comment about naming. Done. Again, IsOsLionOrLater(). https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:16: + (NSBundle*)getAVFoundationBundle; On 2013/09/26 17:09:10, Mark Mentovai wrote: > Normal naming would just be avFoundationBundle. Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:23: + (NSArray *)devices; On 2013/09/26 17:09:10, Mark Mentovai wrote: > Above, you nestled the * up against the type name. Here, you put a space in > between. Be consistent. Chromium code tends to prefer it without the space. Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.h:30: @"AVCaptureDeviceWasConnectedNotification"; On 2013/09/26 17:09:10, Mark Mentovai wrote: > You shouldn’t hard-code these 5 strings, you should pull them out of the bundle > dynamically. I couldn't do this because the strings are defined at the root level of the AVFoundation, not inside a class, and couldn't find a way to get them from the NSBundle... :( https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:17: return [NSBundle On 2013/09/26 17:09:10, Mark Mentovai wrote: > This gets a new bundle each time it’s called, which kind of sucks. It should > cache it. > > static NSBundle* bundle = [NSBundle bundleWithPath:…]; > return bundle; Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:26: + (NSArray *)devices { On 2013/09/26 17:09:10, Mark Mentovai wrote: > Same comment about consistency with spacing around your *s for pointers applies. Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:33: Class AVCaptureDeviceClass = [[AVFoundationGlue getAVFoundationBundle] On 2013/09/26 17:09:10, Mark Mentovai wrote: > AVCaptureDeviceClass is a local variable, and those are named with leading > lowercase letters. > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Variable_Names Done. https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:34: classNamed:@"AVCAptureDevice"]; On 2013/09/26 17:09:10, Mark Mentovai wrote: > This class name is miscapitalized. Did you even test this code? I did but clearly this error did not show up :(, I think because finding classes out of a library dictionary in runtime never handles errors completely correct. This brought me to think about error handling in this case :) https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:35: // See http://goo.gl/hYvBs3 for an explanation of the 2 step process for Might be obvious for you ;) but for me, having to alloc->init a class taken out of a bundle ... :S https://codereview.chromium.org/24615005/diff/1/media/video/capture/mac/avfou... media/video/capture/mac/avfoundation_glue.mm:37: id AVCaptureDeviceInstance =[[AVCaptureDeviceClass alloc] init]; On 2013/09/26 17:09:10, Mark Mentovai wrote: > It seems very suspicious that you would just allocate your own empty > AVCaptureDevice and then ask it if it has a media type. How is it supposed to > know? It’s anonymous, and you just created it. It doesn’t have anything to do > with a real device. Yes absolutely. My bad. The real way is to compose an NSInvocation and retrieve the result of running it on the |device| input.
hi tommyw@, could you PTAL? The CL is not ready for landing, so please take a quick look at objective-c idioms, mistakes etc -- don't spend too much time yet. Thanks!
https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:42: device_arrival_(0), If these are id again now, the initializers should be nil, not 0. id = generic Objective-C pointer, the “zero” value literal is “nil.” Other pointers (Type*) = C/C++ pointer, the “zero” value literal is “NULL.” Non-pointer integral types, the “zero” value literal is 0 (or possibly 0.0, 0.0f, 0U, etc.) https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:67: [nc removeObserver:(id)device_arrival_]; Shouldn’t need these casts anymore since the type is actually id. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:165: forId:device] || Objective-C style: line up the colons. Take a look at the Objective-C style guide. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:168: ++number_video_devices; Since the condition took up so many lines, {braces} around the controlled code are helpful or mandatory depending on your read of the guide. I read it as mandatory. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:180: number_video_devices_ = number_video_devices; The synchronization problem I raised earlier still exists here. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:192: } // anonymous namespace Style: two spaces between } and //. In Chrome, it’s usual for namespace-ending comments to just be } // namespace (with no more words if it’s an unnamed namespace) https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:26: + (NSString *const)avCaptureDeviceWasConnectedNotification; Be consistent with your spacing on the *s. You didn’t put a space between the type name and the * on line 23. That is normal for Chrome code, so this would be NSString* const. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. no (c) necessary. You didn’t use it in your other new file. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:23: return @"AVCaptureDeviceWasConnectedNotification"; OK, the right thing to do here is to do a dlsym to get the symbol by name from the library. NSBundle doesn’t have a great interface for that, so once the bundle is loaded, you should dlopen it too. Then you’ll have two open handles to the same library: one for the NSBundle interface and one for the libdl-family interface. You can call dlopen on -[[NSBundle executablePath] fileSystemRepresentation]. Same for all of the other things that the library exposes as ordinary C data as opposed to Objective-C classes or selectors. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:58: forId:(id)device{ Is there no better generic type than id? If you’ve done this because the type declaration is in the SDK which you can’t use, the right thing to do here is to declare your own similar class. For example, if the real one is named AVCaptureDevice, you can write @interface CrAVCaptureDevice - (BOOL)hasMediaType:(NSString*)mediaType @end That will let you use CrAVCaptureDevice* as a type as a proxy for the real thing, and if you have a CrAVCaptureDevice*, you can call -hasMediaType: on it. You still want to check -respondsToSelector:, but you don’t need to go through the NSInvocation hoops. Much easier this way.
Hi mark@, thanks a bunch for the review, addressed your comments, PTAL. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:42: device_arrival_(0), On 2013/10/01 18:53:23, Mark Mentovai wrote: > If these are id again now, the initializers should be nil, not 0. > > id = generic Objective-C pointer, the “zero” value literal is “nil.” > Other pointers (Type*) = C/C++ pointer, the “zero” value literal is “NULL.” > Non-pointer integral types, the “zero” value literal is 0 (or possibly 0.0, > 0.0f, 0U, etc.) Done. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:67: [nc removeObserver:(id)device_arrival_]; On 2013/10/01 18:53:23, Mark Mentovai wrote: > Shouldn’t need these casts anymore since the type is actually id. Done. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:165: forId:device] || On 2013/10/01 18:53:23, Mark Mentovai wrote: > Objective-C style: line up the colons. Take a look at the Objective-C style > guide. Done. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:168: ++number_video_devices; On 2013/10/01 18:53:23, Mark Mentovai wrote: > Since the condition took up so many lines, {braces} around the controlled code > are helpful or mandatory depending on your read of the guide. I read it as > mandatory. Done. https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:180: number_video_devices_ = number_video_devices; Two things: I added a base::AutoLock to monitor_ so the execution of OnDeviceChanged() will be as critical section, avoiding the race conditions between "connected' and "disconnected events". Same for QtKit. Other thing is that, AFAIK, as long as every connection and disconnection ends up in a call to ProcessDevicesChanged() (bottom of the file), it does not matter if number_video_devices_ increases and decreases very fast. Does this coincide with your understanding? https://codereview.chromium.org/24615005/diff/10001/content/browser/device_mo... content/browser/device_monitor_mac.mm:192: } // anonymous namespace On 2013/10/01 18:53:23, Mark Mentovai wrote: > Style: two spaces between } and //. > In Chrome, it’s usual for namespace-ending comments to just be > > } // namespace > > (with no more words if it’s an unnamed namespace) Done. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:26: + (NSString *const)avCaptureDeviceWasConnectedNotification; On 2013/10/01 18:53:23, Mark Mentovai wrote: > Be consistent with your spacing on the *s. You didn’t put a space between the > type name and the * on line 23. That is normal for Chrome code, so this would be > NSString* const. Done. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/10/01 18:53:23, Mark Mentovai wrote: > no (c) necessary. You didn’t use it in your other new file. Done. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:23: return @"AVCaptureDeviceWasConnectedNotification"; Done. I think (?) that the dlopen does not need to be corresponded with a dlclose since the DeviceMonitorMac is a singleton living as long as Chrome, so we would only need to tear down the library handle on chrome destruction and that happens by default anyway, right? https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:58: forId:(id)device{ Much appreciated tip thanks! Done.
I have not reviewed the rest of your latest patch set but have given you a more complete explanation of the race problem. The lock doesn’t help. https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/10001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:23: return @"AVCaptureDeviceWasConnectedNotification"; miguelao wrote: > I think (?) that the dlopen does not need to be corresponded with a dlclose > since the DeviceMonitorMac is a singleton living as long as Chrome, so we would > only need to tear down the library handle on chrome destruction and that happens > by default anyway, right? Right, since you never release the NSBundle, you wouldn’t need to perform the analogous operation, dlclosing the handle from dlopen. https://codereview.chromium.org/24615005/diff/18001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/18001/content/browser/device_mo... content/browser/device_monitor_mac.mm:73: base::AutoLock lock(monitor_->lock_); How does this help? The major problem here isn’t that multiple threads might enter OnDeviceChanged simultaneously. (Maybe they may, maybe they won’t, it depends on QTKit and AVFoundation internals.) The major problem is that the state of the system can be different at the time the notification is posted (when the system recognizes that a device has been connected or disconnected) and the time you actually process the notification (when your code in OnDeviceChanged runs). Follow this example timeline: 0. 2 devices are present. 1. Unplug a device. 1 device is present now. 2. “Device disconnected” notification generated from (1). 3. Plug in a different device. 2 devices are present now. 4. “Device connected” notification generated from (3). 5. “Device disconnected” notification processed from (2). OnDeviceChanged runs, and finds 2 devices are present. This looks like an unchanged state, so NotifyDeviceChanged is not called. 6. “Device connected” notification processed from (4). OnDeviceChanged runs, and finds 2 devices are present. This looks like an unchanged state, so NotifyDeviceChanged is not called. There are now 2 devices present, but they’re not the same 2 devices that were present in step 0, because one device was removed and a new one was added. NotifyDeviceChanged was never called. Placing steps 5 and 6 under a lock does not help. The race is between step 3 and step 5. You are assuming that step 5 will immediately follow step 2, but that’s not necessarily the case.
https://codereview.chromium.org/24615005/diff/18001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/18001/content/browser/device_mo... content/browser/device_monitor_mac.mm:73: base::AutoLock lock(monitor_->lock_); I thought that when we register to the notifications, both are coming via an unspecified queue, hence using [NSOperationQueue mainQueue], and events cannot be delivered to the application (us) out of order. So you're saying that this NSOperationQueue can deliver out of order? If this is the case then I should create another queue with a simple policy and use it, how does this sound? The lock prevented another case in which both events are delivered (in order) very close to each other and there would be a race condition between them.
Notifications aren’t delivered out of order. There’s a race. You get the number of devices by calling +[QTCaptureDevice inputDevices] or +[AVCaptureDeviceGlue devices]. Those are independent of the notifications. They aren’t synchronized with your notification stream. A new device can be added or removed immediately before you make either of those calls. If that happens, those calls will reflect the state of the devices currently connected to the system, regardless of which notification you happen to be processing at the time. Neither of these calls knows or cares that you’re calling them from a notification observer or which notification triggered your observer, they will both return to you the current state of the system, which might be different than what the state of the system was when the notification was initially sent. Is that clearer?
On 2013/10/02 16:23:24, Mark Mentovai wrote: > Notifications aren’t delivered out of order. There’s a race. You get the number > of devices by calling +[QTCaptureDevice inputDevices] or +[AVCaptureDeviceGlue > devices]. Those are independent of the notifications. They aren’t synchronized > with your notification stream. A new device can be added or removed immediately > before you make either of those calls. If that happens, those calls will reflect > the state of the devices currently connected to the system, regardless of which > notification you happen to be processing at the time. Neither of these calls > knows or cares that you’re calling them from a notification observer or which > notification triggered your observer, they will both return to you the current > state of the system, which might be different than what the state of the system > was when the notification was initially sent. > > Is that clearer? I got it now! :) The solution I implemented is to keep a cached list of the devices uniqueID (and type), and test these names against those in the current system's snapshot.
(Tentative) ping. mark@ could you perhaps suggest another reviewer if you're busy?
Sorry, missed that you had updated this last week. I am here today but I’m out tomorrow and will be gone for two weeks. In my absence, you can contact rsesek for Mac reviews. This still isn’t a full review, but you’re certainly on the right track now. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:25: class DeviceInfo{ Fix the formatting of this class to match the style guide. Space before { on this line and line 27, space before public on the next line, name the enum values properly, name variables (including arguments and member values) properly, no semicolon after the {} on line 33, etc. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:40: DeviceType type; Maybe this can be a bit mask, so that you can stuff a device that does both audio and video into the list only once? https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:53: std::vector<DeviceInfo> cached_devices; Naming: members have trailing underscores. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:97: std::vector<DeviceInfo> snapshot_devices; Maybe snapshot_devices and cached_devices would be better as a set? Their IDs are supposed to be unique. You could DCHECK that they in fact are unique as you build snapshot_devices. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:120: } If cached_devices isn’t going to be used on any other threads while this operation is occurring, you can remove entries from cached_devices as you walk through snapshot_devices to handle device adds. Then, when you’re done with that, anything remaining in cached_devices is guaranteed to not be in snapshot_devices, and you can just walk cached_devices to pick up the removals without having to do a find on snapshot_devices. If cached_devices might still be used on other threads, you’ll probably need to bring the lock back anyway. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:149: std::vector<DeviceInfo> cached_devices; Similar comments apply in here. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:218: // Compare the current system devices snapshot with the ones cached to detect Looks like all, or almost all, of the rest of this function is common to both the QTKit and AVFoundation implementations. You should refactor it so that they share code. https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:222: if( std::find(cached_devices.begin(), cached_devices.end(), *it) Spacing is interesting here and on line 233. https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:11: @implementation AVFoundationGlue I’m not sure that there’s any benefit to this class being an Objective-C class. It has no instances or data other than static data, and has only class methods. There’s a slight runtime penalty to it being Objective-C. Would you consider turning it into a C++ namespace enclosing like functions, or a C++ class that contains only static methods and prevents instantiation via DISALLOW_IMPLICIT_CONSTRUCTORS? https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:26: executablePath] fileSystemRepresentation], RTLD_LAZY | RTLD_LOCAL); Not cool to pass NULL to dlopen in case any of the operations in that sequence fail and wind up returning nil. That’ll give you an RTLD_DEFAULT handle, which is not what you want. You should build up the first argument to dlopen separately and then test it for NULLness before calling dlopen. https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:63: +(NSString* const)readNSStringPtr:(char const* const)symbol Space after +. https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:73: @implementation AVCaptureDeviceGlue On the other hand, this “glue” class makes perfect sense as an Objective-C class because of its direct wrapping of AVCaptureDevice.
When you’re ready for an actual full review, you should probably ask tommi to look it over first, too. Let me know if you have any more pre-full review questions on the Mac implementation.
All comments from mark@ addressed.
rsesek@, could you PTAL?
A note of caution, clang-format in Mac reformatted lines like
void functionNameLongA(int long_argument1,
int long_argument2);
to
void functionNameLongA(int long_argument1,
int long_argument2);
which IMHO is clearer but...
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
File content/browser/device_monitor_mac.mm (right):
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:25: class DeviceInfo{
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Fix the formatting of this class to match the style guide. Space before { on
> this line and line 27, space before public on the next line, name the enum
> values properly, name variables (including arguments and member values)
> properly, no semicolon after the {} on line 33, etc.
Done.
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:40: DeviceType type;
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Maybe this can be a bit mask, so that you can stuff a device that does both
> audio and video into the list only once?
Instead of that I added a new category "kMuxed" and used it, which cleaned the
implementation of OnDeviceChanged() quite a bit. IMHO bit mask would complicate
the situation, having to define const ints, etc.
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:53: std::vector<DeviceInfo>
cached_devices;
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Naming: members have trailing underscores.
Done.
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:97: std::vector<DeviceInfo>
snapshot_devices;
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Maybe snapshot_devices and cached_devices would be better as a set? Their IDs
> are supposed to be unique. You could DCHECK that they in fact are unique as
you
> build snapshot_devices.
I think that would make sense if we would like to order |snapshot_devices| and
|cached_devices| based on their unique_id; AVFoundation always list devices in
its own internal order which is constant but not unique_id-wise increasing.
Practically, |snapshot_devices| and |cached_devices| have what would appear to
be random, but the _same_ random, hence comparisons (find()) between them are OK
without the need to re-order. Does this make sense?
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:120: }
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> If cached_devices isn’t going to be used on any other threads while this
> operation is occurring, you can remove entries from cached_devices as you walk
> through snapshot_devices to handle device adds. Then, when you’re done with
> that, anything remaining in cached_devices is guaranteed to not be in
> snapshot_devices, and you can just walk cached_devices to pick up the removals
> without having to do a find on snapshot_devices.
>
Done.
> If cached_devices might still be used on other threads, you’ll probably need
to
> bring the lock back anyway.
On second thoughts, the lock was brought due to my -incorrect- understanding
that two or more notifications could be served in parallel, hence having a race
condition in this OnDeviceChanged() method. Otherwise the device monitor is a
singleton so there should be no problem. I preventively removed the lock but
accept criticism ;)
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:149: std::vector<DeviceInfo>
cached_devices;
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Similar comments apply in here.
Done.
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:218: // Compare the current system devices
snapshot with the ones cached to detect
Done. Beefing the MacMonitorInterface actually made it no more and interface so
I put some more common parts (variables) in it.
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo...
content/browser/device_monitor_mac.mm:222: if( std::find(cached_devices.begin(),
cached_devices.end(), *it)
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Spacing is interesting here and on line 233.
Done.
https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a...
File media/video/capture/mac/avfoundation_glue.mm (right):
https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a...
media/video/capture/mac/avfoundation_glue.mm:11: @implementation
AVFoundationGlue
Done --second option: Class with static methods and no constructors.
https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a...
media/video/capture/mac/avfoundation_glue.mm:26: executablePath]
fileSystemRepresentation], RTLD_LAZY | RTLD_LOCAL);
Separated, if NULL then DCHECK(FALSE) and return NULL, otherwise sunny-side path
as is now.
https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a...
media/video/capture/mac/avfoundation_glue.mm:63: +(NSString*
const)readNSStringPtr:(char const* const)symbol
On 2013/10/09 16:05:12, Mark Mentovai wrote:
> Space after +.
Done.
https://codereview.chromium.org/24615005/diff/35001/media/video/capture/mac/a...
media/video/capture/mac/avfoundation_glue.mm:73: @implementation
AVCaptureDeviceGlue
Ack!
https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/35001/content/browser/device_mo... content/browser/device_monitor_mac.mm:97: std::vector<DeviceInfo> snapshot_devices; On 2013/10/12 09:59:27, miguelao wrote: > On 2013/10/09 16:05:12, Mark Mentovai wrote: > > Maybe snapshot_devices and cached_devices would be better as a set? Their IDs > > are supposed to be unique. You could DCHECK that they in fact are unique as > you > > build snapshot_devices. > > I think that would make sense if we would like to order |snapshot_devices| and > |cached_devices| based on their unique_id; AVFoundation always list devices in > its own internal order which is constant but not unique_id-wise increasing. > Practically, |snapshot_devices| and |cached_devices| have what would appear to > be random, but the _same_ random, hence comparisons (find()) between them are OK > without the need to re-order. Does this make sense? Side node: unless the order is explicitly guaranteed as part of the AVFoundation docs, this is liable to change in future OS releases and the order is not guaranteed to be stable. You're obviously free to sort them if you like. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:15: class DeviceInfo { If you're going to have public members, this needs to be a struct and the members should not have trailing _ https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:64: int video_device_added = 0; Why are these ints? It seems like you're just using them as bools. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:106: QTKitMonitorImpl() {} Why do you need to provide this constructor? https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:114: DCHECK(monitor); You DCHECK this but it is unused? https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:122: OnDeviceChanged(); These blocks are not formatted correctly. The contents should be indented four spaces and the closing brace should align with the line that has the opening brace. More details: http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:135: if (!monitor_) Couldn't this leave dangling observers? You do not conditionally add them in the ctor. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:177: NSOperationQueue* mainQueue = [NSOperationQueue mainQueue]; naming: main_queue https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:24: static BOOL isAVFoundationSupported(); C++ naming uses capitalization of the first character, i.e. IsAVFoundationSupported(). Here and throughout. https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:23: const char* library_path = [[AVFoundationGlue::avFoundationBundle() You shouldn't need |AVFoundationGlue::| since you're calling another static member of the same class. Here and throughout. https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:26: DCHECK(FALSE); FALSE -> false
hi rsesek@ thanks for the review, could you please take a look? https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:15: class DeviceInfo { I changed them to private and provided getters/accessors, i.e. unique_id_ --> unique_id() https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:64: int video_device_added = 0; Since several devices might have been plugged in/out at once, I'm using int for an easier syntax += (lines 78, 80 etc). It's not strictly necessary, but at the end bool will be implemented as a native integer type (32/64b), so I saw no performance gain. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:106: QTKitMonitorImpl() {} On 2013/10/14 17:26:44, rsesek wrote: > Why do you need to provide this constructor? No need - removed and also AVFoundationMonitorImpl() {} https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:114: DCHECK(monitor); Moved it to the MacMonitor parent class constructor, there it should be (at least) DCHECKed for NULLness. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:122: OnDeviceChanged(); On 2013/10/14 17:26:44, rsesek wrote: > These blocks are not formatted correctly. The contents should be indented four > spaces and the closing brace should align with the line that has the opening > brace. More details: > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... Done. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:135: if (!monitor_) On 2013/10/14 17:26:44, rsesek wrote: > Couldn't this leave dangling observers? You do not conditionally add them in the > ctor. Most likely. RIght now I don't see why a not initialised monitor_ -- just DCHECK()'ed on constructor, could possibly prevent deregistering the observers. This problem possibly never surfaced because the DeviceMonitorMac is a singleton lasting as long as the browser process itself. I'm removing the if(!monitor) checks. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:177: NSOperationQueue* mainQueue = [NSOperationQueue mainQueue]; On 2013/10/14 17:26:44, rsesek wrote: > naming: main_queue Done. https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:24: static BOOL isAVFoundationSupported(); On 2013/10/14 17:26:44, rsesek wrote: > C++ naming uses capitalization of the first character, i.e. > IsAVFoundationSupported(). Here and throughout. Done. https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:23: const char* library_path = [[AVFoundationGlue::avFoundationBundle() On 2013/10/14 17:26:44, rsesek wrote: > You shouldn't need |AVFoundationGlue::| since you're calling another static > member of the same class. Here and throughout. Done. https://codereview.chromium.org/24615005/diff/43001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:26: DCHECK(FALSE); On 2013/10/14 17:26:44, rsesek wrote: > FALSE -> false Done.
https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:64: int video_device_added = 0; On 2013/10/15 11:50:11, miguelao wrote: > Since several devices might have been plugged in/out at once, I'm using int for > an easier syntax += (lines 78, 80 etc). > > It's not strictly necessary, but at the end bool will be implemented as a native > integer type (32/64b), so I saw no performance gain. What about bool and |= ? This isn't really a question of performance, but rather of code comprehension: you're counting integers when you are using it just for testing existence. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:122: OnDeviceChanged(); On 2013/10/15 11:50:11, miguelao wrote: > On 2013/10/14 17:26:44, rsesek wrote: > > These blocks are not formatted correctly. The contents should be indented four > > spaces and the closing brace should align with the line that has the opening > > brace. More details: > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... > > Done. Not quite. Look at the fourth and fifth examples. A new line should exist before the } and it should be indented to align underneath the 'u'. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.h:17: class DeviceMonitorMac { This, like all top-level public declarations, needs a comment describing what it is. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.h:22: void NotifyDeviceChanged(base::SystemMonitor::DeviceType type); As does this. Where am I notified? https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:10: #include "media/video/capture/mac/avfoundation_glue.h" #import because the file being included contains ObjC @interfaces https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:16: nit: remove https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:30: } nit: blank line after https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:31: std::string unique_id() const { return unique_id_; } Return a const ref https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:34: private: nit: 1 space indent https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:45: MacMonitor() {} Remove this ctor? https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:51: virtual ~MacMonitor() {} This dtor needn't be virtual — it does not inherit from anything. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:54: void ConsolidateDevicesListAndNotify( Comments needed. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:179: device_removal_ = nit: check indention after ths line https://codereview.chromium.org/24615005/diff/52001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/52001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:59: + (BOOL)hasMediaType:(NSString const*)mediaType Idiomatic Objective-C doesn't use const at all. I'd remove any const qualifiers for things that inherit from NSObject.
rsesek@ could you PTAL? Thanks! https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:64: int video_device_added = 0; On 2013/10/15 20:44:46, rsesek wrote: > On 2013/10/15 11:50:11, miguelao wrote: > > Since several devices might have been plugged in/out at once, I'm using int > for > > an easier syntax += (lines 78, 80 etc). > > > > It's not strictly necessary, but at the end bool will be implemented as a > native > > integer type (32/64b), so I saw no performance gain. > > What about bool and |= ? This isn't really a question of performance, but rather > of code comprehension: you're counting integers when you are using it just for > testing existence. Done. Boolean it is. https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:122: OnDeviceChanged(); On 2013/10/15 20:44:46, rsesek wrote: > On 2013/10/15 11:50:11, miguelao wrote: > > On 2013/10/14 17:26:44, rsesek wrote: > > > These blocks are not formatted correctly. The contents should be indented > four > > > spaces and the closing brace should align with the line that has the opening > > > brace. More details: > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... > > > > Done. > > Not quite. Look at the fourth and fifth examples. A new line should exist before > the } and it should be indented to align underneath the 'u'. I hope I got it right this time :) Interestingly, it was incorrect in the original code. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.h:17: class DeviceMonitorMac { On 2013/10/15 20:44:46, rsesek wrote: > This, like all top-level public declarations, needs a comment describing what it > is. Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.h:22: void NotifyDeviceChanged(base::SystemMonitor::DeviceType type); On 2013/10/15 20:44:46, rsesek wrote: > As does this. Where am I notified? Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:10: #include "media/video/capture/mac/avfoundation_glue.h" On 2013/10/15 20:44:46, rsesek wrote: > #import because the file being included contains ObjC @interfaces Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:16: On 2013/10/15 20:44:46, rsesek wrote: > nit: remove Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:30: } On 2013/10/15 20:44:46, rsesek wrote: > nit: blank line after Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:31: std::string unique_id() const { return unique_id_; } On 2013/10/15 20:44:46, rsesek wrote: > Return a const ref Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:34: private: On 2013/10/15 20:44:46, rsesek wrote: > nit: 1 space indent Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:45: MacMonitor() {} On 2013/10/15 20:44:46, rsesek wrote: > Remove this ctor? Done. Note that this forces adding a explicit call to the one-argument constructor in the derived classes. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:51: virtual ~MacMonitor() {} On 2013/10/15 20:44:46, rsesek wrote: > This dtor needn't be virtual — it does not inherit from anything. Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:54: void ConsolidateDevicesListAndNotify( On 2013/10/15 20:44:46, rsesek wrote: > Comments needed. Done. https://codereview.chromium.org/24615005/diff/52001/content/browser/device_mo... content/browser/device_monitor_mac.mm:179: device_removal_ = On 2013/10/15 20:44:46, rsesek wrote: > nit: check indention after ths line Done. https://codereview.chromium.org/24615005/diff/52001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/52001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:59: + (BOOL)hasMediaType:(NSString const*)mediaType On 2013/10/15 20:44:46, rsesek wrote: > Idiomatic Objective-C doesn't use const at all. I'd remove any const qualifiers > for things that inherit from NSObject. Done.
This is definitely making progress. Keep it up! https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/43001/content/browser/device_mo... content/browser/device_monitor_mac.mm:122: OnDeviceChanged(); On 2013/10/16 11:04:40, miguelao wrote: > On 2013/10/15 20:44:46, rsesek wrote: > > On 2013/10/15 11:50:11, miguelao wrote: > > > On 2013/10/14 17:26:44, rsesek wrote: > > > > These blocks are not formatted correctly. The contents should be indented > > four > > > > spaces and the closing brace should align with the line that has the > opening > > > > brace. More details: > > > > > > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... > > > > > > Done. > > > > Not quite. Look at the fourth and fifth examples. A new line should exist > before > > the } and it should be indented to align underneath the 'u'. > > I hope I got it right this time :) Interestingly, it was incorrect in the > original code. Yes, is good now. I don't think a Mac person reviewed the initial code :). https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.h:19: // singleton object is created that lasts as long as the browser process. I don't really understand what this is getting at. I don't see base::Singleton here. https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:54: virtual void OnDeviceChanged() = 0; nit: blank line after https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:60: std::vector<DeviceInfo> snapshot_devices); |snapshot_devices| should also be a const-ref. https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:65: id device_arrival_; It's not clear from the names what these are. Comment to the effect of: "// Handles to NSNotificationCenter block observers." https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:126: queue:nil Why nil queue here but mainQueue below? What are the threading restrictions on these classes? SystemMonitor does not document the threading contract for ProcessDevicesChanged(). https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:242: device_monitor_impl_->OnDeviceChanged(); Why isn't this done for the QTKit impl? https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:26: static NSBundle const* AvFoundationBundle(); Why |Av| for the methods bug |AV| for the class? https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:39: static NSString* ReadNSStringPtr(char const* const symbol, Should this be public or private? It looks like it's only called within AVFoundationGlue. The |handle| argument looks like it could also be omitted if so, and it could just call AvFoundationLibraryHandle() directly.
Hi rsesek@ thanks for the quick reply, PTAL! https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.h:19: // singleton object is created that lasts as long as the browser process. Perhaps I used the naming a bit too freely. What I meant is that there's just one object of DeviceMonitorMac class, at once, per browser process. It's instantiated from browser_main_loop.cc [1]. I will rewrite for more clarity. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br... https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:54: virtual void OnDeviceChanged() = 0; On 2013/10/16 23:58:07, rsesek wrote: > nit: blank line after Done. https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:60: std::vector<DeviceInfo> snapshot_devices); On 2013/10/16 23:58:07, rsesek wrote: > |snapshot_devices| should also be a const-ref. Done. https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:65: id device_arrival_; On 2013/10/16 23:58:07, rsesek wrote: > It's not clear from the names what these are. Comment to the effect of: "// > Handles to NSNotificationCenter block observers." Done. https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:126: queue:nil Remainings of an experiment with mmentovai@. The mainqueue is used by default, so both codes are equivalent queue-wise. Put both queue usages to nil. SystemMonitor indeed does not explicit how it behaves with respect to threading. But finally it ends calling all observers via ObserverListThreadSafe pointers. https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:242: device_monitor_impl_->OnDeviceChanged(); On 2013/10/16 23:58:07, rsesek wrote: > Why isn't this done for the QTKit impl? It should be done indeed, to create the |cached_devices_|. https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:26: static NSBundle const* AvFoundationBundle(); On 2013/10/16 23:58:07, rsesek wrote: > Why |Av| for the methods bug |AV| for the class? Not sure if this will make you happy but here it goes: - NSStrings such as the video type are called avMediaTypeVideo in the original. Accesor uppercases first letter only. - Class name refers to AVFoundation, the library name, full three first letters uppercase. - The rest of the methods follow the accessors' pattern of uppercasing just once: AvFoundationBundle() and AvFoundationLibraryHandle(). https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:39: static NSString* ReadNSStringPtr(char const* const symbol, On 2013/10/16 23:58:07, rsesek wrote: > Should this be public or private? It looks like it's only called within > AVFoundationGlue. The |handle| argument looks like it could also be omitted if > so, and it could just call AvFoundationLibraryHandle() directly. Done. Private and removed handle, taking it in the body of the function.
ping.
https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... content/browser/device_monitor_mac.mm:126: queue:nil On 2013/10/17 08:16:36, miguelao wrote: > Remainings of an experiment with mmentovai@. The mainqueue is used by default, > so both codes are equivalent queue-wise. Put both queue usages to nil. That's not accurate: "If you pass nil, the block is run synchronously on the posting thread." https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundat...: > SystemMonitor indeed does not explicit how it behaves with respect to threading. > But finally it ends calling all observers via ObserverListThreadSafe pointers. Yup. https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.h:26: static NSBundle const* AvFoundationBundle(); On 2013/10/17 08:16:36, miguelao wrote: > On 2013/10/16 23:58:07, rsesek wrote: > > Why |Av| for the methods bug |AV| for the class? > > Not sure if this will make you happy but here it goes: > > - NSStrings such as the video type are called avMediaTypeVideo in the original. > Accesor uppercases first letter only. > - Class name refers to AVFoundation, the library name, full three first letters > uppercase. > - The rest of the methods follow the accessors' pattern of uppercasing just > once: AvFoundationBundle() and AvFoundationLibraryHandle(). > > > We generally capitalize initialisms in method names, though (e.g. URL). And since the AVFoundation artifacts to which these are referring are all named AV*, I'd do that. https://codereview.chromium.org/24615005/diff/70001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/70001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:54: NSString** stringPointer = (NSString**)dlsym(AvFoundationLibraryHandle(), You should use reinterpret_cast<> since C-style casts are banned.
On 2013/10/18 18:18:15, rsesek wrote: > https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... > File content/browser/device_monitor_mac.mm (right): > > https://codereview.chromium.org/24615005/diff/60001/content/browser/device_mo... > content/browser/device_monitor_mac.mm:126: queue:nil > On 2013/10/17 08:16:36, miguelao wrote: > > Remainings of an experiment with mmentovai@. The mainqueue is used by default, > > so both codes are equivalent queue-wise. Put both queue usages to nil. > > That's not accurate: > > "If you pass nil, the block is run synchronously on the posting thread." > > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundat...: > True. I confused no queue with mainQueue. The original experiment added queue to prevent out of order delivery of notifications, but that cannot happen anyway. I think that executing the notification synchronously in the posting thread is ok in this case. > > SystemMonitor indeed does not explicit how it behaves with respect to > threading. > > But finally it ends calling all observers via ObserverListThreadSafe pointers. > > Yup. > > https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... > File media/video/capture/mac/avfoundation_glue.h (right): > > https://codereview.chromium.org/24615005/diff/60001/media/video/capture/mac/a... > media/video/capture/mac/avfoundation_glue.h:26: static NSBundle const* > AvFoundationBundle(); > On 2013/10/17 08:16:36, miguelao wrote: > > On 2013/10/16 23:58:07, rsesek wrote: > > > Why |Av| for the methods bug |AV| for the class? > > > > Not sure if this will make you happy but here it goes: > > > > - NSStrings such as the video type are called avMediaTypeVideo in the > original. > > Accesor uppercases first letter only. > > - Class name refers to AVFoundation, the library name, full three first > letters > > uppercase. > > - The rest of the methods follow the accessors' pattern of uppercasing just > > once: AvFoundationBundle() and AvFoundationLibraryHandle(). > > > > > > > > We generally capitalize initialisms in method names, though (e.g. URL). And > since the AVFoundation artifacts to which these are referring are all named AV*, > I'd do that. > Done so. > https://codereview.chromium.org/24615005/diff/70001/media/video/capture/mac/a... > File media/video/capture/mac/avfoundation_glue.mm (right): > > https://codereview.chromium.org/24615005/diff/70001/media/video/capture/mac/a... > media/video/capture/mac/avfoundation_glue.mm:54: NSString** stringPointer = > (NSString**)dlsym(AvFoundationLibraryHandle(), > You should use reinterpret_cast<> since C-style casts are banned. Done as well.
Hi rsesek@ thanks for reviews, please see my immediately previous reply/answer/acknowledge to your comments, PTAL! https://codereview.chromium.org/24615005/diff/70001/media/video/capture/mac/a... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/70001/media/video/capture/mac/a... media/video/capture/mac/avfoundation_glue.mm:54: NSString** stringPointer = (NSString**)dlsym(AvFoundationLibraryHandle(), On 2013/10/18 18:18:15, rsesek wrote: > You should use reinterpret_cast<> since C-style casts are banned. Done.
Can you reupload? Rietveld says there's a chunk-mismatch.
On 2013/10/21 15:09:16, rsesek wrote: > Can you reupload? Rietveld says there's a chunk-mismatch. Rebased. PTAL!
https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:43: class MacMonitor { Hm… maybe this should be more descriptively named? DeviceMonitorMacImpl? https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:69: id device_removal_; nit: blank line after https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:184: AVCaptureDeviceWasConnectedNotification() nit: indent 4 more https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:190: DCHECK(device_arrival_); I don't think you need these DCHECKs. https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:193: AVCaptureDeviceWasDisconnectedNotification() nit: indent 4 more https://codereview.chromium.org/24615005/diff/276001/media/video/capture/mac/... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/276001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.h:40: static NSString* ReadNSStringPtr(char const* const symbol); nit: blank line after
Addressed all rsesek@ comments, could you please take a look? Thanks! https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:43: class MacMonitor { On 2013/10/21 17:31:36, rsesek wrote: > Hm… maybe this should be more descriptively named? DeviceMonitorMacImpl? Done. https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:69: id device_removal_; On 2013/10/21 17:31:36, rsesek wrote: > nit: blank line after Done. https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:184: AVCaptureDeviceWasConnectedNotification() On 2013/10/21 17:31:36, rsesek wrote: > nit: indent 4 more Done. https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:190: DCHECK(device_arrival_); On 2013/10/21 17:31:36, rsesek wrote: > I don't think you need these DCHECKs. Done. https://codereview.chromium.org/24615005/diff/276001/content/browser/device_m... content/browser/device_monitor_mac.mm:193: AVCaptureDeviceWasDisconnectedNotification() On 2013/10/21 17:31:36, rsesek wrote: > nit: indent 4 more Done. https://codereview.chromium.org/24615005/diff/276001/media/video/capture/mac/... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/276001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.h:40: static NSString* ReadNSStringPtr(char const* const symbol); On 2013/10/21 17:31:36, rsesek wrote: > nit: blank line after Done.
LGTM w/ nits https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.h:25: // Method called by the internal MacMonitor object |device_monitor_impl_| when nit: MacMonitor is now renamed https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.mm:152: DeviceInfo::DeviceType device_info; naming: device_info -> device_type. Also maybe move this into the loop? https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.mm:209: DeviceInfo::DeviceType device_info; Same.
rsesek@ nits addressed, thanks! fischman@ could you please have a look at https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.h:25: // Method called by the internal MacMonitor object |device_monitor_impl_| when On 2013/10/22 14:03:50, rsesek wrote: > nit: MacMonitor is now renamed Done. https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.mm:152: DeviceInfo::DeviceType device_info; On 2013/10/22 14:03:50, rsesek wrote: > naming: device_info -> device_type. Also maybe move this into the loop? Done. https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.mm:209: DeviceInfo::DeviceType device_info; On 2013/10/22 14:03:50, rsesek wrote: > Same. Done.
rsesek@ nits addressed, thanks! fischman@ could you please have a look at https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... File content/browser/device_monitor_mac.h (right): https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.h:25: // Method called by the internal MacMonitor object |device_monitor_impl_| when On 2013/10/22 14:03:50, rsesek wrote: > nit: MacMonitor is now renamed Done. https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.mm:152: DeviceInfo::DeviceType device_info; On 2013/10/22 14:03:50, rsesek wrote: > naming: device_info -> device_type. Also maybe move this into the loop? Done. https://codereview.chromium.org/24615005/diff/396001/content/browser/device_m... content/browser/device_monitor_mac.mm:209: DeviceInfo::DeviceType device_info; On 2013/10/22 14:03:50, rsesek wrote: > Same. Done.
rsesek@ nits addressed, thanks! fischman@ could you please have a look at
rsesek@ nits addressed, thanks! fischman@ could you please have a look at
Sorry for the previous spam. Somehow I managed to send 4 replies from Rietveld
by pressing my mouse pad once :) Recapitulating:
tommi@ could you please have a look at files
media/video/capture/mac/avfoundation_glue.{h,mm} ? Thanks!
sky@ could you please have a look at files under content/browser/ ? Thanks!
dalecurtis@ could you have a look at media/media.gyp? Cheers!
media/ lgtm % nits. https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.h:24: static BOOL IsAVFoundationSupported(); Should this just be a bool type? https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.mm:28: static void* library_handle = dlopen(library_path, RTLD_LAZY | RTLD_LOCAL); Presumably rsesek@ and mmentovai@ decided against using base::NativeLibrary ? https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.mm:53: NSString* AVFoundationGlue::ReadNSStringPtr(char const* const symbol) { "const char* symbol" is normally how this is written. What's the weird notation buying you? https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.mm:54: NSString** stringPointer = reinterpret_cast<NSString**>( string_pointer ?
I'm not familiar with this code at all. Is there another OWNER that is more knowledgable?
avi@ could you please have a look at content/browser files? dalecurtis@: thanks, nits addressed. On the (lack of) use of base::NativeLibrary, perhaps mmentovai@ or rsesek@ could give more details as to CFBundle versus NSBundle usage? https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... File media/video/capture/mac/avfoundation_glue.h (right): https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.h:24: static BOOL IsAVFoundationSupported(); On 2013/10/22 19:17:56, DaleCurtis wrote: > Should this just be a bool type? In the implementation there's an AND of a bool and a BOOL type (C++ and Obj-C++, resp.), so I (tentatively/weakly) wanted to keep this situation exposed by the method. Now I read that |bool| should only be used in Obj-C method signatures and since this one isn't, I'll change it. http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=BOOL_... https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.mm:28: static void* library_handle = dlopen(library_path, RTLD_LAZY | RTLD_LOCAL); On 2013/10/22 19:17:56, DaleCurtis wrote: > Presumably rsesek@ and mmentovai@ decided against using base::NativeLibrary ? base::NativeLibrary uses CFBundle and we are using here NSBundle, for a reason lost in the threads, perhaps mmentovai@ or rsesek@ can comment on that? https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.mm:53: NSString* AVFoundationGlue::ReadNSStringPtr(char const* const symbol) { On 2013/10/22 19:17:56, DaleCurtis wrote: > "const char* symbol" is normally how this is written. What's the weird notation > buying you? "const char* symbol" would be a mutable-pointer pointing to a constant-contents char array. Here I wanted to say constant-pointer pointing to a constant-contents char array. The extra constant-ness does not justify the readiness so I changed it back to "const char*". https://codereview.chromium.org/24615005/diff/476001/media/video/capture/mac/... media/video/capture/mac/avfoundation_glue.mm:54: NSString** stringPointer = reinterpret_cast<NSString**>( On 2013/10/22 19:17:56, DaleCurtis wrote: > string_pointer ? Absolutely. Done.
https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:15: class DeviceInfo { This holds just an enum and a string. Make it a struct. Drop all the accessor functions, drop your weird operator==. https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:27: // Operator== is needed here to use this class in a std::vector. If your operator==() function had no comment, I would have had no problem. But your comment makes no sense. Why would a vector care about equality? In addition, the comment misses what's unusual here, that type_ is ignored in the comparison. Why is that the case? https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:43: // never be instantiated on its own. The second sentence here should be dropped. This is an abstract class because it has a pure virtual function (OnDeviceChanged). No one _could_ instantiate it. https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:72: DISALLOW_COPY_AND_ASSIGN(DeviceMonitorMacImpl); DISALLOW must be in the private area.
avi@ thanks for the review, please have another look :) https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:15: class DeviceInfo { On 2013/10/23 14:44:37, Avi wrote: > This holds just an enum and a string. Make it a struct. Drop all the accessor > functions, drop your weird operator==. Glad to do it, but in line 89 there's a std::find operation that needs an operator==. https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:27: // Operator== is needed here to use this class in a std::vector. On 2013/10/23 14:44:37, Avi wrote: > If your operator==() function had no comment, I would have had no problem. But > your comment makes no sense. Why would a vector care about equality? > > In addition, the comment misses what's unusual here, that type_ is ignored in > the comparison. Why is that the case? Yes the comment is wrong, the operator== is necessary for std::find(). Will correct. The type_ is ignored because the unique_id_ is, well, unique, so for comparison purposes, a given unique_id_ will have always the same type_. https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:43: // never be instantiated on its own. On 2013/10/23 14:44:37, Avi wrote: > The second sentence here should be dropped. This is an abstract class because it > has a pure virtual function (OnDeviceChanged). No one _could_ instantiate it. Done. https://codereview.chromium.org/24615005/diff/666001/content/browser/device_m... content/browser/device_monitor_mac.mm:72: DISALLOW_COPY_AND_ASSIGN(DeviceMonitorMacImpl); On 2013/10/23 14:44:37, Avi wrote: > DISALLOW must be in the private area. Done.
Cool. LGTM https://codereview.chromium.org/24615005/diff/756001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/24615005/diff/756001/content/browser/device_m... content/browser/device_monitor_mac.mm:29: return unique_id_ == device.unique_id_; Can you put your explanation as to why you don't compare type_ here?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/24615005/866001
Message was sent while issue was closed.
Change committed as 230553 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
