Bug 156790

Summary: Position of WebGL <canvas> on iOS is incorrect with CSS borders
Product: WebKit Reporter: Antoine Quint <graouts>
Component: WebGLAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, graouts, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=175437
Attachments:
Description Flags
Testcase for border
none
Test case for padding
none
Testcase for box-shadow
none
Patch
none
Patch
none
Patch none

Description Antoine Quint 2016-04-20 03:14:03 PDT
Created attachment 276814 [details]
Testcase for border

Adding a `border` CSS property on a <canvas> element does not correctly position the WebGL content of the canvas, showing it displaced compared to its background. This only happens on iOS and drawing into a WebGL context, OS X positions the content correctly and requesting a 2D context on iOS also draws the context in the right place.
Comment 1 Radar WebKit Bug Importer 2016-04-20 03:14:27 PDT
<rdar://problem/25826290>
Comment 2 Antoine Quint 2016-05-30 06:15:25 PDT
Per my testing, box-shadow and padding also cause problems.
Comment 3 Antoine Quint 2016-05-30 06:16:55 PDT
Created attachment 280091 [details]
Test case for padding
Comment 4 Antoine Quint 2016-05-30 06:17:31 PDT
Created attachment 280092 [details]
Testcase for box-shadow
Comment 5 Antoine Quint 2016-05-30 07:54:56 PDT
It looks like the device pixel ratio may have something to do with this. If we measure the distance from the border to the WebGL layer (dGL) and the distance from the border to the background (dBG) I can see that on a 2x display dBG is 2x dGL while on a 3x display it is 3x dGL. Alas, I can't test on a 1x iOS display, but my guess is that the bug would not reproduce there.
Comment 6 Antoine Quint 2016-05-30 08:39:09 PDT
On the Mac, the WebGLLayer has contentsScale = 2 while on iOS it has contentsScale = 1.
Comment 7 Antoine Quint 2016-05-30 10:01:39 PDT
So, the contentsScale is set to the device pixel ratio on Mac only in `-(id)initWithGraphicsContext3D:(GraphicsContext3D*)context`, while we set a transform with a scale set to the device pixel ratio on iOS only in `PlatformCALayerRemoteCustom::PlatformCALayerRemoteCustom()`.

Overriding `-(void)setPosition:(CGPoint)position` on WebGLLayer to account for the device pixel ratio fixes the issue, but I'm not sure this is the right fix.
Comment 8 Antoine Quint 2016-05-31 01:15:20 PDT
I think the root of the issue is that WKRemoteView applies a scale that is the inverse of the scale factor in - (instancetype)initWithFrame:(CGRect)frame contextID:(uint32_t)contextID hostingDeviceScaleFactor:(float)scaleFactor and this causes the position to be divided by scale factor, which explains the offset varying with the device pixel ratio seen on iOS with WebGL layer.
Comment 9 Antoine Quint 2016-05-31 01:41:43 PDT
Created attachment 280125 [details]
Patch
Comment 10 Antoine Quint 2016-05-31 05:21:31 PDT
I misread the code in RemoteLayerTreeHost::createLayer(), WKRemoteViews are created not just for WebGL layers but also custom layers and video layers.
Comment 11 Antoine Quint 2016-05-31 05:57:10 PDT
The changes were made as part of https://bugs.webkit.org/show_bug.cgi?id=136879.
Comment 12 Antoine Quint 2016-05-31 06:36:00 PDT
I was puzzled by why this didn't apply to <video> elements, and I found there was an ad-hoc fix for those in https://bugs.webkit.org/show_bug.cgi?id=144274 which introduces a container layer which does basically the same thing as my patch does, albeit more elegantly, by applying the transform when setting the position.
Comment 13 Antoine Quint 2016-05-31 06:44:45 PDT
Created attachment 280144 [details]
Patch
Comment 14 Antoine Quint 2016-05-31 07:13:08 PDT
This patch is kind of an ad-hoc fix, much like the one that was made for <video>. I wonder if we should introduce a CALayer subclass that we would add as the layer for WKRemoteView and which would apply the transform, which should let the WebGLLayer and WebVideoContainerLayer be positioned without any manual application of the transform to the CGPoint passed to -setPosition:.
Comment 15 Darin Adler 2016-05-31 10:13:56 PDT
Comment on attachment 280144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280144&action=review

