For optimal pixel snapping results, different kinds of images should have their sizes calculated differently. For RasterImages, we should use either their native size or the size of the HQ scaled frame we'll actually use to draw, depending on whether HQ scaling can be applied. For VectorImages, we generally just want to use the size of the destination rect. For ClippedImages, we need to take the size of the underlying image into account to get the best possible pixel snapping results. For DynamicImages, we need to use the size returned by the underlying gfxDrawable. It's quite possible that there will be more cases in the future. It doesn't make sense to do these calculations in the callers (for example, in nsLayoutUtils::DrawImageInternal). The numbers here could depend on complex internal state. (prefs that can be dynamically changed, decode and scaling jobs happening on multiple threads, etc.) They also can't be moved into imgIContainer::Draw unless we want to move the whole pixel snapping algorithm in there. Right now, we have various hacks that work around this issue in several ways. For VectorImages, we adjust the image size we use for pixel snapping at the caller, and for non-pixel-snapping callers we dynamically rescale the drawing parameters inside VectorImage::Draw. For RasterImages, we do not take HQ scaling into account at all when pixel snapping, which has led in the past to rendering bugs. (We don't have any test coverage for this case either, because we disable HQ scaling for reftests. This is one reason we don't notice an issue unless someone files a bug about a specific website.) This bug is the first step to resolving this problem. It adds a new method to the imgIContainer interface that returns the optimal size for drawing the image, given the dest rect into which it will be drawn. Later bugs will update the imgIContainer::Draw API to make use of this information and will propagate it to the callers, but in this bug we'll focus on providing the foundation.

This first patch adds the new method to the imgIContainer interface.

An implementation for RasterImage. If we have an HQ-scaled frame available, it'll return that size. Otherwise it returns the intrinsic size of the image. (And will trigger an HQ scale job if appropriate.)

A very simple implementation for VectorImage. We just return the requested destination size, since we can scale SVG images freely.

Implementations for the other image types. These are mostly very simple. The only cases where we do anything nontrivial at all are OrientedImage, where we need to "rotate" the size before asking the base image, and ClippedImage, where we need to ensure that the clipping region boundaries fall on pixel boundaries to avoid sampling artifacts.

