Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
616833004:
chrome://media-internals: update MediaInternals when devices capabilities are enumerated. (Closed)
|
Created:
6 years, 2 months ago by mcasas Modified:
6 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionchrome://media-internals: update MediaInternals when devices capabilities are enumerated.
Ping MediaInternals with the capabilities list from VideoCaptureManager. \
Moved VideoCaptureManager::DeviceInfo to media::VideoCaptureDeviceInfo in its
own file. This is used from VCM to send the list of video capture devices to
MediaInternals, after enumeration.
Sprinkle some nice "const auto &" and range for loops on the affected areas,
those features are approved [1].
[1] http://chromium-cpp.appspot.com/
BUG=344882
Committed: https://crrev.com/fcb5c7de8085887dae1359a12c74df1f44692b72
Cr-Commit-Position: refs/heads/master@{#299251}
Patch Set 1 : #
Total comments: 20
Patch Set 2 : burnik@ comments. Rebased. Refactored capabilities update. #
Total comments: 14
Patch Set 3 : Win+Mac, use SetInteger for capture_api_type(). #Patch Set 4 : Moved DeviceInfo to media::VideoCaptureDevice and used from VCM and MediaInternals. Other perkj@ co… #
Total comments: 8
Patch Set 5 : Added VideoCaptureDeviceInfo in its very own file. #
Total comments: 8
Patch Set 6 : perkj@s comments #
Total comments: 18
Patch Set 7 : wolenetz@ comments #
Total comments: 2
Patch Set 8 : Refactored MediaInternalsTest and added a proto video-capture-capabilities TEST_F #
Total comments: 6
Patch Set 9 : wolenetz@ and xhwang@ final comments #Patch Set 10 : FakeVCDFactory needs to specify capture API for Win. Linux/CrOs MediaInternalsVideoCaptureDeviceTes… #Messages
Total messages: 61 (27 generated)
Patchset #1 (id:1) has been deleted
mcasas@chromium.org changed reviewers: + burnik@chromium.org, xians@chromium.org
xians@ PTAL burnik@ FYI/PTAL.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:40001) has been deleted
I'll pull in the patch and come up with a preview. Probably over the weekend. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.h:55: // Cleans up internal cache of all video devices' capabilities. Nit: add empty line. https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:506: // Send an update to MediaInternals later on. Nit: add empty line before comment.
Look closely. :-) https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:217: void MediaInternals::SendEverything() { This is a bit unrelated at this point, but it should go away - e.g. renamed to "SendAudioStreamData". https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:222: "media.onReceiveEverything", &audio_streams_cached_data_); Audio data is not everything. Not your doing, but "Everything" is a terrible way of refering to anything. :-D https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:277: const media::VideoCaptureFormats& formats) { ditto, add: const size_t num_devices https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:288: SendEverything(); Two problems lie here: 1) You're calling MediaInternals::UpdateVideoCaptureDeviceCapabilities asynchronously from the VideoCaptureManager for each device enumerated which is fine until this line. Each update should not send the entire dictionary (up to that point) for each device enumerated. 2) SendEverything() sends only the audio stream dictionary. You should check if everything was enumerated and then send the whole dictionary of video devices. So here you would check and send: if (video_devices_cached_data_.size() == num_devices) { SendVideoCapabilitiesAndPurgeCache(); // There should be a separate event for video devices. } Also you could DCHECK to make sure Purge was done before enumeration was started. DCHECK_LE(video_devices_cached_data_.size(), num_devices); https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:291: void MediaInternals::PurgeVideoCaptureDeviceCapabilities() { We can probably avoid this method alltogether if we had SendVideoCapabilitiesAndPurgeCache(). https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.h:54: const media::VideoCaptureFormats& formats); add const size_t num_devices as last param. You'll need it to know when you're done enumerating. https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:501: MediaInternals::GetInstance()->PurgeVideoCaptureDeviceCapabilities(); Maybe we can avoid this if we start of with an empty dictionary and purge it each time we send it to JS. https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:511: it.supported_formats)); add devices_info_cache_.size() as last argument. So you know when all has been enumerated.
Patchset #2 (id:80001) has been deleted
mcasas@chromium.org changed reviewers: + perkj@chromium.org - xians@chromium.org
burnik@, perkj@ PTAL https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:217: void MediaInternals::SendEverything() { On 2014/10/05 23:08:55, burnik wrote: > This is a bit unrelated at this point, but it should go away - e.g. renamed to > "SendAudioStreamData". Done and cleaned up the "audio" parts of this class so it's explicit what they do. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:222: "media.onReceiveEverything", &audio_streams_cached_data_); On 2014/10/05 23:08:55, burnik wrote: > Audio data is not everything. > Not your doing, but "Everything" is a terrible way of refering to anything. :-D Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:277: const media::VideoCaptureFormats& formats) { On 2014/10/05 23:08:55, burnik wrote: > ditto, add: > const size_t num_devices Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:288: SendEverything(); On 2014/10/05 23:08:55, burnik wrote: > Two problems lie here: > 1) You're calling MediaInternals::UpdateVideoCaptureDeviceCapabilities > asynchronously from the VideoCaptureManager for each device enumerated which is > fine until this line. Each update should not send the entire dictionary (up to > that point) for each device enumerated. > 2) SendEverything() sends only the audio stream dictionary. > > You should check if everything was enumerated and then send the whole dictionary > of video devices. > > So here you would check and send: > if (video_devices_cached_data_.size() == num_devices) { > SendVideoCapabilitiesAndPurgeCache(); // There should be a separate event for > video devices. > } > > Also you could DCHECK to make sure Purge was done before enumeration was > started. > DCHECK_LE(video_devices_cached_data_.size(), num_devices); > Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:291: void MediaInternals::PurgeVideoCaptureDeviceCapabilities() { On 2014/10/05 23:08:55, burnik wrote: > We can probably avoid this method alltogether if we had > SendVideoCapabilitiesAndPurgeCache(). Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.h:54: const media::VideoCaptureFormats& formats); On 2014/10/05 23:08:55, burnik wrote: > add > const size_t num_devices > as last param. You'll need it to know when you're done enumerating. Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/media/me... content/browser/media/media_internals.h:55: // Cleans up internal cache of all video devices' capabilities. On 2014/10/03 11:15:41, burnik wrote: > Nit: add empty line. Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:501: MediaInternals::GetInstance()->PurgeVideoCaptureDeviceCapabilities(); On 2014/10/05 23:08:55, burnik wrote: > Maybe we can avoid this if we start of with an empty dictionary and purge it > each time we send it to JS. Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:506: // Send an update to MediaInternals later on. On 2014/10/03 11:15:41, burnik wrote: > Nit: add empty line before comment. Done. https://codereview.chromium.org/616833004/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:511: it.supported_formats)); On 2014/10/05 23:08:55, burnik wrote: > add > devices_info_cache_.size() > as last argument. So you know when all has been enumerated. Done.
https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:247: // TODO(mcasas): Send the capabilities to JS in a similar way to what remove TODO. It does not belong in here. https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:251: video_devices_cached_data_.Clear(); ? https://codereview.chromium.org/616833004/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:569: for (const auto &it : devices_info_cache_) { is this allowed or not? I was trying to figure it out from the style guide. Also- I think we should keep the style of the file so don't use auto in this file. Wdyt?
LGTM, but fix the bots! :) https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:237: name_and_format.a.capture_api_type()); Looks like capture_api_type() is not a string. You should probably add enum to string conversion method for media::VideoCaptureDevice::Name::CaptureApiType. Maybe linux should have this too, but with a single enum value like "DEFAULT_API" or "NO_API". What do you think?
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
guys PTAL. https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:237: name_and_format.a.capture_api_type()); On 2014/10/07 14:17:02, burnik wrote: > Looks like capture_api_type() is not a string. > You should probably add enum to string conversion method for > media::VideoCaptureDevice::Name::CaptureApiType. > > Maybe linux should have this too, but with a single enum value like > "DEFAULT_API" or "NO_API". What do you think? Yeah, just saw it :) It's more complex to add the stringify, so I'd print it as integer, WDYT? https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:247: // TODO(mcasas): Send the capabilities to JS in a similar way to what On 2014/10/07 14:12:51, perkj wrote: > remove TODO. It does not belong in here. I think it does, since it's to be removed in the next CL addressing the bug... https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:251: video_devices_cached_data_.Clear(); On 2014/10/07 14:12:51, perkj wrote: > ? This line goes with the previous TODO comment, IOW after pinging a la SendAudioStreamData(), we have to clear the cache -- see l.230. Perhaps the comment does not make obvious enough this meta-state after this CL and before the "next" one... How would you signal that? https://codereview.chromium.org/616833004/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:569: for (const auto &it : devices_info_cache_) { On 2014/10/07 14:12:51, perkj wrote: > is this allowed or not? I was trying to figure it out from the style guide. > Also- I think we should keep the style of the file so don't use auto in this > file. Wdyt? I read that C++11 is being introduced all over [1] (including f.e. mass-replaces OVERRIDE with override etc), and that we should replace them one-by-one when we can... As you prefer. [1] http://chromium-cpp.appspot.com/
https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:230: DCHECK(video_devices_cached_data_.empty()); You dont' need a member variable video_devices_cached_data_? https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:247: // TODO(mcasas): Send the capabilities to JS in a similar way to what On 2014/10/07 14:54:44, mcasas wrote: > On 2014/10/07 14:12:51, perkj wrote: > > remove TODO. It does not belong in here. > > I think it does, since it's to be removed in the next > CL addressing the bug... I see. I thought this was actually a comment about MediStreamTrack::capabilities. Ignore this,. https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:251: video_devices_cached_data_.Clear(); On 2014/10/07 14:54:44, mcasas wrote: > On 2014/10/07 14:12:51, perkj wrote: > > ? > > This line goes with the previous TODO comment, IOW > after pinging a la SendAudioStreamData(), we have to > clear the cache -- see l.230. > > Perhaps the comment does not make obvious enough this > meta-state after this CL and before the "next" one... > How would you signal that? > TODO remove once.... https://codereview.chromium.org/616833004/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:577: base::Unretained(MediaInternals::GetInstance()), Should we use devices_info_cache_ instead of creating tuples?
perkj@ PTAL. Moved DeviceInfo from VCM to VCDevice to avoid the extra name+formats copy. https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:230: DCHECK(video_devices_cached_data_.empty()); On 2014/10/07 15:51:23, perkj wrote: > You dont' need a member variable video_devices_cached_data_? Done. https://codereview.chromium.org/616833004/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:251: video_devices_cached_data_.Clear(); On 2014/10/07 15:51:23, perkj wrote: > On 2014/10/07 14:54:44, mcasas wrote: > > On 2014/10/07 14:12:51, perkj wrote: > > > ? > > > > This line goes with the previous TODO comment, IOW > > after pinging a la SendAudioStreamData(), we have to > > clear the cache -- see l.230. > > > > Perhaps the comment does not make obvious enough this > > meta-state after this CL and before the "next" one... > > How would you signal that? > > > > TODO remove once.... Gone anyway since I moved |video_devices_cached_data_| to a method-scoped variable, following your previous comment/remark.
https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... content/browser/media/media_internals.cc:238: int cont = 0; nit. s/ count https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... content/browser/media/media_internals.cc:244: formats_dict); nit:indentation https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... content/browser/media/media_internals.h:16: #include "base/tuple.h" not used? https://codereview.chromium.org/616833004/diff/180001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/616833004/diff/180001/media/video/capture/vid... media/video/capture/video_capture_device.h:169: struct DeviceInfo { Please move this out of VideoCaptureDevice and why not name it VideoCaptureDeviceInfo?
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
perkj@ PTAL https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... content/browser/media/media_internals.cc:238: int cont = 0; On 2014/10/08 11:49:59, perkj wrote: > nit. s/ count Done. https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... content/browser/media/media_internals.cc:244: formats_dict); On 2014/10/08 11:49:59, perkj wrote: > nit:indentation Done. https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/180001/content/browser/media/m... content/browser/media/media_internals.h:16: #include "base/tuple.h" On 2014/10/08 11:49:59, perkj wrote: > not used? Done. https://codereview.chromium.org/616833004/diff/180001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/616833004/diff/180001/media/video/capture/vid... media/video/capture/video_capture_device.h:169: struct DeviceInfo { On 2014/10/08 11:49:59, perkj wrote: > Please move this out of VideoCaptureDevice and why not name it > VideoCaptureDeviceInfo? Done.
looks like you need to update the gn files as well. https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:556: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( Call directly instead as discussed. https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:137: typedef media::VideoCaptureDeviceInfo DeviceInfo; Please rename all DeviceInfo to VideoCaptureDeviceInfo instead. Should be simple. https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... media/video/capture/video_capture_device.cc:60: VideoCaptureDevice::DeviceInfo::DeviceInfo() {} remove https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... media/video/capture/video_capture_device.h:168: // A convenience wrap of a devices' name and associated supported formats. remove
Patchset #6 (id:260001) has been deleted
perkj@ PTAL https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:556: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2014/10/08 14:26:36, perkj wrote: > Call directly instead as discussed. Done. https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/616833004/diff/240001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:137: typedef media::VideoCaptureDeviceInfo DeviceInfo; On 2014/10/08 14:26:36, perkj wrote: > Please rename all DeviceInfo to VideoCaptureDeviceInfo instead. Should be > simple. Done. https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... media/video/capture/video_capture_device.cc:60: VideoCaptureDevice::DeviceInfo::DeviceInfo() {} On 2014/10/08 14:26:36, perkj wrote: > remove Done. https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/616833004/diff/240001/media/video/capture/vid... media/video/capture/video_capture_device.h:168: // A convenience wrap of a devices' name and associated supported formats. On 2014/10/08 14:26:37, perkj wrote: > remove Done.
lgtm. Please let the owner of media_internals also review.
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@: media/{media.gyp,BUILD.gn} and media_internals.*
dalecurtis@chromium.org changed reviewers: + wolenetz@chromium.org
I'm traveling, so +Wolenetz
Thanks for the excuse to page-in some media-internals code :) Other than minor nits/questions, can some test be added? https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:222: "media.onReceiveEverything", &audio_streams_cached_data_); s/onReceiveEverything/onReceiveAudioStreamData/ here and in main.js? Or is that better done when doing the new "TODO(mcasas)", below? https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:231: for (const auto &name_and_format : name_and_formats) { nit: here and below, s/auto &/auto& / https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:239: for (const auto &format : name_and_format.supported_formats) { nit: ditto. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.h:50: // Called to inform of the capabilities enumerated for a video device. nit: s/video device/video capture device/ Also, is this for just one device, or multiple? If the latter, update comment please. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals_proxy.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals_proxy.cc:138: void MediaInternalsProxy::GetEverythingOnIOThread() { ditto: s/Everything/something audio specific/ for now, or leave as-is until everything includes sending also the video capabilities? https://codereview.chromium.org/616833004/diff/280001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:562: for (const auto &it : devices_info_cache_) { nit: ditto auto& https://codereview.chromium.org/616833004/diff/280001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:584: for (media::VideoCaptureDeviceInfos::const_iterator it_device_info = nit: is this outer for-loop a candidate for const auto&, too? https://codereview.chromium.org/616833004/diff/280001/media/video/capture/vid... File media/video/capture/video_capture_device_info.h (right): https://codereview.chromium.org/616833004/diff/280001/media/video/capture/vid... media/video/capture/video_capture_device_info.h:13: // A convenience wrap of a devices' name and associated supported formats. nit: s/devices'/device's/
Also: https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:247: // to JS is implemented in a similar way to how SendAudioStreamData() does. One other nit: you might want to note here that |lock_| may be required if these capabilities are cached in this object as part of that future CL.
wolenetz@ PTAL. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:222: "media.onReceiveEverything", &audio_streams_cached_data_); On 2014/10/09 20:51:43, wolenetz wrote: > s/onReceiveEverything/onReceiveAudioStreamData/ here and in main.js? > Or is that better done when doing the new "TODO(mcasas)", below? Done. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:231: for (const auto &name_and_format : name_and_formats) { On 2014/10/09 20:51:43, wolenetz wrote: > nit: here and below, s/auto &/auto& / Done. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:239: for (const auto &format : name_and_format.supported_formats) { On 2014/10/09 20:51:43, wolenetz wrote: > nit: ditto. Done. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.cc:247: // to JS is implemented in a similar way to how SendAudioStreamData() does. On 2014/10/09 20:55:05, wolenetz wrote: > One other nit: you might want to note here that |lock_| may be required if these > capabilities are cached in this object as part of that future CL. Done. We don't need to use the same |lock_| though. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals.h:50: // Called to inform of the capabilities enumerated for a video device. On 2014/10/09 20:51:43, wolenetz wrote: > nit: s/video device/video capture device/ > Also, is this for just one device, or multiple? If the latter, update comment > please. Done. https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... File content/browser/media/media_internals_proxy.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/media/m... content/browser/media/media_internals_proxy.cc:138: void MediaInternalsProxy::GetEverythingOnIOThread() { On 2014/10/09 20:51:43, wolenetz wrote: > ditto: s/Everything/something audio specific/ for now, or leave as-is until > everything includes sending also the video capabilities? Added a TODO(), I don't think it's worth renaming this method and the caller and reverting the rename in the next CL... https://codereview.chromium.org/616833004/diff/280001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/280001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:562: for (const auto &it : devices_info_cache_) { On 2014/10/09 20:51:43, wolenetz wrote: > nit: ditto auto& Done. https://codereview.chromium.org/616833004/diff/280001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:584: for (media::VideoCaptureDeviceInfos::const_iterator it_device_info = On 2014/10/09 20:51:43, wolenetz wrote: > nit: is this outer for-loop a candidate for const auto&, too? Done. https://codereview.chromium.org/616833004/diff/280001/media/video/capture/vid... File media/video/capture/video_capture_device_info.h (right): https://codereview.chromium.org/616833004/diff/280001/media/video/capture/vid... media/video/capture/video_capture_device_info.h:13: // A convenience wrap of a devices' name and associated supported formats. On 2014/10/09 20:51:43, wolenetz wrote: > nit: s/devices'/device's/ Done.
Patchset #8 (id:660001) has been deleted
lgtm % 1 persistent nit :) Thanks! https://codereview.chromium.org/616833004/diff/440001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/440001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:562: for (const auto &it : devices_info_cache_) { nit: "auto &" -> "auto& ": not done https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... File content/browser/media/media_internals_unittest.cc (right): https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... content/browser/media/media_internals_unittest.cc:110: update_cb_(base::Bind(&MediaInternalsAudioLogTest::UpdateCallbackImpl, aside: Use ..TestBase in bind? On second thought, I think it's best how you have it currently, in case future change overrides that ..TestBase method in this class.
wolenetz@chromium.org changed reviewers: + xhwang@chromium.org
Hmm. It looks like I'm not yet an owner of content/browser/resources/media. +xhwang
lgtm % naming nits https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... content/browser/media/media_internals.cc:228: const media::VideoCaptureDeviceInfos& name_and_formats) { names_and_formats is ambiguous. It's really vector_of_name_and_formats. How about just use video_capture_device_infos? https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... content/browser/media/media_internals.cc:231: for (const auto& name_and_format : name_and_formats) { ditto, s/name_and_format/video_capture_device_info
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616833004/950001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616833004/950001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
Patchset #10 (id:950001) has been deleted
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616833004/800002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Patchset #10 (id:800002) has been deleted
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616833004/1370001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616833004/1370001
Message was sent while issue was closed.
Committed patchset #10 (id:1370001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/fcb5c7de8085887dae1359a12c74df1f44692b72 Cr-Commit-Position: refs/heads/master@{#299251}
Message was sent while issue was closed.
https://codereview.chromium.org/616833004/diff/440001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/616833004/diff/440001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:562: for (const auto &it : devices_info_cache_) { On 2014/10/10 18:16:17, wolenetz wrote: > nit: "auto &" -> "auto& ": not done Sneaky! Double checked all autos, should be done now. https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... content/browser/media/media_internals.cc:228: const media::VideoCaptureDeviceInfos& name_and_formats) { On 2014/10/10 19:40:06, xhwang wrote: > names_and_formats is ambiguous. It's really vector_of_name_and_formats. How > about just use video_capture_device_infos? Done. https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... content/browser/media/media_internals.cc:231: for (const auto& name_and_format : name_and_formats) { On 2014/10/10 19:40:06, xhwang wrote: > ditto, s/name_and_format/video_capture_device_info Done. https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... File content/browser/media/media_internals_unittest.cc (right): https://codereview.chromium.org/616833004/diff/710001/content/browser/media/m... content/browser/media/media_internals_unittest.cc:110: update_cb_(base::Bind(&MediaInternalsAudioLogTest::UpdateCallbackImpl, On 2014/10/10 18:16:17, wolenetz wrote: > aside: Use ..TestBase in bind? On second thought, I think it's best how you have > it currently, in case future change overrides that ..TestBase method in this > class. Acknowledged. |
