Make imgIContainer::Draw simpler, more accurate, and better suited to our needs going forward

RESOLVED FIXED in mozilla34

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 19 obsolete attachments)

7.48 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.71 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
20.81 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.77 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
10.55 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
12.78 KB, patch
Details | Diff | Splinter Review
6.77 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.69 KB, patch
Details | Diff | Splinter Review
22.17 KB, patch
Details | Diff | Splinter Review
15.90 KB, patch
Details | Diff | Splinter Review
31.53 KB, patch
Details | Diff | Splinter Review
1.57 KB, patch
Details | Diff | Splinter Review
16.12 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
20.49 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
This bug is the meat of the Draw API refactoring. It contains a lot of changes
throughout the drawing pipeline, all supporting the following three goals:

1. Rework the imgIContainer::Draw API to be a better match for what the code
   that uses it actually needs. This includes making simple calls to Draw look
   as simple as possible, while still retaining the power that some callers
   need. It also includes choosing a coordinate system to describe the arguments
   in that's a better match for what the downstream code actually needs to use.
   One part of this API change is important enough that it's worth calling out
   as its own goal:

2. Allow Draw callers to explicitly request that an image be drawn at a specific
   "source size". We need this for a variety of reasons. It gives us a path to
   supporting new features like downsample-during-decode and proper
   multiresolution support for icons. It also enables better interaction between
   between the pixel snapping algorithm and features like HQ scaling,
   downscale-during-decode, high-DPI SVG image rendering, and border-image that
   were previous supported by a variety of hacks that resulted in rendering
   issues. This goal thus ties deeply into the final goal:

3. Try, where possible, to avoid operations that decrease the numerical accuracy
   of the calculations we make while drawing. Most significantly, this includes
   avoiding inverting matrices wherever possible. (This also has the nice side
   effect that we don't *need* the matrices in question to be invertible, and
   can avoid some previously-needed error checks.) But a lot of the improvement
   from these patches comes from being smarter about our data representation
   (see the first goal) and enabling a bit more communication between the pixel
   snapping algorithm and later stages of the drawing pipeline (see the second
   goal).

When I started on this work, I intended that each of these goals would be
supported by a separate patch. However, as I continued to work on them, I
realized that these three things were very much intertwined, and satisfying each
goal was easier if the other goals were also satisfied. There are some minor
changes in this patch that could probably be pulled out into a separate bug, but
I think at this point there are diminishing returns from doing so. (The metabug,
bug 1037713, is blocked by some of the larger pieces that seemed reasonable to
review separately.)

There are a lot of small changes in these patches, which mostly result from
following the consequences of the larger changes, but I think it's worth
pointing out the most important ways that each goal is satisfied:

