All callers of imgIContainer::ExtractFrame call it with FRAME_CURRENT, because they want the image to animate. Though we do probably want a way to disable animation for an image, ExtractFrame is not the best approach - it combines frame extraction with clipping, and most of the time we don't want to combine them. We can replace ExtractFrame with a simpler method that better matches our actual needs.

Actually, there is one caller of ExtractFrame that cares about freezing the animation of an image and _not_ clipping: imgRequestProxy::GetStaticRequest. So we need to split ExtractFrame into two concepts: freezing the animation of an image, and clipping a region out of an image. Bug 843895 covers replacing the freezing animation feature.

I think we're finally ready to go on this patch, which has been cooking for a while. The modern plan is to have an ImageWrapper, ClippedImage, take care of the clipping. ClippedImages still animate, and they do not actually result in any image data getting copied - instead the clipping is simulated at draw time. To get a ClippedImage, callers (inside or outside of imagelib) can use ImageOps::Clip(). Patches posted presently.

First part, in which we add the ImageWrapper that creates a clipped "view" of its underlying image.

Here we create ImageOps. Now you can get a ClippedImage via ImageOps::Clip and a FrozenImage via ImageOps::Freeze. imgTools is avoided here because we don't need to expose these kinds of things to JavaScript directly and so the usability tradeoffs of involving an XPCOM service aren't worthwhile.

Here we eliminate one of the two remaining uses of ExtractFrame.

Here we eliminate the other one. Note that ClippedImages animate, unlike the images returned from ExtractFrame, so we don't need to worry about whether the underlying image is animated or not anymore.

With all of that out of the way, we can finally remove ExtractFrame and all of the nastiness that it involved.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=8bcd543e93e2

Comment on attachment 723726 [details] [diff] [review] (Part 3) - Use ClippedImage instead of ExtractFrame for -moz-image-rect. Why do we no longer need to worry about mFlags? Is it taken into account later somehow? r=me if it is.

Comment on attachment 723730 [details] [diff] [review] (Part 4) - Use ClippedImage instead of ExtractFrame for border-image. Hmm. So does this effectively defeat the caching we used to do? Or is the caching just not really needed in the new setup? r=me if the latter.

Comment on attachment 723733 [details] [diff] [review] (Part 5) - Remove imgIContainer::ExtractFrame. Please rev the IID. sr=me with that.

Comment on attachment 723725 [details] [diff] [review] (Part 2) - Create a static utility class for image operations. sr=me

