bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

NEW
Unassigned
(NeedInfo from)

Status

()

Core
Graphics
5 years ago
4 years ago

People

(Reporter: mstange, Unassigned, NeedInfo)

Tracking

Trunk
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P-])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Blocks: 878023
(Not tracking this one, since we're tracking the bug it blocks.)
Whiteboard: [Australis:P-]
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.
Flags: needinfo?(dbaron)
I'm not familiar with that function, but seth is.  (Sorry, should have transferred this sooner.)
Flags: needinfo?(dbaron) → needinfo?(seth)
Created attachment 8455004 [details]
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.
Created attachment 8455075 [details] [diff] [review]
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)
Attachment #8455075 - Flags: review?(mstange)
I will add some tests later if the patch gets granted.
(Reporter)

Comment 7

4 years ago
Comment on attachment 8455075 [details] [diff] [review]
patch

Does the patch from bug 1023190 also fix this?
(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
(Reporter)

Updated

4 years ago
Attachment #8455075 - Flags: review?(mstange) → review?(roc)
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.
Attachment #8455075 - Flags: review?(roc) → review-
(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.
Created attachment 8455239 [details]
testcase of tiling images
(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.
You need to log in before you can comment on or make changes to this bug.