455364 - border-image artifacts

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In the css3.info examples, both show artifacts -- I'm pretty sure they're both the same thing.  The 'round' has lots of little yellow dots inside the block, and the 'stretch' has a big smeared bit on the left and smaller on the right.  Screenshots can be seen on john's blog at http://ejohn.org/blog/border-image-in-firefox/

Comment 1

[Steven Michaud [:smichaud] (Retired)]

The problems also happen (with today's mozilla-central nightly) on Windows and Linux.  So -> All.

Comment 2

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

Has somebody measured the image to make sure this behavior isn't correct?

Comment 3

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

One potential problem is that border-image is doing (or not doing) its own pixel-snapping and probably isn't getting things right in all situations. See bug 458487, it should probably use the new nsLayoutUtils::DrawImage.

Comment 4

[Zack Weinberg (:zwol)]

The image has problems; *all* of the diamonds overflow the boxes that the
styling assigns, and it's not possible to adjust the styling to make all nine
subregions simultaneously correct.

I have made a corrected image and will see if the problems persist.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: original, incorrect border image

Comment 6

[Zack Weinberg (:zwol)]

Attachment: corrected border image

Comment 7

[Zack Weinberg (:zwol)]

Attachment: test case using corrected image

Comment 8

[Zack Weinberg (:zwol)]

Attachment: test case using incorrect image

So I've just attached four things:

 -- the original border image from css3.info
 -- my corrected version of the same image
    (the diamonds aren't exactly the same but that's ok)
 -- a simple, self-contained (uses data: urls) test case
    matching css3.info's design, using my corrected image
 -- the same, using a palette-compressed version of the original

One may clearly see that the corrected-image test case shows no smears but the original-image test case does show smears.

That doesn't mean there's no bug here!  This test case is not doing anything especially evil to the pixel snapping.

Comment 9

[Zack Weinberg (:zwol)]

I've emailed the folks at css3.info about the problem image.

Also, there is a similar image linked from http://www.w3.org/TR/css3-background/#the-border-image ( http://www.w3.org/TR/css3-background/border.png ) -- it is not exactly the same as my corrected image, but its diamonds are within their 27x27 regions, and a test case that uses it doesn't show artifacts either.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: part 1, rev 1: DrawImage variant methods

I've written a series of three patches which, collectively, arrange to use roc's new nsLayoutUtils::Draw*Image methods to draw border images.  They don't work, and I can't figure out why, so I'm posting them here and begging for help.

This patch adds new variants of the Draw*Image methods that take the image as a gfxIImageFrame* or an nsIImage* instead of an imgIContainer*.  This makes it possible to give Draw*Image an image that didn't come directly from the network - for instance, an nsIImage* that is a small piece of another nsIImage*, which the second patch will make easy to create.

It is possible that I am not handling the down-conversion from imgIContainer* correctly; I don't understand what the rectangle returned by gfxIImageFrame::GetRect() is for, and the only thing I can think of that makes any sense at all would imply that the size of an imgIContainer* is not necessarily the size of the nsIImage* that's the current frame, but that seems ridiculous.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: part 2, rev 1: nsIImage::Extract and nsMargin refactor

Part 2 adds an nsIImage::Extract() method which produces another nsIImage containing a rectangular sub-region of the original.  Here again I'm not sure I did all the image manipulation correctly, with particular concern about how the sub-image and related drawing context are created, written to, and finalized.

It also refactors nsMargin a bit so that nsIntMargin has all the same methods as nsMargin even when NSCOORD_IS_FLOAT is true.  Since NSCOORD_IS_FLOAT is not a functional mode right now, that part isn't actually necessary, but I didn't realize that until after I'd done it.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 1: DrawBorderImage rewrite

And here's the actual DrawBorderImage rewrite.  As mentioned above, it doesn't work, and there's a bunch of debugging code intentionally left in the patch.

The way in which it doesn't work is: only the very first call to nsLayoutUtils::Draw*Image from DrawBorderImage, in a given page, draws anything.  In other words, if you have

<div id="a">...</div>
<div id="b">...</div>
<div id="c">...</div>

where #a and #c are styled with -moz-border-image and #b is styled with background-image, then: only the upper left hand corner of the border-image for #a is drawn; the background image for #b is drawn; and nothing is drawn for #c.

The debugging code tells me that this is not a problem with the computation of source and destination rectangles (despite which, I would appreciate any suggestions anyone might have for reducing the amount of repetition involved) and so I am left with the hypothesis that I've done something horribly wrong (and yet invisible) with the image manipulation.

Comment 13

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

(In reply to comment #10)
> It is possible that I am not handling the down-conversion from imgIContainer*
> correctly; I don't understand what the rectangle returned by
> gfxIImageFrame::GetRect() is for, and the only thing I can think of that makes
> any sense at all would imply that the size of an imgIContainer* is not
> necessarily the size of the nsIImage* that's the current frame, but that seems
> ridiculous.

Yes, yes it is. And yet, true. Look at the computation of 'padding' in nsLayoutUtils::DrawImage.

Comment 14

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

You should probably run reftests on patch 1 alone to get rid of the bugs there. Do we really need the full set of DrawImage variants in that patch though? IMHO the DrawSingleImage(Unscaled) variants should not be needed.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: part 1, rev 2: DrawImage variants (fixed)

This revised version of part 1 now passes reftests by itself.  Also I took out all the variant methods that weren't actually being used in part 3 - so we now have nsIImage* variants of DrawImage and DrawSingleImage, but nothing else.

The complete patch series still breaks the border-image tests.  On looking at the code again, I think it's just that in part 3, I didn't compute the destination rectangles for painting correctly, but I won't be able to verify that until later today.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: part 2, rev 2: nsIImage::Extract and nsMargin refactor (fixed)

A slight correction to this part of the patch proved necessary - I had the coordinate transform backward, so ::Extract gave back nonsense for any rectangle not starting at the image origin.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 2: DrawBorderImage rewrite (almost fixed)

Here's the current state of the third part of the patch.  It is *not* ready to go as is, but if anyone wants to look at it, be my guest.  Still to do: make all coordinate calculations on the image take the innerRect into account, verify that 'repeat' and 'round' do the right thing in all cases, and fix a seaming bug which regresses reftest border-image/solid-image-2.html (note the addition of a -2a.html).

My current theory is that the seaming bug boils down to nearest-neighbor sampling not actually always sampling only the pixels in the image (as advertised) as it occurs only with 'stretch' mode and only for specific, large stretch factors.  The patch in bug 463938 does not fix it (unsurprisingly, as the image in question is being upscaled, not downscaled).

Comment 18

[Zack Weinberg (:zwol)]

The patch in bug 468496 fixes border-image/solid-image-2.html.  Adding dependency.

Comment 19

[Zack Weinberg (:zwol)]

Attachment: part 2, rev 3: nsIImage::Extract and nsMargin refactor (works around bug 468496)

This works around bug 468496.  nsIImage objects can now be flagged as always to use image surfaces instead of whatever the platform would prefer, and ::Extract sets that flag on all images it creates.  Thus, border images take correctness over performance, which should be okay since they should be small.  Also, ::Extract calls ::Optimize before returning so that the solid-color optimization gets done.  With this revision, the existing border-image tests do not regress on Linux when all three patches are applied.  When I revise part 3 I shall also include a border-image variation of the 1x2 image testcase from bug 468496 to ensure that this isn't just due to the solid-color optimization now applying.

There is also some minor cleanup to make the #ifdeffage in nsThebesImage.cpp less heinous.  I'd *like* to shove all of this crud down into gfxPlatform, but not in this bug.

Comment 20

[Zack Weinberg (:zwol)]

Since we have a workaround, it's not necessary for this to depend on bug 468496 anymore.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 353877 [details] [diff] [review]
part 2, rev 3: nsIImage::Extract and nsMargin refactor (works around bug 468496)

Erm, what?  This patch seems to change all sorts of stuff, and I don't understand why.


>+  /**
>+   * Extract a rectangular region of the nsIImage and return it as a new
>+   * nsIImage.
>+   * @param aSubimage  the region to extract
>+   * @param aResult    the extracted image
>+   */
>+  virtual nsresult Extract(const nsIntRect& aSubimage,
>+                           nsIImage** aResult NS_OUTPARAM) = 0;
>+

Add this to the end of the interface (not that nsIImage is all that public, but still).

>--- a/gfx/public/nsMargin.h
>+++ b/gfx/public/nsMargin.h

Why are these nsMargin changes here at all?  It just seems like formatting etc. changes.  Are there any functional changes?

>diff --git a/gfx/src/thebes/nsThebesImage.cpp b/gfx/src/thebes/nsThebesImage.cpp
>--- a/gfx/src/thebes/nsThebesImage.cpp
>+++ b/gfx/src/thebes/nsThebesImage.cpp
>@@ -41,54 +41,48 @@
> 
> #include "gfxContext.h"
> #include "gfxPattern.h"
> 
> #include "gfxPlatform.h"
> 
> #include "prenv.h"
> 
>-static PRBool gDisableOptimize = PR_FALSE;

Why are you getting rid of this?  Oh, moving parts of it into Optimize?  I guess that's fine, but there's no need to change this.  Joe is busy collapsing a bunch of these image classes anyway, so any major cleanup here is just going to conflict for no gain.

> nsresult
>-nsThebesImage::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth, nsMaskRequirements aMaskRequirements)
>+nsThebesImage::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth,
>+                    nsMaskRequirements aMaskRequirements)
> {
>     mWidth = aWidth;
>     mHeight = aHeight;
> 
>     // Reject over-wide or over-tall images.
>     if (!AllowedImageSize(aWidth, aHeight))
>         return NS_ERROR_FAILURE;
> 
>@@ -117,43 +111,36 @@ nsThebesImage::Init(PRInt32 aWidth, PRIn
>         default:
>             format = gfxImageSurface::ImageFormatRGB24;
>             mAlphaDepth = 0;
>             break;
>     }
> 
>     mFormat = format;
> 
>-#ifdef XP_WIN
>-    if (!ShouldUseImageSurfaces()) {
>-        mWinSurface = new gfxWindowsSurface(gfxIntSize(mWidth, mHeight), format);
>-        if (mWinSurface && mWinSurface->CairoStatus() == 0) {
>-            // no error
>-            mImageSurface = mWinSurface->GetImageSurface();
>-        }
>-    }
>-
>-    if (!mImageSurface)
>-        mWinSurface = nsnull;
>-#endif

Again, why are you getting rid of this?  There's a specific reason why win32 surfaces have to be created in this way (so that we can extract an image surface that points to the same pixels in the internal win32 DIB).  Creating a gfxImageSurface instead removes that optimization.

The rest of the code seems ok, though I'm still wondering why we can't step back and figure out what we need to fix inside cairo to get the behaviour that we want, to avoid having to do all this stuff in layers above.  Ideally we would start with a pure cairo testcase that shows the problem on the multiple platforms, and then we'd figure out what's needed to change internally to fix the problem.

Comment 22

[Zack Weinberg (:zwol)]

(In reply to comment #21)
> (From update of attachment 353877 [details] [diff] [review])
> Erm, what?  This patch seems to change all sorts of stuff, and I don't
> understand why.

The critical part of this is the new method, nsIImage::Extract().  All else is either cleanups done in passing, or workarounds for cairo problems.

> >+  virtual nsresult Extract(const nsIntRect& aSubimage,
> >+                           nsIImage** aResult NS_OUTPARAM) = 0;
> >+
> 
> Add this to the end of the interface (not that nsIImage is all that
> public, but still).

Ok.

> Why are these nsMargin changes here at all?

To quote comment 11,

# [This piece] refactors nsMargin a bit so that nsIntMargin has
# all the same methods as nsMargin even when NSCOORD_IS_FLOAT is
# true. Since NSCOORD_IS_FLOAT is not a functional mode right now,
# that part isn't actually necessary, but I didn't realize that
# until after I'd done it.

I would be happy to pull that out to a separate bug entirely, if you prefer.

> >-static PRBool gDisableOptimize = PR_FALSE;
> 
> Why are you getting rid of this?  Oh, moving parts of it into Optimize?

Yeah, the thought was that as long as I was messing with the nsIImage constructor, there was no need for anything other than ::Optimize to be aware of the environment variable.

> Joe is busy collapsing a bunch of these image classes anyway, 

Bug number?  I'd be curious to see what it ends up looking like.  The ifdeffage in here makes my chain saw hand itchy.

> so any major cleanup here is just going to conflict for no gain.

I can take this back out too.

> >-#ifdef XP_WIN
> >-    if (!ShouldUseImageSurfaces()) {
> >-        mWinSurface = new gfxWindowsSurface(gfxIntSize(mWidth, mHeight), format);
> >-        if (mWinSurface && mWinSurface->CairoStatus() == 0) {
> >-            // no error
> >-            mImageSurface = mWinSurface->GetImageSurface();
> >-        }
> >-    }
> >-
> >-    if (!mImageSurface)
> >-        mWinSurface = nsnull;
> >-#endif
> 
> Again, why are you getting rid of this?  There's a specific reason why win32
> surfaces have to be created in this way (so that we can extract an image
> surface that points to the same pixels in the internal win32 DIB).  Creating a
> gfxImageSurface instead removes that optimization.

Isn't it functionally equivalent to create an image surface and then wrap it with a win32 surface, as is done below this point after the changes?  The point of the rearrangement is to control all of these optimizations with one if block.

> The rest of the code seems ok, though I'm still wondering why we can't step
> back and figure out what we need to fix inside cairo to get the behaviour that
> we want, to avoid having to do all this stuff in layers above.  Ideally we
> would start with a pure cairo testcase that shows the problem on the multiple
> platforms, and then we'd figure out what's needed to change internally to fix
> the problem.

The new ::Extract method is necessary regardless, unless and until cairo adds the ability for a pattern to draw from a subregion of a surface.

The Cairo/X11 bugs that I'm working around with the "never use device surface" flag are discussed in bug 468496.  I think it would be pretty easy to extend the "stretch single pixel" test case attached to that bug to test for Cairo/Mac bugs, too, but I don't have a Mac.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #22)
> > Joe is busy collapsing a bunch of these image classes anyway, 
> 
> Bug number?  I'd be curious to see what it ends up looking like.  The ifdeffage
> in here makes my chain saw hand itchy.

Don't think there is one yet.

> > >-#ifdef XP_WIN
> > >-    if (!ShouldUseImageSurfaces()) {
> > >-        mWinSurface = new gfxWindowsSurface(gfxIntSize(mWidth, mHeight), format);
> > >-        if (mWinSurface && mWinSurface->CairoStatus() == 0) {
> > >-            // no error
> > >-            mImageSurface = mWinSurface->GetImageSurface();
> > >-        }
> > >-    }
> > >-
> > >-    if (!mImageSurface)
> > >-        mWinSurface = nsnull;
> > >-#endif
> > 
> > Again, why are you getting rid of this?  There's a specific reason why win32
> > surfaces have to be created in this way (so that we can extract an image
> > surface that points to the same pixels in the internal win32 DIB).  Creating a
> > gfxImageSurface instead removes that optimization.
> 
> Isn't it functionally equivalent to create an image surface and then wrap it
> with a win32 surface, as is done below this point after the changes?  The point
> of the rearrangement is to control all of these optimizations with one if
> block.

There's no way to wrap an image surface with a win32 surface.  The surface constructors that take a cairo_surface_t* really should all be protected; they're only used for initializing a new wrapper.  Doing that would require them to all be friends with gfxASurface, but that's probably cleaner in the long run.

> > The rest of the code seems ok, though I'm still wondering why we can't step
> > back and figure out what we need to fix inside cairo to get the behaviour that
> > we want, to avoid having to do all this stuff in layers above.  Ideally we
> > would start with a pure cairo testcase that shows the problem on the multiple
> > platforms, and then we'd figure out what's needed to change internally to fix
> > the problem.
> 
> The new ::Extract method is necessary regardless, unless and until cairo adds
> the ability for a pattern to draw from a subregion of a surface.

Yep, we should get that on the cairo table -- ideally it would just be a new pattern type that takes another pattern and a subregion.  But yeah, I'm ok with Extract, we should just attack this problem at a lower level sometime soon.

> The Cairo/X11 bugs that I'm working around with the "never use device surface"
> flag are discussed in bug 468496.  I think it would be pretty easy to extend
> the "stretch single pixel" test case attached to that bug to test for Cairo/Mac
> bugs, too, but I don't have a Mac.

Comment 24

[Zack Weinberg (:zwol)]

Attachment: part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed) [Checkin: See comment 39 & 49]

revised as requested.  Should I bother filing a bug for the nsMargin/nsIntMargin changes, given how broken NSCOORD_IS_FLOAT is right now?

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 353912 [details] [diff] [review]
part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed)
[Checkin: See comment 39 & 49]

