Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
29423003:
Added video capture capabilities retrieval and caching to VideoCaptureManager (Closed)
|
Created:
7 years, 2 months ago by mcasas Modified:
7 years ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org, tommi (sloooow) - chröme, ncarter (slow), miu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded video capture capabilities retrieval and caching to VideoCaptureManager.
The local cache of video capture names and capabilities is created in a
codepath starting in EnumerateDevices. The cache is update during
StartCaptureForClient() and StopCaptureForClient().
Also added unittests (http://goo.gl/QQbpXW).
BUG=309554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235223
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Changed mechanics of Caps enumeration to be asynchronous and 2 threaded. UT adapted. #
Total comments: 40
Patch Set 3 : tommi@'s round of comments addressed. More UT. Capabilities reset on closing device. #
Total comments: 12
Patch Set 4 : Overhauled following all the inputs. #
Total comments: 20
Patch Set 5 : perkj@ comments. #Patch Set 6 : Defensive programming - against race conditions, id's not found, vectors that self-release etc. #
Total comments: 13
Patch Set 7 : perkj@'s comments #
Total comments: 24
Patch Set 8 : perkj@'s comments #
Total comments: 18
Patch Set 9 : perkj@'s comments #
Total comments: 19
Patch Set 10 : perkj@ nits and ncarter@ comments. #
Total comments: 12
Patch Set 11 : ncarter@: Removed double visit to Device thread. Removed ScopedVector<> use. #
Total comments: 31
Patch Set 12 : perkj@ comments and nits. #
Total comments: 25
Patch Set 13 : xians@ comments. #
Total comments: 16
Patch Set 14 : ncarter@ comments #
Total comments: 11
Patch Set 15 : perkj@ and xians@ comments #Patch Set 16 : Mac implementation double checks for device_id on opening, unnecessary but maintained. #Patch Set 17 : Removed RunUntilIdle between Stop and Close in UT, after Valgrind found a NULL addressing. #Messages
Total messages: 33 (0 generated)
Hi perkj@, continuing our quest could you PTAL?
https://codereview.chromium.org/29423003/diff/90001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/90001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_manager.cc:155: media::VideoCaptureCapabilities& formats = What happen after the camera has been stopped? Then you can restart the camera with all capabilities. https://codereview.chromium.org/29423003/diff/90001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_manager.cc:264: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); What is the plan when it comes to multiple opening of a camera - maybe from multiple processes? Shouln't the |device_info| that actually started the device also continue to get the full capability list? https://codereview.chromium.org/29423003/diff/90001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_manager.cc:264: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); So this is done on the IO thread but you modify |video_capture_capabilities_| on the device thread, thats a no no. On what thread will the request for the cabilities come from? I suspect the IO thread. Can you do all processing of |video_capture_capabilities_| on the IO thread? https://codereview.chromium.org/29423003/diff/90001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_manager.cc:344: media::VideoCaptureDevice::Names::iterator name_it; See how GetAvailableDevicesOnDeviceThread is used for an idea of how to get the capabilities to the IO thread. I think you will need a separate function for retrieving the capabilities back to the IO thread.
Hi perkj@, I changed the mechanics of VCCaps retrieval to follow two threaded, asynchronous like you indicated, could you have another rouch look at those parts? Yet not done: a) Reset device caps on device closing. But: If the device owning the device, after opening it, should ask for device capabilities, the former should receive back the filtered ones and not all of them, with the current pseudo-algorithms, this makes sense to me. Added tommi@ in cc FYI.
drive-by https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:15: #include "media/audio/audio_manager_base.h" remove. (no other change in this file?) https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/media_stream_manager.cc:895: remove this empty line. If there are plans for the implementation here, it might be good to document them with a todo. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_provider.h (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:28: typedef std::vector<VideoCaptureCapability> VideoCaptureCapabilities; only one space after > https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:69: const media::VideoCaptureCapabilities& capabilities) {} why not pure virtual like the other methods? https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:154: // Filter capture capabilities: remove all except the current one. why? (usually that's more important to document than the 'what') https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:317: DVLOG(1) << "OnDevicesEnumerated"; nit: above the listener check https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:339: DVLOG(1) << "OnDeviceCapabilitiesEnumerated device: " nit: move this DLOG above the listener_ check so that we can see that the notification arrived regardless of there being a listener. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:363: // Add to the cache the devices' supported capture formats. nit: sentence structure - it feels more natural to say "Add the capture formats of the devices to the cache." The opposite way (as it is now) feels like structure from a different language and is less readable (I had to read it more than once). https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:367: *name_it, &video_capture_capabilities_[name_it->id()] ); before sending a pointer to the entry in the map, can we have a DCHECK to make sure that that entry does not already exist? (i.e. that find() returns end()) https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:376: *name_it, &video_capture_capabilities_[name_it->id()] ); same here https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:405: if ( it != video_capture_capabilities_.end()) fix spaces https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:88: // reply comes via OnDeviceCapabilitiesEnumerated(). nit: nice comment in general but I don't think you need to explain the private implementation in detail. It would be good to mention on what thread the caller should expect a callback and when. The callback (as far as the caller is concerned) is actually DeviceCapabilitiesEnumerated on the listener interface and not OnDeviceCapabilitiesEnumerated which is a part of the private implementation. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:196: // EnumerateDeviceCapabilities(), read only. is this last part true? It looks like EnumerateDeviceCapabilities doesn't touch this variable and it really shouldn't since it does not run on the device thread. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:198: video_capture_capabilities_; Instead of an extra map, can we change video_capture_devices_ to be a list of VideoCaptureDevice::Name + VideoCaptureCapabilities? (note singular in both cases). As is, we have to be careful about keeping these two containers in sync. Something like: struct VideoCaptureDevice { media::VideoCaptureDevice::Name name; media::VideoCaptureCapabilities capabilities; }; std::list<VideoDevice> video_capture_devices_; (std::vector might also be fine) https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:176: 1 empty line https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:185: message_loop_->RunUntilIdle(); can you add a comment that explains what happens during this call? https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:186: DCHECK_GE(device_capabilities.size(), 1u); ASSERT_GE? https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:192: DCHECK_GE(format->width, 1); EXPECT_GE? (same below) https://codereview.chromium.org/29423003/diff/160001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/160001/media/video/capture/vide... media/video/capture/video_capture_device.h:27: // Represents a capture device name and ID. If we decide to add capabilities to the Name class, please update the documentation. Maybe the name should also be changed? https://codereview.chromium.org/29423003/diff/160001/media/video/capture/vide... media/video/capture/video_capture_device.h:89: VideoCaptureCapabilities capture_formats_; I haven't made up my mind whether adding capabilities to this class is a good idea but right now it doesn't look like this variable is ever used.
tommi@: thanks for the drive-in ;) -- comments addressed. perkj@ could you have a look please? Some other comments: - when a device is opened, the list of the capabilities is trimmed to the current capture format. This way, if a second getUserMedia comes from the same renderer, the filtered list would be returned. (With UT). - added resetting to whole device capabilities list after closing device. (With UT) https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:15: #include "media/audio/audio_manager_base.h" On 2013/10/25 14:15:34, tommi wrote: > remove. (no other change in this file?) Removed. This file mocks a MediaStreamProviderListener (shouldn't); originally I added DeviceCapabilitiesEnumerated as a virtual=0 method to that interface, then I noticed I had to mock it here -- whole thing turned too complex so I reverted, and must have forgotten this. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/media_stream_manager.cc:895: On 2013/10/25 14:15:34, tommi wrote: > remove this empty line. > > If there are plans for the implementation here, it might be good to document > them with a todo. Done & done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_provider.h (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:28: typedef std::vector<VideoCaptureCapability> VideoCaptureCapabilities; On 2013/10/25 14:15:34, tommi wrote: > only one space after > Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:69: const media::VideoCaptureCapabilities& capabilities) {} On 2013/10/25 14:15:34, tommi wrote: > why not pure virtual like the other methods? Because the AudioInputDeviceManager also implements this interface and it should not have to override this method (nor pull video-related data structures). https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:154: // Filter capture capabilities: remove all except the current one. On 2013/10/25 14:15:34, tommi wrote: > why? (usually that's more important to document than the 'what') Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:317: DVLOG(1) << "OnDevicesEnumerated"; On 2013/10/25 14:15:34, tommi wrote: > nit: above the listener check Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:339: DVLOG(1) << "OnDeviceCapabilitiesEnumerated device: " On 2013/10/25 14:15:34, tommi wrote: > nit: move this DLOG above the listener_ check so that we can see that the > notification arrived regardless of there being a listener. Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:363: // Add to the cache the devices' supported capture formats. On 2013/10/25 14:15:34, tommi wrote: > nit: sentence structure - it feels more natural to say "Add the capture formats > of the devices to the cache." The opposite way (as it is now) feels like > structure from a different language and is less readable (I had to read it more > than once). Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:367: *name_it, &video_capture_capabilities_[name_it->id()] ); On 2013/10/25 14:15:34, tommi wrote: > before sending a pointer to the entry in the map, can we have a DCHECK to make > sure that that entry does not already exist? (i.e. that find() returns end()) I guess this doesn't apply since the map is gone. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:376: *name_it, &video_capture_capabilities_[name_it->id()] ); On 2013/10/25 14:15:34, tommi wrote: > same here dixit. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:405: if ( it != video_capture_capabilities_.end()) On 2013/10/25 14:15:34, tommi wrote: > fix spaces Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:88: // reply comes via OnDeviceCapabilitiesEnumerated(). On 2013/10/25 14:15:34, tommi wrote: > nit: nice comment in general but I don't think you need to explain the private > implementation in detail. It would be good to mention on what thread the caller > should expect a callback and when. The callback (as far as the caller is > concerned) is actually DeviceCapabilitiesEnumerated on the listener interface > and not OnDeviceCapabilitiesEnumerated which is a part of the private > implementation. Boosted prosaic skills :) + corrected callback name. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:196: // EnumerateDeviceCapabilities(), read only. On 2013/10/25 14:15:34, tommi wrote: > is this last part true? It looks like EnumerateDeviceCapabilities doesn't touch > this variable and it really shouldn't since it does not run on the device > thread. Correct, this comment has not been updated after I moved from ignoring-threads implementation to now-let's-respect-threads implementation. It should read "retrieved _via_..." since that's what clients of the class will see. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:198: video_capture_capabilities_; Done. It turned out to be a class - first due to Chromium style (complex constructor due to non-POD members), and bc it needed a two param constructor for the vector. Had to add a method outside this class called FindDeviceInfoById(), which might have rendered useless Name::FindById(). https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:176: On 2013/10/25 14:15:34, tommi wrote: > 1 empty line Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:185: message_loop_->RunUntilIdle(); On 2013/10/25 14:15:34, tommi wrote: > can you add a comment that explains what happens during this call? (Tentatively) Done. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:186: DCHECK_GE(device_capabilities.size(), 1u); On 2013/10/25 14:15:34, tommi wrote: > ASSERT_GE? Hmm device documentation says devices may return empty capability lists, and further below there's a |vcm_->Unregister()| that should be called even if returned list is empty. https://codereview.chromium.org/29423003/diff/160001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:192: DCHECK_GE(format->width, 1); Oops :) Yes, all should be EXPECT_GE https://codereview.chromium.org/29423003/diff/160001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/160001/media/video/capture/vide... media/video/capture/video_capture_device.h:27: // Represents a capture device name and ID. On 2013/10/25 14:15:34, tommi wrote: > If we decide to add capabilities to the Name class, please update the > documentation. Maybe the name should also be changed? On second thoughts I think is best to leave this class untouched - except the styleguide warnings addressed below: complex constructor should not be inline. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_... https://codereview.chromium.org/29423003/diff/160001/media/video/capture/vide... media/video/capture/video_capture_device.h:89: VideoCaptureCapabilities capture_formats_; This should go, is left from an experiment. https://codereview.chromium.org/29423003/diff/490001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/490001/media/video/capture/vide... media/video/capture/video_capture_device.h:21: #include "media/base/video_frame.h" VideoFrame was used but its header included transitively via video_capture_types.h, according to [1] is a better-don't. [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...
https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_provider.h (right): https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:65: // Called by a MediaStreamProvider when available devices' formats have been Why here? Didn't we agree that this should go through the VideoCaptureHost? and not throught the MediaStreamManager? https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:380: media::VideoCaptureDevice::GetDeviceSupportedFormats(*name_it, In the previous Cl we said that media::VideoCaptureDevice::GetDeviceSupportedFormats will not be called on a capture device that has already been started. So can we make sure to only query capabilities for new devices? https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:503: class VideoCaptureManager::DeviceInfo* remove class https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:90: // listener_->DeviceCapabilitiesEnumerated(). This is not what we discussed. You want to access the capabilities from the VideoCaptureHost as we talked about and as you wrote in your email. Also - since you have already cached the capabilities you should be able make this syncronous. The actual device is identified in the same way as when you start a capture device, according to the comments in StartCaptureForClient it is |capture_params.session_id|. https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:168: TEST_F(VideoCaptureManagerTest, EnumerateDevicesAndCheckCapabilities) { You don't need to be able to enumerate all devices and query the capabilities. You need to be able to query the capabilities of a certain VideoCaptureParams.session_id it seems like before you call Start on the device.
You should consult nick@ for changes to VideoCaptureManager and VideoCaptureDevice, as he just completed a major clean-up there about a month ago. Drive-by style/nit comments... https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:191: class DeviceInfo { C++ style: struct, not class https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:206: std::vector<class DeviceInfo> devices_info_cache_; remove "class " https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:206: std::vector<class DeviceInfo> devices_info_cache_; Consider making this a vector<DeviceInfo*> instead, to avoid unnecessary copying of the struct. https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:207: class DeviceInfo* FindDeviceInfoById(const std::string& id); remove "class " https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:207: class DeviceInfo* FindDeviceInfoById(const std::string& id); C++ style: Move this method up (before data members).
https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/490001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:206: std::vector<class DeviceInfo> devices_info_cache_; On 2013/10/29 18:43:27, Yuri wrote: > Consider making this a vector<DeviceInfo*> instead, to avoid unnecessary copying > of the struct. +1. base::ScopedVector could come in handy: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
perkj@, miu@, tommi@ thanks for the review. Instead of closing every comment I overhauled a bit how things are done, but I believe I addressed every point. perkj@, could you PTAL? Thanks! Overview: - All devices' name and capabilities caching and manipulating is done in Browser::IO thread (this was a concern of nick@). - All start/stop |devices_info_cache_| manipulation is done in the IO thread as well. - Device caps retrieval is now synchronous, since is just search-get in the IO thread. - Device caps retrieval will be done from the VideoCaptureHost, using session id. This limits caps usage to the devices' lifetime Open-Start-Stop-Close. Nothing prevents from retrieving the caps via device id, but this is a (way) future provision. - Unittests changed to reflect all this. - Using base::ScopedVector for the |devices_info_cache_|. - Removed struct/class X references. You guys have clearly never read this :) http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp000... (okay, this is typedef, but in a way is the same).
A few more comments. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_provider.h (right): https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:56: virtual void Error(MediaStreamType stream_type, Please revert changes in this file. I have a cl where I remove this function all together since it is unused. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:145: DeviceInfo* device_info, can you just pass the media::VideoCaptureDevice::Name instead of the device_info here. It does not look like you need anything else. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:230: entry, found, params_as_capability, Just pass the name? https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:271: media::VideoCaptureCapabilities VideoCaptureManager::GetDeviceCapabilities( would it make more sence to have this void GetDeviceCapabilities(int session_id, media::VideoCaptureCapabilities*) ? that way you don't need to create a new array. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:277: if (it != sessions_.end()) { suggest (D)CHECK(it != sessions_end()) - It should never happen that we ask for capabilies of an unknown session_id. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:278: DVLOG(1) << "EnumerateDeviceCapabilites for: " << it->second.name; nit: This function is not called EnumerateDeviceCapabilites https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:337: media::VideoCaptureDevice::GetDeviceSupportedFormats(*it, &capabilities); We do not want to actually call this function on the IO thread, since it can take a substantial amount of time depending on OS and drivers. That must still happen on the device thread and it must have completed before we call listener_->DevicesEnumerated(stream_type, devices) to avoid a potential race. Suggestion: When enumeration is called: 1. Get the available names on device thread -> return to io thread 2. For each new name -> retrieve capabilities and return to IO thread. You can post an array with all new names to the device thread. 3. For each removed name- > remove from DeviceInfo array 4. When the process above is done -> call listener->DevicesEnumerated. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:426: // When the device is closed, its capabilities are reread from the device ok I see. May I suggest a different strategy? Instead of clearing and rereading capabilities- store the currently used capability in VideoCaptureManager::DeviceInfo and clear it when the device is stopped. GetCaptureCapabilities can return only the currently used capability if it is set. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:89: // open the device(s). This call is synchronous. nit: remove comment about syncrounous. It is obvious. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:130: DeviceEntry* entry, why both?
Hi perkj@ following your review and offline comments, some more changes (also in .h file cache documentation). PTAL! - |devices_info_cache_| creation/update works: EnumerateDevices() jumps to GetAvailableDevicesOnDeviceThread() and OnDeviceNamesEnumerated(). From there, a posttask-reply GetDevicesCapabilitiesOnDeviceThread() and OnDeviceNameAndCapabilitiesEnumerated() is carried out. From the last one, the |listener_| OnDevicesEnumerated is called. - OnDeviceNamesEnumerated() only requests capabilities for newly enumerated devices. OnDeviceNameAndCapabilitiesEnumerated() consolidates the cache by adding new devices' names and caps, removing not present ones. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_provider.h (right): https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/media_stream_provider.h:56: virtual void Error(MediaStreamType stream_type, On 2013/10/31 09:46:00, perkj wrote: > Please revert changes in this file. I have a cl where I remove this function all > together since it is unused. Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:145: DeviceInfo* device_info, On 2013/10/31 09:46:00, perkj wrote: > can you just pass the media::VideoCaptureDevice::Name instead of the device_info > here. It does not look like you need anything else. Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:230: entry, found, params_as_capability, On 2013/10/31 09:46:00, perkj wrote: > Just pass the name? Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:271: media::VideoCaptureCapabilities VideoCaptureManager::GetDeviceCapabilities( On 2013/10/31 09:46:00, perkj wrote: > would it make more sence to have this > > void GetDeviceCapabilities(int session_id, media::VideoCaptureCapabilities*) ? > > that way you don't need to create a new array. Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:277: if (it != sessions_.end()) { On 2013/10/31 09:46:00, perkj wrote: > suggest (D)CHECK(it != sessions_end()) - It should never happen that we ask for > capabilies of an unknown session_id. Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:278: DVLOG(1) << "EnumerateDeviceCapabilites for: " << it->second.name; On 2013/10/31 09:46:00, perkj wrote: > nit: This function is not called EnumerateDeviceCapabilites Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:337: media::VideoCaptureDevice::GetDeviceSupportedFormats(*it, &capabilities); On 2013/10/31 09:46:00, perkj wrote: > We do not want to actually call this function on the IO thread, since it can > take a substantial amount of time depending on OS and drivers. That must still > happen on the device thread and it must have completed before we call > listener_->DevicesEnumerated(stream_type, devices) to avoid a potential race. > > Suggestion: When enumeration is called: > 1. Get the available names on device thread -> return to io thread > 2. For each new name -> retrieve capabilities and return to IO thread. You can > post an array with all new names to the device thread. > 3. For each removed name- > remove from DeviceInfo array > 4. When the process above is done -> call listener->DevicesEnumerated. Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:426: // When the device is closed, its capabilities are reread from the device On 2013/10/31 09:46:00, perkj wrote: > ok I see. May I suggest a different strategy? > Instead of clearing and rereading capabilities- store the currently used > capability in > VideoCaptureManager::DeviceInfo and clear it when the device is stopped. > GetCaptureCapabilities can return only the currently used capability if it is > set. > > Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:89: // open the device(s). This call is synchronous. On 2013/10/31 09:46:00, perkj wrote: > nit: remove comment about syncrounous. It is obvious. Done. https://codereview.chromium.org/29423003/diff/630001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:130: DeviceEntry* entry, On 2013/10/31 09:46:00, perkj wrote: > why both? See more comments below, changed to const media::VideoCaptureDevice::Name& device_name.
A few more. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:241: found->device_in_use_ = true; Can you move this to before you do PostTask? https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:289: if (device-> device_in_use_) nit: remove extra space. Have you run lint? https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:337: DeviceInfo* device_info; move this to inside the for loop to minimize the scope. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:376: devices_info_cache_.swap(new_devices_info); This only contains the new devices- not the devices that already existed that you did not ask for the capabilities. How about updating the |devices_info_cache_| in OnDeviceNamesEnumerated with new and removed capturers and just store the new found capabilities into |devices_info_cache_| here? istener_->DevicesEnumerated(stream_type, devices) must get all currently available devices. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:491: // When the device is closed, its capability in use is reset. this comment is a bit confusing - can we just remove it? https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:91: media::VideoCaptureCapabilities& capabilities); none const refs are not allowed- see style guide. Please use media::VideoCaptureCapabilities* capabilities
perkj@'s comments addressed/answered, PTAL! https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:241: found->device_in_use_ = true; On 2013/11/04 11:08:23, perkj wrote: > Can you move this to before you do PostTask? Done. Please note that DoStartDeviceOnDeviceThread() must be called even if it's with a NULL |found| device. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:289: if (device-> device_in_use_) On 2013/11/04 11:08:23, perkj wrote: > nit: remove extra space. Have you run lint? Done. This was done on emacs console mode on an ssh line. No Eclipse, nothing fancy. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:337: DeviceInfo* device_info; On 2013/11/04 11:08:23, perkj wrote: > move this to inside the for loop to minimize the scope. Done. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:376: devices_info_cache_.swap(new_devices_info); On 2013/11/04 11:08:23, perkj wrote: > This only contains the new devices- not the devices that already existed that > you did not ask for the capabilities. > Note that iterator |it_name|, which walks the current devices in the system, is used to search in the |devices_info_cache_|: if the current device is in the cache, it gets added to the never-seen devices info in |new_devices_info|. At the end of this loop we have all newly enumerated and still-standing devices in the system, and we just swap them I thought this just-adding and swap process was easier than removing not-present devices (harder search) and adding new ones. The |devices_info_cache| will be released via out-of-scope and the destructor of ScopedVector. > How about updating the |devices_info_cache_| in OnDeviceNamesEnumerated with new > and removed capturers and just store the new found capabilities into > |devices_info_cache_| here? > > istener_->DevicesEnumerated(stream_type, devices) must get all currently > available devices. He shalt have them all. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:491: // When the device is closed, its capability in use is reset. On 2013/11/04 11:08:23, perkj wrote: > this comment is a bit confusing - can we just remove it? Done. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:91: media::VideoCaptureCapabilities& capabilities); On 2013/11/04 11:08:23, perkj wrote: > none const refs are not allowed- see style guide. > Please use > media::VideoCaptureCapabilities* capabilities Most reasonable - changed. Pardon my style-guide-less-ness :)
Sorry I missread |names_snapshot|. https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/790001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:376: devices_info_cache_.swap(new_devices_info); Ok - I misread what |names_snapshot| contains, sorry. So this is only passed along from OnDeviceNamesEnumerated and contain all available devices. I wonder if you can write this in an easier / more readable way. On 2013/11/04 18:32:16, mcasas1 wrote: > On 2013/11/04 11:08:23, perkj wrote: > > This only contains the new devices- not the devices that already existed that > > you did not ask for the capabilities. > > > > Note that iterator |it_name|, which walks the current devices in the system, > is used to search in the |devices_info_cache_|: if the current device is in > the cache, it gets added to the never-seen devices info in |new_devices_info|. > At the end of this loop we have all newly enumerated and still-standing > devices in the system, and we just swap them > > I thought this just-adding and swap process was easier than removing > not-present devices (harder search) and adding new ones. > > The |devices_info_cache| will be released via out-of-scope and the > destructor of ScopedVector. > > > How about updating the |devices_info_cache_| in OnDeviceNamesEnumerated with > new > > and removed capturers and just store the new found capabilities into > > |devices_info_cache_| here? > > > > istener_->DevicesEnumerated(stream_type, devices) must get all currently > > available devices. > > He shalt have them all. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:336: media::VideoCaptureDevice::Names new_device_names; suggest something like |names_snapshot| contain all currently available devices. We need to retrieve the capabilities of all new devices. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:337: media::VideoCaptureDevice::Names::const_iterator it; please try to be consistent with existing for loops and move the iterator to the scope of the for loop. Here and else where. for (media::VideoCaptureDevice::Names::const_iterator it = names_snapshot.begin(); ... https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:354: ScopedVector<DeviceInfo> new_devices_info) { I think ScopedVector<DeviceInfo> new_devices_info should be const ref as well instead of creating an implicit copy here. If that is done - would it be easier to remove unexisting devices from |devices_info_cache_| in OnDeviceNamesEnumerated ? In that case you don't need |names_snapshot| here at all. Move to OnDeviceNamesEnumerated and do something like ScopedVector<DeviceInfo>::iterator it_device_info = devices_info_cache_.begin(); while(it_device_info != devices_info_cache_.end()) { if !FindDeviceNameWithId(names_snapshot, it_device_info->id) devices_info_cache_.erase(it_device_info++) else ++it_device_info; } https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:377: // Walk the returned |device_names| and transform from VCD::Name to Please add something like |names_snapshot| contain all available devices. |new_devices_info| contain capabilities of new devices. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:380: ScopedVector<DeviceInfo>::const_iterator it; dito https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:429: media::VideoCaptureDevice::Names::const_iterator it; dito https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:438: device_info_vector.push_back(new DeviceInfo(*it, capabilities)); This creates a copy of capabilities. How about creating the DeviceInfo before calling GetDeviceSupportedFormats and pass device_info->capabilities to GetDeviceSupportedFormats to avoid a copy. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:527: ScopedVector<DeviceInfo>::iterator it; dito https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:168: TEST_F(VideoCaptureManagerTest, ManipulateDeviceAndCheckCapabilities) { Please keep the old test and add a new test for testing the capabilities. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:187: media::VideoCaptureCapabilities::const_iterator format; declare where you use it. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:188: EXPECT_EQ(device_capabilities[0].width, 640); Is this relevant? ie - do you really care about absolut numbers? Just check that device_capabilities.size >1 and maybe that these values below != 0. That way this wont break just because the fake devices change. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:201: EXPECT_EQ(device_capabilities[0].width, 320); This should probably verify that device_capabilities[0] == the capability used in StartClient. Or if you don't want to store the capability in StartClient, just check ASSERT_EQ(device_capabilities.size(), 1u) as you already do.
Hi perkj@, PTAL! https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:336: media::VideoCaptureDevice::Names new_device_names; On 2013/11/05 09:34:34, perkj wrote: > suggest something like > > |names_snapshot| contain all currently available devices. > We need to retrieve the capabilities of all new devices. Done. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:337: media::VideoCaptureDevice::Names::const_iterator it; On 2013/11/05 09:34:34, perkj wrote: > please try to be consistent with existing for loops and move the iterator to the > scope of the for loop. > Here and else where. > > for (media::VideoCaptureDevice::Names::const_iterator it = > names_snapshot.begin(); ... I think it clutters the lines a lot, but hey you're the reviewer. Done. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:354: ScopedVector<DeviceInfo> new_devices_info) { Moved to OnDeviceNamesEnumerated: First search for devices that are in the cache but not on the current system snapshot, and remove them from the cache. Then compose the full new devices list and retrieve their capabilities. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:377: // Walk the returned |device_names| and transform from VCD::Name to This comment does not make sense. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:380: ScopedVector<DeviceInfo>::const_iterator it; On 2013/11/05 09:34:34, perkj wrote: > dito Done. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:429: media::VideoCaptureDevice::Names::const_iterator it; On 2013/11/05 09:34:34, perkj wrote: > dito Done. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:438: device_info_vector.push_back(new DeviceInfo(*it, capabilities)); On 2013/11/05 09:34:34, perkj wrote: > This creates a copy of capabilities. > How about creating the DeviceInfo before calling GetDeviceSupportedFormats > and pass device_info->capabilities to GetDeviceSupportedFormats to avoid a copy. Done. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:527: ScopedVector<DeviceInfo>::iterator it; On 2013/11/05 09:34:34, perkj wrote: > dito Done. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:168: TEST_F(VideoCaptureManagerTest, ManipulateDeviceAndCheckCapabilities) { On 2013/11/05 09:34:34, perkj wrote: > Please keep the old test and add a new test for testing the capabilities. Sure, I don't know how I removed the old one :S https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:187: media::VideoCaptureCapabilities::const_iterator format; On 2013/11/05 09:34:34, perkj wrote: > declare where you use it. Interestingly enough, the compiler did not warn about unused variable (??). Gone. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:188: EXPECT_EQ(device_capabilities[0].width, 640); On 2013/11/05 09:34:34, perkj wrote: > Is this relevant? ie - do you really care about absolut numbers? Just check that > device_capabilities.size >1 and maybe that these values below != 0. > That way this wont break just because the fake devices change. Wouldn't we like to break-adapt if fake device changes? How often would that happen? Done but I find this comment overly cautious. https://codereview.chromium.org/29423003/diff/870001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:201: EXPECT_EQ(device_capabilities[0].width, 320); On 2013/11/05 09:34:34, perkj wrote: > This should probably verify that device_capabilities[0] == the capability used > in StartClient. Or if you don't want to store the capability in StartClient, > just check ASSERT_EQ(device_capabilities.size(), 1u) as you already do. Again, overly cautious but hey, you're the owner.
Hej If you fix the below I think this cl is ready for another reviewer. Can you ask Nick since he did the great refactoring? https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:353: names_snapshot.begin(); it != names_snapshot.end(); ++it) { nit: I think this should be for (media::VideoCaptureDevice::Names::const_iterator it = names_snapshot.begin(); it != names_snapshot.end(); ++it) { See other existing places that do the same in this file. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:373: it != new_devices_info.end(); for (ScopedVector<DeviceInfo>::const_iterator it = new_devices_info.begin(); it != new_devices_info.end(); ++it) https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:375: devices_info_cache_.push_back(new DeviceInfo((*it)->name_, Sorry I am a bit back and forth regarding const ScopedVector<DeviceInfo>& new_devices_info. See it as if I am learning as well. I talked about it with Tommi and it seems like you were right from the beginning. It should be by value (ScopedVector<DeviceInfo> new_devices_info) and not by ref since ScopedVector handles the magic behind the scenes and avoids a copy. So if you change it back to by value you can avoid creating new DeviceInfo objects by doing something like it = new_device_info.begin() while (it != new_device_info.end()) { device_info_cache_.push_back(it); it = new_device_info.weak_erase(it); } https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:383: devices_info_cache_.begin(); nit: intendend devices_info_cache_.begin() 4 steps in from ( on the line above. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:433: device_names.begin(); dito https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:537: ++it) { nit: ++it fits on line above https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:170: .Times(1).WillOnce(SaveArg<1>(&devices)); indentation was right from the beginning. https://codereview.chromium.org/29423003/diff/980001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/980001/media/video/capture/vide... media/video/capture/video_capture_device.h:22: #include "media/video/capture/video_capture_types.h" Why is this changed in this cl ? https://codereview.chromium.org/29423003/diff/980001/media/video/capture/vide... media/video/capture/video_capture_device.h:117: // TODO(mcasas): Possibly not used anymore, double check. You can find out by removing it and see if it compiles.
Hi perkj@, now the code looks pristine and changes are taken in. What about an LGTM? :D Thanks! ncarter@ could you have a look? Thanks! https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:353: names_snapshot.begin(); it != names_snapshot.end(); ++it) { On 2013/11/06 17:05:51, perkj wrote: > nit: I think this should be > for (media::VideoCaptureDevice::Names::const_iterator it = > names_snapshot.begin(); it != names_snapshot.end(); ++it) { > > See other existing places that do the same in this file. Done but I couldn't find the style guide entry saying so. :S https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:373: it != new_devices_info.end(); On 2013/11/06 17:05:51, perkj wrote: > for (ScopedVector<DeviceInfo>::const_iterator it = new_devices_info.begin(); > it != new_devices_info.end(); ++it) Done. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:375: devices_info_cache_.push_back(new DeviceInfo((*it)->name_, On 2013/11/06 17:05:51, perkj wrote: > Sorry I am a bit back and forth regarding const ScopedVector<DeviceInfo>& > new_devices_info. See it as if I am learning as well. I talked about it with > Tommi and it seems like you were right from the beginning. > It should be by value (ScopedVector<DeviceInfo> new_devices_info) and not by ref > since ScopedVector handles the magic behind the scenes and avoids a copy. > > So if you change it back to by value you can avoid creating new DeviceInfo > objects by doing something like > > it = new_device_info.begin() > while (it != new_device_info.end()) { > device_info_cache_.push_back(it); > it = new_device_info.weak_erase(it); > } Done. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:383: devices_info_cache_.begin(); On 2013/11/06 17:05:51, perkj wrote: > nit: intendend devices_info_cache_.begin() 4 steps in from ( on the line above. Done. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:433: device_names.begin(); On 2013/11/06 17:05:51, perkj wrote: > dito Done. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:537: ++it) { On 2013/11/06 17:05:51, perkj wrote: > nit: ++it fits on line above Done. https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/980001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager_unittest.cc:170: .Times(1).WillOnce(SaveArg<1>(&devices)); On 2013/11/06 17:05:51, perkj wrote: > indentation was right from the beginning. Done. https://codereview.chromium.org/29423003/diff/980001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/980001/media/video/capture/vide... media/video/capture/video_capture_device.h:22: #include "media/video/capture/video_capture_types.h" On 2013/11/06 17:05:51, perkj wrote: > Why is this changed in this cl ? I knew it will raise eyebrows, so I added a comment, but it was lost in time. Reason: [1] "do not rely on the symbol being brought in transitively via headers not directly included." And we use VideoFrame. Since I'm modifying the file, I clean it up. [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar... https://codereview.chromium.org/29423003/diff/980001/media/video/capture/vide... media/video/capture/video_capture_device.h:117: // TODO(mcasas): Possibly not used anymore, double check. On 2013/11/06 17:05:51, perkj wrote: > You can find out by removing it and see if it compiles. You're a genius! I'll try right away. It worked! Removing it.
LGTM with a few nits. Have you tested what happens if you remove a USB device? Actually adding nick to the list of reviewers. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:383: it != devices_info_cache_.end(); ++it) { nit: indentation https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:170: .Times(1).WillOnce(SaveArg<1>(&devices)); nit: this is still wrong indentation. Should be 4 steps in.
ncarter@, PTAL! perkj@: Removing a Fake device is part of the unittests. Removing a real USB device is not yet tested, will do next week, when I get back to my Linux box. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:383: it != devices_info_cache_.end(); ++it) { On 2013/11/07 10:49:11, perkj wrote: > nit: indentation Ok, clang-format for Chromium style, and we leave it to it. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:170: .Times(1).WillOnce(SaveArg<1>(&devices)); On 2013/11/07 10:49:11, perkj wrote: > nit: this is still wrong indentation. Should be 4 steps in. We'll let clang-format decide. It will put each .X in a new line, which is style-guide-wise correct(er) than this.
https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:235: found->device_in_use_ = true; The VideoCaptureManager already knows whether a particular device is in use or not, on the basis of whether an active DeviceEntry exists with that id and type, so I wonder if it's redundant to track it in a bool here in a parallel struct. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:236: found->capability_in_use_ = params_as_capability; Would it be better to just cache the Capability in the Controller? If we track it in the DeviceInfo, then we wind up with three different structs that are tracking the current active state of the device: the DeviceEntry, the Controller, and now the DeviceInfo. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:276: void VideoCaptureManager::GetDeviceCapabilities( How does the client of this API know when to call this function? It seems to become valid only after the device enumeration event occurs. Is that the intended usage -- you request enumeration, and when the enumeration completes, you call back to get the extra info here? https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:329: if (!listener_) { Why do we check the listener_, if we're not invoking the listener? https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:337: while(it_device_info != devices_info_cache_.end()) { space after while https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:378: // Walk the |devices_info_cache_|| and transform from VCD::Name to || -> | https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:388: if(listener_) space after the if https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:202: bool device_in_use_; Why do we need this?
ncarter@: PTAL! FWIW, I understand your concern of keeping the caps triplicated. Is also true that the one in the VCController is pretty buried and only lives as long as the device, hence I answered to the other concern, meaning: DeviceEntry vs. DeviceInfo. I thought about merging them but they have different lifetimes, plz see comment below. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:235: found->device_in_use_ = true; On 2013/11/07 22:43:01, ncarter wrote: > The VideoCaptureManager already knows whether a particular device is in use or > not, on the basis of whether an active DeviceEntry exists with that id and type, > so I wonder if it's redundant to track it in a bool here in a parallel struct. That's true. It implies an extra search, but that's not as bad as duplicating info. So, done away with |device_in_use|. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:236: found->capability_in_use_ = params_as_capability; On 2013/11/07 22:43:01, ncarter wrote: > Would it be better to just cache the Capability in the Controller? > > If we track it in the DeviceInfo, then we wind up with three different structs > that are tracking the current active state of the device: the DeviceEntry, the > Controller, and now the DeviceInfo. Struct DeviceEntry and struct DeviceInfo actually cache different things, the first one being a dynamic id-controller*-device* used during the device's runtime, and DeviceInfo which is mostly static, containing names and capabilities. Moreover they are created on different moments, the DeviceInfo on early times and DeviceEntry during action (capture) time. The only touching point is the case of the capability in use. Do you think is ok to keep |capability_in_use_| and just remove |device_in_use_|? (L.235) https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:276: void VideoCaptureManager::GetDeviceCapabilities( On 2013/11/07 22:43:01, ncarter wrote: > How does the client of this API know when to call this function? It seems to > become valid only after the device enumeration event occurs. Is that the > intended usage -- you request enumeration, and when the enumeration completes, > you call back to get the extra info here? Yes exactly. There's a comment in the header file: // Retrieve the available capture capabilities for a particular device. The // capabilities are cached during device(s) enumeration and updated as clients // open/close the device(s). Next CL(s) will call wire the logic to call this method whenever RTCVideoCapturer::GetSupportedFormats() is used during capabilities negotiation in renderer side. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:329: if (!listener_) { On 2013/11/07 22:43:01, ncarter wrote: > Why do we check the listener_, if we're not invoking the listener? It's used in OnDeviceNameAndCapabilitiesEnumerated() but if there's no listener there's no sense in continuing with the logic at this point. Plus there are even unittests that hit this transient: |listener_| gets removed between GetAvailableDevicesOnDeviceThread and this method ! https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:337: while(it_device_info != devices_info_cache_.end()) { On 2013/11/07 22:43:01, ncarter wrote: > space after while Done. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:378: // Walk the |devices_info_cache_|| and transform from VCD::Name to On 2013/11/07 22:43:01, ncarter wrote: > || -> | Done. https://codereview.chromium.org/29423003/diff/1090001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:388: if(listener_) On 2013/11/07 22:43:01, ncarter wrote: > space after the if Done.
https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:150: const media::VideoCaptureDevice::Name* device_name, The interface to this function gets kind of weird with the addition of the device_name parameter, because in the MEDIA_DEVICE_VIDEO_CAPTURE case, we never read entry->id at all. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:232: DeviceInfo* found = FindDeviceInfoById(entry->id); Should we be doing this for only for MEDIA_DEVICE_VIDEO_CAPTURE ? https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:234: found->capability_in_use_ = params_as_capability; Because you're mirroring this state into the DeviceInfo, it seems you would need a corresponding reset when the DeviceEntry goes away in DestroyDeviceEntryIfNoClients. For this reason I think it's better to just store params_as_capability in the DeviceEntry or the controller. It is a valid thing to track in the controller. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:239: (found ? & found->name_ : NULL), Isn't this use of found->name_ unsafe? What guarantees that the pointer will still exist when it's dereferenced on the other thread? https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:368: stream_type)); Could we make this enumeration work with only one visitation of the device thread (the current code does IO -> Device -> IO -> Device -> IO)? A couple approaches come to mind. - Always enumerate the capabilities of all devices -- eliminate the caching, and just return the device info along with the enumeration. - Keep a list of already-enumerated device IDs on the Device thread, and only enumerate the capabilities of each once. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:202: media::VideoCaptureCapability capability_in_use_; Because this state only pertains to an active device, I really think it should either go in the DeviceEntry or in DeviceEntry.video_capture_controller. This will mean that you don't have to worry about keeping it valid or up to date when e.g. the existing user of a device goes away.
ncarter@, perkj@, PTAL. Some comments: - Caching of a device running caps happens in the controller. - The flag |device_in_use_| in the DeviceInfo is back; the presence of an entry in DeviceEntries doesn't have the same meaning: the former means device running, the latter means device "allocated" for a client (perkj@). - Moved the name and caps retrieval and consolidation to a single Device thread visit; since we need to send a copy of the current cached names+caps, I couldn't use ScopedVector<> and reverted to std::vector<> template. Perhaps the chromium master-reviewers can give some guidance on this? ;) - Added a UT for device connect-removal, that forced adding an (static) accesor and a mutator in fake device. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:150: const media::VideoCaptureDevice::Name* device_name, On 2013/11/09 01:49:11, ncarter wrote: > The interface to this function gets kind of weird with the addition of the > device_name parameter, because in the MEDIA_DEVICE_VIDEO_CAPTURE case, we never > read entry->id at all. Done. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:232: DeviceInfo* found = FindDeviceInfoById(entry->id); On 2013/11/09 01:49:11, ncarter wrote: > Should we be doing this for only for MEDIA_DEVICE_VIDEO_CAPTURE ? Done. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:234: found->capability_in_use_ = params_as_capability; On 2013/11/09 01:49:11, ncarter wrote: > Because you're mirroring this state into the DeviceInfo, it seems you would need > a corresponding reset when the DeviceEntry goes away in > DestroyDeviceEntryIfNoClients. > > For this reason I think it's better to just store params_as_capability in the > DeviceEntry or the controller. It is a valid thing to track in the controller. Done. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:239: (found ? & found->name_ : NULL), On 2013/11/09 01:49:11, ncarter wrote: > Isn't this use of found->name_ unsafe? What guarantees that the pointer will > still exist when it's dereferenced on the other thread? Done. https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:368: stream_type)); See global comments :) short answer: yes https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:202: media::VideoCaptureCapability capability_in_use_; On 2013/11/09 01:49:11, ncarter wrote: > Because this state only pertains to an active device, I really think it should > either go in the DeviceEntry or in DeviceEntry.video_capture_controller. This > will mean that you don't have to worry about keeping it valid or up to date when > e.g. the existing user of a device goes away. Ok, kept and retrieved from the controller. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:14: #include "base/task_runner_util.h" For PostTaskAndReplyWithResult() https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:296: media::VideoCaptureCapability current_format( This is a silly copy but a consequence of our dividing a device's capabilities and parameters :B https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:197: TEST_F(VideoCaptureManagerTest, ConnectAndDisconnectDevices) { Attending to a vague question (request?) from perkj@. https://codereview.chromium.org/29423003/diff/1350001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.cc (right): https://codereview.chromium.org/29423003/diff/1350001/media/video/capture/fak... media/video/capture/fake_video_capture_device.cc:22: static int numberOfFakeDevices = 2; Why was this an enum in the first place?
https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:107: const media::VideoCaptureParams& getVideoCaptureParams() const { GetVideoCaptureParams() https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:87: GetAvailableDevicesAndCapabilitiesOnDeviceThread, 4 step indentation https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:291: DeviceEntry* const existing_device = please rename existing_device to something else. device_in_use maybe? https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:295: existing_device->video_capture_controller->getVideoCaptureParams(); This seems wrong to me. How do you know that the device is started just because it exists in sessions_? https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:340: DevicesInfo& new_devices_info_cache) { const ref? https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:342: << new_devices_info_cache.size(); indentation https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:367: const DevicesInfo& devices_info_cache_copy) { No need to say that this is a copy. Maybe call this old_device_info_cache? https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:393: while (it_device_info != devices_info_cache_copy.end()) { nit: Looks like this can be a for loop. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:475: device_info->device_in_use_ = false; Where to you use this info now? https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:20: #include "base/memory/scoped_vector.h" remove if unused. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:194: struct DeviceInfo { Please replace your foward declaration of DeviceInfo with this. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:197: TEST_F(VideoCaptureManagerTest, ConnectAndDisconnectDevices) { On 2013/11/11 17:40:00, mcasas1 wrote: > Attending to a vague question (request?) from perkj@. Thanks. I guess I was after testing that the capabilities are not retrieved multiple times when you added or removed devices. But this is ok. https://codereview.chromium.org/29423003/diff/1350001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.cc (right): https://codereview.chromium.org/29423003/diff/1350001/media/video/capture/fak... media/video/capture/fake_video_capture_device.cc:22: static int numberOfFakeDevices = 2; On 2013/11/11 17:40:00, mcasas1 wrote: > Why was this an enum in the first place? Move this to be a member with the initial size = kNumberOfDevices.
Thanks for the review perkj@, PTAL. ncarter@ any further comments? A bit of reviewer reshuffling, tommi@ and miu@ are moved to CC:, and I'm asking xians@ instead. xians@ could you have a look at content/browser/renderer_host/media/* ? Thanks! https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:107: const media::VideoCaptureParams& getVideoCaptureParams() const { On 2013/11/12 11:10:58, perkj wrote: > GetVideoCaptureParams() Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:14: #include "base/task_runner_util.h" On 2013/11/11 17:40:00, mcasas1 wrote: > For PostTaskAndReplyWithResult() Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:87: GetAvailableDevicesAndCapabilitiesOnDeviceThread, On 2013/11/12 11:10:58, perkj wrote: > 4 step indentation This is what clang-format makes of it :S https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:291: DeviceEntry* const existing_device = On 2013/11/12 11:10:58, perkj wrote: > please rename existing_device to something else. device_in_use maybe? See next reply. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:295: existing_device->video_capture_controller->getVideoCaptureParams(); On 2013/11/12 11:10:58, perkj wrote: > This seems wrong to me. How do you know that the device is started just because > it exists in sessions_? It is partially wrong, I should have tested here for |device_in_use_| so we are sure that the device exists, has a controller associated, and is in use. Otherwise we should return all their capabilities. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:296: media::VideoCaptureCapability current_format( On 2013/11/11 17:40:00, mcasas1 wrote: > This is a silly copy but a consequence of our dividing a > device's capabilities and parameters :B Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:340: DevicesInfo& new_devices_info_cache) { On 2013/11/12 11:10:58, perkj wrote: > const ref? Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:342: << new_devices_info_cache.size(); On 2013/11/12 11:10:58, perkj wrote: > indentation Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:367: const DevicesInfo& devices_info_cache_copy) { On 2013/11/12 11:10:58, perkj wrote: > No need to say that this is a copy. Maybe call this old_device_info_cache? Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:393: while (it_device_info != devices_info_cache_copy.end()) { On 2013/11/12 11:10:58, perkj wrote: > nit: Looks like this can be a for loop. Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:475: device_info->device_in_use_ = false; On 2013/11/12 11:10:58, perkj wrote: > Where to you use this info now? See comment on line 295, basically I forgot to add a check for it :) https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:20: #include "base/memory/scoped_vector.h" On 2013/11/12 11:10:58, perkj wrote: > remove if unused. Done. https://codereview.chromium.org/29423003/diff/1350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:194: struct DeviceInfo { On 2013/11/12 11:10:58, perkj wrote: > Please replace your foward declaration of DeviceInfo with this. Done. https://codereview.chromium.org/29423003/diff/1350001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.cc (right): https://codereview.chromium.org/29423003/diff/1350001/media/video/capture/fak... media/video/capture/fake_video_capture_device.cc:22: static int numberOfFakeDevices = 2; On 2013/11/12 11:10:58, perkj wrote: > On 2013/11/11 17:40:00, mcasas1 wrote: > > Why was this an enum in the first place? > > Move this to be a member with the initial size = kNumberOfDevices. > Done.
It generally looks good. I don't know much about the video capability, so only do some sanity check on the code. SX https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:107: const media::VideoCaptureParams& GetVideoCaptureParams() const { You should either change the name to video_capture_params() const and make it inline. Or move the implementation to cc file. Also, it looks like video_capture_params_ should be accessed only on IO thread, add a thread check DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); So I would say that moving the implementation to cc file will be the best. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (left): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:291: return; why do you change these, please added back the thread check and early return if the listen has gone. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:169: ? media::FakeVideoCaptureDevice::Create(found->name_) nit, why ? changed here https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:237: DeviceInfo* found = FindDeviceInfoById(entry->id, devices_info_cache_); nit, remove the local variable, just use if (FindDeviceInfoById(entry->id, devices_info_cache_)) https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:290: if (device) { early return if (!device) { // Add comment. *capabilities = device->capabilities_; return; } DeviceEntry* const existing_device = ... https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:355: // |listener_| might have disappeared while we were scanning the devices. I think you should do nothing if the listener_ has gone away. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:399: break; I think you can remove the if two lines below if you do if (it_device_info->name_.id() == it->id()) { new_devices_info_cache.push_back(*it_device_info); names_snapshot.erase(it); break; } https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:515: return it.base(); FYI, chrome rarely uses .base(), most of the case it just uses &(*it) I will let you decide which one to use. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:106: media::VideoCaptureCapabilities capabilities_; the members of struct do not need to be ended with _ https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:157: DevicesInfo& device_vector); nit, const DevicesInfo& devices_info https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.cc (right): https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... media/video/capture/fake_video_capture_device.cc:31: return numberOfFakeDevices; you are having a getter and setter, probably numberOfFakeDevices should be just a private member instead of a static member https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... media/video/capture/fake_video_capture_device.h:68: static int numberOfFakeDevices; I am confused here, how this numberOfFakeDevices is used? and why it is static?
xians@ thanks for the review, PTAL! https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:107: const media::VideoCaptureParams& GetVideoCaptureParams() const { On 2013/11/12 16:48:00, xians1 wrote: > You should either change the name to video_capture_params() const and make it > inline. Or move the implementation to cc file. > > Also, it looks like video_capture_params_ should be accessed only on IO thread, > add a thread check DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); > > So I would say that moving the implementation to cc file will be the best. Done. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (left): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:291: return; On 2013/11/12 16:48:00, xians1 wrote: > why do you change these, please added back the thread check and early return if > the listen has gone. Done. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:169: ? media::FakeVideoCaptureDevice::Create(found->name_) On 2013/11/12 16:48:00, xians1 wrote: > nit, why ? changed here clang-format would make something entirely different: if (found) { video_capture_device.reset( use_fake_device_ ? media::FakeVideoCaptureDevice::Create(found->name) : media::VideoCaptureDevice::Create(found->name)); And I must have reverted this a couple of times, hence the shift. Changed back so is as similar as possible to before. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:237: DeviceInfo* found = FindDeviceInfoById(entry->id, devices_info_cache_); On 2013/11/12 16:48:00, xians1 wrote: > nit, remove the local variable, just use if (FindDeviceInfoById(entry->id, > devices_info_cache_)) ? I used the |found| twice, one to check != NULL and again to set the |device_in_use|. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:290: if (device) { On 2013/11/12 16:48:00, xians1 wrote: > early return > if (!device) { > // Add comment. > *capabilities = device->capabilities_; > return; > } > > DeviceEntry* const existing_device = ... > Done. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:355: // |listener_| might have disappeared while we were scanning the devices. On 2013/11/12 16:48:00, xians1 wrote: > I think you should do nothing if the listener_ has gone away. Done. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:399: break; On 2013/11/12 16:48:00, xians1 wrote: > I think you can remove the if two lines below if you do > if (it_device_info->name_.id() == it->id()) { > new_devices_info_cache.push_back(*it_device_info); > names_snapshot.erase(it); > break; > } Good catch! I think so too, so Done. https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:515: return it.base(); On 2013/11/12 16:48:00, xians1 wrote: > FYI, chrome rarely uses .base(), most of the case it just uses &(*it) > I will let you decide which one to use. Chromiumisms are my thing! https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:106: media::VideoCaptureCapabilities capabilities_; On 2013/11/12 16:48:00, xians1 wrote: > the members of struct do not need to be ended with _ Argh true. It used to be a class :) https://codereview.chromium.org/29423003/diff/1490001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:157: DevicesInfo& device_vector); On 2013/11/12 16:48:00, xians1 wrote: > nit, const DevicesInfo& devices_info This would force the return to be a const*const, and the return value is used to manipulate the contents, so it should stay like it is. https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.cc (right): https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... media/video/capture/fake_video_capture_device.cc:31: return numberOfFakeDevices; On 2013/11/12 16:48:00, xians1 wrote: > you are having a getter and setter, probably numberOfFakeDevices should be just > a private member instead of a static member Again, is used as a global variable of the whole FakeVCD, and usually there are more than one. It is used in l.34 for example, and in the factory of l.68, to create as many FakeVCDs as needed. Plus, from the UT, which uses is the only use of these getter and setter, we got no references to the devices themselves :( https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... media/video/capture/fake_video_capture_device.h:68: static int numberOfFakeDevices; On 2013/11/12 16:48:00, xians1 wrote: > I am confused here, how this numberOfFakeDevices is used? and why it is static? Originally there was a file-scope enum =2 that was used during static GetDeviceNames() to create as many fake video capture devices. So, there were 2 video capture devices. I made this static so we can manipulate during the unittests how many (fake) video capture devices are in the system, and add a test case that adds-removes devices and checks that the DeviceInfo cache introduced works as intended. Needs to be static because is used from static methods and only pertains to all Fake VCDs as a community.
LGTM with the following fixes. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:147: const media::VideoCaptureParams& params) { It might be slightly better to cache off just params.requested_format in this function (making the new member a Format rather than a Params). The difference being that a Controller can serve multiple session_ids at any given time, and so it might not make sense to designate any as a controlling session. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:156: video_capture_params_ = params; It may make sense to update this only if it's the first client (params is a request, and the policy implemented in VCM is that the first requesting client gets its way). You could do this either by checking that the format in video_capture_params_ is valid, or else by checking whether controller_clients_ is empty. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:291: if (!device->device_in_use) { I think you can get rid of device_in_use by writing this block like so: DeviceInfo* device = FindDeviceInfoById(it->second.id, devices_info_cache_); if (device) { DeviceEntry* const existing_device = GetDeviceEntryForMediaStreamDevice(it->second); if (!existing_device) { // If the device is not in use, just return all its cached capabilities. *capabilities = device->capabilities; return; } else { // Otherwise, get the video capture parameters in use from the controller // associated to the device. media::VideoCaptureParams params = existing_device->video_capture_controller->GetVideoCaptureParams(); media::VideoCaptureCapability current_format( params.requested_format.width, params.requested_format.height, params.requested_format.frame_rate, media::PIXEL_FORMAT_UNKNOWN, params.requested_format.frame_size_type); capabilities->push_back(current_format); } } https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:298: DeviceEntry* const existing_device = Optional: I wonder if we should rename DeviceEntry to ActiveDevice. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:307: media::PIXEL_FORMAT_UNKNOWN, Should we not always advertise I420 in the values returned by this function? Or, else, should this be returning Formats instead of Capabilitieses? My guess is that the consumer can't do anything useful with the PIXEL_FORMAT enum here. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:363: if (listener_) You don't need this check (it happens earlier in the function). https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/fak... media/video/capture/fake_video_capture_device.h:68: static int numberOfFakeDevices; number_of_fake_devices_; https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/vid... media/video/capture/video_capture_device.h:113: typedef std::list<Name> Names; Hooray!
ncarter@: comments addressed, thanks! Let me bring forward one of my replies for perkj@: > Ok I was confused. When the VCM calls VCC:AddClient, this > method also starts the client, meaning that being added > to |devices_| is equivalent to having set the client > capture parameters. > > (Before, there was an intermediate stage in which several > clients could exist, and the first one to start the > device would fix its resolution.). > https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:147: const media::VideoCaptureParams& params) { On 2013/11/12 20:56:46, ncarter wrote: > It might be slightly better to cache off just params.requested_format in this > function (making the new member a Format rather than a Params). The difference > being that a Controller can serve multiple session_ids at any given time, and so > it might not make sense to designate any as a controlling session. Done. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:156: video_capture_params_ = params; On 2013/11/12 20:56:46, ncarter wrote: > It may make sense to update this only if it's the first client (params is a > request, and the policy implemented in VCM is that the first requesting client > gets its way). You could do this either by checking that the format in > video_capture_params_ is valid, or else by checking whether controller_clients_ > is empty. Done. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:291: if (!device->device_in_use) { Ok I was confused. When the VCM calls VCC:AddClient, this method also starts the client, meaning that being added to |devices_| is equivalent to having set the client capture parameters. (Before, there was an intermediate stage in which several clients could exist, and the first one to start the device would fix its resolution.). With the current status quo, indeed we don't need |device_in_use|, is implied by the existence of Entry in |devices_|. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:307: media::PIXEL_FORMAT_UNKNOWN, On 2013/11/12 20:56:46, ncarter wrote: > Should we not always advertise I420 in the values returned by this function? Or, > else, should this be returning Formats instead of Capabilitieses? > > My guess is that the consumer can't do anything useful with the PIXEL_FORMAT > enum here. Controller returns Format now, and indeed I'm setting the pixel format to I420 since that's what the controller would return anyway. I think is unfortunate though that the chain does not support suggesting to the device the pixel format to use, and use it in the capability matching mix, but we'll get to that ;) https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:363: if (listener_) On 2013/11/12 20:56:46, ncarter wrote: > You don't need this check (it happens earlier in the function). Done. https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/fak... media/video/capture/fake_video_capture_device.h:68: static int numberOfFakeDevices; On 2013/11/12 20:56:46, ncarter wrote: > number_of_fake_devices_; Done. https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1570001/media/video/capture/vid... media/video/capture/video_capture_device.h:113: typedef std::list<Name> Names; On 2013/11/12 20:56:46, ncarter wrote: > Hooray! Done.
Just one question and a few nits. https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:158: video_capture_format_ = params.requested_format; So adding the first client implies that the capture device is actually started with this resolution? And does stopping the capture device imply that the controller is deleted? Will you still be able to retrieve all capabilities in the render process media::VideoCapture before you start capturing? https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:284: DeviceInfo* device = FindDeviceInfoById(it->second.id, devices_info_cache_); dcheck(device) ? Is it possible that device is null here? https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:286: DeviceEntry* const existing_device = name device_in_use ? https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:88: // open/close the device(s). nit start/stop https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:201: // from EnumerateDevices()--, and this snapshot is used to update this list in please update this text.
some nits, lgtm if you address them. https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/29423003/diff/1490001/media/video/capture/fak... media/video/capture/fake_video_capture_device.h:68: static int numberOfFakeDevices; On 2013/11/12 18:10:16, mcasas1 wrote: > On 2013/11/12 16:48:00, xians1 wrote: > > I am confused here, how this numberOfFakeDevices is used? and why it is > static? > > Originally there was a file-scope enum =2 that was used during > static GetDeviceNames() to create as many fake video capture > devices. So, there were 2 video capture devices. > > I made this static so we can manipulate during the unittests > how many (fake) video capture devices are in the system, and > add a test case that adds-removes devices and checks that the > DeviceInfo cache introduced works as intended. > > Needs to be static because is used from static methods and > only pertains to all Fake VCDs as a community. I see, thanks for the clarification. https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1570001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:86: // Retrieve the available capture capabilities for a particular device. The nit, Retrieves https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:158: video_capture_format_ = params.requested_format; curiously, what is this video_capture_format_ cached for? should this video_capture_format_ be default_video_capture_format_ or preferred_video_capture_format_? https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:237: VideoCaptureController::GetVideoCaptureFormat() const { same question on the naming since there might be multiple capture formats. https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:285: if (device) { either adding a DCHECK(device) or doing an early return if (!device) return; https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:295: if (existing_device) { nit, you don't need to check existing_device again. https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:339: const DevicesInfo& new_devices_info_cache) { nit, add back the thread check. https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/29423003/diff/1670001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:86: // Retrieve the available capture capabilities for a particular device. The nit, Retrieves
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/29423003/1480004
Message was sent while issue was closed.
Change committed as 235223 |