1. imgIContainer::Draw now takes a much-simplified set of arguments. The
   user-space-to-image-space matrix concept has been eliminated: there's now
   only one matrix involved, the one on the gfxContext. Additionally, the fill
   and subimage arguments, which are very closely related (the subimage is just
   the fill projected into image space, and snapped to include only the pixels
   that should actually be sampled) are combined into a single ImageRegion type
   which serves both roles.
   
   The ImageRegion is the fill in *image-space*, which is the coordinate space
   we actually need it in throughout the drawing pipeline. It optionally also
   includes a sampling restriction rect, which takes the place of the subimage.
   (The fact this is optional is convenient for non-pixel-snapping callers -
   everyone but DrawImageInternal - since they don't need this at all.) When
   these arguments are transformed for various reasons in the drawing pipeline,
   they both always need to have the same transformations applied, and
   ImageRegion also serves to ensure that that always happens.

2. Also as part of the changes to the imgIContainer::Draw arguments, a new size
   parameter lets the caller specify the image size they want to draw. The image
   can then produce the requested size by whatever means it has available,
   including select from the multiple resolution layers of ICO images, using an
   HQ scaled frame, or just adding a scale to the gfxContext matrix.
   
   When we want to pixel snap, this is additionally supported by
   imgIContainer::OptimalSizeForDest (introduced in bug 1034345), which lets the
   caller learn the best size the image has available for the given dest rect
   and use that size as an input to the pixel snapping algorithm. This means
   that pixel snapping can correctly take into account things like HQ scaling
   which it currently is unaware of, fixing rendering issues that we currently
   can't avoid.

3. One way these patches improve numerical accuracy is simply to work in image
   space where possible and avoid matrix inversions and coordinate space
   conversions that not doing so forced on us. When we need both a matrix and
   its inverse (for example, in ComputedSnappedDrawingParameters or in
   OrientedImage::Draw), we compute both from scratch instead of computing one
   and then inverting it.

   A second way is really just goal two! It turns out that the hacks we used to
   make things like HQ scaling and high-DPI SVG rendering work introduce extra
   calculations that reduce numerical accuracy. We can improve accuracy by just
   *doing fewer calculations*, and goal two helps us do that.

   The final major numerical accuracy change is to introduce a second pixel
   snapping algorithm in ComputeSnappedDrawingParameters. The existing algorithm
   is hard to improve on for the general case, but it turns out that we don't
   need its power much of the time. For the simple, common case, where we are
   drawing a single copy of an image and we don't have a special anchor point to
   worry about, we can afford to use a more straightforward algorithm that just
   computes the transformation from the rect we're sampling from in the source
   image to the rect we're drawing into. This introduces much less opportunity
   for numerical error when we can use it.

In the end, these patches give us a cleaner API, fix every known image rendering
issue associated with numerical accuracy (see the bugs that bug 1037713 blocks),
and lay the foundation for new features that we really want, including
downsample-on-decode (which will enable major memory savings for some pages) and
media fragments for images (which have been unable to land because of technical
issues that these patches solve). There's a lot to review here, but hopefully
those benefits will make it worth it.
(Assignee)

Updated

3 years ago
Blocks: 846427
(Assignee)

Comment 1

3 years ago
Created attachment 8461757 [details] [diff] [review]
(Part 0) - Revert bug 1006123 part 1

First, some setup. We'll revert bug 1006123 part 1, which switched us from using
floats in some RasterImage methods related to drawing to using doubles. This
fixed bug 1006123 at the default zoom level, but as bug 1016018 shows, there's
still a problem when the page gets zoomed, so the gain from switching to doubles
is limited. In this bug we'll fix the problem in a different, more fundamental
way that enables us to keep using floats, so let's revert that patch.
Attachment #8461757 - Flags: review?(dholbert)
(Assignee)

Comment 2

3 years ago
Created attachment 8461759 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

This patch introduces the new imgIContainer::Draw API and the ImageRegion type which supports it. As mentioned in comment 0, ImageRegion captures both the aFill and the aSubimage parameters from the old API.
Attachment #8461759 - Flags: review?(tnikkel)
Attachment #8461759 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8461759 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 3

3 years ago
Created attachment 8461771 [details] [diff] [review]
(Part 2) - Update RasterImage to the new Draw API

This patch updates RasterImage to the new Draw API.
Attachment #8461771 - Flags: review?(tnikkel)
(Assignee)

Comment 4

3 years ago
Created attachment 8461777 [details] [diff] [review]
(Part 3) - Update VectorImage and supporting classes to the new Draw API

This patch updates VectorImage to the new API. It also propagates more-or-less minor changes to several VectorImage-related classes. The two significant changes outside of VectorImage are:

* SVGSVGElement::SetImageOverridePreserveAspectRatio now recognizes _UNKNOWN
  values and treats them as a no-op.

* SVGImageContext now contains the viewport size as well.
Attachment #8461777 - Flags: review?(dholbert)
(Assignee)

Comment 5

3 years ago
Created attachment 8461780 [details] [diff] [review]
(Part 4) - Update ClippedImage to the new Draw API

This patch updates ClippedImage to the new Draw API.
Attachment #8461780 - Flags: review?(tnikkel)
(Assignee)

Comment 6

3 years ago
Created attachment 8461783 [details] [diff] [review]
(Part 5) - Update OrientedImage to the new Draw API

Same for OrientedImage. (Note, by the way, that OrientedImage::FrameRect was implemented incorrectly before.)
Attachment #8461783 - Flags: review?(tnikkel)
(Assignee)

Comment 7

3 years ago
Created attachment 8461785 [details] [diff] [review]
(Part 6) - Update the remaining Image classes to the new Draw API

This covers the rest of the Image subclasses.
Attachment #8461785 - Flags: review?(tnikkel)
(Assignee)

Comment 8

3 years ago
Created attachment 8461788 [details] [diff] [review]
(Part 7) - Propagate the Draw API changes to imgFrame

This patch propagates the API changes into imgFrame.

Michael, I think you've touched this code the most recently, and I rebased this code over a lot of changes from you, so it'd be great if you could verify that I took all of the recent changes into account.
Attachment #8461788 - Flags: review?(tnikkel)
Attachment #8461788 - Flags: review?(mwu)
(Assignee)

Comment 9

3 years ago
Created attachment 8461792 [details] [diff] [review]
(Part 8) - Propagate the Draw API changes into gfxUtils

This patch propagates the Draw API changes into gfxUtils::DrawPixelSnapped and related functions.

Right now DrawPixelSnapped and friends are only called by imagelib, so I suspect it should actually get moved there, but this bug doesn't seem like the right place to do it.
Attachment #8461792 - Flags: review?(matt.woodrow)
(Assignee)

Comment 10

3 years ago
Created attachment 8461803 [details] [diff] [review]
(Part 9) Update canvas to use the new Draw API

Now we switch from propagating deeper into the drawing pipeline, past
imgIContainer::Draw, to propogating to callers of imgIContainer::Draw. First up,
canvas. This patch updates canvas to use the new Draw API. By doing so, it also
fixes the existing bug where the context scale is not taken into account when
drawing SVGs via canvas, resulting in blurry results. (That's bug 941467.)
Attachment #8461803 - Flags: review?(bjacob)
(Assignee)

Comment 11

3 years ago
Created attachment 8461832 [details] [diff] [review]
(Part 10) Update nsCocoaUtils to use the new Draw API

This propagates the changes to nsCocoaUtils.
Attachment #8461832 - Flags: review?(mstange)
(Assignee)

Comment 12

3 years ago
Created attachment 8461903 [details] [diff] [review]
(Part 11) Update nsBaseDragService to use the new Draw API

Same for nsBaseDragService.
Attachment #8461903 - Flags: review?(roc)
(Assignee)

Comment 13

3 years ago
Created attachment 8461918 [details] [diff] [review]
(Part 12) Improve pixel snapping accuracy and use the new Draw API in nsLayoutUtils

This patch propagates the new Draw API to nsLayoutUtils, and further propagates
some minor changes to the DrawImage* APIs nsLayoutUtils exports.

There are substantial changes to ComputeSnappedImageDrawingParameters to make
its output match the requirements of the new imgIContainer::Draw API better, and
to make it take advantage of OptimalImageSizeForDest.

In addition, the new code also avoids the inverting matrices to increase
numerical accuracy, and it adds a new super-simple method of calculation the
transformation matrix for cases where we don't need the full complexity of
anchor points. In those simple cases, the new method produces results with less
error.
Attachment #8461918 - Flags: review?(roc)
(Assignee)

Comment 14

3 years ago
Comment on attachment 8461759 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

(Switching the superreviewer for the API change to Jeff.)
Attachment #8461759 - Flags: superreview?(bzbarsky) → superreview?(jmuizelaar)
(Assignee)

Comment 15

3 years ago
Created attachment 8461920 [details] [diff] [review]
(Part 13) Update the rest of layout to reflect the nsLayoutUtils changes

Finally, this propagates the nsLayoutUtils::Draw*Image changes to the rest of layout.
Attachment #8461920 - Flags: review?(roc)
(Assignee)

Comment 16

3 years ago
Whew, that's all of the code-related changes. Part 14 is still coming with reftest changes. I'm thinking about adding a couple more reftests, so I'll hold off on posting that.
(Assignee)

Updated

3 years ago
Depends on: 913586
Comment on attachment 8461792 [details] [diff] [review]
(Part 8) - Propagate the Draw API changes into gfxUtils

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

::: gfx/thebes/gfxUtils.h
@@ +76,5 @@
>       * algorithm before passing them on to this method.
>       */
> +    static void DrawPixelSnapped(gfxContext*        aContext,
> +                                 gfxDrawable*       aDrawable,
> +                                 const gfxRect&     aImageRect,

All callers of this pass 0,0 for the position component of aImageRect, so let's change this to a size. It also appears like things wouldn't work correctly if someone passed an offset, so best not to give that option.
Attachment #8461792 - Flags: review?(matt.woodrow) → review+
Attachment #8461757 - Flags: review?(dholbert) → review+
Version: unspecified → Trunk
Comment on attachment 8461803 [details] [diff] [review]
(Part 9) Update canvas to use the new Draw API

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

I'm not going to be a good reviewer for this patch, because I had no clue about "the new Draw API". Defaulting to Matt since he's already involved on this bug / will know whom to reassign again if needed.
Attachment #8461803 - Flags: review?(bjacob) → review?(matt.woodrow)
(Assignee)

Comment 19

3 years ago
(In reply to Benoit Jacob [:bjacob] from comment #18)
> I'm not going to be a good reviewer for this patch, because I had no clue
> about "the new Draw API". Defaulting to Matt since he's already involved on
> this bug / will know whom to reassign again if needed.

The new Draw API is the one defined in Part 1 in this bug...
Comment on attachment 8461918 [details] [diff] [review]
(Part 12) Improve pixel snapping accuracy and use the new Draw API in nsLayoutUtils

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

::: layout/base/nsLayoutUtils.cpp
@@ +4903,5 @@
> + * @param aFrom The rect to be transformed.
> + * @param aTo The rect that aFrom should be mapped onto by the transformation.
> + */
> +static gfxMatrix
> +TransformBetweenRects(const gfxRect& aFrom, const gfxRect& aTo)

gfxUtils has TransformRectToRect for this.
Comment on attachment 8461803 [details] [diff] [review]
(Part 9) Update canvas to use the new Draw API

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3470,5 @@
> +  dest.Scale(contextScale.width, contextScale.height);
> +
> +  // Scale the image size to the dest rect, and adjust the source rect to match.
> +  gfxSize scale(dest.width / src.width, dest.height / src.height);
> +  nsIntSize imageSize(std::ceil(imgSize.width * scale.width),

imgSize and imageSize seem very similar, can we name the latter destImageSize, devPixelImageSize or similar?

@@ +3472,5 @@
> +  // Scale the image size to the dest rect, and adjust the source rect to match.
> +  gfxSize scale(dest.width / src.width, dest.height / src.height);
> +  nsIntSize imageSize(std::ceil(imgSize.width * scale.width),
> +                      std::ceil(imgSize.height * scale.height));
> +  src.Scale(scale.width, scale.height);

Extracting the context scale factors and moving them onto the image size instead seems very similar to what the nsLayoutUtils functions are doing (among other things). Is it possible to use that code somehow instead of reimplementing it here?

@@ +3478,5 @@
>    nsRefPtr<gfxContext> context = new gfxContext(tempTarget);
>    context->SetMatrix(contextMatrix);
> +  context->Scale(1.0 / contextScale.width, 1.0 / contextScale.height);
> +  context->Translate(gfxPoint(dest.x - src.x, dest.y - src.y));
> +  

Trailing whitespace.
(Assignee)

Comment 22

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> gfxUtils has TransformRectToRect for this.

Nice! I hadn't noticed that.
Comment on attachment 8461832 [details] [diff] [review]
(Part 10) Update nsCocoaUtils to use the new Draw API

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

::: widget/cocoa/nsCocoaUtils.mm
@@ +484,4 @@
>  
>      RefPtr<DrawTarget> drawTarget = gfxPlatform::GetPlatform()->
> +      CreateOffscreenContentDrawTarget(IntSize(scaledSize.width,
> +                                               scaledSize.height),

you could use ToIntSize from gfx2DGlue.h here if you want to
Attachment #8461832 - Flags: review?(mstange) → review+

Comment 24

3 years ago
Comment on attachment 8461788 [details] [diff] [review]
(Part 7) - Propagate the Draw API changes to imgFrame

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

The drawing path is probably the part of the image code that I understand the least. Things don't look wrong though..

::: image/src/imgFrame.cpp
@@ +347,5 @@
> +                                    matrix._21,
> +                                    matrix._12,
> +                                    matrix._22,
> +                                    matrix._31,
> +                                    matrix._32));

Looks like there's a ToMatrix in gfx2DGlue.h for this.
Attachment #8461788 - Flags: review?(mwu) → feedback+
Seth, could it be that this (or one of the other related bugs) also fixes bug 993325?

Sebastian
Blocks: 1044702
Attachment #8461903 - Flags: review?(roc) → review+
Comment on attachment 8461918 [details] [diff] [review]
(Part 12) Improve pixel snapping accuracy and use the new Draw API in nsLayoutUtils

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

Looks good modulo the pointer issue.

::: layout/base/nsLayoutUtils.cpp
@@ +5059,5 @@
> +  // account when computing the matrices above.
> +  if (!didSnap) {
> +    transform = transform * currentMatrix;
> +  }
> +    

trailing whitespace

::: layout/base/nsLayoutUtils.h
@@ +1545,5 @@
> +                                  imgIContainer*      aImage,
> +                                  GraphicsFilter      aGraphicsFilter,
> +                                  const nsRect&       aDest,
> +                                  const nsRect&       aDirty,
> +                                  const mozilla::Maybe<mozilla::SVGImageContext>& aSVGContext,

Why this change?

Sure, it's conceptually cleaner in some sense, but using NULL pointers to turn pointers into a "Some<T> | None" is deeply ingrained into C++ and Gecko and I don't think it's worth backing away from.

@@ +1575,5 @@
>    /**
> +   * Given an imgIContainer, this method attempts to obtain an intrinsic
> +   * px-valued height & width for it.  If the imgIContainer has a non-pixel
> +   * value for either height or width, this method tries to generate a pixel
> +   * value for that dimension using the intrinsic ratio (if available). If

Mention that aFallbackSize is used in conjunction with the ratio in this case.
Comment on attachment 8461920 [details] [diff] [review]
(Part 13) Update the rest of layout to reflect the nsLayoutUtils changes

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

Mostly won't be necessary if you revert the Maybe change :-)
Attachment #8461920 - Flags: review?(roc) → review-
(Assignee)

Comment 28

3 years ago
Thanks for the reviews!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Why this change?
> 
> Sure, it's conceptually cleaner in some sense, but using NULL pointers to
> turn pointers into a "Some<T> | None" is deeply ingrained into C++ and Gecko
> and I don't think it's worth backing away from.

I mainly made this change for consistency with imgIContainer::Draw's new API, which also uses Maybe<T> for this parameter. The case is stronger there, because switching to Maybe<T> resulted in a substantial cleanup of the code.

I agree, though, that other than consistency switching to Maybe<T> for nsLayoutUtils::DrawSingleImage doesn't add much value right now. I'll revert that change; we can always reconsider if down the road new code would make more use of it.
Comment on attachment 8461759 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

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

Overall, this seems like an improvement.

::: image/src/ImageRegion.h
@@ +16,5 @@
> + * restriction rect. The drawing code ensures that if a sampling restriction
> + * rect is present, any pixels sampled during the drawing process are found
> + * within that rect.
> + */
> +class ImageRegion

Image region makes me think that it's related to nsRegion, maybe another name would be better. Since the rects are in image space why are they floating point?
Attachment #8461759 - Flags: superreview?(jmuizelaar) → superreview+
(Assignee)

Comment 30

3 years ago
Created attachment 8463652 [details] [diff] [review]
(Part 14) - Update reftests

The final part: reftest updates. The new reftests are actually added in bug
942364 and bug 925611, since those are the original scaling bugs that inspired
the numerical accuracy changes in this bug. They're marked as random in those
bugs, so here we just remove the random annotation. There are also a couple of
previously failing reftests that now pass and a couple of fuzziness changes.
(Alas, there are probably many improvements to fuzziness, but TBPL
doesn't tell you about that. It might be interesting to write some code to
analyze TBPL logs and figure out where we have unnecessary fuzziness.)
Attachment #8463652 - Flags: review?(tnikkel)
Attachment #8463652 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 31

3 years ago
Thanks for the reviews!

(In reply to Jeff Muizelaar [:jrmuizel] from comment #29)
> Image region makes me think that it's related to nsRegion, maybe another
> name would be better. Since the rects are in image space why are they
> floating point?

I'm open to suggestions of alternate names. =) I haven't had much luck coming up with one that seems better.

ImageRegion::mRect is floating point because it's valid to request that we draw part of an image pixel. We have tests that rely on that working.

ImageRegion::mRestriction semantically *should* be an integer rect, but the thing is that we may perform multiple transformations on it in sequence. Imagine an image that has an xywh media fragment, causing clipping, and on top of that has the CSS image-orientation property set on it. The clipping and the rotation will each transform the restriction, and then RasterImage::DrawWithPreDownscaleIfNeeded might additionally scale it. If we used an integer rect, we'd round at each of these intermediate points, and the accumulated error could be huge.
(Assignee)

Comment 32

3 years ago
(In reply to Sebastian Zartner from comment #25)
> Seth, could it be that this (or one of the other related bugs) also fixes
> bug 993325?
> 
> Sebastian

I'll take a look and see.
(Assignee)

Comment 33

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Extracting the context scale factors and moving them onto the image size
> instead seems very similar to what the nsLayoutUtils functions are doing
> (among other things). Is it possible to use that code somehow instead of
> reimplementing it here?

I see what you mean (actually the code in RasterImage::DrawWithPreDownscaleIfNeeded is even more similar), but I don't see an obvious way to merge them. I considered adding a new imgIContainer::DrawSimple in this patch that took a source rect and a dest rect and figured things out from there. If that existed, the implementation could share the code in DrawWithPreDownscaleIfNeeded, and it'd clean up the CanvasRenderingContext2D call site a bit. Probably that's a level of complexity that's better suited for a followup, though.
(Assignee)

Updated

3 years ago
Blocks: 1045418
(Assignee)

Comment 34

3 years ago
I added bug 1045418 as a followup to use gfxUtils::TransformRectToRect, since I think we need a new overload to use it cleanly.
(Assignee)

Comment 35

3 years ago
Created attachment 8463779 [details] [diff] [review]
(Part 2) - Update RasterImage to the new Draw API

OK, it's time to update the parts that have changed to reflect review comments
up to this point. I've incorporated everyone's feedback so far unless otherwise
mentioned above. Several of the parts had to be updated because of Matt's
feedback for part 8, so if you're wondering why I'm updating parts that haven't
had any feedback yet, that's why.
Attachment #8463779 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8461771 - Attachment is obsolete: true
Attachment #8461771 - Flags: review?(tnikkel)
(Assignee)

Comment 36

3 years ago
Created attachment 8463780 [details] [diff] [review]
(Part 3) - Update VectorImage and supporting classes to the new Draw API
Attachment #8463780 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8461777 - Attachment is obsolete: true
Attachment #8461777 - Flags: review?(dholbert)
(Assignee)

Comment 37

3 years ago
Created attachment 8463781 [details] [diff] [review]
(Part 4) - Update ClippedImage to the new Draw API
Attachment #8463781 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8461780 - Attachment is obsolete: true
Attachment #8461780 - Flags: review?(tnikkel)
(Assignee)

Comment 38

3 years ago
Created attachment 8463782 [details] [diff] [review]
(Part 5) - Update OrientedImage to the new Draw API
Attachment #8463782 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8461783 - Attachment is obsolete: true
Attachment #8461783 - Flags: review?(tnikkel)
(Assignee)

Comment 39

3 years ago
Created attachment 8463783 [details] [diff] [review]
(Part 6) - Update the remaining Image classes to the new Draw API
Attachment #8463783 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8461785 - Attachment is obsolete: true
Attachment #8461785 - Flags: review?(tnikkel)
(Assignee)

Comment 40

3 years ago
Created attachment 8463784 [details] [diff] [review]
(Part 7) - Propagate the Draw API changes to imgFrame
Attachment #8463784 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8461788 - Attachment is obsolete: true
Attachment #8461788 - Flags: review?(tnikkel)
(Assignee)

Comment 41

3 years ago
Created attachment 8463785 [details] [diff] [review]
(Part 8) - Propagate the Draw API changes into gfxUtils
(Assignee)

Updated

3 years ago
Attachment #8461792 - Attachment is obsolete: true
(Assignee)

Comment 42

3 years ago
Created attachment 8463786 [details] [diff] [review]
(Part 9) Update canvas to use the new Draw API

Updated to include all of your feedback except merging the calculations. (See comment 33.)
Attachment #8463786 - Flags: review?(matt.woodrow)
(Assignee)

Updated

3 years ago
Attachment #8461803 - Attachment is obsolete: true
Attachment #8461803 - Flags: review?(matt.woodrow)
(Assignee)

Comment 43

3 years ago
Created attachment 8463787 [details] [diff] [review]
(Part 10) Update nsCocoaUtils to use the new Draw API
(Assignee)

Updated

3 years ago
Attachment #8461832 - Attachment is obsolete: true
(Assignee)

Comment 44

3 years ago
Created attachment 8463789 [details] [diff] [review]
(Part 12) Improve pixel snapping accuracy and use the new Draw API in nsLayoutUtils

All the feedback so far has been addressed. I'm not 100% what you were going for
with the comment for ComputeSizeForDrawingWithFallback, but I've reworded it to
hopefully be clearer. The Maybe<T> API change has been removed.
Attachment #8463789 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8461918 - Attachment is obsolete: true
Attachment #8461918 - Flags: review?(roc)
(Assignee)

Comment 45

3 years ago
Created attachment 8463790 [details] [diff] [review]
(Part 13) Update the rest of layout to reflect the nsLayoutUtils changes

There's indeed almost nothing in this patch anymore, but that should make it an
easy review. =) All of these patches will end up getting folded together anyway,
so I'll just leave this as a separate patch for now.
Attachment #8463790 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8461920 - Attachment is obsolete: true
Attachment #8463786 - Flags: review?(matt.woodrow) → review+
(In reply to Seth Fowler [:seth] from comment #31)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #29)
> > Image region makes me think that it's related to nsRegion, maybe another
> > name would be better. Since the rects are in image space why are they
> > floating point?
> 
> I'm open to suggestions of alternate names. =) I haven't had much luck
> coming up with one that seems better.

ImagePart?
Attachment #8463789 - Flags: review?(roc) → review+
Attachment #8463790 - Flags: review?(roc) → review+
(I'm assuming this bug should depend on bug 1034345, since at least patch 1 and 12 here mention / use OptimalImageSizeForDest.)
Depends on: 1034345
Comment on attachment 8461759 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

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

> Bug 1043560 (Part 1) - Refactor the imgIContainer::Draw API. r=tn,dholbert sr=bz
(The commit message needs s/bz/jrmuizel/, but you probably already know that or did it locally.)

::: image/public/imgIContainer.idl
@@ +233,5 @@
>     *
>     * @param aContext The Thebes context to draw the image to.
> +   * @param aSize The size at which the image should be drawn. For raster
> +   *              images with multiple resolutions available, the closest
> +   *              matching resolution will be used, and scaling will be used to 

Delete the end-of-line whitespace.

@@ +238,5 @@
> +   *              ensure that the image is drawn at the requested size. For
> +   *              vector images, the exact size requested will be drawn. Callers
> +   *              can select an optimal value for aSize using
> +   *              optimalImageSizeForDest if there are no special size
> +   *              requirements.

s/select/request/, perhaps?

Also, add "()" after optimalImageSizeForDest, to make clear that it's a function (not e.g. another arg or a flag or something).

@@ +240,5 @@
> +   *              can select an optimal value for aSize using
> +   *              optimalImageSizeForDest if there are no special size
> +   *              requirements.
> +   * @param aRegion The region in the context to draw pixels to. When aFlags
> +   *                includes includes FLAG_CLAMP, the image will be extended to

s/includes includes/includes/

@@ +244,5 @@
> +   *                includes includes FLAG_CLAMP, the image will be extended to
> +   *                this area by clamping image sample coordinates. Otherwise,
> +   *                the image will be automatically tiled as necessary. aRegion
> +   *                also optionally specifies a restricted area of the image, in
> +   *                pixels, that we are allowed to sample from.

I was very confused by this comment about the optional dual nature of aRegion, until I read the ImageRegion header file.

Could you clarify this comment a bit, to make it clearer that the ImageRegion type (and aRegion) actually stores two distinct regions?  (Right now, it sounds (to me at least) like this arg is a *single* region in space which optionally can be interpreted to mean two different things.)

@@ +268,2 @@
>                         in gfxGraphicsFilter aFilter,
> +                       [const] in MaybeSVGImageContext aSVGContext,

Why is aSVGContext using Maybe<> now, instead of a pointer? Might be worth mentioning the reason for that choice in this function's documentation.

::: image/src/ImageRegion.h
@@ +32,5 @@
> +
> +  static ImageRegion Create(const nsIntSize& aSize)
> +  {
> +    return ImageRegion(gfxRect(0, 0, aSize.width, aSize.height));
> +  }

Nit: do we really need this nsIntSize-accepting convenience-wrapper?

Note that we don't have this for the CreateWithSamplingRestriction() factory-method -- just for Create(). I'm guessing this is just for the convenience of one or two clients? (It looks like Part 11 uses it for nsBaseDragService::DrawDragForImage; I didn't look closely enough at the other patches to see if they use it as well.)

If it's not too much more complex, it seems like it might be better to drop this convenience factory-method and force the client to do the conversion, if any conversion needs doing. Otherwise, we're abstracting away some potentially-lossy conversion in a non-obvious way, which could make it more confusing to track down the source of a loss-of-precision. I'd prefer making the int-to-float conversion more explicit in the clients. (unless it really happens all over the place & this wrapper saves us tons of code)

(Also: if there are any clients that have already created a gfxSize or gfxRect for their target-size, then it will also become clearer [if we drop the nsIntSize method] that they should pass *that* [the gfxSize/gfxRect] to ::Create(), instead of passing the nsIntSize that they derived it from [and redundantly converting from int to float].)

@@ +41,5 @@
> +    return ImageRegion(aRect, aRestriction);
> +  }
> +
> +  gfxRect Rect() const         { return mRect; }
> +  gfxRect Restriction() const  { return mRestriction; }

Perhaps Restriction() should assert about mIsRestricted?  (I imagine callers be checking whether we're restricted before trying to read mRestriction; and if anyone tries to read the Restriction() on a non-restricted ImageRegion, that's probably a bug.)

Also, perhaps these accessors should return type "const gfxRect&", to reduce needless copying around of full rects?

@@ +134,5 @@
> +  { }
> +
> +  gfxRect mRect;
> +  gfxRect mRestriction;
> +  bool    mIsRestricted : 1;

I'm not sure it's worth forcing this bool to be 1 bit here, given that it's the only bool (it's not part of a bitfield).  IIRC there can be a (small) performance penalty to doing this, so it only really makes sense when there are multiple bools.

So, maybe consider dropping the ": 1" suffix here.

@@ +138,5 @@
> +  bool    mIsRestricted : 1;
> +};
> +
> +}
> +}

Mention the namespace being closed, e.g.:

} // namespace image
} // namespace mozilla
Attachment #8461759 - Flags: review?(dholbert) → review+
(Assignee)

Comment 49

3 years ago
Thanks for the reviews! Very useful feedback!

(In reply to Daniel Holbert [:dholbert] from comment #48)
> Nit: do we really need this nsIntSize-accepting convenience-wrapper?
> 
> If it's not too much more complex, it seems like it might be better to drop
> this convenience factory-method and force the client to do the conversion,
> if any conversion needs doing. Otherwise, we're abstracting away some
> potentially-lossy conversion in a non-obvious way, which could make it more
> confusing to track down the source of a loss-of-precision.

There's no loss of precision converting from an int32_t to a double, so it should be OK to do it automatically. There are several callers using this overload, so I'd prefer not to remove it.

> Note that we don't have this for the CreateWithSamplingRestriction()
> factory-method -- just for Create().

We don't have this for CreateWithSamplingRestriction() because there's only one caller that uses that - it's only for pixel snapping code, which means it only gets called in nsLayoutUtils::ComputeSnappedImageDrawingParameters() right now. Actually, one of the reasons for introducing the ImageRegion type is exactly that only one caller needed to specify a restricted sampling region (the old aSubimage argument). Thus it's expected that we'd have more overloads for Create() than for CreateWithSamplingRestriction().
Comment on attachment 8461759 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

>+   * @param aSize The size at which the image should be drawn. For raster
>+   *              images with multiple resolutions available, the closest
>+   *              matching resolution will be used, and scaling will be used to 
>+   *              ensure that the image is drawn at the requested size. For
>+   *              vector images, the exact size requested will be drawn. Callers
>+   *              can select an optimal value for aSize using
>+   *              optimalImageSizeForDest if there are no special size
>+   *              requirements.

This is just a hint, right? We could draw without it, just with possibly lower quality?

>+   * @param aRegion The region in the context to draw pixels to. When aFlags
>+   *                includes includes FLAG_CLAMP, the image will be extended to
>+   *                this area by clamping image sample coordinates. Otherwise,
>+   *                the image will be automatically tiled as necessary. aRegion
>+   *                also optionally specifies a restricted area of the image, in
>+   *                pixels, that we are allowed to sample from.

So the rect in aRegion is in context (user) coordinates, but the restriction in aRegion is in image space?


>    * @param aViewportSize
>    *          The size (in CSS pixels) of the viewport that would be available
>    *          for the full image to occupy, if we were drawing the full image.
>    *          (Note that we might not actually be drawing the full image -- we
>    *          might be restricted by aSubimage -- but we still need the full
>    *          image's viewport-size in order for SVG images with the "viewBox"
>    *          attribute to position their content correctly.)

aViewportSize is gone.

>+  ImageRegion() : mIsRestricted(false) { }

Is there a reason to have this public? Otherwise the only other public constructors are the Create* functions.
Attachment #8463783 - Flags: review?(tnikkel) → review+
Comment on attachment 8463782 [details] [diff] [review]
(Part 5) - Update OrientedImage to the new Draw API

>@@ -21,22 +23,37 @@ using layers::ImageContainer;
> 
> namespace image {
> 
> NS_IMPL_ISUPPORTS_INHERITED0(OrientedImage, ImageWrapper)
> 
> nsIntRect
> OrientedImage::FrameRect(uint32_t aWhichFrame)
> {
>+  nsresult rv;
>+
>+  // Retrieve the frame rect of the inner image.
>+  nsIntRect innerRect = InnerImage()->FrameRect(aWhichFrame);
>+  if (mOrientation.IsIdentity()) {
>+    return innerRect;
>+  }
>+
>+  // Get the underlying image's dimensions.
>+  nsIntSize size;
>+  rv = InnerImage()->GetWidth(&size.width);
>+  NS_ENSURE_SUCCESS(rv, innerRect);
>+  rv = InnerImage()->GetHeight(&size.height);
>+  NS_ENSURE_SUCCESS(rv, innerRect);
>   if (mOrientation.SwapsWidthAndHeight()) {
>-    nsIntRect innerRect = InnerImage()->FrameRect(aWhichFrame);
>-    return nsIntRect(innerRect.x, innerRect.y, innerRect.height, innerRect.width);
>-  } else {
>-    return InnerImage()->FrameRect(aWhichFrame);
>+    swap(size.width, size.height);
>   }
>+
>+  // Transform the frame rect.
>+  gfxRect finalRect = OrientationMatrix(size).TransformBounds(innerRect);
>+  return nsIntRect(finalRect.x, finalRect.y, finalRect.width, finalRect.height);
> }

Why do we need the SwapsWidthAndHeight() if here? Doesn't OrientationMatrix handle that for us?
Comment on attachment 8463780 [details] [diff] [review]
(Part 3) - Update VectorImage and supporting classes to the new Draw API

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

Partial review of Part 3 (not including the actual changes to VectorImage.cpp itself -- I'll review the changes to that file shortly, in a separate bug-comment):

::: content/svg/content/src/SVGSVGElement.cpp
@@ +1093,5 @@
> +  // 'UNKNOWN' values mean to preserve the existing state.
> +  if (aPAR.GetAlign() == SVG_PRESERVEASPECTRATIO_UNKNOWN &&
> +      aPAR.GetMeetOrSlice() == SVG_MEETORSLICE_UNKNOWN) {
> +    return;
> +  }

I'd rather not add an explicit dependency on _UNKNOWN here, as discussed in IRC, since I don't think we actually end up having that value in play anywhere (except for places where you're creating usages via dummy default-initialized values).  Plus, it feels a bit silly for us to have [up-one-level-in-the-callstack] "mHaveOverrides" telling us that we have an override PAR value, only to get here and discover that we actually don't have a meaningful one.

So: Let's (1) either abstract this _UNKNOWN away into a SVGImageContext helper-function, or use Maybe<SVGPreserveAspectRatio>, and (2) check for validity up one level in AutoSVGRenderingState's init list (when initializing mHaveOverrides).

(So, that means we shouldn't have to touch SVGSVGElement.cpp in this patch after all.)

::: content/svg/content/src/SVGSVGElement.h
@@ +405,4 @@
>                          float aFrameTime,
>                          dom::SVGSVGElement* aRootElem
>                          MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +    : mHaveOverrides(aSVGContext.isSome())

(Per above, mHaveOverrides here should now also check whether aSVGContext has a valid PAR value.)

::: image/src/SurfaceCache.h
@@ +66,5 @@
>  
>    uint32_t Hash() const
>    {
>      uint32_t hash = HashGeneric(mSize.width, mSize.height);
> +    hash = AddToHash(hash, mSVGContext.refOr(sDefaultSVGContext).Hash());

I'm not sure it makes sense to use sDefaultSVGContext here.

Right now, this line implies that having *no* SVGImageContext is "equivalent" (from a hashing perspective) to having a default-initialized SVGImageContext.  And that doesn't make sense to me -- those two scenarios (no-context vs. default-initialized-context) are not actually equivalent.

So -- I'd prefer that we just omit mSVGContext from our hash value, if it's not initialized. Something like:
 if (mSVGContext.isSome()) {
    hash = AddToHash(hash, mSVGContext.ref.Hash());
 }

I think this means we can get rid of sDefaultSVGContext, since this hashing is its only usage.

::: layout/svg/SVGImageContext.h
@@ +27,3 @@
>    { }
>  
> +  nsIntSize GetViewportSize() const {

This accessor should probably return "const nsIntSize&"
(Assignee)

Comment 53

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #51)
> Why do we need the SwapsWidthAndHeight() if here? Doesn't OrientationMatrix
> handle that for us?

OrientationMatrix used to work this way, but since we now have to pass in the desired size anyway, I decided to require that callers pass in the desired size from the underlying image's perspective. The size passed to Draw() is the size after any rotation is applied, so we may need to swap width and height to get the underlying image's desired size.

The fact that that wasn't clear means I need to add a comment about it.
Comment on attachment 8463781 [details] [diff] [review]
(Part 4) - Update ClippedImage to the new Draw API

> ClippedImage::DrawSingleTile(gfxContext* aContext,

>+  // We restrict our drawing to only the clipping region, and translate so that
>+  // the clipping region is placed at the position the caller expects.
>+  ImageRegion region(aRegion);
>+  region.MoveBy(clip.x, clip.y);
>+  region = region.Intersect(clip);

I can't convince myself that this shouldn't be -clip.x, -clip.y.
Comment on attachment 8463780 [details] [diff] [review]
(Part 3) - Update VectorImage and supporting classes to the new Draw API

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

Just a few line-length nits on VectorImage.cpp:

::: image/src/VectorImage.cpp
@@ +845,5 @@
>    }
>  
> +  nsRefPtr<gfxDrawable> drawable =
> +    SurfaceCache::Lookup(ImageKey(this),
> +                         SurfaceKey(params.size, aSVGContext, animTime, aFlags));

This is > 80 characters -- add a newline somewhere. (maybe before "animTime"?)

@@ +898,5 @@
>  
>    // Actually draw. (We use FILTER_NEAREST since we never scale here.)
> +  gfxUtils::DrawPixelSnapped(ctx, svgDrawable,
> +                             ThebesIntSize(aParams.size),
> +                             ImageRegion::Create(ThebesIntRect(aParams.imageRect)),

This is > 80 characters -- maybe use a local-var to help? (Either for the ThebesIntRect or for the ImageRegion.)
Attachment #8463780 - Flags: review?(dholbert) → review+
(r=me on part 3, with comments 52 & 55 addressed)
(Assignee)

Comment 57

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #54)
> I can't convince myself that this shouldn't be -clip.x, -clip.y.

It's correct. The |ImageRegion| tells us the region within tiled image space that we want to draw. Say the caller of |Draw()| provides a region at (5,5) with size (10,10). In the underlying image, this region is *actually* located at (clip.x + 5, clip.y + 5) with size (min(clip.width, 10), min(clip.height, 10)). In other words, the |ImageRegion| is in image space already, we just need to shift its origin to |(clip.x, clip.y)| (with MoveBy) and restrict its size to |(clip.width, clip.height)| (with Intersect).

The basic difference between the new way and the old way is that we're working in image space all the time now. The old way was in some sense the "opposite" of the new way.
(Assignee)

Comment 58

3 years ago
Comment on attachment 8463784 [details] [diff] [review]
(Part 7) - Propagate the Draw API changes to imgFrame

Adding Jonathan as a possible alternate reviewer for the imgFrame drawing code.
Attachment #8463784 - Flags: review?(jwatt)
Attachment #8463779 - Flags: review?(tnikkel) → review+
(In reply to Seth Fowler [:seth] from comment #53)
> (In reply to Timothy Nikkel (:tn) from comment #51)
> > Why do we need the SwapsWidthAndHeight() if here? Doesn't OrientationMatrix
> > handle that for us?
> 
> OrientationMatrix used to work this way, but since we now have to pass in
> the desired size anyway, I decided to require that callers pass in the
> desired size from the underlying image's perspective. The size passed to
> Draw() is the size after any rotation is applied, so we may need to swap
> width and height to get the underlying image's desired size.
> 
> The fact that that wasn't clear means I need to add a comment about it.

Okay, so then doesn't OrientedImage::GetImageSpaceInvalidationRect need to be updated then to the new way OrientationMatrix() works?
(Assignee)

Comment 60

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #59)
> Okay, so then doesn't OrientedImage::GetImageSpaceInvalidationRect need to
> be updated then to the new way OrientationMatrix() works?

Actually, no - it was wrong before! =) Passing in the underlying image's size is definitely right. I wrote it after I wrote this code and rebased it, and it looks like I messed up the original rebase.

However, looking at it made me realize that it should be passing true for aInvert instead of manually inverting the matrix. I'll fix that.
(In reply to Seth Fowler [:seth] from comment #57)
> (In reply to Timothy Nikkel (:tn) from comment #54)
> > I can't convince myself that this shouldn't be -clip.x, -clip.y.
> 
> It's correct. The |ImageRegion| tells us the region within tiled image space
> that we want to draw. Say the caller of |Draw()| provides a region at (5,5)
> with size (10,10). In the underlying image, this region is *actually*
> located at (clip.x + 5, clip.y + 5) with size (min(clip.width, 10),
> min(clip.height, 10)). In other words, the |ImageRegion| is in image space
> already, we just need to shift its origin to |(clip.x, clip.y)| (with
> MoveBy) and restrict its size to |(clip.width, clip.height)| (with
> Intersect).
> 
> The basic difference between the new way and the old way is that we're
> working in image space all the time now. The old way was in some sense the
> "opposite" of the new way.

Okay, shouldn't we be using mClip then? And not the scaled clip?
(Assignee)

Comment 62

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #50)
> This is just a hint, right? We could draw without it, just with possibly
> lower quality?

Yes, it's best effort. However, if the image can't fulfill the request (e.g., it doesn't have an HQ scaled frame at that size, or an ICO resolution layer at that size, and it's not an SVG so it can't be drawn at an arbitrary size) then at a minimum it must draw with an appropriate scale so that the rest of the parameters are in the correct coordinate space. You could also think of this as an explicit scale parameter, but by providing the size rather than the scale we avoid some numerical accuracy issues.

> So the rect in aRegion is in context (user) coordinates, but the restriction
> in aRegion is in image space?

Thanks for pointing that out; I shouldn't have written it that way. In fact they're both in image space, but we've set up the context so that user space *is* image space. It would be much clearer to say image space here; I'll change it.

> aViewportSize is gone.

Yup, bad rebase. Thanks.

> >+  ImageRegion() : mIsRestricted(false) { }
> 
> Is there a reason to have this public? Otherwise the only other public
> constructors are the Create* functions.

I'll take another look and see if I can get rid of it. I can't remember exactly why it was needed - might have been template-related.
(Assignee)

Comment 63

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #61)
> Okay, shouldn't we be using mClip then? And not the scaled clip?

So there was a discussion on IRC in some sense about this exact issue. It's clear I need to improve the comments on the new imgIContainer::Draw API, because the same question keeps coming up in different forms. I'll try to make things much clearer. I even have an ASCII art diagram planned!

When you call imgIContainer::Draw,

(1) The image looks at aSize, and satisfies that request one way or another. In the case of RasterImage, for example, it may substitute a different, HQ scaled imgFrame for the 'normal' imgFrame. If RasterImage doesn't have a matching HQ scaled imgFrame, it compensates by adding a scale to the gfxContext instead.

(2) Now that the aSize request is satisfied, the image gets drawn using the rest of the parameters. This means that the rest of the parameters are specified relative to aSize, *not* relative to the intrinsic size of the image (if it has one).

This means that, since we are passing |size| as the |aSize| parameter of |InnerImage()->Draw()|, we need to use |clip| (which is in the same space) instead of |mClip| (which isn't, since it's not scaled appropriately).
(Assignee)

Comment 64

3 years ago
Created attachment 8464453 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

This new version of part 1 includes fixes for all the feedback so far. Daniel, you already r+'ed, but if you feel like it I wouldn't mind any feedback you may have on the new comments for imgIContainer::Draw. I really want to make sure that it's easy to understand.
Attachment #8464453 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8461759 - Attachment is obsolete: true
Attachment #8461759 - Flags: review?(tnikkel)
(Assignee)

Comment 65

3 years ago
Created attachment 8464454 [details] [diff] [review]
(Part 3) - Update VectorImage and supporting classes to the new Draw API

Addressed all the review comments.
(Assignee)

Updated

3 years ago
Attachment #8463780 - Attachment is obsolete: true
Comment on attachment 8464453 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

Can you explicitly say that aRegion is in scaled image coordinates (scaled to aSize) and not intrinsic image coordinates in the parameter description of aRegion?

>+  ImageRegion() : mIsRestricted(false) { }

If you couldn't get rid of this then you could make sure to initialize the rect? Or does something make sure it's 0 initalized?
Attachment #8464453 - Flags: review?(tnikkel) → review+
(In reply to Seth Fowler [:seth] from comment #53)
> (In reply to Timothy Nikkel (:tn) from comment #51)
> > Why do we need the SwapsWidthAndHeight() if here? Doesn't OrientationMatrix
> > handle that for us?
> 
> OrientationMatrix used to work this way, but since we now have to pass in
> the desired size anyway, I decided to require that callers pass in the
> desired size from the underlying image's perspective. The size passed to
> Draw() is the size after any rotation is applied, so we may need to swap
> width and height to get the underlying image's desired size.
> 
> The fact that that wasn't clear means I need to add a comment about it.

I'm confused. We call OrientationMatrix twice in OrientedImage::Draw, once to get the inverse and once normally, but we pass the same size argument to it. Isn't that wrong?
Attachment #8463781 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 68

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #66)
> Can you explicitly say that aRegion is in scaled image coordinates (scaled
> to aSize) and not intrinsic image coordinates in the parameter description
> of aRegion?

That's a good idea. I'll make that change.

> If you couldn't get rid of this

Actually that's the one thing I didn't address yet. I'll try removing it tomorrow.
(Assignee)

Comment 69

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #67)
> I'm confused. We call OrientationMatrix twice in OrientedImage::Draw, once
> to get the inverse and once normally, but we pass the same size argument to
> it. Isn't that wrong?

Nah, it's always supposed to be the size of the underlying image. OrientationMatrix itself does the same thing either way; it's MatrixBuilder that "inverts" each individual operation when it's told to build an inverse matrix.

(By the way, after looking at that code again I realize that I really need to add |/* aInverse = */ true| when calling OrientationMatrix to get the inverse. I'll make that change next time I update that patch.)
Comment on attachment 8464453 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

Dude. This ASCII art is *beautiful*.

One minor issue that I discovered with it, after opening the "edit as comment" view, though: in your top diagram, many of the "padding" space characters are some sort of weird unicode character, when they should just be normal space characters. (No need for unicode whitespace.)

To see it (on Linux at least), view https://bugzilla.mozilla.org/attachment.cgi?id=8464453&action=edit , hit "edit as comment", and page down to the ASCII art.

It looks like the following (not sure if it'll show up in my bugzilla comment, but I'm seeing "â" mixed with bogus-character boxes, in this edit field that I'm typing in):
>+   *                           +------------------+
>+   *                           |      Image       |

The good news is, it should be easy to fix with search-and-replace in the patch file or the IDL file. 


Also, two nits from comment 48 still need fixing:

>+   * @param aSize The size to which the image should be scaled before drawing.
>+   *              This requirement may be satisfied using HQ scaled frames,
>+   *              selecting from different resolution layers, drawing at a
>+   *              higher DPI, or just performing additional scaling on the
>+   *              graphics context. Callers can use optimalImageSizeForDest

Add () after optimalImageSizeForDest to make it clearer that it's a separate function (and not some feature / parameter / flag of *this* function)

>+   * @param aRegion The region in tiled image space which will be drawn onto the
>+   *                graphics context. When aFlags includes includes FLAG_CLAMP,

s/includes includes/includes/

r=me with those tweaks. Thanks!
Attachment #8464453 - Flags: review+
(Assignee)

Comment 71

3 years ago
Created attachment 8465008 [details] [diff] [review]
(Part 1) - Refactor the imgIContainer::Draw API

This version of part 1 removes those UTF-8 space characters and some other
undesirable whitespace and fixes the textual issues pointed out by Timothy and
Daniel above.

Also, the public default constructor for ImageRegion has been removed and
replaced with another factory method, ImageRegion::Empty().
(Assignee)

Updated

3 years ago
Attachment #8464453 - Attachment is obsolete: true
(Assignee)

Comment 72

3 years ago
Created attachment 8465013 [details] [diff] [review]
(Part 5) - Update OrientedImage to the new Draw API

This new version of part 5 makes the tweaks discussed in various comments above.
Most importantly, there is a documentation comment for OrientationMatrix() and
comments at the callsites whenever it's called with |aInverse = true|.
Attachment #8465013 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8463782 - Attachment is obsolete: true
Attachment #8463782 - Flags: review?(tnikkel)
(Assignee)

Comment 73

3 years ago
Created attachment 8465014 [details] [diff] [review]
(Part 12) Improve pixel snapping accuracy and use the new Draw API in nsLayoutUtils

This just rebases part 12 against the changes in part 1. (In particular, ImageRegion::Empty() is now used instead of the default constructor.)
(Assignee)

Updated

3 years ago
Attachment #8463789 - Attachment is obsolete: true
(Assignee)

Comment 74

3 years ago
Created attachment 8465015 [details] [diff] [review]
(Part 13) Update the rest of layout to reflect the nsLayoutUtils changes

This rebases this part against the changes in part 3.
(Assignee)

Updated

3 years ago
Attachment #8463790 - Attachment is obsolete: true
(Assignee)

Comment 75

3 years ago
By the way, I'm glad the ASCII art was well received! Thanks to the feedback from everyone who's reviewing in this bug, I think the documentation has become - dare I say it - downright clear. Thanks to everyone for your help getting it there!
(Assignee)

Comment 76

3 years ago
Created attachment 8465153 [details] [diff] [review]
(Part 5) - Update OrientedImage to the new Draw API

This version of the patch addresses all the concerns raised so far. In
particular, FrameRect and GetFrame no longer swap the width and height they get
from the inner image before calling OrientationMatrix(). I not only fixed the
code but tried to make the comments for OrientationMatrix() clearer to try to
prevent such an oversight in the future.
Attachment #8465153 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8465013 - Attachment is obsolete: true
Attachment #8465013 - Flags: review?(tnikkel)
Attachment #8465153 - Flags: review?(tnikkel) → review+
Comment on attachment 8463784 [details] [diff] [review]
(Part 7) - Propagate the Draw API changes to imgFrame

Hopefully Jonathan can take this review.
Attachment #8463784 - Flags: review?(tnikkel)
(Assignee)

Comment 78

3 years ago
Comment on attachment 8463784 [details] [diff] [review]
(Part 7) - Propagate the Draw API changes to imgFrame

I haven't been able to get in touch with Jonathan, so reassigning this review to Jeff.
Attachment #8463784 - Flags: review?(jwatt) → review?(jmuizelaar)
Attachment #8463784 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 79

3 years ago
Thanks Jonathan!

Looks like this bug is ready to go. As soon as its dependencies are ready to go (which should be very soon), this can land!
(Assignee)

Updated

3 years ago
Blocks: 1049949
Blocks: 986403
(Assignee)

Updated

3 years ago
Blocks: 1003213
(Assignee)

Comment 80

3 years ago
Created attachment 8477617 [details] [diff] [review]
(Part 14) - Update reftests

Rebase.
(Assignee)

Updated

3 years ago
Attachment #8463652 - Attachment is obsolete: true
(Assignee)

Comment 81

3 years ago
Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d51132a0099
I landed two reftest.list tweaks to fix oranges that philor pointed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8912092cc511 to fix an intermittent Win7 reftest orange with slightly more fuzziness.  (It's a little disturbing that scaling on win7 is intermittently different.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09d1cd08b1c to fix a permanent orange that was introduced with a merge to mozilla-inbound *following* the landing of this bug.  In other words, somebody regressed one of the tests that this patch re-enabled before this patch reached central.  It was definitely something in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=09ee525ce99d which makes it seem most likely bug 974242.
I filed bug 1057881 on fixing the dowwnscale-svg-1d.html failure.
https://hg.mozilla.org/mozilla-central/rev/3d51132a0099
https://hg.mozilla.org/mozilla-central/rev/8912092cc511
https://hg.mozilla.org/mozilla-central/rev/b09d1cd08b1c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
I'm not sure how important this is, but while backfilling Fennec ARMv7 data for areweslimyet.com, I noticed that there were are a large chunk of inbound builds that were crashing before the test harness could finish its run.

The regression range [1] pointed squarely to this bug. The problem was 100% reproducible, and was fixed in the range at [2]. So there's likely an error somewhere in the patch on this bug that was fixed by Matt's patches. It may not affect people in practice but it was certainly affecting the AWSY harness; and if you're considering uplifting this anywhere then we should investigate further and uplift the fix along with it.

[1] https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3488976ecf0f&tochange=3d51132a0099
[2] https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5d3e70bda517&tochange=7cf240d88642

Updated

3 years ago
Depends on: 1068881
Depends on: 1067191

Updated

3 years ago
Depends on: 1071774
Blocks: 1074486

Updated

3 years ago
Depends on: 1085783

Comment 86

3 years ago
[Tracking Requested - why for this release]:
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?

Comment 87

3 years ago
sorry spam
status-firefox34: affected → ---
status-firefox35: affected → ---
status-firefox36: affected → ---
status-firefox37: affected → ---
tracking-firefox35: ? → ---
tracking-firefox36: ? → ---
tracking-firefox37: ? → ---

Updated

3 years ago
Depends on: 1106602

Updated

3 years ago
Depends on: 1110496
You need to log in before you can comment on or make changes to this bug.