Comment on attachment 8454721 [details] [diff] [review] (Part 3) - Implement OptimalImageSizeForDest for VectorImage >+nsIntSize >+VectorImage::OptimalImageSizeForDest(const gfxSize& aDest, uint32_t aWhichFrame, GraphicsFilter aFilter, uint32_t aFlags) >+{ >+ // We can rescale SVGs freely, so just return the provided destination size. >+ return nsIntSize(ceil(aDest.width), ceil(aDest.height)); What about integer-overflow here, if aDest.width and/or height happen to be huge (or NaN)? If someone gets us to use a huge destination-size, will anything go wrong? (Can clients of this method handle the returned nsIntSize having negative components, without crashing or hanging or doing anything nasty?)

Comment on attachment 8454721 [details] [diff] [review] (Part 3) - Implement OptimalImageSizeForDest for VectorImage r=me with some subset of {comment, assertion, bounds-checking} regarding the possibility of integer overflow here. (We'd probably want the same in RasterImage in part 2, since it starts with the same nsIntSize(ceil(aDest.width), ceil(aDest.height)) conversion.)

Comment on attachment 8454719 [details] [diff] [review] (Part 2) - Implement OptimalImageSizeForDest for RasterImage Shouldn't we be using gfxSize instead of gfx::Size in light of that twitter bug where gfx:Size caused a problem? Also, in OptimalImageSizeForDest you duplicate the "mLockCount == 1" check that RequestScale is going to be doing.

(In reply to Daniel Holbert [:dholbert] from comment #6) > Comment on attachment 8454721 [details] [diff] [review] > (Part 3) - Implement OptimalImageSizeForDest for VectorImage > > r=me with some subset of {comment, assertion, bounds-checking} regarding the > possibility of integer overflow here. > > (We'd probably want the same in RasterImage in part 2, since it starts with > the same nsIntSize(ceil(aDest.width), ceil(aDest.height)) conversion.) Thanks, Daniel. I agree that we a check would be useful here. I suspect an assertion would be best, since we don't expect calling code like layout to feed us garbage - it would indicate a bug.

Thanks, Timothy! (In reply to Timothy Nikkel (:tn) from comment #7) > Shouldn't we be using gfxSize instead of gfx::Size in light of that twitter > bug where gfx:Size caused a problem? I just need to rebase the patch, but yes. Though actually we should be able to revert the switch back to gfxSize once the rest of my patch series lands, but I don't want to make this bug depend on it. > Also, in OptimalImageSizeForDest you duplicate the "mLockCount == 1" check > that RequestScale is going to be doing. Good catch, thanks.

Comment on attachment 8454716 [details] [diff] [review] (Part 1) - Add OptimalImageSizeForDest to the imgIContainer interface Please document what aFlags means. Presumably it's supposed to match the flags that will be passed to some other API call, but you should say which one. Probably similar for aFilter, actually, but it's particularly unclear for aFlags. sr=me

Comment on attachment 8454725 [details] [diff] [review] (Part 4) - Implement OptimalImageSizeForDest for other image types >+nsIntSize >+ClippedImage::OptimalImageSizeForDest(const gfxSize& aDest, uint32_t aWhichFrame, >+ GraphicsFilter aFilter, uint32_t aFlags) >+{ >+ if (!ShouldClip()) { >+ return InnerImage()->OptimalImageSizeForDest(aDest, aWhichFrame, aFilter, aFlags); >+ } >+ >+ int32_t imgWidth, imgHeight; >+ if (NS_SUCCEEDED(InnerImage()->GetWidth(&imgWidth)) && >+ NS_SUCCEEDED(InnerImage()->GetHeight(&imgHeight))) { >+ // To avoid ugly sampling artifacts, ClippedImage needs the image size to >+ // be chosen such that the clipping region lies on pixel boundaries. >+ >+ // First, we select a scale that's good for ClippedImage. An integer multiple >+ // of the size of the clipping region is always fine. >+ nsIntSize scale(SafeScale(aDest.width / mClip.width, >+ aDest.height / mClip.height)); >+ >+ // Determine the size we'd prefer to render the inner image at, and ask the >+ // inner image what size we should actually use. >+ gfxSize desiredSize(imgWidth * scale.width, imgHeight * scale.height); >+ nsIntSize innerDesiredSize = >+ InnerImage()->OptimalImageSizeForDest(desiredSize, aWhichFrame, >+ aFilter, aFlags); >+ >+ // To get our final result, we take the inner image's desired size and >+ // determine how large the clipped region would be at that scale. (Again, we >+ // ensure an integer multiple of the size of the clipping region.) >+ nsIntSize finalScale(SafeScale(innerDesiredSize.width / imgWidth, >+ innerDesiredSize.height / imgHeight)); >+ return mClip.Size() * finalScale; >+ } else { >+ MOZ_ASSERT(false, "If ShouldClip() led us to draw then we should never get here"); >+ return InnerImage()->OptimalImageSizeForDest(aDest, aWhichFrame, aFilter, aFlags); >+ } >+} I can't help but feel this is too complicated. Do we actually need this? In this code we convert from float to int by passing floats to SafeScale which takes int. We should make this explicit. Also, don't we want to take the ceiling (and not round or floor)?

(In reply to Timothy Nikkel (:tn) from comment #11) > I can't help but feel this is too complicated. Do we actually need this? In the entire series of refactoring patches I'm in the process of posting to Bugzilla now, this was one of the trickiest things to get right. We do need this, unfortunately, if we want SVG images to be rendered at higher resolution (to the degree that we can get away with it) while also avoiding sampling issues. Given that we want to aim for whatever InnerImage()->OptimalImageSizeForDest returns, as best we can, to get higher resolution rendering for SVG images, the considerations that led to this approach are as follows: (1) We must do our best to ensure that the scaled clipping region maps onto integer coordinates within the scaled underlying image. (2) If we request an OptimalImageSizeForDest from RasterImage, RasterImage will make a decision as to whether it can scale to that size, and will go ahead and kick off a scaling job if it could but doesn't have an existing HQ scaled frame around. If we change the size we return after calling RasterImage::OptimalImageSizeForDest, we may change whether RasterImage would have approved scaling to that size at all, and we will definitely render the new scaling job useless. So it pays off to request a size that we know we can live with. (3) However, there is no particular contract as to what the inner image may return. That means that we still need to ensure that the result it gives us will avoid causing sampling issues when clipping. So we still need to normalize the result to ensure it's an integer scale. Neither RasterImage nor VectorImage will actually return anything, currently, that should be affected by this normalization, but I see this as something that could change. > In this code we convert from float to int by passing floats to SafeScale > which takes int. We should make this explicit. I agree that that should be more explicit. > Also, don't we want to take the ceiling (and not round or floor)? Yeah, maybe so. I debated with myself about that, and I honestly can't remember now why I came to the conclusion that round-towards-zero was better. I have no objection to switching to ceil.

Comment on attachment 8454725 [details] [diff] [review] (Part 4) - Implement OptimalImageSizeForDest for other image types Alright.

Address review comments and rebase.

Address review comments and rebase.

Address review comments and rebase.

Address review comments and rebase.

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/45c27b13ba12 https://hg.mozilla.org/integration/mozilla-inbound/rev/2201890b1c3d https://hg.mozilla.org/integration/mozilla-inbound/rev/46afb11b77a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f1d2855ea8

https://hg.mozilla.org/mozilla-central/rev/45c27b13ba12 https://hg.mozilla.org/mozilla-central/rev/2201890b1c3d https://hg.mozilla.org/mozilla-central/rev/46afb11b77a2 https://hg.mozilla.org/mozilla-central/rev/b0f1d2855ea8