967532 - DrawTargetCG / CoreGraphics does not support ExtendMode::CLAMP, leading to seams when fractional background-size is used

Status: NEW

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

There's a testcase in bug 878023 (attachment 761905 [details], only works in non-HiDPI) that shows when this hurts us: When we have a no-repeat CSS backgorund with a fractional background-size, we snap the fill rect but leave the dest rect fractional. We expect rendering to fill all pixels of the fill rect, but on Mac, that doesn't happen and leaves seams. ExtendMode::CLAMP is supposed to avoid that, but CoreGraphics does not support it.

Bug 878023 comment 14 has instructions for reproducing the bug with the tabs in the browser UI that still work as of mozilla-central revision f550b112a19b.

Comment 1

[Justin Dolske [:Dolske]]

(Not tracking this one, since we're tracking the bug it blocks.)

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

According to my investigation, it might not be a bug in DrawTargetCG. This bug occurs because a non-snapped transform matrix is passed in. I don't know why other platforms perform a different way, but I think the DrawTargetCG's behavior is reasonable.

I have tracked the calling flow for the testcase, and found this should be a bug of ComputeSnappedImageDrawingParameters in nsLayoutUtils.cpp. In this case, DrawImageInternal calls this function with a fractional aDest and aFill, and the function returns a snapped fill rect with a transform matrix based on the non-snapped destination. The difference between the rect and the matrix finally causes this problem.

However, ComputeSnappedImageDrawingParameters seems to be a complex function and I currently do not have enough time to analyse this function. Hence, I present my result here, and hopefully someone could fix it.

David, I suppose you are familiar with that function.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not familiar with that function, but seth is.  (Sorry, should have transferred this sooner.)

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

It is a testcase to show the intrinsic problem in ComputeSnappedImageDrawingParameters.

In this testcase, the background image is 10x10px, consists of a pure black border with pure white filling. As it is rendered in a 10x10 block, the result image should be exactly identical to the original image. However, either in Windows or in Mac, Firefox does not do so. There might be a light grey line beside the left edge, and the right edge is never pure black.

In Mac, both Safari and Chrome generate the expected result, while in Windows, IE has the same result as Firefox, which, IMO, is not optimal.

Although it seems that problem described in this bug is not occurred in Windows or other platforms, I think it is because the CLAMP mode hides this intrinsic problem.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

try server: https://tbpl.mozilla.org/?tree=Try&rev=be3f319aac70

It seems that this patch passes all the reftests, and even accidentally fixes bug 446100 on Android platform. (The TEST-UNEXPECTED-PASSs)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I will add some tests later if the patch gets granted.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8455075 [details] [diff] [review]
patch

Does the patch from bug 1023190 also fix this?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Markus Stange [:mstange] from comment #7)
> Comment on attachment 8455075 [details] [diff] [review]
> patch
> 
> Does the patch from bug 1023190 also fix this?

That might work, but I wonder whether that patch is able to pass reftests, especially those related to zooming. In fact, I tried a very similar solution before, and it failed to pass a lot of tests, hence I didn't submit it.

My previous try is here: https://tbpl.mozilla.org/?tree=Try&rev=107b17cd26a9

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8455075 [details] [diff] [review]
patch

Review of attachment 8455075 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4996,5 @@
> +  if (didSnap) {
> +    aCtx->UserToDevicePixelSnapped(dest, false);
> +  }
> +  gfxFloat scaleX = imageSize.width/dest.width;
> +  gfxFloat scaleY = imageSize.height/dest.height;

I don't think this is correct, unfortunately.

For example, if the dest-rect is 3.5 pixels wide and we're tiling the image, this will make the tiles be 3 or 4 pixels wide which will radically change the rendering when there are a lot of tiles.

Many of the comments in bug 1023190 apply here too. Especially the URLs I provided there for background reading.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 8455075 [details] [diff] [review]
> patch
> 
> Review of attachment 8455075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +4996,5 @@
> > +  if (didSnap) {
> > +    aCtx->UserToDevicePixelSnapped(dest, false);
> > +  }
> > +  gfxFloat scaleX = imageSize.width/dest.width;
> > +  gfxFloat scaleY = imageSize.height/dest.height;
> 
> I don't think this is correct, unfortunately.
> 
> For example, if the dest-rect is 3.5 pixels wide and we're tiling the image,
> this will make the tiles be 3 or 4 pixels wide which will radically change
> the rendering when there are a lot of tiles.

Right, but I think it is reasonable to snap them here as well. Is there any specification says that we must not snap the image to device pixel when tiling? I don't think it is the preferred effect for authors to have non-snapped image even if they specify a fractional tiling width. In fact, WebKit and Blink both snap images in this case.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase of tiling images

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan (UTC+10) from comment #10)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > Comment on attachment 8455075 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8455075 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/base/nsLayoutUtils.cpp
> > @@ +4996,5 @@
> > > +  if (didSnap) {
> > > +    aCtx->UserToDevicePixelSnapped(dest, false);
> > > +  }
> > > +  gfxFloat scaleX = imageSize.width/dest.width;
> > > +  gfxFloat scaleY = imageSize.height/dest.height;
> > 
> > I don't think this is correct, unfortunately.
> > 
> > For example, if the dest-rect is 3.5 pixels wide and we're tiling the image,
> > this will make the tiles be 3 or 4 pixels wide which will radically change
> > the rendering when there are a lot of tiles.
> 
> Right, but I think it is reasonable to snap them here as well. Is there any
> specification says that we must not snap the image to device pixel when
> tiling? I don't think it is the preferred effect for authors to have
> non-snapped image even if they specify a fractional tiling width. In fact,
> WebKit and Blink both snap images in this case.

Well, there is a consideration that snapping the images will lead to incontinuity on transition of background-size.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.