Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763743002:
Android: not to pause screen capture when Chrome is put to background (Closed)
|
Created:
3 years, 9 months ago by braveyao Modified:
3 years, 8 months ago CC:
avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: not to pause screen capture when Chrome is put to background
It's a valid case that screen capture continues when Chrome is put to
background on Android(while camera capture should be paused.).
Here a query interface is added in MediaStreamDispatch to return all
the non-screen capture devices on the current Tab. Then RenderViewImpl
can suspend/resume every non-screen capture device only when
OnPageWasHidden/Shown() is called.
And the SuspendDevices() is removed from VideoCaptureManagerImpl to
simplify the logic.
Also because each MediaStreamDispatch corresponds one single Tab, this
means now RenderViewImpl can pause/resume the devices related to the
current Tab only. Which is also a fix to crbug/593192.
BUG=487935, 593192
Review-Url: https://codereview.chromium.org/2763743002
Cr-Commit-Position: refs/heads/master@{#460537}
Committed: https://chromium.googlesource.com/chromium/src/+/1d8ee7b06197fabd1441787a015cdda24e49c838
Patch Set 1 #
Total comments: 20
Patch Set 2 : address comments on PS#1 #
Total comments: 22
Patch Set 3 : address comments on PS#2 #
Total comments: 1
Patch Set 4 : restore logic in VideoCaptureImplManager #
Total comments: 2
Patch Set 5 : address nits #
Total comments: 2
Patch Set 6 : address nits #
Messages
Total messages: 57 (43 generated)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + miu@chromium.org, tommi@chromium.org
Hi miu@, thanks so much for the suggestions, which leads to this cl and can solve several issues. I didn't add a class in RenderViewImpl and save the paused devices as suggested, because I prefer to query the latest devices list from the target tab each time when OnPageWasHidden/Shown is called. Please take a look if this is good. tommi@, I believe this patch can also solve the issue593192. Please help to double check if this can be the fix to issue593192, or you still have other plan to that issue.
https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:179: StreamDeviceInfoArray& video_array) { see comment in header for style issue https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:181: for (const auto& device_it : stream_it.second.video_array) { |device_it| is misleading, since the variable is not an iterator but rather the actual item in the array. https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.h (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.h:79: void GetVideoCaptureDevices(StreamDeviceInfoArray& video_array); StreamDeviceInfoArray* or change the method to return StreamDeviceInfoArray? https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher_unittest.cc:385: new MediaStreamDispatcher(NULL)); nullptr https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher_unittest.cc:402: EXPECT_EQ(dispatcher->requests_.size(), size_t(0)); EXPECT_TRUE(dispatcher->requests_.empty()); In Chromium we also don't use C style casts such as (type)i or type(i). Instead we use one of static_cast<>, reinterpret_cast<> or other custom casting templates available in base/ However, for casting a constant to size_t, you don't need a cast. If an unsigned value is all you want, just append 'u'. E.g. 2u. https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2572: void RenderViewImpl::SuspendVideoCaptureDevices(bool suspend) { can we have a thread check for this function? https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2578: if (media_stream_dispatcher) { Could GetMediaStreamDispatcher() ever return null when SuspendVideoCaptureDevices() is called? https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2581: for (const StreamDeviceInfo& device_it : video_array) { |device_it| isn't correct. Suggest |device_info| instead. https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2582: if (suspend) use {} as per coding guidelines
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks tommi@. All comments are addressed, except the thread check one(see my question in the reply). PTAL. https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:179: StreamDeviceInfoArray& video_array) { On 2017/03/21 09:21:17, tommi - chröme wrote: > see comment in header for style issue Done. https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:181: for (const auto& device_it : stream_it.second.video_array) { On 2017/03/21 09:21:17, tommi - chröme wrote: > |device_it| is misleading, since the variable is not an iterator but rather the > actual item in the array. Done. https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.h (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.h:79: void GetVideoCaptureDevices(StreamDeviceInfoArray& video_array); On 2017/03/21 09:21:17, tommi - chröme wrote: > StreamDeviceInfoArray* > > or change the method to return StreamDeviceInfoArray? Done. I'm more comfortable to pass it in as a parameter. :) https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher_unittest.cc:385: new MediaStreamDispatcher(NULL)); On 2017/03/21 09:21:17, tommi - chröme wrote: > nullptr Done. https://codereview.chromium.org/2763743002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher_unittest.cc:402: EXPECT_EQ(dispatcher->requests_.size(), size_t(0)); On 2017/03/21 09:21:17, tommi - chröme wrote: > EXPECT_TRUE(dispatcher->requests_.empty()); > > In Chromium we also don't use C style casts such as (type)i or type(i). Instead > we use one of static_cast<>, reinterpret_cast<> or other custom casting > templates available in base/ > > However, for casting a constant to size_t, you don't need a cast. If an > unsigned value is all you want, just append 'u'. E.g. 2u. Acknowledged. Done. https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2572: void RenderViewImpl::SuspendVideoCaptureDevices(bool suspend) { On 2017/03/21 09:21:17, tommi - chröme wrote: > can we have a thread check for this function? There is no thread check yet in this file. Add thread check to this function only or all the functions where thread check can apply? https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2578: if (media_stream_dispatcher) { On 2017/03/21 09:21:18, tommi - chröme wrote: > Could GetMediaStreamDispatcher() ever return null when > SuspendVideoCaptureDevices() is called? Just the codes[1] shows there is such possibility. Other place uses DCHECK. I prefer real check. Hope it's OK. [1]:https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?type=cs&q=GetMediaStreamDispatcher&l=1473 https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2581: for (const StreamDeviceInfo& device_it : video_array) { On 2017/03/21 09:21:17, tommi - chröme wrote: > |device_it| isn't correct. Suggest |device_info| instead. Done. https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2582: if (suspend) On 2017/03/21 09:21:17, tommi - chröme wrote: > use {} as per coding guidelines Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
Comments on PS2: https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2572: void RenderViewImpl::SuspendVideoCaptureDevices(bool suspend) { On 2017/03/22 00:45:33, braveyao wrote: > On 2017/03/21 09:21:17, tommi - chröme wrote: > > can we have a thread check for this function? > > There is no thread check yet in this file. Add thread check to this function > only or all the functions where thread check can apply? IMHO, it's not necessary: I think it's generally-known that RenderViewImpl runs entirely on the main thread by default. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_dispatcher.cc (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher.cc:182: if (video_device.device.type == MEDIA_DEVICE_VIDEO_CAPTURE) Please change this to: if (!IsScreenCaptureMediaType(video_device.device.type)) video_array->push_back(video_device); ref: https://cs.chromium.org/chromium/src/content/public/common/media_stream_reque... https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_dispatcher.h (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher.h:78: // Get all the media devices of video capture, e.g. webcam. nit: It would be helpful to add to this comment, something like "This is the set of devices that should be suspended when the content frame is no longer being shown to the user." https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher.h:79: void GetVideoCaptureDevices(StreamDeviceInfoArray* video_array); This naming doesn't suggest it is filtering based on certain device tyes. How about: GetNonScreenCaptureDevices() https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher_unittest.cc:383: TEST_F(MediaStreamDispatcherTest, GetVideoCaptureDevices) { ditto: GetNonScreenCaptureDevices https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher_unittest.cc:431: // Only the device with |kVideoType| will be got. s/got/returned/ https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2281: #if defined(OS_ANDROID) && BUILDFLAG(ENABLE_WEBRTC) Why does BUILDFLAG(ENABLE_WEBRTC) matter? This logic should be applicable to ALL remoting technologies. Let's change this to: #if defined(OS_ANDROID) SuspendVideoCaptureDevices(); #if BUILDFLAG(ENABLE_WEBRTC) ...speech recog stuff... #endif #endif https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2301: #if defined(OS_ANDROID) && BUILDFLAG(ENABLE_WEBRTC) Remove the BUILDFLAG(ENABLE_WEBRTC) part. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2578: if (media_stream_dispatcher) { nit: if (!media_stream_dispatcher) return; ... rest of code one indent level less... https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2582: if (suspend) { See comments in header file about just having two methods instead of this bool and the extra run-time cost here. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.h:585: // Make the video capture devices(e.g. webcam) stop/resume delivering video nit: space before '(' https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.h:588: void SuspendVideoCaptureDevices(bool suspend); Instead of having a boolean argument, I'd suggest just having two methods: void SuspendVideoCaptureDevicesOnPageHidden(); void ResumeVideoCaptureDevicesOnPageShown();
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks all for the comments! All addressed. PTAL. https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:2572: void RenderViewImpl::SuspendVideoCaptureDevices(bool suspend) { On 2017/03/22 23:09:42, miu wrote: > On 2017/03/22 00:45:33, braveyao wrote: > > On 2017/03/21 09:21:17, tommi - chröme wrote: > > > can we have a thread check for this function? > > > > There is no thread check yet in this file. Add thread check to this function > > only or all the functions where thread check can apply? > > IMHO, it's not necessary: I think it's generally-known that RenderViewImpl runs > entirely on the main thread by default. Acknowledged. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_dispatcher.cc (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher.cc:182: if (video_device.device.type == MEDIA_DEVICE_VIDEO_CAPTURE) On 2017/03/22 23:09:42, miu wrote: > Please change this to: > > if (!IsScreenCaptureMediaType(video_device.device.type)) > video_array->push_back(video_device); > > ref: > https://cs.chromium.org/chromium/src/content/public/common/media_stream_reque... Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_dispatcher.h (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher.h:78: // Get all the media devices of video capture, e.g. webcam. On 2017/03/22 23:09:42, miu wrote: > nit: It would be helpful to add to this comment, something like "This is the set > of devices that should be suspended when the content frame is no longer being > shown to the user." Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher.h:79: void GetVideoCaptureDevices(StreamDeviceInfoArray* video_array); On 2017/03/22 23:09:43, miu wrote: > This naming doesn't suggest it is filtering based on certain device tyes. How > about: GetNonScreenCaptureDevices() Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher_unittest.cc:383: TEST_F(MediaStreamDispatcherTest, GetVideoCaptureDevices) { On 2017/03/22 23:09:43, miu wrote: > ditto: GetNonScreenCaptureDevices Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_dispatcher_unittest.cc:431: // Only the device with |kVideoType| will be got. On 2017/03/22 23:09:43, miu wrote: > s/got/returned/ Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2281: #if defined(OS_ANDROID) && BUILDFLAG(ENABLE_WEBRTC) On 2017/03/22 23:09:43, miu wrote: > Why does BUILDFLAG(ENABLE_WEBRTC) matter? This logic should be applicable to ALL > remoting technologies. Let's change this to: > > #if defined(OS_ANDROID) > SuspendVideoCaptureDevices(); > #if BUILDFLAG(ENABLE_WEBRTC) > ...speech recog stuff... > #endif > #endif Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2301: #if defined(OS_ANDROID) && BUILDFLAG(ENABLE_WEBRTC) On 2017/03/22 23:09:43, miu wrote: > Remove the BUILDFLAG(ENABLE_WEBRTC) part. Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2578: if (media_stream_dispatcher) { On 2017/03/22 23:09:43, miu wrote: > nit: > > if (!media_stream_dispatcher) > return; > > ... rest of code one indent level less... Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:2582: if (suspend) { On 2017/03/22 23:09:43, miu wrote: > See comments in header file about just having two methods instead of this bool > and the extra run-time cost here. Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.h:585: // Make the video capture devices(e.g. webcam) stop/resume delivering video On 2017/03/22 23:09:43, miu wrote: > nit: space before '(' Done. https://codereview.chromium.org/2763743002/diff/20001/content/renderer/render... content/renderer/render_view_impl.h:588: void SuspendVideoCaptureDevices(bool suspend); On 2017/03/22 23:09:43, miu wrote: > Instead of having a boolean argument, I'd suggest just having two methods: > > void SuspendVideoCaptureDevicesOnPageHidden(); > void ResumeVideoCaptureDevicesOnPageShown(); Done.
https://codereview.chromium.org/2763743002/diff/100001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/100001/content/renderer/rende... content/renderer/render_view_impl.cc:2602: RenderThreadImpl::current()->video_capture_impl_manager()->Resume( Sorry, I didn't realize this in the last round of review: The logic we deleted from VideoCaptureImplManager tracked whether each device was suspended because of a "suspend all" operation (on render view hidden), or if it was individually suspended for some other reason. Devices that are individually suspended should not be resumed when the "resume all" happens. For example, the old code handled this case correctly: 1. Code in c/r/m/media_stream_video_capturer_source.cc calls VideoCaptureImplManager::Suspend() to suspend a device because there are no consumers of its feed. 2. RenderViewImpl calls VideoCaptureImplManager::SuspendDevices(true) when a page is hidden. All devices are suspended 3. Once the page is showing again, RenderViewImpl calls VideoCaptureImplManager::SuspendDevices(false). However, the device suspended in #1 is NOT resumed. See: https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp... The current code would resume previously-suspended devices that should remain suspended because they have no consumers.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Oops, you're right. Eventually we can's simplify the logic in VCIM... Restore the removed method and adjusted it by adding the device array as an argument. PTAL.
lgtm % nit: https://codereview.chromium.org/2763743002/diff/120001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:2584: if (!video_array.empty()) { nit: The empty() check here isn't necessary. It won't affect the behavior of VCIM::SuspendDevices().
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Thanks miu@! tommi@, PTAL. https://codereview.chromium.org/2763743002/diff/120001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2763743002/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:2584: if (!video_array.empty()) { On 2017/03/28 23:07:52, miu wrote: > nit: The empty() check here isn't necessary. It won't affect the behavior of > VCIM::SuspendDevices(). Done.
lgtm https://codereview.chromium.org/2763743002/diff/140001/content/renderer/media... File content/renderer/media/media_stream_dispatcher.h (right): https://codereview.chromium.org/2763743002/diff/140001/content/renderer/media... content/renderer/media/media_stream_dispatcher.h:81: void GetNonScreenCaptureDevices(StreamDeviceInfoArray* video_array); I would still prefer to have this function return the array. It's consistent with the rest of the chromium code.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + aelias@chromium.org
Thanks tommi@. It's addressed. + aelias@ for owner's review to content/renderer/render_view_impl.cc content/renderer/render_view_impl.h https://codereview.chromium.org/2763743002/diff/140001/content/renderer/media... File content/renderer/media/media_stream_dispatcher.h (right): https://codereview.chromium.org/2763743002/diff/140001/content/renderer/media... content/renderer/media/media_stream_dispatcher.h:81: void GetNonScreenCaptureDevices(StreamDeviceInfoArray* video_array); On 2017/03/29 07:25:10, tommi - chröme wrote: > I would still prefer to have this function return the array. It's consistent > with the rest of the chromium code. Done.
lgtm
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2763743002/#ps160001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490821257324970,
"parent_rev": "64eeaf6fafdb64505f589a8f1391460362ea09af", "commit_rev":
"1d8ee7b06197fabd1441787a015cdda24e49c838"}
Message was sent while issue was closed.
Description was changed from ========== Android: not to pause screen capture when Chrome is put to background It's a valid case that screen capture continues when Chrome is put to background on Android(while camera capture should be paused.). Here a query interface is added in MediaStreamDispatch to return all the non-screen capture devices on the current Tab. Then RenderViewImpl can suspend/resume every non-screen capture device only when OnPageWasHidden/Shown() is called. And the SuspendDevices() is removed from VideoCaptureManagerImpl to simplify the logic. Also because each MediaStreamDispatch corresponds one single Tab, this means now RenderViewImpl can pause/resume the devices related to the current Tab only. Which is also a fix to crbug/593192. BUG=487935, 593192 ========== to ========== Android: not to pause screen capture when Chrome is put to background It's a valid case that screen capture continues when Chrome is put to background on Android(while camera capture should be paused.). Here a query interface is added in MediaStreamDispatch to return all the non-screen capture devices on the current Tab. Then RenderViewImpl can suspend/resume every non-screen capture device only when OnPageWasHidden/Shown() is called. And the SuspendDevices() is removed from VideoCaptureManagerImpl to simplify the logic. Also because each MediaStreamDispatch corresponds one single Tab, this means now RenderViewImpl can pause/resume the devices related to the current Tab only. Which is also a fix to crbug/593192. BUG=487935, 593192 Review-Url: https://codereview.chromium.org/2763743002 Cr-Commit-Position: refs/heads/master@{#460537} Committed: https://chromium.googlesource.com/chromium/src/+/1d8ee7b06197fabd1441787a015c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/1d8ee7b06197fabd1441787a015c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