Thanks for the reviews Boris! (In reply to Boris Zbarsky (:bz) from comment #11) > Hmm. So does this effectively defeat the caching we used to do? Or is the > caching just not really needed in the new setup? I am doubtful that the caching makes sense anymore for typical use cases of border-image. It certainly does not make sense for the corner slices, which will typically be drawn just by adjusting the parameters to imgIContainer::Draw. (This is handled by ClippedImage, introduced in part 1.) A temporary surface _does_ have to be created when we draw the slices that tile, but I doubt that for typical uses this significantly impacts performance, because I expect the slices to be very small. We also win on memory usage here by not storing all of the slices persistently. And we no longer have to recopy everything every single time we redraw in the presence of animation - in that situation, this patch is a strict win. If there's reason to believe it would be beneficial, we could easily add caching of slices at the imagelib layer. (i.e., inside ClippedImage) Basically, we'd just need to memoize the surface drawn by ClippedImage::DrawSingleTile for the last N combinations of arguments. (And take the current animation frame into account.) N=1 might be fine, but it depends how border-image usage looks in the wild.

David, see comment 14?

(In reply to Seth Fowler [:seth] from comment #14) > N=1 might be fine, but it > depends how border-image usage looks in the wild. Ack, sorry, momentarily forgot that ClippedImages are per-instance. N=1 is definitely fine.

(In reply to Boris Zbarsky (:bz) from comment #10) > Why do we no longer need to worry about mFlags? Is it taken into account > later somehow? It's taken into account because imgIContainer::Draw will also be passed FLAG_SYNC_DECODE later, and the decode will happen then. Prior to this patch, we needed to ensure it happened in PrepareImage in the moz-image-rect case because ExtractFrame would fail otherwise. ClippedImage doesn't have that issue. (Note that we rely on this effect already in PrepareImage; you can see this happening in the "Special case" mentioned at the beginning of the method.)

I'm not particularly sure why you'd think I'd have an opinion here, but I discussed this with seth and it sounds reasonable to me, at the very least from the perspective of not doing premature optimization.

Comment on attachment 723721 [details] [diff] [review] (Part 1) - Add ClippedImage. Review of attachment 723721 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/ClippedImage.cpp @@ +168,5 @@ > + new gfxImageSurface(gfxIntSize(mClip.width, mClip.height), > + gfxASurface::ImageFormatARGB32); > + gfxContext ctx(surface); > + nsIntRect clipAtOrigin(0, 0, mClip.width, mClip.height); > + nsresult rv = Draw(&ctx, gfxPattern::FILTER_FAST, gfxMatrix(), clipAtOrigin, Is there a reason you're using FILTER_FAST here? @@ +299,5 @@ > + if (sourceRect.width > mClip.width || sourceRect.height > mClip.height) { > + transform.Multiply(gfxMatrix().Translate(gfxPoint(-sourceRect.x, -sourceRect.y))); > + transform.Multiply(gfxMatrix().Scale(ClampFactor(sourceRect.width, mClip.width), > + ClampFactor(sourceRect.height, mClip.height))); > + transform.Multiply(gfxMatrix().Translate(gfxPoint(sourceRect.x, sourceRect.y))); I presume you're not terribly interested in me going into great detail on your math here ::: image/src/ClippedImage.h @@ +69,5 @@ > + uint32_t aWhichFrame, > + uint32_t aFlags); > + > + nsIntRect mClip; // The region to clip to. > + Maybe<bool> mShouldClip; // Memoized ShouldClip() if present. Is there a way we can make this be Maybe<nsIntRect> mClip?

Comment on attachment 723725 [details] [diff] [review] (Part 2) - Create a static utility class for image operations. Review of attachment 723725 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/ImageOps.h @@ +29,5 @@ > + static already_AddRefed<Image> Freeze(Image* aImage); > + static already_AddRefed<imgIContainer> Freeze(imgIContainer* aImage); > + > + /** > + * Creates a clipped version of an existing image. You should mention that it has no effect on animation, I suppose

Comment on attachment 723733 [details] [diff] [review] (Part 5) - Remove imgIContainer::ExtractFrame. Review of attachment 723733 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/public/imgIContainer.idl @@ +57,4 @@ > * > * Internally, imgIContainer also manages animation of images. > */ > [scriptable, builtinclass, uuid(01c4f92f-f883-4837-a127-d8f30920e374)] uuid needs to change

(In reply to Joe Drew (:JOEDREW! \o/) from comment #19) > > + nsresult rv = Draw(&ctx, gfxPattern::FILTER_FAST, gfxMatrix(), clipAtOrigin, > > Is there a reason you're using FILTER_FAST here? Yeah, because the intention was that no scaling would be happening. (I'm passing the identity matrix for the transform after all.) Is that invalid reasoning? > @@ +299,5 @@ > > + if (sourceRect.width > mClip.width || sourceRect.height > mClip.height) { > > + transform.Multiply(gfxMatrix().Translate(gfxPoint(-sourceRect.x, -sourceRect.y))); > > + transform.Multiply(gfxMatrix().Scale(ClampFactor(sourceRect.width, mClip.width), > > + ClampFactor(sourceRect.height, mClip.height))); > > + transform.Multiply(gfxMatrix().Translate(gfxPoint(sourceRect.x, sourceRect.y))); > > I presume you're not terribly interested in me going into great detail on > your math here I definitely am, especially if there's a problem. My linear algebra is quite rusty. =) TBH I am surprised that code is even needed, because I didn't expect to get transforms passed in that would result in grabbing a non-integer number of pixels. (That is, where aUserspaceToImagespace.Transform(aFill) results in a rectangle with non-integer width and height.) Without that code, I saw tiny lines at one end of border-image slices in some reftests. Now that I look at this code again, it's pretty obvious that I could also do this with only one call to transform.Multiply(). I should fix that. Still getting used to this API. > ::: image/src/ClippedImage.h > @@ +69,5 @@ > > + uint32_t aWhichFrame, > > + uint32_t aFlags); > > + > > + nsIntRect mClip; // The region to clip to. > > + Maybe<bool> mShouldClip; // Memoized ShouldClip() if present. > > Is there a way we can make this be Maybe<nsIntRect> mClip? The problem is we actually have three states here: "can't evaluate whether we should clip yet", "evaluated and the answer is no", "evaluated and the answer is yes". Maybe<bool> gives you three states quite nicely, but alas Maybe<nsIntRect> only gives you two.

Thanks for the reviews, by the way, Joe!

(In reply to Seth Fowler [:seth] from comment #22) > Yeah, because the intention was that no scaling would be happening. (I'm > passing the identity matrix for the transform after all.) Is that invalid > reasoning? Whoops; somehow I missed FILTER_NEAREST in the list. Nearest neighbor is actually the right thing here, I think.

Switch to FILTER_NEAREST in ClippedImage::GetFrame. Get rid of some unneeded matrix multiplies.

Address review comment.

Address review comment.

There were a few oranges in the try job from cases where an assertion stating that we wouldn't tile in DrawSingleTile failed. It turns out this happens when gfxCallbackDrawable fails to create an offscreen surface and punts, sending the draw request directly to the callback as a last resort. There's not much that can be done in that case that won't result in some sort of visual artifact. I turned the assertion into a warning and activated FLAG_CLAMP when it happens, so at least we get something on the screen. I can't reproduce this locally on OS X or Ubuntu, so hopefully it's difficult to encounter this IRL.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=ee4c2c90848b

Try pretty much looks good although there are some weird errors on some of the Windows runs that seem to be harness or infrastructure issues. Just to be sure, I retriggered those tests a couple of times. If those retriggers come out green I think we're go for a push.

I find these Windows-only oranges a bit troubling. They're all timeout related. I'm willing to bet that it's just the copious amounts of logging code in the try version of this patch. Here's a Windows-only try push for the exact code that is posted to the bug, without the logging: https://tbpl.mozilla.org/?tree=Try&rev=4b83799ee742

Argh. I just don't feel comfortable landing this without some more inquiry. On Windows there are a couple of tests where gfxCallbackDrawable (which we use explicitly in situations where we need to tile, since we can't tile a subimage directly) fails to allocate an offscreen surface and just bails and passes through the full, tiled Draw request to ClippedImage::DrawSingleTile. That method can't deal with such a request and we fail (by not drawing the tiled image properly). Given that this happens so reliably in the mochitests (there are two specific tests that seem to trigger this behavior) I have to imagine that users might hit this too. It seems that there's no way around it, so I'm going to try to revise the patch to handle this situation when it occurs.

Rebase and minor tweaks.

Rebase.

Rebase.

Rebase.

Rebase.

So I am no longer able to reproduce the issues on Windows. See this try job, where everything looks nice and green: https://tbpl.mozilla.org/?tree=Try&rev=9ae543ece79d

Pushed in: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e573640e390 https://hg.mozilla.org/integration/mozilla-inbound/rev/98258ea50c1e https://hg.mozilla.org/integration/mozilla-inbound/rev/9129587531fe https://hg.mozilla.org/integration/mozilla-inbound/rev/3c65d8f508c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b97cab51e59

Backed out for Android reftest-2 failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/51ecec435845 https://tbpl.mozilla.org/php/getParsedLog.php?id=21123157&tree=Mozilla-Inbound

For convenience, here's the reftest analyzer link. Looks like layout/reftests/image-rect/background-with-other-properties.html, test five. https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#log=REFTEST%2520TEST-UNEXPECTED-FAIL%2520%257C%2520http%253A//10.250.49.166%253A30210/tests/layout/reftests/image-rect/background-with-other-properties.html%2520%257C%2520image%2520comparison%2520%2528%253D%253D%2529%252C%2520max%2520difference%253A%2520113%252C%2520number%2520of%2520differing%2520pixels%253A%2520124%250AREFTEST%2520%2520%2520IMAGE%25201%2520%2528TEST%2529%253A%2520data%253Aimage/png%253Bbase64%252CiVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAmy5qxAAAS50lEQVR4nO3ZsXHDMAAEQbSkUDWxF5bE1ujAuR0APNLyLgajVMxw8+MEAACIjLv/AAAA8H8IEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgsC5Bx8wEAAJ5PgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQ8XIHAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgsC5Axec4xeQEAgMcTIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABk1r3cZwNCgAAAwMcTIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkvNwBAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADLLAmSMey8AAPB8AgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACATBcgs0eAAADAnydAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMise7r/WiAXXwAA4PEECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZL3cAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgMyyABmT5xyTFwAAeDwBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBm3ct9NiAECAAAfDwBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEDGyx0AAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAILMsQMa+neN433P3bdVnAAAAF1oXIMfrHHed473qMwAAgAstDJC3AAEAAH5kAQEAADIWEAAAIGMBAQAAMhYQAAAgYwEBAAAyFhAAACBjAQEAADIWEAAAIGMBAQAAMhYQAAAgYwEBAAAyFhAAACBjAQEAADIWEAAAIGMBAQAAMhYQAAAgYwEBAAAyFhAAACBjAQEAADIWEAAAIGMBAQAAMhYQAAAgYwEBAAAyFhAAACBjAQEAADIWEAAAIGMBAQAAMhYQAAAgsy5A9u07Qo5X/7tvqz4DAAC40LIAAQAA+I0AAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgMwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHiqLwyJd4xcgMpMAAAAAElFTkSuQmCC%250AREFTEST%2520%2520%2520IMAGE%25202%2520%2528REFERENCE%2529%253A%2520data%253Aimage/png%253Bbase64%252CiVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAmy5qxAAASq0lEQVR4nO3XMRZCIRAEwb3/pb9HMAAb1Kp95ITT8wAAAETm9AcAAID/IUAAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgsy1A5vABAAD3EyAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZCx3AAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAIDMtgCZxXtm8QEAANcTIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABk9i331YAQIAAA8PMECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZyx0AAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAILMtQGbOPgAA4H4CBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAIBMFyCrJ0AAAODrCRAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMvum+9sC+fADAACuJ0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyFjuAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAACZbQEyi/fM4gMAAK4nQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADI7FvuqwEhQAAA4OcJEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyljsAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQGZbgMzhAwAA7idAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMgIEAAAICNAAACAjAABAAAyAgQAAMhY7gAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQGYAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAG71At1hfFQgiipFAAAAAElFTkSuQmCC%250AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAG71At1hfFQgiipFAAAAAElFTkSuQmCC%250A

Create our own temporary surface any time gfxUtils::DrawPixelSnapped would have if it was called on desktop Gecko. (On mobile DrawPixelSnapped won't create a temporary surface, but we need that.)

Rebase.

I'm gonna take a gamble here because I think the fix is really straightforward. (We just needed to create a temporary surface in more cases because of differences between desktop and mobile.) I don't think this needs a try run. (I did test locally.) Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/1799cffbba63 https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c38836a706 https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd51895fb0c https://hg.mozilla.org/integration/mozilla-inbound/rev/a880fd3e6e2a https://hg.mozilla.org/integration/mozilla-inbound/rev/957f2b35ce83

Still failing the same way. Backed out again. https://hg.mozilla.org/integration/mozilla-inbound/rev/446b90989fdd

Argh. Sorry about that Ryan. Will investigate further.

I was able to resolve the Android reftest failures, although not in the way I had initially hoped. There are some cases where gfxUtils::DrawPixelSnapped ends up calling CreateSamplingRestrictedDrawable despite my best efforts, and in those cases the rendering differed between Android and desktop because that code path isn't enabled on Android. I ended up approaching the problem by making it possible to override that restriction by setting a bit on the gfxContext. We also check the size of the surface we're going to create in CreateSamplingRestrictedDrawable, and refuse to allocate anything bigger than 256x256. I did this so that we'd still keep the cases we did this for and the amount of memory consumed in check; now there's just the possibility of getting nice scaling for images of reasonable size. (And the Android reftest that was failing now passes!) Try jobs here (first Linux/OS X/Windows, then Android): https://tbpl.mozilla.org/?tree=Try&rev=de96f335b1e5 https://tbpl.mozilla.org/?tree=Try&rev=45ef9b3b7a80

Comment on attachment 732166 [details] [diff] [review] Part 6 - Fix Android rendering issues caused by sampling error. Review of attachment 732166 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxContext.h @@ +708,5 @@ > void CopyAsDataURL(); > #endif > > + bool ForceCorrectResampling() { return mForceCorrectResampling; } > + void SetForceCorrectResampling(bool aForce) { mForceCorrectResampling = aForce; } Doing this feels like you're abusing gfxContext to store some internal image drawing state. I'd be much happier if this state was left to imagelib itself.

Rebased. When we need to generate a temporary surface, we always use the same code now, whether it occurs in Draw or GetFrame. (The code is implemented in GetFrameInternal.)

Rebase.

Per discussion of this issue between Jeff and myself and further discussion at the Rendering meeting, marked layout/reftests/image-rect/background-with-other-properties.html as fuzzy on Android and B2G so we don't need any workarounds.

Rebase.

Rebase.

Looks like we're good to go on Android. The reftests are all green on this try job: https://tbpl.mozilla.org/?tree=Try&rev=862a00358b5d

https://hg.mozilla.org/integration/mozilla-inbound/rev/521215a7b32e https://hg.mozilla.org/integration/mozilla-inbound/rev/fb83f2bd2eb1 https://hg.mozilla.org/integration/mozilla-inbound/rev/011194378653 https://hg.mozilla.org/integration/mozilla-inbound/rev/79cfda08fdbb https://hg.mozilla.org/integration/mozilla-inbound/rev/051cf1c1449c

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/4c45dbd81a32 - apparently just over 2 hours between your try push and your inbound push was enough time for someone to undermine you, image-rect/background-with-other-properties.html | image comparison (==), max difference: 113, number of differing pixels: 124

Ah man. That's because I forgot to push the change that added a fuzzy-if to that test. =( This bug just won't die!

I really hope this is the last time! Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/f04bd50a5c13 https://hg.mozilla.org/integration/mozilla-inbound/rev/548590c42367 https://hg.mozilla.org/integration/mozilla-inbound/rev/96764c6123e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0f65680925 https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4448dadcc3

Could this be causing the large TResize talos regressions? Regression: Mozilla-Inbound-Non-PGO - TResize - WINNT 6.2 x64 - 28.6% increase Regression: Mozilla-Inbound-Non-PGO - TResize - Win7 - 57.8% increase as well as smaller regressions for Paint: Regression: Mozilla-Inbound-Non-PGO - Paint - Win7 - 10.6% increase and Ts, Paint: Regression: Mozilla-Inbound-Non-PGO - Ts, Paint - Win7 - 3.63% increase Regression: Mozilla-Inbound-Non-PGO - Ts Paint, MAX Dirty Profile - Win7 - 3.52% increase Regression: Mozilla-Inbound-Non-PGO - Ts Paint, MED Dirty Profile - Win7 - 3.97% increase

https://hg.mozilla.org/mozilla-central/rev/f04bd50a5c13 https://hg.mozilla.org/mozilla-central/rev/548590c42367 https://hg.mozilla.org/mozilla-central/rev/96764c6123e2 https://hg.mozilla.org/mozilla-central/rev/ec0f65680925 https://hg.mozilla.org/mozilla-central/rev/ef4448dadcc3

These changes do appear to have caused the Tp regression Boris notes in comment 59. http://graphs.mozilla.org/graph.html#tests=[[255,131,12]]&sel=1365190444103.1843,1365211468436.9038&displayrange=7&datatype=running Testing this now to confirm.

They did. I am about to go into a meeting, but once I'm out I'll file a followup bug on making ClippedImage faster. Basically we need to memoize whatever temporary surfaces we end up creating. Because I think there's some potential for error there I wanted to do that as a separate bug.

OK, the regressions will be fixed in bug 859377. bz, jtd, I'll CC you.