|
|
DescriptionUse PrintWindow() to implement snapshots on windows 8.1+
This allows us to capture what DirectComposition is actually displaying
and make GPU integration tests able to detect more possible problems.
The existing Aura capture path is used on Windows versions before 8.1,
because they don't support PW_RENDERFULLCONTENT and then PrintWindow may
not get the correct content.
NativeDesktopMediaList::CaptureAuraWindowThumbnail also will use the
aura path, because it's used in production for webrtc and needs to be
efficient.
BUG=702023
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2752373002
Cr-Commit-Position: refs/heads/master@{#462631}
Committed: https://chromium.googlesource.com/chromium/src/+/0c2c9631a434330c2982f0062a159f167879e2e8
Patch Set 1 #Patch Set 2 : Fix BUILD.gn #Patch Set 3 : fix high-dpi #Patch Set 4 : fix osmesa #Patch Set 5 : fix aura build #Patch Set 6 : use aura for win7 #Patch Set 7 : cleanup #Patch Set 8 : cleanup #
Total comments: 5
Patch Set 9 : post-review changes #Patch Set 10 : fix build #
Total comments: 2
Patch Set 11 : rename variables #Patch Set 12 : rebase #
Messages
Total messages: 76 (64 generated)
The CQ bit was checked by jbauman@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...
Description was changed from ========== Use PrintWindow() to implement snapshots on windows. This guarantees that we get what the OS actually displays. BUG= ========== to ========== Use PrintWindow() to implement snapshots on windows. This guarantees that we get what the OS actually displays. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by jbauman@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_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by jbauman@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_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jbauman@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbauman@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbauman@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_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jbauman@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/...)
The CQ bit was checked by jbauman@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.
The CQ bit was checked by jbauman@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Use PrintWindow() to implement snapshots on windows. This guarantees that we get what the OS actually displays. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Use PrintWindow() to implement snapshots on windows 8.1+ This allows us to capture what DirectComposition is actually displaying and make GPU integration tests able to detect more possible problems. The existing Aura capture path is used on Windows versions before 8.1, before they don't support PW_RENDERFULLCONTENT and then PrintWindow may not get the correct content. NativeDesktopMediaList::CaptureAuraWindowThumbnail also will use the aura path, because it's used in production for webrtc and needs to be efficient. BUG=702023 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Use PrintWindow() to implement snapshots on windows 8.1+ This allows us to capture what DirectComposition is actually displaying and make GPU integration tests able to detect more possible problems. The existing Aura capture path is used on Windows versions before 8.1, before they don't support PW_RENDERFULLCONTENT and then PrintWindow may not get the correct content. NativeDesktopMediaList::CaptureAuraWindowThumbnail also will use the aura path, because it's used in production for webrtc and needs to be efficient. BUG=702023 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Use PrintWindow() to implement snapshots on windows 8.1+ This allows us to capture what DirectComposition is actually displaying and make GPU integration tests able to detect more possible problems. The existing Aura capture path is used on Windows versions before 8.1, because they don't support PW_RENDERFULLCONTENT and then PrintWindow may not get the correct content. NativeDesktopMediaList::CaptureAuraWindowThumbnail also will use the aura path, because it's used in production for webrtc and needs to be efficient. BUG=702023 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
jbauman@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org changed reviewers: + ananta@chromium.org
+ananta for snapshot_win.cc I'm rather nervous about using an undocumented flag. Seems easy for us to get burned in the future. https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/BUILD.gn File ui/snapshot/BUILD.gn (right): https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/BUILD.gn#n... ui/snapshot/BUILD.gn:18: "snapshot_aura.cc", add snapshot_aura.h. https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_aura.cc (right): https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.cc:30: bool GrabWindowSnapshotAura(gfx::NativeWindow window, If we are adding aura specific functions, what is the point in having a function that always return false? https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... File ui/snapshot/snapshot_aura.h (right): https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.h:15: SNAPSHOT_EXPORT bool GrabWindowSnapshotAura(gfx::NativeWindow window, As these functions are only for aura can these take an aura::Window* instead of gfx::NativeView/Window? Also, can there be just one given NativeView and NativeWindow are the same for aura? https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... ui/snapshot/snapshot_aura.h:16: const gfx::Rect& snapshot_bounds, Are the bounds in these functions pixels or dips? https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_w... File ui/snapshot/snapshot_win.h (right): https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_w... ui/snapshot/snapshot_win.h:22: // is available (ie. tests). DO NOT use in a result of user action. Can you put this file in a target that is not part of snapshot and is internal for tests and the snapshot target? If you do that gn check should fail if someone tries to use this outside of that environment.
The CQ bit was checked by jbauman@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: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by jbauman@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_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PTAL On 2017/03/23 03:29:14, sky wrote: > +ananta for snapshot_win.cc > > I'm rather nervous about using an undocumented flag. Seems easy for us to get > burned in the future. > Yeah, I'd prefer not to as well, but this is the only way to be sure that what we're presenting to the screen (or at least DWM) is correct. Without this there's a big gap in our testing. We should be able to switch back in the future if there's a problem. > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/BUILD.gn > File ui/snapshot/BUILD.gn (right): > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/BUILD.gn#n... > ui/snapshot/BUILD.gn:18: "snapshot_aura.cc", > add snapshot_aura.h. Done. > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... > File ui/snapshot/snapshot_aura.cc (right): > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... > ui/snapshot/snapshot_aura.cc:30: bool GrabWindowSnapshotAura(gfx::NativeWindow > window, > If we are adding aura specific functions, what is the point in having a function > that always return false? I liked the consistency with the normal snapshot functions, but it is a bit silly. Removed. > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... > File ui/snapshot/snapshot_aura.h (right): > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... > ui/snapshot/snapshot_aura.h:15: SNAPSHOT_EXPORT bool > GrabWindowSnapshotAura(gfx::NativeWindow window, > As these functions are only for aura can these take an aura::Window* instead of > gfx::NativeView/Window? Also, can there be just one given NativeView and > NativeWindow are the same for aura? Done. > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_a... > ui/snapshot/snapshot_aura.h:16: const gfx::Rect& snapshot_bounds, > Are the bounds in these functions pixels or dips? DIPs actually. Added a comment about that. > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_w... > File ui/snapshot/snapshot_win.h (right): > > https://codereview.chromium.org/2752373002/diff/140001/ui/snapshot/snapshot_w... > ui/snapshot/snapshot_win.h:22: // is available (ie. tests). DO NOT use in a > result of user action. > Can you put this file in a target that is not part of snapshot and is internal > for tests and the snapshot target? If you do that gn check should fail if > someone tries to use this outside of that environment. Done (I think).
https://codereview.chromium.org/2752373002/diff/180001/ui/snapshot/snapshot_w... File ui/snapshot/snapshot_win.cc (right): https://codereview.chromium.org/2752373002/diff/180001/ui/snapshot/snapshot_w... ui/snapshot/snapshot_win.cc:36: const gfx::Rect& snapshot_bounds, Please name these in pixels. https://codereview.chromium.org/2752373002/diff/180001/ui/snapshot/snapshot_w... ui/snapshot/snapshot_win.cc:101: window_bounds.Intersect(host->window()->bounds()); Can you get the bounds directly from the hwnd? I'm concerned the way you have it now that you might end up with a size that is bigger than the size of the hwnd (because of enclosing rect below).
The CQ bit was checked by jbauman@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...
On 2017/03/23 23:20:33, sky wrote: > https://codereview.chromium.org/2752373002/diff/180001/ui/snapshot/snapshot_w... > File ui/snapshot/snapshot_win.cc (right): > > https://codereview.chromium.org/2752373002/diff/180001/ui/snapshot/snapshot_w... > ui/snapshot/snapshot_win.cc:36: const gfx::Rect& snapshot_bounds, > Please name these in pixels. Done. > > https://codereview.chromium.org/2752373002/diff/180001/ui/snapshot/snapshot_w... > ui/snapshot/snapshot_win.cc:101: > window_bounds.Intersect(host->window()->bounds()); > Can you get the bounds directly from the hwnd? I'm concerned the way you have it > now that you might end up with a size that is bigger than the size of the hwnd > (because of enclosing rect below). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
LGTM
ananta, does the GrabHwndSnapshot part seem ok?
The CQ bit was checked by jbauman@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 jbauman@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.
jbauman@chromium.org changed reviewers: + piman@chromium.org
piman: content/ OWNERS review.
LGTM
lgtm
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2752373002/#ps210001 (title: "rebase")
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": 210001, "attempt_start_ts": 1491512725819240, "parent_rev": "f645439ea16d03a4c70722d72f611a85eeea5ffd", "commit_rev": "0c2c9631a434330c2982f0062a159f167879e2e8"}
Message was sent while issue was closed.
Description was changed from ========== Use PrintWindow() to implement snapshots on windows 8.1+ This allows us to capture what DirectComposition is actually displaying and make GPU integration tests able to detect more possible problems. The existing Aura capture path is used on Windows versions before 8.1, because they don't support PW_RENDERFULLCONTENT and then PrintWindow may not get the correct content. NativeDesktopMediaList::CaptureAuraWindowThumbnail also will use the aura path, because it's used in production for webrtc and needs to be efficient. BUG=702023 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Use PrintWindow() to implement snapshots on windows 8.1+ This allows us to capture what DirectComposition is actually displaying and make GPU integration tests able to detect more possible problems. The existing Aura capture path is used on Windows versions before 8.1, because they don't support PW_RENDERFULLCONTENT and then PrintWindow may not get the correct content. NativeDesktopMediaList::CaptureAuraWindowThumbnail also will use the aura path, because it's used in production for webrtc and needs to be efficient. BUG=702023 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2752373002 Cr-Commit-Position: refs/heads/master@{#462631} Committed: https://chromium.googlesource.com/chromium/src/+/0c2c9631a434330c2982f0062a15... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001) as https://chromium.googlesource.com/chromium/src/+/0c2c9631a434330c2982f0062a15... |