Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709523003:
BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame (Closed)
|
Created:
3 years, 10 months ago by Hzj_jie Modified:
3 years, 9 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Jamie Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionBlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame
DXGI capturer highly depends on video adapter and its driver, as well as Windows
itself. I recently found it cannot work on my virtualbox instance any more,
which indicates it may not work well on some specific systems. What worse is,
the APIs do not return a failure in such case.
So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several
pixels in the frame returned by a DesktopCapturer implementation. If all the
pixels selected are blank, this wrapper returns a failure. A typical usage is to
combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper,
and use GDI capturer in case of failure.
Usually less than 10000 pixels are checked, so the
BlankDetectorDesktopCapturerWrapper should not significant impact the capturer
performance.
This change is expected to resolve bug 682112 in another dimension.
BUG=682112
Review-Url: https://codereview.webrtc.org/2709523003
Cr-Original-Commit-Position: refs/heads/master@{#16984}
Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24409a2e626599255
Review-Url: https://codereview.webrtc.org/2709523003
Cr-Commit-Position: refs/heads/master@{#17024}
Committed: https://chromium.googlesource.com/external/webrtc/+/ccf57a71eb2a550744367a4cbec285d85494c3c2
Patch Set 1 #Patch Set 2 : Check more pixels and add test cases #
Total comments: 2
Patch Set 3 : Resolve review comments #
Total comments: 12
Patch Set 4 : Resolve review comments #Patch Set 5 : Resolve review comments #
Total comments: 14
Patch Set 6 : Resolve review comments #
Total comments: 6
Patch Set 7 : Resolve review comments #Patch Set 8 : Remove HISTOGRAM #
Messages
Total messages: 86 (63 generated)
The CQ bit was checked by zijiehe@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.webrtc.org/...
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. This change is expected to resolve bug 682112 in every dimension. BUG=682112 ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. This change is expected to resolve bug 682112 in another dimension. BUG=682112 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. This change is expected to resolve bug 682112 in another dimension. BUG=682112 ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 100 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am working on an optimization of this class, and adding test cases.
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23037) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19044)
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #2 (id:20001) has been deleted
Do I understand it correctly that the problem is that the capturer doesn't work correctly at all on some systems? Are you sure it's a bug in the API and now in how we are using it? https://codereview.webrtc.org/2709523003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:79: if (!IsBlankFrame(*frame)) { Do we need to check every frame only only a few first frames?
On 2017/02/21 21:58:48, Sergey Ulanov wrote: > Do I understand it correctly that the problem is that the capturer doesn't work > correctly at all on some systems? Are you sure it's a bug in the API and now in > how we are using it? > > https://codereview.webrtc.org/2709523003/diff/40001/webrtc/modules/desktop_ca... > File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc > (right): > > https://codereview.webrtc.org/2709523003/diff/40001/webrtc/modules/desktop_ca... > webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:79: if > (!IsBlankFrame(*frame)) { > Do we need to check every frame only only a few first frames? I cannot say this is a bug in DXGI APIs. The APIs highly depend on video adapters and their drivers. At least on my virtualbox instance, after some mysterious Windows 10 or virtualbox update, these APIs won't work anymore. But they terribly return correct value without any error notifications. I think DXGI APIs should work on 99% of eligible systems, but for the last 1%, this change can help to ensure we won't break their scenario totally. At least I do not think there is no user to use Chrome or Chrome Remote Desktop within virtualbox instance.
The CQ bit was checked by zijiehe@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.webrtc.org/...
https://codereview.webrtc.org/2709523003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:79: if (!IsBlankFrame(*frame)) { On 2017/02/21 21:58:48, Sergey Ulanov wrote: > Do we need to check every frame only only a few first frames? Oh, yes, at least when permanent_failure_threshold_ is reached, we should skip this checking. But permanent error has been disabled for DX capturer, falling back to GDI capturer after resuming from a black screen saver does not look right. Checking only first several frames may also be impacted by black screen saver. And I also doubt whether DX capturer may fail in some specific display modes. i.e. Users get only black screen after switching from a supported to a non-supported display mode.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 100 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 500 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 ==========
I'm not sure we actually need BlankDetectorDesktopCapturerWrapper. Let's try enabling fallback capturer for DX and see if there is any case it's not enough. WDYT?
On 2017/02/23 23:02:02, Sergey Ulanov wrote: > I'm not sure we actually need BlankDetectorDesktopCapturerWrapper. Let's try > enabling fallback capturer for DX and see if there is any case it's not enough. > WDYT? On my virtual machine, DX capturer cannot work correctly, but the APIs do not return an error. The FallbackDesktopCapturerWrapper depends on the implementation to return an error, but since DX APIs do not return an error, we cannot predict its failure. I have reported the issue on virtualbox forum. But this API is rarely used, I have not got any useful feedback. Since I have also tested the DXGI desktop duplication sample, which is provided by Microsoft (https://code.msdn.microsoft.com/windowsdesktop/Desktop-Duplication-Sample-da4...). It also "works" well with black frame, which proves this to be a driver / virtual hardware / Windows bug, we cannot easily resolve.
On 2017/02/24 01:03:09, Hzj_jie wrote: > On 2017/02/23 23:02:02, Sergey Ulanov wrote: > > I'm not sure we actually need BlankDetectorDesktopCapturerWrapper. Let's try > > enabling fallback capturer for DX and see if there is any case it's not > enough. > > WDYT? > > On my virtual machine, DX capturer cannot work correctly, but the APIs do not > return an error. > The FallbackDesktopCapturerWrapper depends on the implementation to return an > error, but since DX APIs do not return an error, we cannot predict its failure. > > I have reported the issue on virtualbox forum. But this API is rarely used, I > have not got any useful feedback. > > Since I have also tested the DXGI desktop duplication sample, which is provided > by Microsoft > (https://code.msdn.microsoft.com/windowsdesktop/Desktop-Duplication-Sample-da4...). > It also "works" well with black frame, which proves this to be a driver / > virtual hardware / Windows bug, we cannot easily resolve. DX APIs failed on my virtual machine (it uses DX duplication sample application provided by Microsoft.) https://drive.google.com/open?id=0B0OFNI4uoZGRNzdkSkZVcUdqOEk The same binary works well on my laptop https://drive.google.com/open?id=0B0OFNI4uoZGRdTFfbzctSml1RXc CRD failed with the exactly same output as DX duplication sample application on my virtual machine. https://drive.google.com/open?id=0B0OFNI4uoZGRdThfV2ZaSjRyd3M
I think it would be useful to collect some stats to analyze how often we hit the issue. Can you please add RTC_HISTOGRAM.. calls for that? (maybe in a different CL). I think we need to know many users are affected and how often we get blank frames on affected systems. https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:14: #include "webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h" This should be the first include in this file. https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:83: if (!IsBlankFrame(*frame)) { Can we avoid calling it for every frame? Particularly it doesn't make sense to check if the frame blank if it's the same as the previous frame. Maybe only check the pixels in updated_region() or skip frames with empty updated_region()? Also, if we know that the first frame is not black, do we need to check all frames after it? https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:102: (frame.size().width() < frame.size().height() ? frame.size().width() std::min(frame.size().width(), frame.size().height()) https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:109: const int y2 = frame.size().height() - i - 1; As far as I can tell this always ignores center of the screen for non-square screen, except for a single pixel. If the user views 9:16 photos on a 16:9 screen (note different orientation) then all pixels that are checked here will be black. If the central pixel happens to be black then this function would return incorrect result. Can we distribute the pixels being checked throughout the frame. E.g. we could look at every 1000th pixel (i.e. distribute the pixels we look at uniformly throughout the frame). https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:111: !IsBlankPixel(frame, x2, y1) || !IsBlankPixel(frame, x2, y2)) { I would expect performance of this code to be highly dependent on memory cache size and behavior. It's likely to have a non-negligible effect on systems with small L2 cache (where full frame doesn't fit in the cache). I'm not sure that checking four pixels at the time like you do here will perform better than just scanning the same pixels in the order in which they are stored in memory. Can you please try that and see if it performs any different?
The CQ bit was checked by zijiehe@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.webrtc.org/...
https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:14: #include "webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h" On 2017/02/25 00:05:28, Sergey Ulanov wrote: > This should be the first include in this file. Done. https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:83: if (!IsBlankFrame(*frame)) { On 2017/02/25 00:05:28, Sergey Ulanov wrote: > Can we avoid calling it for every frame? Particularly it doesn't make sense to > check if the frame blank if it's the same as the previous frame. Maybe only > check the pixels in updated_region() or skip frames with empty updated_region()? Done. > > Also, if we know that the first frame is not black, do we need to check all > frames after it? This class is more like the escape routes on the aircraft. If by any chance, DX APIs fail on some certain modes, say, a specific resolution, this class can help to find a backup plan for users automatically. https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:102: (frame.size().width() < frame.size().height() ? frame.size().width() On 2017/02/25 00:05:29, Sergey Ulanov wrote: > std::min(frame.size().width(), frame.size().height()) Done. https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:109: const int y2 = frame.size().height() - i - 1; On 2017/02/25 00:05:28, Sergey Ulanov wrote: > As far as I can tell this always ignores center of the screen for non-square > screen, except for a single pixel. If the user views 9:16 photos on a 16:9 > screen (note different orientation) then all pixels that are checked here will > be black. If the central pixel happens to be black then this function would > return incorrect result. Can we distribute the pixels being checked throughout > the frame. E.g. we could look at every 1000th pixel (i.e. distribute the pixels > we look at uniformly throughout the frame). The center pixel has been checked at the end of this function. Your suggestion seems better, I have updated the code. https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:111: !IsBlankPixel(frame, x2, y1) || !IsBlankPixel(frame, x2, y2)) { On 2017/02/25 00:05:29, Sergey Ulanov wrote: > I would expect performance of this code to be highly dependent on memory cache > size and behavior. It's likely to have a non-negligible effect on systems with > small L2 cache (where full frame doesn't fit in the cache). I'm not sure that > checking four pixels at the time like you do here will perform better than just > scanning the same pixels in the order in which they are stored in memory. Can > you please try that and see if it performs any different? If we check pixels sequentially (Say, one per 1000 pixels), we should be fine here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:83: if (!IsBlankFrame(*frame)) { On 2017/02/25 02:06:38, Hzj_jie wrote: > On 2017/02/25 00:05:28, Sergey Ulanov wrote: > > Can we avoid calling it for every frame? Particularly it doesn't make sense to > > check if the frame blank if it's the same as the previous frame. Maybe only > > check the pixels in updated_region() or skip frames with empty > updated_region()? > > Done. > > > > > Also, if we know that the first frame is not black, do we need to check all > > frames after it? > > This class is more like the escape routes on the aircraft. If by any chance, DX > APIs fail on some certain modes, say, a specific resolution, this class can help > to find a backup plan for users automatically. I don't think we should be doing this unless we know there are cases when it solves a problem. It's impossible to workaround every possible fault in the API implementation caused by a driver bug. E.g. what if the API always returns white frames? What if the API returns garbage? We don't know if such bugs exist and we don't try to workaround them a propri. Currently there is only one specific issue that we know about: the API may return black frame on a VirtualBox machine. To work it around it's enough to verify that the first frame is not black. I don't think we want to extend this workaround to other cases when the capturer may return black frames. Otherwise all users would be paying with wasted CPU cycles to workaround a bug that may not even exist, and there is a risk for other regressions (e.g. in the case screen is partially black).
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was checked by zijiehe@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.webrtc.org/...
https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:83: if (!IsBlankFrame(*frame)) { On 2017/02/27 23:26:25, Sergey Ulanov wrote: > On 2017/02/25 02:06:38, Hzj_jie wrote: > > On 2017/02/25 00:05:28, Sergey Ulanov wrote: > > > Can we avoid calling it for every frame? Particularly it doesn't make sense > to > > > check if the frame blank if it's the same as the previous frame. Maybe only > > > check the pixels in updated_region() or skip frames with empty > > updated_region()? > > > > Done. > > > > > > > > Also, if we know that the first frame is not black, do we need to check all > > > frames after it? > > > > This class is more like the escape routes on the aircraft. If by any chance, > DX > > APIs fail on some certain modes, say, a specific resolution, this class can > help > > to find a backup plan for users automatically. > > I don't think we should be doing this unless we know there are cases when it > solves a problem. It's impossible to workaround every possible fault in the API > implementation caused by a driver bug. E.g. what if the API always returns white > frames? What if the API returns garbage? We don't know if such bugs exist and we > don't try to workaround them a propri. > > Currently there is only one specific issue that we know about: the API may > return black frame on a VirtualBox machine. To work it around it's enough to > verify that the first frame is not black. I don't think we want to extend this > workaround to other cases when the capturer may return black frames. Otherwise > all users would be paying with wasted CPU cycles to workaround a bug that may > not even exist, and there is a risk for other regressions (e.g. in the case > screen is partially black). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:48: capturer_->CaptureFrame(); Maybe check permanent_failure_reached() here instead of in OnCaptureResult()? https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:76: if (!frame) { This is a bug in the underlying capturer. DCHECK() would be more appropriate https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:114: for (int i = 0; i < frame.size().width() * frame.size().height(); i += 1000) { Hm. I think this filter can still be problematic in some cases as it verifies only limited portion of the screen when width is close to 1000. Something like this may work better: y = i / W x = (i + y * P) % W Here P is some arbitrary number. I think a prime number under 100 would work well, e.g. 31. WDYT? Also did you verify if this gives significant performance boost? Since we only need to check a few first frames it may be appropriate to verify the whole frame. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:122: return IsBlankPixel(frame, frame.size().width() >> 1, Add a comment that we are verifying the pixel in the center as well. Also replace >> 1 with / 2. It's more readable and the compiler should know to optimize it with shift. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:131: return RgbaColor(pixel_data) == blank_pixel_; Do we want to verify the alpha byte? Is it guaranteed to be set? It may work with DX capturer, but the behavior is not consistent across other capturers (IIRC some set it to 0xff, others return 0). https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:135: return permanent_failure_threshold_ >= 0; It's not documented in the header that permanent_failure_threshold=0 allows to disable permanent failure. Do we actually need this feature? If we do then please add a comment about it in the header. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h:17: #include "webrtc/modules/desktop_capture/rgba_color.h" Comments for RgbaColor say that it's for tests only and that was my understanding as well. Do we really need to use it here? This class could just detect black frames, I don't think we need to pass blank_pixel as a parameter.
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23765)
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #6 (id:120001) has been deleted
https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:48: capturer_->CaptureFrame(); On 2017/03/01 22:18:32, Sergey Ulanov wrote: > Maybe check permanent_failure_reached() here instead of in OnCaptureResult()? Done. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:76: if (!frame) { On 2017/03/01 22:18:32, Sergey Ulanov wrote: > This is a bug in the underlying capturer. DCHECK() would be more appropriate Done. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:114: for (int i = 0; i < frame.size().width() * frame.size().height(); i += 1000) { On 2017/03/01 22:18:32, Sergey Ulanov wrote: > Hm. I think this filter can still be problematic in some cases as it verifies > only limited portion of the screen when width is close to 1000. Something like > this may work better: > y = i / W > x = (i + y * P) % W > Here P is some arbitrary number. I think a prime number under 100 would work > well, e.g. 31. > WDYT? > > Also did you verify if this gives significant performance boost? Since we only > need to check a few first frames it may be appropriate to verify the whole > frame. According to DesktopCapturerDifferWrapperTest, it takes ~10ms to compare two full frames. But BlankDetectorDesktopCapturerWrapper takes only 0.05ms. I think a simpler choice is to decrease the 1000 to a composite number ~100. i.e. Usually it's not divisible by a screen width to avoid we fall back into a vertical line. 96, 105 or 108 could be good choice. It will take around 0.5ms, but still cover a very large range of the screen. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:122: return IsBlankPixel(frame, frame.size().width() >> 1, On 2017/03/01 22:18:32, Sergey Ulanov wrote: > Add a comment that we are verifying the pixel in the center as well. Also > replace >> 1 with / 2. It's more readable and the compiler should know to > optimize it with shift. Done. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:131: return RgbaColor(pixel_data) == blank_pixel_; On 2017/03/01 22:18:32, Sergey Ulanov wrote: > Do we want to verify the alpha byte? Is it guaranteed to be set? > It may work with DX capturer, but the behavior is not consistent across other > capturers (IIRC some set it to 0xff, others return 0). Yes, so the constructor of BlankDetectorDesktopCapturerWrapper has a parameter to define the "blank_pixel". It's a platform dependent value. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:135: return permanent_failure_threshold_ >= 0; On 2017/03/01 22:18:32, Sergey Ulanov wrote: > It's not documented in the header that permanent_failure_threshold=0 allows to > disable permanent failure. Do we actually need this feature? If we do then > please add a comment about it in the header. Done. https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2709523003/diff/100001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h:17: #include "webrtc/modules/desktop_capture/rgba_color.h" On 2017/03/01 22:18:32, Sergey Ulanov wrote: > Comments for RgbaColor say that it's for tests only and that was my > understanding as well. Do we really need to use it here? This class could just > detect black frames, I don't think we need to pass blank_pixel as a parameter. AFAICT, on Linux, the blank pixel is 0xff, and on Windows 8, it's 0x00, on Windows 10, it's 0xff. But DX APIs return 0x00. So it's easy for us to use different RgbaColors for different platforms.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #6 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but please see my comments https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:111: // We will check 768 pixels for a frame with 1024 x 768 resolution. update this comment https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h:36: // every frame, and never return permanent error. Do we actually need this feature? It adds complexity, but I don't see a case when it can be useful. https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h:38: RgbaColor blank_pixel, Do we need this parameter? Will we even need to pass anything other then 0?
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/2709523003/#ps180001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488504967848420,
"parent_rev": "4285b698fb15ba60ac3a067a02e7254ee9994d63", "commit_rev":
"c4e9d210b3516c7b2faa32f24409a2e626599255"}
Message was sent while issue was closed.
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 500 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 500 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24...
Message was sent while issue was closed.
https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.cc:111: // We will check 768 pixels for a frame with 1024 x 768 resolution. On 2017/03/02 23:45:51, Sergey Ulanov wrote: > update this comment Done. https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h:36: // every frame, and never return permanent error. On 2017/03/02 23:45:51, Sergey Ulanov wrote: > Do we actually need this feature? It adds complexity, but I don't see a case > when it can be useful. I am OK to remove the permanent_failure_threshold. Once we need it, an individual capturer wrapper can help to reduce the complexity. https://codereview.webrtc.org/2709523003/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/blank_detector_desktop_capturer_wrapper.h:38: RgbaColor blank_pixel, On 2017/03/02 23:45:51, Sergey Ulanov wrote: > Do we need this parameter? Will we even need to pass anything other then 0? If we use this class with GDI capturer on Windows 10, we need to provide 0xff instead of 0x00. So it looks like a useful parameter. Unlike permanent_failure_threshold, it does not make the logic complex, we always need to compare the pixel with a color. So I would prefer to keep it.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:180001) has been created in https://codereview.webrtc.org/2726983005/ by perkj@webrtc.org. The reason for reverting is: Misses deps for RTC_HISTOGRAM in Chrome. email sent separately. Also see https://codereview.chromium.org/2725143004/..
Message was sent while issue was closed.
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 500 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 500 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... ==========
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 500 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 10000 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not significant impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... ==========
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/2709523003/#ps200001 (title: "Remove HISTOGRAM")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488580604477700,
"parent_rev": "d3501adf17fda4ad270faee89b6fe4f4a496b147", "commit_rev":
"ccf57a71eb2a550744367a4cbec285d85494c3c2"}
Message was sent while issue was closed.
Description was changed from ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 10000 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not significant impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... ========== to ========== BlankDetectorDesktopCapturerWrapper to detect a blank DesktopFrame DXGI capturer highly depends on video adapter and its driver, as well as Windows itself. I recently found it cannot work on my virtualbox instance any more, which indicates it may not work well on some specific systems. What worse is, the APIs do not return a failure in such case. So this change adds a BlankDetectorDesktopCapturerWrapper, which samples several pixels in the frame returned by a DesktopCapturer implementation. If all the pixels selected are blank, this wrapper returns a failure. A typical usage is to combine BlankDetectorDesktopCapturerWrapper with FallbackDesktopCapturerWrapper, and use GDI capturer in case of failure. Usually less than 10000 pixels are checked, so the BlankDetectorDesktopCapturerWrapper should not significant impact the capturer performance. This change is expected to resolve bug 682112 in another dimension. BUG=682112 Review-Url: https://codereview.webrtc.org/2709523003 Cr-Original-Commit-Position: refs/heads/master@{#16984} Committed: https://chromium.googlesource.com/external/webrtc/+/c4e9d210b3516c7b2faa32f24... Review-Url: https://codereview.webrtc.org/2709523003 Cr-Commit-Position: refs/heads/master@{#17024} Committed: https://chromium.googlesource.com/external/webrtc/+/ccf57a71eb2a550744367a4cb... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/ccf57a71eb2a550744367a4cb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