I’m saying review+ but I am not sure this is the best fix.

> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185
> +- (void)setPosition:(CGPoint)position

Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method.

> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:192
> +    if (!CATransform3DIsIdentity(self.transform)) {
> +        // Account for the transform applied to the WKRemoteView hosting the WebGLLayer
> +        // which applies a scale that is the inverse of the device pixel ratio.
> +        // https://bugs.webkit.org/show_bug.cgi?id=156790
> +        position = CGPointApplyAffineTransform(position, CATransform3DGetAffineTransform(self.transform));
> +    }

Why do we need to check CATransform3DIsIdentity? Is this a performance optimization? Does it improve correctness?
Comment 16 Dean Jackson 2016-05-31 12:11:35 PDT
Comment on attachment 280144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280144&action=review

>> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185
>> +- (void)setPosition:(CGPoint)position
> 
> Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method.

I agree with Darin.

I'm also worried about a few things:
- If the transform changes, is setPosition always called? If not, the layer will be in the wrong place.
- This seems to be an issue in the WKRemoteView. Maybe it should special-case WebGLLayer? Why aren't other WKRemoteViews having the same problem?
- (Something I've been meaning to fix for a while) It's annoying that we have iOS-only code in platform/graphics/mac.
Comment 17 Antoine Quint 2016-05-31 15:34:10 PDT
(In reply to comment #16)
> Comment on attachment 280144 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280144&action=review
> 
> >> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185
> >> +- (void)setPosition:(CGPoint)position
> > 
> > Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method.
> 
> I agree with Darin.

I think I'll investigate adding a container layer for WKRemoteView which applies the inverse transform and let sublayers of that container be positioned, which would correctly position things with respect to the page scale factor.

> I'm also worried about a few things:
> - If the transform changes, is setPosition always called? If not, the layer
> will be in the wrong place.
> - This seems to be an issue in the WKRemoteView. Maybe it should
> special-case WebGLLayer? Why aren't other WKRemoteViews having the same
> problem?

They do, see my comment above about WebVideoContainerLayer.
Comment 18 Dean Jackson 2016-05-31 18:49:36 PDT
Comment on attachment 280144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280144&action=review

>>>> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185
>>>> +- (void)setPosition:(CGPoint)position
>>> 
>>> Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method.
>> 
>> I agree with Darin.
>> 
>> I'm also worried about a few things:
>> - If the transform changes, is setPosition always called? If not, the layer will be in the wrong place.
>> - This seems to be an issue in the WKRemoteView. Maybe it should special-case WebGLLayer? Why aren't other WKRemoteViews having the same problem?
>> - (Something I've been meaning to fix for a while) It's annoying that we have iOS-only code in platform/graphics/mac.
> 
> I think I'll investigate adding a container layer for WKRemoteView which applies the inverse transform and let sublayers of that container be positioned, which would correctly position things with respect to the page scale factor.

Sorry, I didn't notice your comment earlier on bugzilla.

Yeah, I like this idea. Hopefully that means we can be certain that transform and position are in sync.
Comment 19 Antoine Quint 2016-06-06 12:02:43 PDT
Created attachment 280614 [details]
Patch
Comment 20 WebKit Commit Bot 2016-06-06 13:56:54 PDT
Comment on attachment 280614 [details]
Patch

Clearing flags on attachment: 280614

Committed r201727: <http://trac.webkit.org/changeset/201727>
Comment 21 WebKit Commit Bot 2016-06-06 13:56:58 PDT
All reviewed patches have been landed.  Closing bug.