Yeah, that looks fine; thanks!  For the nsMargin stuff, sure, though I think there are a lot more NSCOORD_IS_FLOAT type things floating around out there.

Comment 26

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 3: DrawBorderImage rewrite, done

Here's what I hope is the final iteration of the changes to DrawBorderImage itself.  This should properly handle the innerRect and repeat/round.  However, I'm not sure how much either of those are tested by the reftests we've got for border-image.  Also, the seaming regression is gone.

These three patches, together, pass reftests on Linux.  I have limited ability to test on Windows (i.e. I can do it but it takes a very long time due to virtual machine slowness) and no ability to test on Mac.

Comment 27

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

+  // (For vector images the split points would be in CSS pixels.)

I'm not sure what this means. For SVG images, we would define one image pixel to be one CSS pixel *of the SVG document*, which could be different from CSS pixels in the document containing the image. So this comment could be misleading and I think you should just remove it.

+  NS_ASSERTION(innerRect.width == img->GetWidth(), "img inner width off");
+  NS_ASSERTION(innerRect.height == img->GetHeight(), "img inner height off");

Why are these valid?

+      case eStyleUnit_Percent:
+        if (s == NS_SIDE_TOP || s == NS_SIDE_BOTTOM)
+          split.side(s) = coord.GetPercentValue() * imageSize.height;
+        else
+          split.side(s) = coord.GetPercentValue() * imageSize.width;
+        break;
+      case eStyleUnit_Factor:
+        split.side(s) = coord.GetFactorValue();
+        break;

