1285320 - SVG border-image rendering changes on reload (after a resize), if SVG has no width and height and uses percent-of-viewport-based sizing

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

(Spinning this off of bug 1284798)

STR:
 1. Load https://bugzilla.mozilla.org/attachment.cgi?id=8768760  (Ignore the "correct"/"incorrect" descriptive text, for the purposes of this bug here.)

 2. Resize your browser-window, horizontally.

 3. Reload the page.

EXPECTED RESULTS:
 Rendering shouldn't change on reload.

ACTUAL RESULTS:
 The top example ("no width or height including circles") changes its border rendering on reload. (pretty significantly, depending on how much you resized the browser)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug

Comment 2

[Daniel Holbert [:dholbert]]

Regression range for when the rendering started changing, between steps 2 and 3 of STR (from the reload):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=05c087337043dd8e71cc27bdb5b9d55fd00aaa26&tochange=af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86

So, this "regressed" from bug 619500, but our behavior before that bug landed wasn't particularly useful or interoperable either (the rendering didn't really make sense, even if it was consistent around reloads), so I'm not too concerned about tracking this as an actual regression.

Comment 3

[Sebastian Zartner [:sebo]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> EXPECTED RESULTS:
>  Rendering shouldn't change on reload.

To clarify this: The rendering after the reload is the expected one.
So, a complete recalculation of the border image is needed when the border image area changes its size instead of just stretching the slices.

Sebastian

Comment 4

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

Attachment: Don't cache clipped image when using SVG source without fixed aspect ratio.

When doing the resize, nsImageRenderer will get the cached images of previous border painting; however, when using SVG image without fixed aspect ratio as source we should re-clip the image in every function call.

In this patch, I add a condition to see if we should cache the clipped images.

I will mark the review tag after finish the try run.

Comment 5

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

I think this case is a little trivial, do I need to add a test for this bug ?

Thanks.

Comment 6

[Sebastian Zartner [:sebo]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #4)
> Created attachment 8770832 [details] [diff] [review]
> Don't cache clipped image when using SVG source without fixed aspect ratio.

Quality-wise it would be better to also not cache SVGs *with* fixed aspect ratio, but I guess that may cause performance issues.

Sebastian

Comment 7

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

(In reply to Sebastian Zartner [:sebo] from comment #6)
> (In reply to Kevin Chen[:kechen] (UTC + 8) from comment #4)
> > Created attachment 8770832 [details] [diff] [review]
> > Don't cache clipped image when using SVG source without fixed aspect ratio.
> 
> Quality-wise it would be better to also not cache SVGs *with* fixed aspect
> ratio, but I guess that may cause performance issues.
> 
> Sebastian

I am not sure about how expensive the re-cliping will be, maybe we can do some evaluations to see if it is worthy to trade the performance for the quality.
Another option is that we can dump the cache only when the images go very distorted.

Do you have any case that cached images make the output display looks distorted ?

Comment 8

[Sebastian Zartner [:sebo]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #7)
> (In reply to Sebastian Zartner [:sebo] from comment #6)
> > (In reply to Kevin Chen[:kechen] (UTC + 8) from comment #4)
> > > Created attachment 8770832 [details] [diff] [review]
> > > Don't cache clipped image when using SVG source without fixed aspect ratio.
> > 
> > Quality-wise it would be better to also not cache SVGs *with* fixed aspect
> > ratio, but I guess that may cause performance issues.
> > 
> > Sebastian
> 
> I am not sure about how expensive the re-cliping will be, maybe we can do
> some evaluations to see if it is worthy to trade the performance for the
> quality.
> Another option is that we can dump the cache only when the images go very
> distorted.
> 
> Do you have any case that cached images make the output display looks
> distorted ?

I did some testing but couldn't come up with a test case where the images look notably distorted. Generally the display may look a little bit distorted when thin lines are almost parallel to the border edge, i.e. where the stretching gets visible, but the effect is marginal. So, this probably doesn't justify a performance tradeoff by recalculating the image everytime the layout size changes.

Sebastian

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #5)
> I think this case is a little trivial, do I need to add a test for this bug ?

Yes please. (Generally, if it's reasonably possible to include an automated test for a fix, then we should do so. That means pretty much all fixes for rendering bugs should include a test.)

I think you should be able to test this using MozReftestInvalidate-triggered tweak to a div's width. E.g. your testcase would have a <div style="width:400px">, and then when MozReftestInvalidate fires, you'd reduce that div's width (say, to 100px). And then the reference case could have the exact same source, except it'd be static (no JS) and it'd already have width:100px on the div.)

Comment 10

[Robert Longson [:longsonr]]

+        if (!(drawFlags | imgIContainer::FLAG_FORCE_UNIFORM_SCALING)) {

Won't this always be false? I.e. didn't you mean & here?

Comment 11

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

Yes, you're right. Should be 
"if (!(drawFlags & imgIContainer::FLAG_FORCE_UNIFORM_SCALING))" .

Thank you for the correction, I will update it with my test case later.

Comment 13

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

Attachment: Don't cache clipped image when using SVG source without fixed aspect ratio.

Fix some nits and rename the reftest.

There are some errors in the try run, but they are not related to border image.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4116f5273b8f

Daniel, Can you help me to review this patch ?

Thank you.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8771895 [details] [diff] [review]
Don't cache clipped image when using SVG source without fixed aspect ratio.

@@ -5527,17 +5527,19 @@ nsImageRenderer::DrawBorderImageComponent(nsPresContext*       aPresContext,
>     if (mType == eStyleImageType_Image) {
>       if ((subImage = mImage->GetSubImage(aIndex)) == nullptr) {
>         subImage = ImageOps::Clip(mImageContainer, srcRect, aSVGViewportSize);
>-        mImage->SetSubImage(aIndex, subImage);
>+        if (!(drawFlags & imgIContainer::FLAG_FORCE_UNIFORM_SCALING)) {
>+          mImage->SetSubImage(aIndex, subImage);
>+        }

I'm not sure I agree with the strategy here (skipping the cache entirely for these images).  Seems like there are plenty of cases where we'd still benefit from having these subimages cached -- e.g. anything that requires a repaint, *other than* a resize.  (e.g. scrolling the element into & out of view -- or the element changing from opacity:1 to opacity:0.5, or changing "visibility".  All of these require a repaint, and there's no reason these repaints shouldn't benefit from our cache of clipped images.)  So, it's possible we should just make our cache usage here a bit smarter (let it detect resizes) instead of bypassing it entirely.

I'm not sure, but I suspect aSVGViewportSize is the size that really matters here.  So -- perhaps nsStyleImage should be broadened to store a single "mCachedSVGViewportSize", and GetSubImage/SetSubImage should be changed to take an "aSVGViewportSize" arg, and the getter should flush its cached SubImages & return null if the caller's size doesn't match the cached size?

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8771895 [details] [diff] [review]
Don't cache clipped image when using SVG source without fixed aspect ratio.

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

(r- for now)

Comment 16

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

Attachment: Don't cache clipped image when using SVG source without fixed aspect ratio.

Hi Daniel,

I add a member "mCachedSVGViewportSize" in nsStyleImage to monitor if viewport size changes and clear cached images bases on it.

But I have a concern about if nsStyleImage is the right place to save the cached information in since it comes from StyleContext and is always a const variable. Maybe nsImageRenderer is more better to store these information?

Comment 17

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

Comment on attachment 8773603 [details] [diff] [review]
Don't cache clipped image when using SVG source without fixed aspect ratio.

Sorry I misunderstand something, nsImageRenderer is only a temporal variable for holding the context, maybe nsStyleImage is the proper place to store these information.

Comment 18

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

The following is the try result, couple errors but no one is related to this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31593a3862e4

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8773603 [details] [diff] [review]
Don't cache clipped image when using SVG source without fixed aspect ratio.

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

Thanks! This is going in the right direction.

First nit: the commit message needs an update. Right now it says "don't cache clipped images..." but that's not accurate anymore. I think it should say something like "Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio."  (I think that accurately describes what the patch is now aiming to do; please fix/reword if not.)

::: layout/base/nsCSSRendering.cpp
@@ +5646,5 @@
>    return image.forget();
>  }
>  
> +void
> +nsImageRenderer::ClearExpiredCache(const Maybe<nsSize>& aSVGViewportSize)

This function-name doesn't quite sound right.  "ClearExpiredCache" implies to me that (a) there's a time-based "expiration", (b) we know the cache has expired and needs clearing, and (c) we're definitely going to clear it.  None of those are necessarily true.  Possibly-better name: "nsImageRenderer::UpdateSVGViewportSize"?

@@ +5649,5 @@
> +void
> +nsImageRenderer::ClearExpiredCache(const Maybe<nsSize>& aSVGViewportSize)
> +{
> +  mImage->ClearExpiredCache(aSVGViewportSize,
> +                            ComputeIntrinsicSize().HasRatio());

It's nice to abstract away all of the "do we need to clear the cache" logic into a single API on mImage, but there's a cost -- it means we have to unconditionally perform this ComputeIntrinsicSize() step, which is not free. (ComputeIntrinsicSize involves a good bit of computation and some virtual function calls under the hood).

So, I'd prefer we refactor out the logic to be pulled out & be made more explicit here, so that we can avoid calling ComputeIntrinsicSize() unless we actually need it.  Something like:

  if (mImage->GetCachedSVGViewportSize() == aSVGViewportSize &&
      !ComputeIntrinsicSize().HasRatio()) {
    mImage->PurgeCacheForViewportChange(aSVGViewportSize);
  }

...where mImage->PurgeCacheForViewportChange() would (a) assert that the size is actually changing, (b) unconditionally clear mSubImages, and (c) save the new aSVGViewportSize.

::: layout/style/nsStyleStruct.h
@@ +99,5 @@
>  
>  // Additional bits for nsRuleNode's mNoneBits:
>  #define NS_RULE_NODE_HAS_ANIMATION_DATA     0x80000000
>  
> +using mozilla::Maybe;

The coding style guide says: |No "using" statements are allowed in header files|
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

So: please drop this line. Any mozilla-namespaced stuff in header files can just use the "mozilla::Whatever" prefix.

@@ +379,5 @@
>  
>    // This is _currently_ used only in conjunction with eStyleImageType_Image.
>    nsAutoPtr<nsStyleSides> mCropRect;
> +
> +  // Record the virwport size when caching border-image paintings.

Typo: "virwport"

@@ +380,5 @@
>    // This is _currently_ used only in conjunction with eStyleImageType_Image.
>    nsAutoPtr<nsStyleSides> mCropRect;
> +
> +  // Record the virwport size when caching border-image paintings.
> +  Maybe<nsSize> mCachedSVGViewportSize;

Hmm.  This will increase the size of the nsStyleImage struct a bit (with a nsSize plus a bool).

Tha'ts unfortunate, since this new variable really only matters for a niche edge case ("border-image", with SVG source data, with no intrinsic size on the SVG).

Since this is only ever going to be used if mSubImages is also being used (right?), could we bundle this and mSubImages into a single lazily-allocated struct?  Something like:
 mozilla::UniquePtr<CachedBorderImageData> mCachedBIData;

Here, "CachedBorderImageData" would be a new struct which would store mSubImages and mCachedSVGViewportSize.

If this makes sense to you, it's probably best to split this bug into two patches:
* part 1: Just refactor existing code to create "CachedBorderImageData" struct, which *only* contains mSubImages -- and refactor mSubImages' client code accordingly. (This patch shouldn't affect behavior.)  This could even be done on a helper-bug, if you like, but a "part 1" on this bug is fine too.

* part 2: The "main patch" -- introduce mCachedSVGViewportSize as a member-variable in CachedBorderImageData, and all the rest of the changes that you've got in your main patch here right now.

::: layout/style/nsStyleStructInlines.h
@@ +17,5 @@
>  #include "nsIContent.h" // for GetParent()
>  #include "nsTextFrame.h" // for nsTextFrame::ShouldSuppressLineBreak
>  
>  inline void
> +nsStyleImage::ClearExpiredCache(const Maybe<nsSize>& aSVGViewportSize,

Any "Maybe<...>" types in this file need to be declared as "mozilla::Maybe<...>" (with the namespace prefix), since this is a header file, which won't benefit from "using" declarations. (except perhaps via getting it from some .cpp file, by chance, during unified builds.)

Also, I think this function should be split into a getter and a cache-clearer -- see my notes above for the callsite in nsCSSRendering.cpp.

Comment 20

[Daniel Holbert [:dholbert]]

(Also, on the ComputeIntrinsicSize-being-expensive topic: please be sure we only get to the ComputeIntrinsicSize().HasRatio() check *if* we have a SVG source for our border-image -- we don't want to take that perf hit, if we're using a raster image. I suspect that might Just Work with my suggested refactoring (e.g. from the SVG viewport size always getting set to the same bogus value when we've got a raster image, maybe), but I haven't looked closely enough to be sure. Please double-check that this is the case.)

Comment 21

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

Attachment: (Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

Comment 22

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

Attachment: (Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

Comment 23

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

The following link is the try result :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de9ce59af397

There are some errors but none of them are related to border image.

Comment 24

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

Comment on attachment 8775007 [details] [diff] [review]
(Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

Hi Daniel,

Can you help me to review this patch?

In this patch I add a struct "CachedBorderImageData" to contain cached data for border image, and no flow and behavior is changed in this patch.

The try result is in comment 23.

Thank you.

Comment 25

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

Comment on attachment 8775008 [details] [diff] [review]
(Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

Hi Daniel,

Can you help me to review this patch?

In this patch I add a member "mCachedSVGViewport" to decide whether to clear the border image's cached data, and only vector images without fixed ratio need to refresh the cached data.

The try result is in comment 23.

Thank you.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8775007 [details] [diff] [review]
(Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

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

r=me with nits addressed:

::: layout/style/nsStyleStruct.cpp
@@ +1913,5 @@
> +// CachedBorderImageData
> +//
> +CachedBorderImageData::CachedBorderImageData() {}
> +
> +CachedBorderImageData::~CachedBorderImageData() { mSubImages.Clear(); }

(As I note for the .h file below, I think you can get rid of the constructor/destructor for this class.)

::: layout/style/nsStyleStruct.h
@@ +248,5 @@
>  
> +struct CachedBorderImageData
> +{
> +  CachedBorderImageData();
> +  ~CachedBorderImageData();

I don't think this class needs a constructor or destructor here, since your constructor implementation is empty and your destructor implementation looks unnecessary (you just clear mSubImages, which should happen automatically when mSubImages gets destructed).

So: please delete these two lines.

@@ +374,5 @@
>  private:
>    void DoCopy(const nsStyleImage& aOther);
>  
>    // Cache for border-image painting.
> +  mozilla::UniquePtr<CachedBorderImageData> mCachedBIData;

This file needs an #include for UniquePtr.h, up at the top, to provide this type.

::: layout/style/nsStyleStructInlines.h
@@ +19,5 @@
>  inline void
>  nsStyleImage::SetSubImage(uint8_t aIndex, imgIContainer* aSubImage) const
>  {
> +  if (!mCachedBIData) {
> +    const_cast<nsStyleImage*>(this)->mCachedBIData =

Please add a comment somewhere around here (or perhaps alongside the declaration of mCachedBIData) to say that it's lazily allocated. (i.e. that's the only reason we expect this variable to be null -- not because it's been cleared/reassigned or anything)

@@ +30,5 @@
>  nsStyleImage::GetSubImage(uint8_t aIndex) const
>  {
> +  if (mCachedBIData)
> +    return mCachedBIData->GetSubImage(aIndex);
> +  return nullptr;

The coding style guidelines say that this should use curly-braces. (The old code here didn't, but that's because it was old code, and old code is sometimes sloppy. :))

BUT, rather than adding curly braces, it'd be better to just simplify this to a one-liner, I think:
  return mCachedBIData ? mCachedBIData->GetSubImage(aIndex) : nullptr;

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8775008 [details] [diff] [review]
(Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

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

r=me with the following addressed:

::: layout/base/nsCSSRendering.cpp
@@ +5653,5 @@
> +  // ratio to keep source image from changing.
> +  if (mImageContainer &&
> +      mImageContainer->GetType() == imgIContainer::TYPE_VECTOR &&
> +      aSVGViewportSize != mImage->GetCachedSVGViewportSize() &&
> +      !(ComputeIntrinsicSize().HasRatio())) {

For SVG images that *do* have an intrinsic ratio (and whose viewport size never changes), I think we'll end up calling ComputeIntrinsicSize() here *every time they paint* (only to rediscover that there's a ratio & we don't have to purge the cache).

That's basically-unnecessary ComputeIntrinsicSize() churn (including some virtual function calls) on every paint, which is a bit unfortunate. It'd be nice if we could only do the ComputeIntrinsicSize.HasRatio() check once, and then save that state as part of mImage's cached data, perhaps as a Maybe<bool> mSubImagesDependOnSVGViewport, which (if emplaced) would indicate definitively whether we need to bother checking/updating the viewport.

That optimization probably doesn't need to happen in this patch, but would you mind filing a followup on that?  Or even adding a "part 3" patch here, if you're up for that?

::: layout/style/nsStyleStruct.cpp
@@ +1912,5 @@
>  // --------------------
>  // CachedBorderImageData
>  //
> +CachedBorderImageData::CachedBorderImageData()
> +  : mCachedSVGViewportSize(Nothing())

Maybe<> variables get automatically initialized to Nothing, so you don't need to initialize it here. (which means you still don't need a constructor at all, as noted above for part 1)

::: layout/style/nsStyleStruct.h
@@ +251,5 @@
>  {
>    CachedBorderImageData();
>    ~CachedBorderImageData();
> +
> +  void SetCachedSVGViewportSize(const mozilla::Maybe<nsSize>& aSVGViewportSize);

Add a line of documentation here, explaining that the caller is expected to ensure that aSVGViewportSize doesn't match the already-cached viewport size.)

@@ +258,5 @@
>    void SetSubImage(uint8_t aIndex, imgIContainer* aSubImage);
>    imgIContainer* GetSubImage(uint8_t aIndex);
>  
>  private:
> +  // Record the viewport size when caching border-image paintings.

This is somewhat misleading -- "the viewport" sounds like the browser's viewport size. And that's not actually what we're talking about here.

Replace with a clearer comment, something like:
  // If this is a SVG border-image, we save the size of the SVG viewport that
  // we used when rasterizing any cached border-image subimages. (The viewport
  // size matters for percent-valued sizes & positions in inner SVG doc).

::: layout/style/nsStyleStructInlines.h
@@ +21,5 @@
> +{
> +  if (!mCachedBIData) {
> +    const_cast<nsStyleImage*>(this)->mCachedBIData =
> +      mozilla::MakeUnique<CachedBorderImageData>();
> +  }

Hmm -- so with this patch, we'll have these exact 4 lines repeated 3 times (once in the first patch, twice in this patch).

Probably worth refactoring them into a private helper method, named something like "void LazyAllocCachedBIData()". Then all the callsites can be one-liners instead of 4-liners, and it'll be easier to track the lifetime of this variable.

(Probably best to just do that refactoring up-front in your "part 1" patch, where you add the first copy of this lazy-allocation code, rather than doing the refactoring here.  So, "part 1" should create & use a private method called LazyAllocCachedBIData or something like that.)

@@ +29,5 @@
> +inline void
> +nsStyleImage::PurgeCacheForViewportChange(
> +  const mozilla::Maybe<nsSize>& aSVGViewportSize) const
> +{
> +  MOZ_ASSERT((aSVGViewportSize != mCachedBIData->GetCachedSVGViewportSize()),

Drop the extra layer of parens around the condition here -- they're unnecessary.

Comment 28

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

Attachment: (Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

Carry r+ from comment 26.

Comment 29

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

Attachment: (Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

In this patch I add a member "mCachedSVGViewport" to decide whether to clear the border image's cached data, and only vector images without fixed ratio need to refresh the cached data.

Comment 30

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

Comment on attachment 8775522 [details] [diff] [review]
(Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

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

Hi Daniel,

I changed some logics to this patch can you help me to review it again ?

Thank you.

::: layout/base/nsCSSRendering.cpp
@@ +3850,5 @@
> +  // In this condition, we pass imageSize(a resolved size comes from
> +  // default sizing algorithm) to renderer as the viewport size.
> +  Maybe<nsSize> svgViewportSize = intrinsicSize.CanComputeConcreteSize() ?
> +    Nothing() : Some(imageSize);
> +  bool hasRatio = intrinsicSize.HasRatio();

I found that we've done ComputeIntrinsicSize() before[1], so I just reused its result here and stored the hasRatio bool.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3711

@@ +3851,5 @@
> +  // default sizing algorithm) to renderer as the viewport size.
> +  Maybe<nsSize> svgViewportSize = intrinsicSize.CanComputeConcreteSize() ?
> +    Nothing() : Some(imageSize);
> +  bool hasRatio = intrinsicSize.HasRatio();
> +  renderer.PurgeCacheForViewportChange(svgViewportSize, hasRatio);

I changed the name "UpdateSVGViewportSize" since update viewport size is not the main purpose to do in this function. Is it a proper name for this function ?

@@ +5598,5 @@
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
>      // For those SVG image sources which don't have fixed aspect ratio (i.e.
>      // without viewport size and viewBox), we should scale the source uniformly
>      // after the viewport size is decided by "Default Sizing Algorithm".
> +    if (!aHasRatio) {

I also refactor the code here since we stored the information before, is it ok that I also refactor this in the patch ? Or should I file another bug for this ?

::: layout/style/nsStyleStructInlines.h
@@ +28,5 @@
>  inline void
> +nsStyleImage::PurgeCacheForViewportChange(
> +  const mozilla::Maybe<nsSize>& aSVGViewportSize, const bool aHasRatio) const
> +{
> +  EnsureCachedBIData();

Is it proper to use "EnsureCachedBIData()" as the function name for lazily allocating mCachedBIData ?

@@ +33,5 @@
> +
> +  // Clear cached images when viewport size is updated and we don't have fixed
> +  // ratio to keep source image from changing.
> +  if (aSVGViewportSize != mCachedBIData->GetCachedSVGViewportSize() &&
> +      !aHasRatio) {

I move the viewport and ratio logics here to keep the logics in single API. Is it ok ?

Comment 31

[Daniel Holbert [:dholbert]]

I'll post a few review nits on the updated patch, but generally looks good -- responses to your questions:

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #30)
> I found that we've done ComputeIntrinsicSize() before[1], so I just reused
> its result here and stored the hasRatio bool.

Great! Sounds good.

> > +  renderer.PurgeCacheForViewportChange(svgViewportSize, hasRatio);
> 
> I changed the name [...] Is it a proper name for this
> function ?

Seems fine, yeah.

> > +    if (!aHasRatio) {
> 
> I also refactor the code here since we stored the information before, is it
> ok that I also refactor this in the patch ? Or should I file another bug for
> this ?

Meh, maybe slightly better to do it in a separate patch, but not a big deal; it's fine here too.

> Is it proper to use "EnsureCachedBIData()" as the function name for lazily
> allocating mCachedBIData ?

Sounds good to me.

> I move the viewport and ratio logics here to keep the logics in single API.
> Is it ok ?

Sounds good.

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8775522 [details] [diff] [review]
(Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

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

::: layout/base/nsCSSRendering.cpp
@@ +5701,5 @@
> +void
> +nsImageRenderer::PurgeCacheForViewportChange(const Maybe<nsSize>& aSVGViewportSize,
> +                                             const bool aHasRatio)
> +{
> +  // Check if we should flush the cached data, only vector images need to do

Grammar nit: please replace the comma here with a hyphen or semicolon.

(s/data, only/data - only/)

::: layout/base/nsCSSRendering.h
@@ +247,5 @@
>                             uint8_t              aVFill,
>                             const nsSize&        aUnitSize,
>                             uint8_t              aIndex,
> +                           const mozilla::Maybe<nsSize>& aSVGViewportSize,
> +                           const bool           aHasRatio);

Two nits:
 #1: "aHasRatio" is probably too vague here -- please rename to "aHasIntrinsicRatio"

 #2: Please also update the documentation for this method, to mention this parameter. (It looks like the other params all have a line or more of documentation, so this one should as well.)

::: layout/style/nsStyleStructInlines.h
@@ +31,5 @@
> +{
> +  EnsureCachedBIData();
> +
> +  // Clear cached images when viewport size is updated and we don't have fixed
> +  // ratio to keep source image from changing.

Two nits:
 #1: it's not quite clear to me what this comment is saying. (in particular, the "to keep source image from changing" phrase at the end is mysterious.  It's unclear why/how we might be expecting the source image to change, and why/how it might be kept from changing)

Please replace with something clearer, like:
  // If we're redrawing with a different viewport-size than we used for our
  // cached subimages, then we can't trust that our subimages are valid;
  // any percent sizes/positions in our SVG doc may be different now. Purge!
  // (We don't have to purge if the SVG document has an intrinsic ratio,
  // though, because [...])
And please fill in [...] with a brief hint/explanation for why the ratio saves us, which I don't fully recall right now. :)

 #2: Probably best to just drop "inline" from this function declaration, and move it to nsStyleStruct.cpp.  (I'm not sure it's a clear win to force the compiler to make this function inline.  And purging the cache is inherently kind of rare & expensive anyway, as an overall operation [since it means we're going to have to repopulate the cached subimages], so there's no strong reason this *needs* to be an optimized hot path.  And this is getting to be longer than most of the inline functions in this file, especially with my desire for verbose comments. :))

Comment 33

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

Attachment: (Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

Carry r+ from comment 32.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acf56d9e08c5
(Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b872e47ecf
(Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio. r=dholbert

Comment 35

[Carsten Book [:Tomcat]]

sorry had to back this out because this conflict with the merge down from m-c in (run 'hg heads' to see heads, 'hg merge' to merge)
merging layout/style/nsComputedDOMStyle.cpp
merging layout/style/nsStyleStruct.cpp
merging layout/style/nsStyleStruct.h
warning: conflicts while merging layout/style/nsStyleStruct.h! (edit, then use 'hg resolve --mark')
149 files updated, 2 files merged, 0 files removed, 1 files unresolved

Comment 36

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3cd11b98793
Backed out changeset a0b872e47ecf 
https://hg.mozilla.org/integration/mozilla-inbound/rev/bff4f9be4961
Backed out changeset acf56d9e08c5 for conflicting with merge from m-c to mozilla-inbound

Comment 37

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

Attachment: (Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

Carry r+ from comment 26.

Comment 38

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

Attachment: (Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

Carry r+ from comment 32.

Comment 39

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

Hi Carsten,

The conflict might be caused by Bug 1288797 which reorder the header include.

I've already fixed it and uploaded the patches, please help me to land it again.

Thank you.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df2fb7caf5e1
Part 1: Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a517dfb9fc0b
Part 2: Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio. r=dholbert

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df2fb7caf5e1
https://hg.mozilla.org/mozilla-central/rev/a517dfb9fc0b

Comment 42

[Sebastian Zartner [:sebo]]

Verified that it's failing in Firefox 49.0a2 (2016-08-01) and working as expected in Nightly 51.0a1 (2016-08-03).

Sebastian

Comment 43

[Andrew Overholt [:overholt]]

Can we get this uplifted to 50 (which is also affected)?

Comment 44

[Daniel Holbert [:dholbert]]

(In reply to Andrew Overholt [:overholt] from comment #43)
> Can we get this uplifted to 50 (which is also affected)?

Note: as implied by comment 2, Firefox 48 and 49 were also affected (and we were also pretty broken before that).  I'll set those releases' statuses to "wontfix", to make it clearer they were affected.

It's probably still worth uplifting this to Aurora, though, since this fix is relatively straightforward (and relatively low-risk -- just flushing cached stuff a bit more eagerly), and we might as well get the fix out there sooner.

Comment 45

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

Ok, I will try to apply these patches to Aurora and run the tests before request for uplift.

Comment 46

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

Comment on attachment 8776445 [details] [diff] [review]
(Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

Approval Request Comment
[Feature/regressing bug #]:
  bug 619500
[User impact if declined]:
  Painting error like this bug describes will happen.
[Describe test coverage new/current, TreeHerder]:
  A reftest is added in part2 patch.
[Risks and why]: 
  I think the risk is low since this patch only decide whether to cache the data. If the patch fails, it may just fall back to original situation.
  And I've pushed the patches to try again, no related errors happened.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbde8805872d&selectedJob=25888092
[String/UUID change made/needed]:
  No

Comment 47

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

Comment on attachment 8776446 [details] [diff] [review]
(Part 2) Purge border-image cache when hypothetical SVG viewport changes, if using SVG image with no aspect ratio.

Approval Request Comment
[Feature/regressing bug #]:
  bug 619500
[User impact if declined]:
  Painting error like this bug describes will happen.
[Describe test coverage new/current, TreeHerder]:
  A reftest is added in part2 patch.
[Risks and why]: 
  I think the risk is low since this patch only decide whether to cache the data. If the patch fails, it may just fall back to original situation.
  And I've pushed the patches to try again, no related errors happened.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbde8805872d&selectedJob=25888092
[String/UUID change made/needed]:
  No

Comment 48

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776445 [details] [diff] [review]
(Part 1) Add struct "CachedBorderImageData" to keep cached data for border in nsStyleImage.

Recent regression, fix was verified on Nightly, let's uplift to Aurora50.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20dbba05457b
https://hg.mozilla.org/releases/mozilla-aurora/rev/2065ffb539e1