1284797 - Small SVG as border image incorrectly stretches

Status: NEW

(Core :: SVG, defect, P3)

Description

[Sebastian Zartner [:sebo]]

When the size of an SVG image used as border image is small, it is incorrectly scaled up to the size of the border image area.

The reason for this is obviously that percentages used in 'border-image-slice' are computed to integers instead of floats.

Sebastian

Comment 1

[Sebastian Zartner [:sebo]]

Attachment: SVG 5 pixels

Comment 2

[Sebastian Zartner [:sebo]]

Attachment: SVG 60 pixels

Comment 3

[Sebastian Zartner [:sebo]]

Attachment: test case

This is a test case showing an incorrectly and a correctly stretched border image.

Sebastian

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Sebastian Zartner [:sebo] from comment #0)
> The reason for this is obviously that percentages used in
> 'border-image-slice' are computed to integers instead of floats.

Hmm, yeah -- I think our imagelib code (probably ClippedImage in particular, or something similar) creates a fake "image" for each corner, and images are expected to have pixel-valued intrinsic sizes. (e.g. you can't have a PNG image that's 50.5px wide)

To fix this thoroughly, we might need to change our imagelib APIs to relax that assumption...

Comment 5

[Daniel Holbert [:dholbert]]

(so the problem here is that we end up with an "image" (a ClippedImage) whose intrinsic size is 25% * 5px across, i.e. it's 1.25px by 1.25px. And under the hood, we round that up to 2px by 2px, which gets us an unexpectedly-large region of SVG space.)

Perhaps a simpler solution (compared to relaxing the imagelib assumption noted in comment 4): Allow ourselves to round the image's intrinsic size to pixels (so, 2px by 2px), BUT, keep track of the fact that we're *supposed to* be 1.25px by 1.25px.  And then accordingly scale down the region of SVG space that we're sampling, e.g. by applying an extra transform when painting.

Comment 6

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Regression in 48 from bug 619500.  Hoping these can ride the trains with 50 if we have a fix soon.

Comment 7

[Daniel Holbert [:dholbert]]

Nope, this bug here is not a regression from bug 619500.

Firefox 47 (current release, which lacks bug 619500's patch) and Firefox Nightly (50) render the testcase here exactly the same, for me.  In other words: this bug is older than Firefox 47.

(Note that Firefox 48 beta does render the testcase differently from Firefox47/Nightly50, but that's not due to this bug here -- that's due to bug 1264809, I think.)

--> Reverting "regression" keyword, and adding "affected" keywords for 47 and 50. (48 and 49 are ambiguously affected, since their rendering is separately-broken by bug 1264809.)

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-WIP.patch

Hi Daniel,

In this patch, I added a member variable "mGfxClip" in ClippedImage to keep real size we want to clip and scaled the source image to this size when calling the "draw" function.

However, in this way, the scale size we pass to the innerImage's "draw"[1] function will be float instead of integer, and the idl doesn't support overload function so that we add another "draw" function using float as parameter; therefore, I changed the type of aSize[2] in the "draw" function from nsIntSize to gfxRect.

I am not sure if it is okay to make this change because it will change some codes, Can you give me some advices?

Thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/image/ClippedImage.cpp#470
[2] https://dxr.mozilla.org/mozilla-central/source/image/imgIContainer.idl#392

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #8)
> I am not sure if it is okay to make this change because it will change some
> codes, Can you give me some advices?

It's not OK to make that change. It would not be meaningful to support floating point sizes for raster images, and it's not normally meaningful for SVG-as-image either - just in this particular corner case.

I think that's the wrong route anyway, though. It's probably going to be hard to solve this problem using the ClippedImage approach that we use right now. The good news is that I don't think that approach is very good anyway for other reasons. ClippedImage exists in part to solve a performance issue with the graphics APIs we used prior to Moz2D, and the caching and so forth it does are actually counterproductive now. Instead of trying to force this use-case to work with the ImageLib API, we should accept that it's a mismatch, stop using ClippedImage for border-image, and solve this problem at a different layer. (Probably nsImageRenderer in nsCSSRendering.cpp.)

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

What if I add another draw function which takes gfxSize as type of aSize in class VectorImage, and if the inner image of the ClippedImage is a vector image and need to dealing with the floating point image size case, just static casting the image container to VectorImage and call the new draw function, in this way, we don't have to change the base class and won't effect other code flows. Does this solution work for you ?

I think I can also try to remove ClippedImage from border-image either, but I am not very sure about why it is counterproductive ? Can you state it more specific ? Thank you.

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #10)
> What if I add another draw function which takes gfxSize as type of aSize in
> class VectorImage, and if the inner image of the ClippedImage is a vector
> image and need to dealing with the floating point image size case, just
> static casting the image container to VectorImage and call the new draw
> function, in this way, we don't have to change the base class and won't
> effect other code flows. Does this solution work for you ?

No, definitely not. This is going to be a maintenance nightmare. What you're doing is specific to border-image, so implement your changes in border-image code.

> I think I can also try to remove ClippedImage from border-image either, but
> I am not very sure about why it is counterproductive ? Can you state it more
> specific ? Thank you.

It's not really relevant to this bug, but the idea of ClippedImage was to let us cache pre-clipped versions of the image so that (1) we would not need to keep a decoded version of the whole image in memory, and (2) we could avoid the need to copy the image deeper in the pipeline to avoid oversampling and the like. ClippedImage was implemented primarily to support media fragments, and it was implemented when both ImageLib and the graphics pipeline worked much differently than they do today.

(1) is irrelevant for border-image because border-image uses the entire image.
(2) is irrelevant now in all cases, because Moz2D has support for source clipping, which is the graphics API feature we used to emulate with an expensive copy. This lets us avoid oversampling issues when drawing a small region of a larger texture, without needing to copy that small region to a separate texture.

For media fragments, even, (1) isn't relevant anymore, because now we can support multiple decoded versions of the same image via the SurfaceCache, which is a much more efficient way to go about things. When ClippedImage was first implemented, there could only be one decoded version of a given RasterImage at a time.

Overall, ClippedImage is useless in all cases, and we want to remove it. It would be a good first step in that direction if you reworked the border-image code to not use it. Please do that instead of adding hacks to ClippedImage and VectorImage. You'll be implementing this stuff at the layer where it actually makes sense, you'll be improving the quality of our code, and you'll have my undying gratitude for getting us closer to being able to rip ClippedImage out.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Thanks for working on this, btw, Kevin. I think we're going to end up with much nicer code in this area because of your work. This stuff has needed some love for some time.

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

I'm not actively working on this.