There's rounding going on here. I'd prefer it to be explicit, using NS_lround probably. And it's probably also worth using common subexpressions so you can write something like

   PRInt32 imgDimension = (s == NS_SIDE_TOP || s == NS_SIDE_BOTTOM) ? imageSize.height : imageSize.width;
   double dim;
   switch (...) {
   case eStyleUnit_Percent:
     dim = coord.GetPercentValue()*imgDimension;
     break;
   case eStyleUnit_Factor:
     dim = coord.GetFactorValue();
     break;
   }
   if (dim < 0) {
     dim = 0;
   } else if (dim > imgDimension) {
     dim = imgDimension;
   }
   split.side(s) = NS_lround(dim);

This table-driven code looks better than what was there, but I think it could be improved by making the main loop a nested i/j loop from 0 to 2 and 0 to 2. Then destAreas and subAreas would not be necessary, since those rects can be trivially computed at each iteration. fillStyleH and fillStyleV can then be replaced with a simple "j == 1 ? aBorderStyle.mBorderImageHFill : NS_STYLE_BORDER_IMAGE_STRETCH" etc. unitSize's components can each be written in the form "j != 1 ? borderWidth[j] : (i != 1 ? borderHeight[i] : splitWidth[i]*hFactor)". I suppose an enum would be better than 0,1,2 though.

