Closed Bug 1290782 Opened 8 years ago Closed 7 years ago

SVG border-image without viewport size and viewBox is broken when changing the opacity.

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kechen, Assigned: lochang)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 5 obsolete files)

59 bytes, text/x-review-board-request
dholbert
: review+
u459114
: review+
Details
636 bytes, text/html
Details
4.94 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
u459114
: review+
dholbert
: review+
Details
59 bytes, text/x-review-board-request
u459114
: review+
Details
User-Agent: firefox-49.0a2

STR:
 Open the test file in the attachment.

EXPECTED RESULTS:
 The border image should not be broken when the opacity changes.

ACTUAL RESULTS:
 When changing opacity from 1 to 0.5, the rendering border image will be broken at the first time and correct again after a while.
Attached file test case (obsolete) —
test case for this bug.
Attached file testcase 2 (obsolete) —
This case also happened when we do the scrolling.
Following link is the try result, no border-image-related errors :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb645ac8fb4
Comment on attachment 8779621 [details]
Bug 1290782 - Force using image size as SVG's viewport size when drawing the border amd the SVG doesn't have fixed ratio.

https://reviewboard.mozilla.org/r/70570/#review70458

(Sorry for the delayed review response here -- I've been trying to clear my review queue, and haven't entirely been processing it in first-in-first-out order, and this patch got stuck at the back without me noticing for a little while. Sorry about that! I promise to give a better review turnaround once you've posted a version with comments addressed.)

::: layout/base/nsLayoutUtils.cpp:6548
(Diff revision 1)
>    gfxSize imageSize(intImageSize.width, intImageSize.height);
>  
>    // XXX(seth): May be buggy; see bug 1151016.
> -  CSSIntSize svgViewportSize = currentMatrix.IsIdentity()
> +  CSSIntSize svgViewportSize =
> +    (currentMatrix.IsIdentity() ||
> +     (aImageFlags & imgIContainer::FLAG_FORCE_UNIFORM_SCALING))

Please add a comment to explain how we're deciding on the correct svgViewportSize here (now that that decision is getting a bit more complex).

::: layout/base/nsLayoutUtils.cpp:6550
(Diff revision 1)
>    // XXX(seth): May be buggy; see bug 1151016.
> -  CSSIntSize svgViewportSize = currentMatrix.IsIdentity()
> +  CSSIntSize svgViewportSize =
> +    (currentMatrix.IsIdentity() ||
> +     (aImageFlags & imgIContainer::FLAG_FORCE_UNIFORM_SCALING))
> -    ? CSSIntSize(intImageSize.width, intImageSize.height)
> +      ? CSSIntSize(intImageSize.width, intImageSize.height)
> -    : CSSIntSize::Truncate(devPixelDest.width, devPixelDest.height);
> +      : CSSIntSize::Truncate(devPixelDest.width, devPixelDest.height);

No need to add indentation to these 2 lines (as this patch is doing).  They're still at the same level of logic-nesting that they were before.

::: layout/reftests/border-image/svg-as-border-image-5.html:1
(Diff revision 1)
> +<html class="reftest-wait">

I'm uneasy with the setTimeout/scrolling reliance in this reftest -- in part because setTimeout-reliance is hacky/worth-avoiding, and in part because the test doesn't actually fail for me locally if I load it directly and scroll. (Nothing disappears for me if I load the testcase in Firefox & manually scroll. The border does disappear if I scroll *and then reload*, but not if I just scroll.)

Good news, though -- I think the main thing that both of your testcases are triggering here (when the failure happens) is *layerization*.  You can test that by itself (persistently) by simply setting "will-change: opacity" on the element.  So, I think you really want one or two static reftests here (static = no animations/scripting), based on either (or both) of the testcase you've attached on this bug.  The reftest would have "will-change: opacity" in the testcase, and the reference case would be identical except *without* that will-change declaration.  For me locally, that seems to reliably trigger the difference in rendering.

::: layout/reftests/border-image/svg-as-border-image-5.html:30
(Diff revision 1)
> +document.addEventListener('MozReftestInvalidate', scrollDown, false);
> +</script>
> +</head>
> +<body>
> +<div id="borderdiv"></div>
> +<br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/><br/>

(These <br> elements are unnecessarily long. It would be better to have <div style="height: 2000px"></div>.

But really, you can just get rid of all of these <br>'s, since you shouldn't be relying on scrolling/scripting here, as noted above.)
Attachment #8779621 - Flags: review?(dholbert)
One more nit here -- there's a typo in the commit message: 
> Bug 1290782 - Force using image size as SVG's viewport size when drawing the border amd the SVG doesn't have fixed ratio

s/amd/and/
Sorry I am working on other task currently. I will come back and continue to fix this bug in these days.
I've modified the patch according to comment 7; however, there are some rendering errors in the test case when using will-change property.

Still investigating the root cause.
Attachment #8779621 - Attachment is obsolete: true
Kevin, are you still working on this bug?
Priority: -- → P3
Un-assign me since I am working on other buga currently.
Assignee: kechen → nobody
Hi Daniel,

The rendering issue mentioned in comment 7 is subtle differences in some pixels.
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80f3b79c7ee513d93cc3dca1d96806e92d852323

Do you have any idea about the rendering issue? Or maybe we can just use fuzzy for the test?
Flags: needinfo?(dholbert)
(In reply to Louis Chang [:louis] from comment #14)
> The rendering issue mentioned in comment 7 is subtle differences in some
> pixels.

I think maybe you're talking about something different from Comment 7...?

In comment 7, the only reftest that I commented on was "svg-as-border-image-5.html", which was a reftest that had "reftest-wait" and setTimeout and a bunch of scrolling and <br>'s.

Whereas: the reftest in your Try run is called "svg-as-border-image-4b.html" and doesn't have any of those. (no reftest-wait, no setTimeout, no scrolling, no <br>'s

> Do you have any idea about the rendering issue? Or maybe we can just use
> fuzzy for the test?

Yeah, this is a case where fuzzy() is appropriate. (max difference: 2, number of differing pixels: 2, with the mismatching pixels on the very edge of a shape)

Possibly a layerization thing, or possibly a random edge-case thing -- and since it's very minimal and imperceptible, it's not really worth worrying about too much.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #15) 
> I think maybe you're talking about something different from Comment 7...?

Sorry about that, I was talking about comment 10...

> Possibly a layerization thing, or possibly a random edge-case thing -- and
> since it's very minimal and imperceptible, it's not really worth worrying
> about too much.

Ok, then I will set it as fuzzy. And could you please review the patch for me? Thanks.
Comment on attachment 8899378 [details]
Bug 1290782 Part 3 - Add test cases for using an SVG image as border-image.

https://reviewboard.mozilla.org/r/170618/#review176492

Thanks for resurrecting this bug!  r- for now, though the code might be correct -- just a few requests for clarification below.

::: commit-message-7dddb:1
(Diff revision 4)
> +Bug 1290782 - Force using image size as SVG's viewport size when drawing the border and the SVG doesn't have fixed ratio. r?dholbert

This commit message could be a bit clearer (and should mention "border-image").

It should probably start out like "When drawing a border-image using an SVG image with no aspect ratio, [...]" and then explain what the change is.

::: layout/base/nsLayoutUtils.cpp:6550
(Diff revision 4)
> -  CSSIntSize svgViewportSize = currentMatrix.IsIdentity()
> +  // For images with FLAG_FORCE_UNIFORM_SCALING, the ratio of the viewport size
> +  // should not be changed. Therefore, the svgViewportSize in that case is
> +  // assigned to an uniformly-scaled size from OptimalImageSizeForDest.
> +  CSSIntSize svgViewportSize =
> +    (currentMatrix.IsIdentity() ||
> +    (aImageFlags & imgIContainer::FLAG_FORCE_UNIFORM_SCALING))
>      ? CSSIntSize(intImageSize.width, intImageSize.height)
>      : CSSIntSize::Truncate(devPixelDest.width, devPixelDest.height);

This comment needs some rewording/clarifying. In particular:
 - s/an/a/ -- should be "a uniformly-scaled"

 - It talks about "the ratio of the viewport size" - what does that mean? the aspect ratio? Or some scale-factor "ratio" between some other size & the viewport size? I'm not at all clear.

 - Overall, it's not clear (from the comment) what the impact is of using devPixelDest vs. intImageSize here.  Could you make that clearer?

::: layout/reftests/border-image/reftest.list:93
(Diff revision 4)
>  == svg-as-border-image-1b.html svg-as-border-image-1-ref.html
>  == svg-as-border-image-1c.html svg-as-border-image-1-ref.html
> -fails-if(webrender) == svg-as-border-image-2.html svg-as-border-image-2-ref.html # see bug 1151016. The svg viewport size may be wrong
> +== svg-as-border-image-2.html svg-as-border-image-2-ref.html
>  == svg-as-border-image-3.html svg-as-border-image-3-ref.html
> -== svg-as-border-image-4.html svg-as-border-image-4-ref.html
> +== svg-as-border-image-4a.html svg-as-border-image-4-ref.html
> +fuzzy(2,2) == svg-as-border-image-4b.html svg-as-border-image-4-ref.html

If this is *always* exactly this level of fuzziness (and I think it is, based on your Comment 14 Try run): please use the stricter range-based annotation, which I think is:
 fuzzy(2-2,2-2)

That way, if this improves, we'll find out and we can remove the annotation.

::: layout/reftests/border-image/svg-as-border-image-4b.html:11
(Diff revision 4)
> +  border-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg"><circle cx="50%" cy="50%" r="50%" fill="red"/></svg>') 5% stretch;
> +  will-change: opacity;
> +}
> +</style>
> +</head>
> +<body>
> +<div></div>

So in affected builds (e.g. current Nightly), it looks like this test's failure mode is for *nothing to paint at all*.  At least, that's what happens on my machine.

However -- if I change "5%" to "25%" at the end of this style, we fail differently -- by painting *too much* (a few extra rectangular chunks on each edge).

This patch fixes both cases (which is great!) but I'd like to test the 25% too-much-painting case too.  

So anyway, my request -- could you extend this test (and its "4a" variant and its "4-ref" file) to have two different <div> elements, with 5% on the first one and 25% on the second one?  This could happen as part of this patch or as a separate patch, whichever you prefer.  That way we'll be testing the issue in two different ways.

You'll probably want to pull the 'border-image' style out into a dedicated CSS rule so that it can vary between the two divs -- like "div.border5p { ... } " and  "div.border25p { ... }" or something.
Attachment #8899378 - Flags: review?(dholbert) → review-
Assignee: nobody → lochang
I took some time to study the border-image related background knowledge and look into the bug again. I found that the main problem in this bug is that if we set will-change: opacity will change the elements _31 and _32 of currentMatrix [1]. Which means it is no more identity matrix and result in using the wrong way to calculate svgViewportSize.
As I known, elements _31 and _32 should be something related to translate? So I think it is abnormal that opacity will effect these elements. Maybe the correct fix is to see why will-change: opacity will effect elements _31, _32 instead of adding aImageFlags to the condition?

[1] The matrix we used to determine the way to calculate svgViewportSize. http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6550
Flags: needinfo?(dholbert)
I also found that on single process (no e10s), the test case is broken no matter will-change: opacity is set or not.
Attachment #8776449 - Attachment is obsolete: true
Attachment #8779620 - Attachment is obsolete: true
Attachment #8796383 - Attachment is obsolete: true
Update:
It seems reasonable that _31, _32 are changed when will-change: opacity is set since it will create a statcking context. So maybe the right way to fix the bug is to fix the "if" condition of not using matrix.IsIdentiy() as you mentioned in bug 1151016 comment 2?
(In reply to Louis Chang [:louis] from comment #25)
> So maybe the right way to fix
> the bug is to fix the "if" condition of not using matrix.IsIdentiy() as you
> mentioned in bug 1151016 comment 2?

That might be reasonable, yeah.

(Though: I'm guessing that "IsIdentity()" check was intended to just be an easy/simple case, and the more complex fallback case is intended to work as well, and the fact that it's not means some logic is broken there... but I'm not sure.)
Flags: needinfo?(dholbert)
Attached patch WIP (obsolete) — Splinter Review
Hi Louis, please see what I did in this patch
(In reply to C.J. Ku[:cjku](UTC+8) from comment #27)
> Created attachment 8907538 [details] [diff] [review]
> WIP
> 
> Hi Louis, please see what I did in this patch

And, this patch can also fix bug 1151016.
(In reply to C.J. Ku[:cjku](UTC+8) from comment #28)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #27)
> > Created attachment 8907538 [details] [diff] [review]
> > WIP
> > 
> > Hi Louis, please see what I did in this patch
> 
> And, this patch can also fix bug 1151016.
And try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78c7ed8c59582151bf1ba789d576eb2cb6eb403
I notices several minor things that we should fix while reading relative code. Since you are here, please considerate to fix them:
1. Please remove "snappedDestSize.IsEmpty()" in "if" statement at [1] since snappedDestSize can not be empty. Please fix it in a separate patch.
2. destCtx is not needed.

[1] http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6539
[2] http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6713
Attached patch WIPSplinter Review
Attachment #8907538 - Attachment is obsolete: true
Basically the changes of the patch part 1 is to use OptimalImageSizeForDest with devPixelDest to calculate svg viewport size. Since svg viewport size should not account on any transformation [1]. Only reuse intImageSize for svg viewport size when we have no scaling so we don't have to calculate again. 

[1] http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6410-6413
Comment on attachment 8908221 [details]
Bug 1290782 Part 2 - Remove redundant destCtx and IsEmpty check of snappedDestSize since it should always have a value.

https://reviewboard.mozilla.org/r/179888/#review185256
Attachment #8908221 - Flags: review?(cku) → review+
Comment on attachment 8908220 [details]
Bug 1290782 Part 1 - When drawing a border-image using an SVG image, we should not take into account any transformation of currentMatrix when computing svg viewport size.

https://reviewboard.mozilla.org/r/179886/#review185260
Attachment #8908220 - Flags: review?(cku) → review+
Comment on attachment 8899378 [details]
Bug 1290782 Part 3 - Add test cases for using an SVG image as border-image.

https://reviewboard.mozilla.org/r/170618/#review185266

::: layout/reftests/border-image/svg-as-border-image-4b.html:11
(Diff revision 5)
> +  width: 100px;
> +  height: 100px;
> +  margin: 30px;
> +  border-width: 30px;
> +  border-style: solid;
> +  will-change: opacity;

This bug has no direct relation with will-change. Can we use transform property instead? For exp:
transform: translate(100px, 200px);

::: layout/reftests/border-image/svg-as-border-image-4b.html:21
(Diff revision 5)
> +#border25p {
> +  border-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg"><circle cx="50%" cy="50%" r="50%" fill="red"/></svg>') 25% stretch;
> +}
> +</style>
> +</head>
> +<body>

<body style="transform: translate(-100px, -200px);">
Attachment #8899378 - Flags: review?(cku)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #40)
> This bug has no direct relation with will-change. Can we use transform
> property instead? For exp:
> transform: translate(100px, 200px);

Sure. Try results look good (comment 41).
Comment on attachment 8899378 [details]
Bug 1290782 Part 3 - Add test cases for using an SVG image as border-image.

https://reviewboard.mozilla.org/r/170618/#review185830
Attachment #8899378 - Flags: review?(cku) → review+
Comment on attachment 8899378 [details]
Bug 1290782 Part 3 - Add test cases for using an SVG image as border-image.

https://reviewboard.mozilla.org/r/170618/#review186622
Attachment #8899378 - Flags: review?(dholbert) → review+
Comment on attachment 8908220 [details]
Bug 1290782 Part 1 - When drawing a border-image using an SVG image, we should not take into account any transformation of currentMatrix when computing svg viewport size.

https://reviewboard.mozilla.org/r/179886/#review186626

r=me

::: layout/base/nsLayoutUtils.cpp:6548
(Diff revision 2)
> -  // XXX(seth): May be buggy; see bug 1151016.
> -  CSSIntSize svgViewportSize = currentMatrix.IsIdentity()
> -    ? CSSIntSize(intImageSize.width, intImageSize.height)
> -    : CSSIntSize::Truncate(devPixelDest.width, devPixelDest.height);
> +  nsIntSize svgViewportSize =
> +    (scaleFactors.width == 1.0 && scaleFactors.height == 1.0)
> +    // intImageSize is scaled by currentMatrix. But since there is no scale
> +    // factors in currentMatrix, it is safe to assign intImageSize to
> +    // svgViewportSize directly.
> +    ? intImageSize
> +    // We should not take into account any transformation of currentMatrix
> +    // when computing svg viewport size. Since currentMatrix contains scale
> +    // factors, we need to recompute SVG viewport by unscaled devPixelDest.
> +    : aImage->OptimalImageSizeForDest(devPixelDest.Size(),
> +                                      imgIContainer::FRAME_CURRENT,
> +                                      aSamplingFilter, aImageFlags);
> +

Thank you for adding these explanatory comments here!

Unfortunately, they've made the ternary expression a bit hard to follow, though.

(In particular: now there are 3 lines separating the ternary expression's condition from its associated "?" character; and there are 8 lines (including two different comment-blocks) separating the "=" from one of the expressions it might be using for assignment. Both of those make this whole expression hard to follow, IMO.)

Could you restructure this as "if/else"?  e.g.:

nsIntSize svgViewportSize;
if (scaleFactors.width == 1.0 && scaleFactors.height == 1.0) {
  // ...
  svgViewportSize = intImageSize;
} else {
  // ...
  svgViewportSize = OptimalSizeForDest(...)
}

This will make your new code-comments clearer, too, because they'll be {}-scoped to where what they're saying is strictly true.

::: layout/base/nsLayoutUtils.cpp:6550
(Diff revision 2)
> -    ? CSSIntSize(intImageSize.width, intImageSize.height)
> -    : CSSIntSize::Truncate(devPixelDest.width, devPixelDest.height);
> +    // intImageSize is scaled by currentMatrix. But since there is no scale
> +    // factors in currentMatrix, it is safe to assign intImageSize to

s/there is/there are/
Attachment #8908220 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4dade7ac1e24
Part 1 - When drawing a border-image using an SVG image, we should not take into account any transformation of currentMatrix when computing svg viewport size. r=cjku,dholbert
https://hg.mozilla.org/integration/autoland/rev/f00ccd10b4f5
Part 2 - Remove redundant destCtx and IsEmpty check of snappedDestSize since it should always have a value. r=cjku
https://hg.mozilla.org/integration/autoland/rev/b3619f0787cd
Part 3 - Add test cases for using an SVG image as border-image. r=cjku,dholbert
Keywords: checkin-needed
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: