Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973853002:
desktop_capture: crop border in window_capture on Win8/10 (Closed)
Descriptiondesktop_capture: crop border in window_capture on Win8/10
On Windows8/10, we prefer cropping desired window out from a whole screen
capture due to some reasons. The problem is Win10 has an invisible border
around the window. If we leave the border, it will expose background a bit.
This cl is about to always remove the border of desired window on Win8/10.
This will help a lot to capturing still windows during window sharing.
This cl still can't handle the background exposure issue when you move the
target window around during capturing. More investigation is needed.
BUG=chromium:737278
Review-Url: https://codereview.webrtc.org/2973853002
Cr-Commit-Position: refs/heads/master@{#18921}
Committed: https://chromium.googlesource.com/external/webrtc/+/4a494ffd1255561ee9c17d3d2b3646f8c849e784
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #Messages
Total messages: 31 (21 generated)
The CQ bit was checked by braveyao@webrtc.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.
braveyao@webrtc.org changed reviewers: + sergeyu@chromium.org
Hi sergeyu@, please take a look.
sergeyu@google.com changed reviewers: + sergeyu@google.com
https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right): https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:12: #include "webrtc/base/win32.h" I think you want to include webrtc/rtc_base/win32.h . webrtc/base/win32.h is deprecated https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:12: #include "webrtc/base/win32.h" add empty line here. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:39: rect.top, Here we don't crop the top border. Is that the behavior we want on Win8?
https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right): https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:33: if (rtc::IsWindows8OrLater() || Please add a comment here to explain why we want to crop the border only after win8
The CQ bit was checked by braveyao@webrtc.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_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
Thanks for the comments! All addressed/answered. PTAL. https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right): https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:12: #include "webrtc/base/win32.h" On 2017/07/06 20:35:49, Do not use (sergeyu) wrote: > add empty line here. > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:12: #include "webrtc/base/win32.h" On 2017/07/06 20:35:49, Do not use (sergeyu) wrote: > I think you want to include webrtc/rtc_base/win32.h . webrtc/base/win32.h is > deprecated Done. https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:33: if (rtc::IsWindows8OrLater() || On 2017/07/06 20:36:37, Do not use (sergeyu) wrote: > Please add a comment here to explain why we want to crop the border only after > win8 Done. https://codereview.webrtc.org/2973853002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/window_capture_utils.cc:39: rect.top, On 2017/07/06 20:35:49, Do not use (sergeyu) wrote: > Here we don't crop the top border. Is that the behavior we want on Win8? Yes. The top doesn't have that border.
lgtm
Description was changed from ========== desktop_capture: crop border in window_capture on Win8/10 On Windows8/10, we prefer cropping desired window out from a whole screen capture due to some reasons. The problem is Win10 has an invisible border around the window. If we leave the border, it will expose background a bit. This cl is about to always remove the border of desired window on Win8/10. This will help a lot to capturing still windows during window sharing. This cl still can't handle the background exposure issue when you move the target window around during capturing. More investigation is needed. BUG=chromium:737278 ========== to ========== desktop_capture: crop border in window_capture on Win8/10 On Windows8/10, we prefer cropping desired window out from a whole screen capture due to some reasons. The problem is Win10 has an invisible border around the window. If we leave the border, it will expose background a bit. This cl is about to always remove the border of desired window on Win8/10. This will help a lot to capturing still windows during window sharing. This cl still can't handle the background exposure issue when you move the target window around during capturing. More investigation is needed. BUG=chromium:737278 ==========
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
On 2017/07/06 22:47:08, Do not use (sergeyu) wrote: > lgtm lgtm
The CQ bit was checked by braveyao@webrtc.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
Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
The CQ bit was checked by braveyao@webrtc.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 braveyao@webrtc.org
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": 20001, "attempt_start_ts": 1499397462966650,
"parent_rev": "f07e6b4c00e0f00eb59a6a968db0d9b785dd563c", "commit_rev":
"4a494ffd1255561ee9c17d3d2b3646f8c849e784"}
Message was sent while issue was closed.
Description was changed from ========== desktop_capture: crop border in window_capture on Win8/10 On Windows8/10, we prefer cropping desired window out from a whole screen capture due to some reasons. The problem is Win10 has an invisible border around the window. If we leave the border, it will expose background a bit. This cl is about to always remove the border of desired window on Win8/10. This will help a lot to capturing still windows during window sharing. This cl still can't handle the background exposure issue when you move the target window around during capturing. More investigation is needed. BUG=chromium:737278 ========== to ========== desktop_capture: crop border in window_capture on Win8/10 On Windows8/10, we prefer cropping desired window out from a whole screen capture due to some reasons. The problem is Win10 has an invisible border around the window. If we leave the border, it will expose background a bit. This cl is about to always remove the border of desired window on Win8/10. This will help a lot to capturing still windows during window sharing. This cl still can't handle the background exposure issue when you move the target window around during capturing. More investigation is needed. BUG=chromium:737278 Review-Url: https://codereview.webrtc.org/2973853002 Cr-Commit-Position: refs/heads/master@{#18921} Committed: https://chromium.googlesource.com/external/webrtc/+/4a494ffd1255561ee9c17d3d2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/4a494ffd1255561ee9c17d3d2... |