+  for (int i = 0; i < 9; i++)
+    DrawBorderImageComponent(aRenderingContext, img, aDirtyRect,
+                             destAreas[i], subAreas[i],
+                             fillStyleH[i], fillStyleV[i], unitSize[i]);

Put the loop body in {}

+  nsRect aScale;

Don't start the name with 'a' since it's not a parameter. Plus the name is not very descriptive. I would call this "nsRect dest;" and rename the "aDest" parameter to "aFill".

+  switch (aHFill) {
+    case NS_STYLE_BORDER_IMAGE_STRETCH:
+      aScale.y = aDest.y;

Shouldn't that be "aVFill"? If this really passed tests, please add a test it wouldn't have passed...

Comment 28

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 4: addressing roc's comments

(In reply to comment #27)
> +  NS_ASSERTION(innerRect.width == img->GetWidth(), "img inner width off");
> +  NS_ASSERTION(innerRect.height == img->GetHeight(), "img inner height off");
> 
> Why are these valid?

Is it not an invariant of the image classes that the rectangle produced by gfxIImageFrame::GetRect() precisely encloses the pixels of the nsIImage that you can GetInterface() from that gfxIImageFrame?  These assertions check that invariant.  I've added a comment to that effect.

All your other comments should be addressed in this new revision.

Comment 29

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

(In reply to comment #28)
> Is it not an invariant of the image classes that the rectangle produced by
> gfxIImageFrame::GetRect() precisely encloses the pixels of the nsIImage that
> you can GetInterface() from that gfxIImageFrame?  These assertions check that
> invariant.  I've added a comment to that effect.

I don't think so.  I think imagelib currently exposes animated images where some subframes only cover parts of the image to the callers through its API.  I wish it didn't, though.

Comment 30

[Zack Weinberg (:zwol)]

(In reply to comment #29)
> (In reply to comment #28)
> > Is it not an invariant of the image classes that the rectangle produced by
> > gfxIImageFrame::GetRect() precisely encloses the pixels of the nsIImage that
> > you can GetInterface() from that gfxIImageFrame?  These assertions check that
> > invariant.  I've added a comment to that effect.
> 
> I don't think so.  I think imagelib currently exposes animated images where
> some subframes only cover parts of the image to the callers through its API.  I
> wish it didn't, though.

Uh, as far as I can tell, that's exactly what GetRect() does.  The way I think it works is, if you've got an imgIContainer for an animated image, and some subframes only cover part of the image, then the gfxIImageFrame for each of those frames is going to report its own width and height equal to all the other frames, but if you convert it to an nsIImage, that will report a smaller width and height, and the rectangle returned by GetRect() tells you where to place the nsIImage within the larger image.  So that rectangle should always have width and height equal to the nsIImage's reported width and height.

I haven't tried to use an animated gif for a border image though (and I'm not sure how to create one with this particular property, anyway).

Comment 31

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

Joe Drew can enlighten us.

Comment 32

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

I think I was just confusing the imgIContainer/gfxImageFrame relationship with the gfxImageFrame/nsIImage relationship.

Comment 33

[Zack Weinberg (:zwol)]

If I've got it wrong, it's not just a problem with the assertions - there's code to compensate for this imagelib wart that would be wrong too.

Comment 34

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 4a: rediffed

I'm not sure what went into trunk to cause the patch to be inapplicable, but here it is rediffed.

Comment 35

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

+        unitSize.height = splitHeight[j]*hFactor;

Shouldn't this be vFactor? Currently vFactor is unused... Sounds like we need another test here to cover that issue!

Comment 36

[Zack Weinberg (:zwol)]

Attachment: part 1, rev 2a: rediffed [Checkin: Comment 38 & 48]

part 1 needed rediffing after some churn in nsLayoutUtils.h.

Comment 37

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 5: fixes hFactor/vFactor typo

(In reply to comment #35)
> +        unitSize.height = splitHeight[j]*hFactor;
> 
> Shouldn't this be vFactor? Currently vFactor is unused... Sounds like we need
> another test here to cover that issue!

Yeah, that was supposed to be vFactor.  This revision fixes that and adds tests for the center image scaling.  Unfortunately I couldn't think of any way to do the reference other than a fixed PNG.

Comment 38

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

Pushed part 1 to trunk: http://hg.mozilla.org/mozilla-central/rev/fed00e0efe5c. I plan to land the other parts over the next few days.

Comment 39

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

Pushed part 2: http://hg.mozilla.org/mozilla-central/rev/ecc20943472d

I would have pushed part 3 as well but I'm a bit afraid the reftests will fail and I already have some slightly risky stuff in this push.

Comment 40

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

Pushed part 3: http://hg.mozilla.org/mozilla-central/rev/a142df653655

Comment 41

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

Backed out part 3. It caused test failures on Mac and Windows:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1233996521.1233999465.20619.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1233996521.1234002366.24862.gz

Comment 42

[Zack Weinberg (:zwol)]

Bad news: the failures that appear on both Mac and Windows are down to the workarounds for the X video driver issues with scaling.  Specifically: if I change nsThebesImage::Draw() to always use nearest-neighbor scaling, irrespective of platform, then the failures of center-scaling-[23].html go away.

There is no workaround for this, unless you can think of a way to duplicate the visual effect that border-image produces without using border-image or a reference PNG.  I can't.

IMO we need to find a better workaround for those scaling problems.  I have an idea, which I will describe in bug 468496 either later tonight or tomorrow morning.

Comment 43

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

You could hand-split the image into various subimages and then compose them together using a set of positioned <img> images, which will let you do the scaling. If that doesn't work, maybe CSS transforms to do the scaling, or even SVG.

Comment 44

[Zack Weinberg (:zwol)]

Attachment: part 3, rev 5a: hand-tiled image grids for reference (fixes Windows failures) [Checkin: Comment 46]

This revision just changes the center-scaling* tests to use HTML references, with the border image manually cut up into its nine components and tiled with <img width=X height=Y> scaling.  All the border-image reftests now pass on Windows as well as Linux.  I also took out the XFAILs for the tests that are now passing on Mac.

Comment 45

[Zack Weinberg (:zwol)]

Committers: please try to re-land "part 3, rev 5a" only.

Comment 46

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

Pushed http://hg.mozilla.org/mozilla-central/rev/749dfebefd83

Comment 47

[philippe (part-time)]

(In reply to comment #46)
> Pushed http://hg.mozilla.org/mozilla-central/rev/749dfebefd83

This has broken the display of tabs in chrome parts: add-on manager, error console, the main window tabs of the Grapple theme. See bug 477732.

Comment 48

[Serge Gautherie (:sgautherie)]

Comment on attachment 358240 [details] [diff] [review]
part 1, rev 2a: rediffed
[Checkin: Comment 38 & 48]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6dee06e4fd7

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 353912 [details] [diff] [review]
part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed)
[Checkin: See comment 39 & 49]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bb6ce950bc11

after fixing context for
{
patching file gfx/src/thebes/nsThebesImage.h
Hunk #2 FAILED at 153
1 out of 2 hunks FAILED
}

Comment 50

[Serge Gautherie (:sgautherie)]

Comment on attachment 362367 [details] [diff] [review]
part 3, rev 5a: hand-tiled image grids for reference (fixes Windows failures)
[Checkin: Comment 46]

Does not apply to 1.9.1:
{
patching file layout/base/nsCSSRendering.cpp
Hunk #1 FAILED at 260
Hunk #3 FAILED at 1713
2 out of 3 hunks FAILED
}

Comment 51

[Zack Weinberg (:zwol)]

Attachment: part 3 for 1.9.1 branch [Checkin: Comment 52]

Here's part 3 rediffed to apply to the 1.9.1 branch; no functional changes were necessary.  I get bizarre errors ("REFTEST TEST-UNEXPECTED-FAIL | | EXCEPTION: ReferenceError: xulRuntime is not defined") when I try to run the reftests, so this has not actually been tested.

Comment 52

[Serge Gautherie (:sgautherie)]

Comment on attachment 363805 [details] [diff] [review]
part 3 for 1.9.1 branch
[Checkin: Comment 52]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/70f75e6e8fcf

Comment 53

[Zack Weinberg (:zwol)]

dbaron clued me in on IRC as to what I was doing wrong.  The 1.9.1 branch reftests pass with the above patch applied.