|
|
DescriptionAdd fromSurface optional parameter to devtools Page.CaptureScreenshot
This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency.
BUG=687407
Review-Url: https://codereview.chromium.org/2732923002
Cr-Commit-Position: refs/heads/master@{#456309}
Committed: https://chromium.googlesource.com/chromium/src/+/b8f3abee06225b664de730b8cd2f7e61897f80ee
Patch Set 1 #Patch Set 2 : nit change method name #
Total comments: 6
Patch Set 3 : Added fromSurface optional parameter to Page.CaptureScreenshot #Patch Set 4 : Remove switches include #
Total comments: 11
Patch Set 5 : Use only one Screenshot callback #Patch Set 6 : Marked fromSurface parameter as experimental #
Total comments: 5
Patch Set 7 : Remove PendingSnapshotFromSurfaceMap #
Total comments: 2
Patch Set 8 : added from_surface parameter to GetSnapshotFromBrowser #
Total comments: 4
Patch Set 9 : nits #Messages
Total messages: 46 (30 generated)
The CQ bit was checked by dvallet@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 CopyFromSurface method in headless mode for CaptureScreenshot. This allows headless screenshots to work more consistently in all platforms and fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ========== to ========== Use CopyFromSurface method in headless mode for CaptureScreenshot. This allows headless screenshots to work more consistently in all platforms and fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ==========
dvallet@chromium.org changed reviewers: + eseckler@chromium.org, skyostil@chromium.org
PTAL, I've confirmed that this patch works on Mac, Windows, Linux. Currently I don't set up any snapshot bounds, do you think that it's needed for headless?
PTAL, I've confirmed that this patch works on Mac, Windows, Linux. Currently I don't set up any snapshot bounds, do you think that it's needed for headless?
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_...)
The CQ bit was checked by dvallet@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.
eseckler@chromium.org changed reviewers: + pfeldman@chromium.org
+pfeldman Looks good, thanks! Some comments below :) https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:90: std::string EncodeBitmap(const SkBitmap& bitmap, Don't think you need this specialized version of EncodeImage. You can do what ScreencastFrameCaptured does: base::Bind(&EncodeImage, gfx::Image::CreateFrom1xBitmap(bitmap), format, quality) https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2366: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { If we use the headless switch to determine how we do the capture and which callbacks we look for, can we add documentation/dchecks to GetSnapshotFromBrowser / GetBitmapSnapshotFromBrowser that says when either method is supported? Alternatively, maybe we can make both work on all platforms? E.g. by checking whether pending_browser_snapshots_ and/or pending_bitmap_browser_snapshots_ are non-empty and then taking the corresponding snapshot - or both.
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman is your guy!
Let's introduce an optional parameter "fromSurface" (defaulting to false) to the Page.captureScreenshot method and pass it all the way to RenderWidgetHostImpl. Since we are the only user of snapshotting, this should not affect anyone else. In the long term we'll see whether it's possible to always grab from surface and just remove snapshotting from the view. https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2366: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { We should not special-case headless here.
The CQ bit was checked by dvallet@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 CopyFromSurface method in headless mode for CaptureScreenshot. This allows headless screenshots to work more consistently in all platforms and fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ========== to ========== Add fromSurface optional parameter to devtools Page.CaptureScreenshot This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ==========
PTAL https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:90: std::string EncodeBitmap(const SkBitmap& bitmap, On 2017/03/06 07:32:53, Eric Seckler wrote: > Don't think you need this specialized version of EncodeImage. You can do what > ScreencastFrameCaptured does: > > base::Bind(&EncodeImage, gfx::Image::CreateFrom1xBitmap(bitmap), format, > quality) Done, thanks for the tip! https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2366: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { On 2017/03/06 18:45:34, dgozman wrote: > We should not special-case headless here. Done. https://codereview.chromium.org/2732923002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2366: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { On 2017/03/06 07:32:53, Eric Seckler wrote: > If we use the headless switch to determine how we do the capture and which > callbacks we look for, can we add documentation/dchecks to > GetSnapshotFromBrowser / GetBitmapSnapshotFromBrowser that says when either > method is supported? > > Alternatively, maybe we can make both work on all platforms? E.g. by checking > whether pending_browser_snapshots_ and/or pending_bitmap_browser_snapshots_ are > non-empty and then taking the corresponding snapshot - or both. Done. I opted to check both.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:612: base::Bind(&PageHandler::ScreenshotFromSurfaceEncoded, You can bind directly to callback: base::Bind(&CaptureScreenshotCallback::sendSuccess, base::Passed(std::move(callback))) https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2411: static const int kMaxRetries = 5; Why do we need retries? https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:215: void GetSurfaceSnapshotFromBrowser( Can we unify this with the other method by always returning SkBitmap or gfx::Image? https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": "fromSurface", "type": "boolean", "optional": true, "description": "Capture the screenshot from the surface, rather than the view. Defaults to false." } Please mark it "experimental": true https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ScreenCaptureModel.js (right): https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ScreenCaptureModel.js:46: * @param {boolean|undefined} fromSurface Let's remove this parameter, and always pass false below.
The CQ bit was checked by dvallet@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...
https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:612: base::Bind(&PageHandler::ScreenshotFromSurfaceEncoded, On 2017/03/08 19:21:36, dgozman wrote: > You can bind directly to callback: > > base::Bind(&CaptureScreenshotCallback::sendSuccess, > base::Passed(std::move(callback))) Done. https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2411: static const int kMaxRetries = 5; On 2017/03/08 19:21:36, dgozman wrote: > Why do we need retries? I copied this code from Eric's patch https://codereview.chromium.org/2592983002/#ps40001. I'm guessing it mimics https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h... https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:215: void GetSurfaceSnapshotFromBrowser( On 2017/03/08 19:21:36, dgozman wrote: > Can we unify this with the other method by always returning SkBitmap or > gfx::Image? Done. https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": "fromSurface", "type": "boolean", "optional": true, "description": "Capture the screenshot from the surface, rather than the view. Defaults to false." } On 2017/03/08 19:21:36, dgozman wrote: > Please mark it "experimental": true Done. https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ScreenCaptureModel.js (right): https://codereview.chromium.org/2732923002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ScreenCaptureModel.js:46: * @param {boolean|undefined} fromSurface On 2017/03/08 19:21:37, dgozman wrote: > Let's remove this parameter, and always pass false below. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Thanks! :) https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2411: static const int kMaxRetries = 5; On 2017/03/09 06:19:47, dvallet wrote: > On 2017/03/08 19:21:36, dgozman wrote: > > Why do we need retries? > > I copied this code from Eric's patch > https://codereview.chromium.org/2592983002/#ps40001. > > I'm guessing it mimics > https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h... Yup, and it also mimics https://cs.chromium.org/chromium/src/ui/snapshot/snapshot_aura.cc?l=53 It seems "common" that the copy may fail for some reason and retrying would likely help. I don't know enough about readbacks to say why it might fail, though. https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2428: it->second.Run(gfx::Image::CreateFrom1xBitmap(bitmap)); nit: create gfx::Image outside the loop. Maybe also explictly create and use an empty gfx::Image if response != READBACK_SUCCESS. https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:872: PendingSnapshotFromSurfaceMap pending_surface_browser_snapshots_; nit: could be PendingSnapshotMap now, too.
Thanks for the comments! https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2428: it->second.Run(gfx::Image::CreateFrom1xBitmap(bitmap)); On 2017/03/09 09:55:22, Eric Seckler wrote: > nit: create gfx::Image outside the loop. > > Maybe also explictly create and use an empty gfx::Image if response != > READBACK_SUCCESS. Done on outside loop image creation. Regarding empty image it says in https://cs.chromium.org/chromium/src/ui/gfx/image/image.h?rcl=e1ccd9677ef5e7b... that attempting to use an empty image will result in a crash, so I'm not sure if we want to return it in the error case. https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:872: PendingSnapshotFromSurfaceMap pending_surface_browser_snapshots_; On 2017/03/09 09:55:22, Eric Seckler wrote: > nit: could be PendingSnapshotMap now, too. Done.
Description was changed from ========== Add fromSurface optional parameter to devtools Page.CaptureScreenshot This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ========== to ========== Add fromSurface optional parameter to devtools Page.CaptureScreenshot This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ==========
Almost there! https://codereview.chromium.org/2732923002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:211: void GetSnapshotFromBrowser(const GetSnapshotFromBrowserCallback& callback); Let's just add |from_surface| parameter here and comment on surface vs. view difference above.
https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2428: it->second.Run(gfx::Image::CreateFrom1xBitmap(bitmap)); On 2017/03/09 22:43:52, dvallet wrote: > On 2017/03/09 09:55:22, Eric Seckler wrote: > > nit: create gfx::Image outside the loop. > > > > Maybe also explictly create and use an empty gfx::Image if response != > > READBACK_SUCCESS. > > Done on outside loop image creation. > Regarding empty image it says in > https://cs.chromium.org/chromium/src/ui/gfx/image/image.h?rcl=e1ccd9677ef5e7b... > that attempting to use an empty image will result in a crash, so I'm not sure if > we want to return it in the error case. I see that GetSnapshotFromBrowser does indicate that it can return an empty image. I'm not sure if SkBitmap is empty if response != READBACK_SUCCESS, so I'll add it for good measure. https://codereview.chromium.org/2732923002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:211: void GetSnapshotFromBrowser(const GetSnapshotFromBrowserCallback& callback); On 2017/03/09 23:17:36, dgozman wrote: > Let's just add |from_surface| parameter here and comment on surface vs. view > difference above. Done.
The CQ bit was checked by dvallet@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2402: gfx::Image image = gfx::Image(); nit: it's unnecessary to call default constructor: gfx::Image image; https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:207: // view (On MacOS, the snapshot is taken fromthe Cocoa view for end-to-end typo: fromthe
https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2402: gfx::Image image = gfx::Image(); On 2017/03/10 at 21:28:17, dgozman wrote: > nit: it's unnecessary to call default constructor: > gfx::Image image; Done https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:207: // view (On MacOS, the snapshot is taken fromthe Cocoa view for end-to-end On 2017/03/10 at 21:28:17, dgozman wrote: > typo: fromthe Done
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2732923002/#ps160001 (title: "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": 1489358213685850, "parent_rev": "a05466fbe6b8cac0df697f61e2387e3f536aa614", "commit_rev": "b8f3abee06225b664de730b8cd2f7e61897f80ee"}
Message was sent while issue was closed.
Description was changed from ========== Add fromSurface optional parameter to devtools Page.CaptureScreenshot This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 ========== to ========== Add fromSurface optional parameter to devtools Page.CaptureScreenshot This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 Review-Url: https://codereview.chromium.org/2732923002 Cr-Commit-Position: refs/heads/master@{#456309} Committed: https://chromium.googlesource.com/chromium/src/+/b8f3abee06225b664de730b8cd2f... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/b8f3abee06225b664de730b8cd2f... |