Summary: | Implement PlatformWebView::windowSnapshotImage and createBitmapContextFromWebView for iOS devices | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bdakin, buildbot, commit-queue, lforschler, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2017-03-09 10:12:12 PST
Created attachment 303931 [details]
Patch
Created attachment 304051 [details]
Patch
Comment on attachment 304051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304051&action=review > Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:60 > + CGSize viewSize = [[mainFrame webView] frame].size; You should use CARenderServerRenderLayerWithTransform into an IOSurface and make a CGImage from the IOSurface. > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:263 > + RetainPtr<CGImageRef> cgImage = adoptCF(CGBitmapContextCreateImage(context)); Here you can use CARenderServerRenderLayerWithTransform like we do in WKWebView (not the YUV part, but the rest) or maybe try the WK2 snapshotting API which just does that. renderInContext: is comparatively slow and imperfect. (In reply to comment #4) > Comment on attachment 304051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304051&action=review > > > Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:60 > > + CGSize viewSize = [[mainFrame webView] frame].size; > > You should use CARenderServerRenderLayerWithTransform into an IOSurface and > make a CGImage from the IOSurface. > > > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:263 > > + RetainPtr<CGImageRef> cgImage = adoptCF(CGBitmapContextCreateImage(context)); > > Here you can use CARenderServerRenderLayerWithTransform like we do in > WKWebView (not the YUV part, but the rest) or maybe try the WK2 snapshotting > API which just does that. renderInContext: is comparatively slow and > imperfect. If you use the WK2 snapshotting API you can probably remove the #if and just use one codepath (though WK2 will decide internally between a number of different options). Created attachment 304282 [details]
Patch
Comment on attachment 304282 [details] Patch Attachment 304282 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3309499 New failing tests: compositing/regions/position-layers-inside-region-overflow-hidden.html fast/harness/snapshot-captures-compositing.html compositing/regions/position-layer-inside-region-overflow-hidden.html compositing/hidpi-subpixel-transform-origin.html compositing/regions/position-layers-inside-regions-overflow-hidden.html compositing/geometry/clipped-out-perspective.html Created attachment 304291 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Something bad is happening with the software painting path. For now, just put the new WK2 code inside the #if USE(IOSURFACE) and put the sim-specific code back in the #else block in WKTR. Created attachment 304790 [details]
Patch
Comment on attachment 304790 [details] Patch Attachment 304790 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3350655 New failing tests: compositing/regions/position-layers-inside-region-overflow-hidden.html fast/harness/snapshot-captures-compositing.html compositing/regions/position-layer-inside-region-overflow-hidden.html compositing/hidpi-subpixel-transform-origin.html compositing/regions/position-layers-inside-regions-overflow-hidden.html compositing/geometry/clipped-out-perspective.html Created attachment 304801 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 306861 [details]
Patch
Created attachment 306870 [details]
Patch
Comment on attachment 306870 [details] Patch Attachment 306870 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3520867 New failing tests: compositing/regions/position-layers-inside-region-overflow-hidden.html fast/harness/snapshot-captures-compositing.html compositing/regions/position-layer-inside-region-overflow-hidden.html compositing/hidpi-subpixel-transform-origin.html compositing/regions/position-layers-inside-regions-overflow-hidden.html compositing/geometry/clipped-out-perspective.html Created attachment 306878 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #15) > Comment on attachment 306870 [details] > Patch > > Attachment 306870 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/3520867 > > New failing tests: > compositing/regions/position-layers-inside-region-overflow-hidden.html > fast/harness/snapshot-captures-compositing.html > compositing/regions/position-layer-inside-region-overflow-hidden.html > compositing/hidpi-subpixel-transform-origin.html > compositing/regions/position-layers-inside-regions-overflow-hidden.html > compositing/geometry/clipped-out-perspective.html Looking at these failures, this seems to be a bug in the WK2 snapshot code. Our reference images are consistent with what is shown on the screen of a device. Created attachment 306913 [details]
Patch
Created attachment 306991 [details]
Patch
Created attachment 309183 [details]
Patch
(In reply to Jonathan Bedard from comment #20) > Created attachment 309183 [details] > Patch Updating this patch so it is up-to-date with the repository Comment on attachment 309183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309183&action=review > LayoutTests/platform/ios/TestExpectations:2960 > +compositing/geometry/clipped-out-perspective.html [ ImageOnlyFailure ] > +compositing/hidpi-subpixel-transform-origin.html [ ImageOnlyFailure ] > +compositing/regions/position-layer-inside-region-overflow-hidden.html [ ImageOnlyFailure ] > +compositing/regions/position-layers-inside-region-overflow-hidden.html [ ImageOnlyFailure ] > +compositing/regions/position-layers-inside-regions-overflow-hidden.html [ ImageOnlyFailure ] You should talk to Simon about these. Otherwise, maybe split this into the completely non-contentious part (and use my review for that) and make a new patch for the other bit. Comment on attachment 309183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309183&action=review > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:266 > + [m_view > + takeSnapshotWithConfiguration:snapshotConfiguration.get() Don't split this here. >> LayoutTests/platform/ios/TestExpectations:2960 >> +compositing/regions/position-layers-inside-regions-overflow-hidden.html [ ImageOnlyFailure ] > > You should talk to Simon about these. > > Otherwise, maybe split this into the completely non-contentious part (and use my review for that) and make a new patch for the other bit. These need to go into a device-specific TE file. Created attachment 309220 [details]
Patch
Created attachment 309224 [details]
Patch
Comment on attachment 309224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309224&action=review > Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:84 > + RetainPtr<CGImageRef> cgImage = adoptCF(CGImageCreate(bufferWidth, bufferHeight, 8, 32, rowBytes, sRGBSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host, provider, 0, false, kCGRenderingIntentDefault)); I'm not sure this will do the right thing if WebCore::screenSupportsExtendedColor() above cause you to make a 10-bit surface. Here you're assuming it's sRGB, 8-bit. Does the snapshot work on a iPhone7 test? > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:273 > + completionHandler:^(UIImage* snapshotImage, NSError *error) { completionHandler: should go on the previous line. Comment on attachment 309224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309224&action=review >> Tools/DumpRenderTree/ios/PixelDumpSupportIOS.mm:84 >> + RetainPtr<CGImageRef> cgImage = adoptCF(CGImageCreate(bufferWidth, bufferHeight, 8, 32, rowBytes, sRGBSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host, provider, 0, false, kCGRenderingIntentDefault)); > > I'm not sure this will do the right thing if WebCore::screenSupportsExtendedColor() above cause you to make a 10-bit surface. Here you're assuming it's sRGB, 8-bit. Does the snapshot work on a iPhone7 test? Err, actually, could we use WebCore::IOSurface here? Or CGIOSurfaceContextCreateImage, like it does? Comment on attachment 309224 [details]
Patch
Making the changes you've both suggested (I think Tim's surface.createImage() renders Simon's comment about the 7 moot) but I need to rebuild to confirm them, so it's going to be a bit.
Created attachment 309248 [details]
Patch
Comment on attachment 309248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309248&action=review > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:269 > + [snapshotConfiguration setRect:NSMakeRect(0, 0, m_view.frame.size.width, m_view.frame.size.height)]; CGRectMake > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:272 > + [m_view takeSnapshotWithConfiguration:snapshotConfiguration.get() completionHandler:^(UIImage* snapshotImage, NSError *error) { Stars on the wrong side :) > Tools/WebKitTestRunner/ios/PlatformWebViewIOS.mm:278 > + while (!isDone) > + [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]]; Don't we have a wrapper for this? If not, it's fine. Created attachment 309417 [details]
Patch
Comment on attachment 309417 [details] Patch Clearing flags on attachment: 309417 Committed r216462: <http://trac.webkit.org/changeset/216462> All reviewed patches have been landed. Closing bug. |