Bug 169421 – Implement PlatformWebView::windowSnapshotImage and createBitmapContextFromWebView for iOS devices

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Currently, iOS devices do not have an implementation of PlatformWebView::windowSnapshotImage.  This needs to be implemented for running ImageDiff tests on iOS devices.

[Radar WebKit Bug Importer]

<rdar://problem/30950171>

[Jonathan Bedard]

Created attachment 303931 [details]
Patch

[Jonathan Bedard]

Created attachment 304051 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

(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).

[Jonathan Bedard]

Created attachment 304282 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

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.

[Jonathan Bedard]

Created attachment 304790 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Jonathan Bedard]

Created attachment 306861 [details]
Patch

[Jonathan Bedard]

Created attachment 306870 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 306913 [details]
Patch

[Jonathan Bedard]

Created attachment 306991 [details]
Patch

[Jonathan Bedard]

Created attachment 309183 [details]
Patch

[Jonathan Bedard]

(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

[Tim Horton]

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.

[Simon Fraser (smfr)]

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.

[Jonathan Bedard]

Created attachment 309220 [details]
Patch

[Jonathan Bedard]

Created attachment 309224 [details]
Patch

[Simon Fraser (smfr)]

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.

[Tim Horton]

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?

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 309248 [details]
Patch

[Tim Horton]

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.

[Jonathan Bedard]

Created attachment 309417 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 309417 [details]
Patch

Clearing flags on attachment: 309417

Committed r216462: <http://trac.webkit.org/changeset/216462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.