Closed Bug 826093 Opened 12 years ago Closed 12 years ago

Remove ExtractFrame entirely and replace with a Clip method that returns an animated image

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 16 obsolete files)

(Part 1) - Add ClippedImage.
17.34 KB, patch
Details | Diff | Splinter Review
(Part 2) - Create a static utility class for image operations.
8.83 KB, patch
Details | Diff | Splinter Review
(Part 3) - Use ClippedImage instead of ExtractFrame for -moz-image-rect.
4.44 KB, patch
Details | Diff | Splinter Review
(Part 4) - Use ClippedImage instead of ExtractFrame for border-image.
1.81 KB, patch
Details | Diff | Splinter Review
(Part 5) - Remove imgIContainer::ExtractFrame.
23.87 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 822506
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.
Depends on: 843895
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.
Attached patch (Part 1) - Add ClippedImage. (obsolete) — — Splinter Review
First part, in which we add the ImageWrapper that creates a clipped "view" of its underlying image.
Attachment #723721 - Flags: review?(joe)
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.
Attachment #723725 - Flags: review?(joe)
Attachment #723725 - Flags: superreview?(bzbarsky)
Here we eliminate one of the two remaining uses of ExtractFrame.
Attachment #723726 - Flags: review?(bzbarsky)
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.
Attachment #723730 - Flags: review?(bzbarsky)
With all of that out of the way, we can finally remove ExtractFrame and all of the nastiness that it involved.
Attachment #723733 - Flags: review?(joe)
Attachment #723733 - Flags: superreview?(bzbarsky)
Blocks: 590792
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.
Attachment #723726 - Flags: review?(bzbarsky) → review+
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.
Attachment #723730 - Flags: review?(bzbarsky) → review+
Comment on attachment 723733 [details] [diff] [review] (Part 5) - Remove imgIContainer::ExtractFrame. Please rev the IID. sr=me with that.
Attachment #723733 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 723725 [details] [diff] [review] (Part 2) - Create a static utility class for image operations. sr=me
Attachment #723725 - Flags: superreview?(bzbarsky) → superreview+
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?
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
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?
Attachment #723721 - Flags: review?(joe) → review+
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
Attachment #723725 - Flags: review?(joe) → review+
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
Attachment #723733 - Flags: review?(joe) → review+
(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.
Attached patch (Part 1) - Add ClippedImage. (obsolete) — — Splinter Review
Switch to FILTER_NEAREST in ClippedImage::GetFrame. Get rid of some unneeded matrix multiplies.
Attachment #723721 - Attachment is obsolete: true
Address review comment.
Attachment #723725 - Attachment is obsolete: true
Address review comment.
Attachment #723733 - Attachment is obsolete: true
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.
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.
Blocks: 695763
Attached patch (Part 1) - Add ClippedImage. (obsolete) — — Splinter Review
Rebase and minor tweaks.
Attachment #724780 - Attachment is obsolete: true
Attachment #724781 - Attachment is obsolete: true
Attachment #723726 - Attachment is obsolete: true
Attachment #723730 - Attachment is obsolete: true
Rebase.
Attachment #724782 - Attachment is obsolete: true
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
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
Attached patch (Part 1) - Add ClippedImage. (obsolete) — — Splinter Review
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.)
Attachment #729680 - Attachment is obsolete: true
Rebase.
Attachment #729687 - Attachment is obsolete: true
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
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
Attachment #732166 - Flags: review?(jmuizelaar)
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.
Attachment #732166 - Flags: review?(jmuizelaar) → review-
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.)
Attachment #729856 - Attachment is obsolete: true
Attachment #729682 - Attachment is obsolete: true
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.
Attachment #729683 - Attachment is obsolete: true
Attachment #729686 - Attachment is obsolete: true
Attachment #729857 - Attachment is obsolete: true
Attachment #732166 - Attachment is obsolete: true
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
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!
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
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.
Blocks: 859377
OK, the regressions will be fixed in bug 859377. bz, jtd, I'll CC you.
Blocks: 505959
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: