Closed
Bug 1043560
Opened 10 years ago
Closed 10 years ago
Make imgIContainer::Draw simpler, more accurate, and better suited to our needs going forward
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: seth, Assigned: seth)
References
(Blocks 3 open bugs, Regressed 1 open bug)
Details
Attachments
(15 files, 19 obsolete files)
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Attachment #8461759 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 3•10 years ago
|
||
This patch updates RasterImage to the new Draw API.
Attachment #8461771 -
Flags: review?(tnikkel)
Assignee | ||
Comment 4•10 years ago
|
||
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•10 years ago
|
||
This patch updates ClippedImage to the new Draw API.
Attachment #8461780 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•10 years ago
|
||
Same for OrientedImage. (Note, by the way, that OrientedImage::FrameRect was implemented incorrectly before.)
Attachment #8461783 -
Flags: review?(tnikkel)
Assignee | ||
Comment 7•10 years ago
|
||
This covers the rest of the Image subclasses.
Attachment #8461785 -
Flags: review?(tnikkel)
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
This propagates the changes to nsCocoaUtils.
Attachment #8461832 -
Flags: review?(mstange)
Assignee | ||
Comment 12•10 years ago
|
||
Same for nsBaseDragService.
Attachment #8461903 -
Flags: review?(roc)
Assignee | ||
Comment 13•10 years ago
|
||
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•10 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•10 years ago
|
||
Finally, this propagates the nsLayoutUtils::Draw*Image changes to the rest of layout.
Attachment #8461920 -
Flags: review?(roc)
Assignee | ||
Comment 16•10 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.
Comment 17•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8461757 -
Flags: review?(dholbert) → review+
Updated•10 years ago
|
Version: unspecified → Trunk
Comment 18•10 years ago
|
||
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•10 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 20•10 years ago
|
||
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 21•10 years ago
|
||
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•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> gfxUtils has TransformRectToRect for this.
Nice! I hadn't noticed that.
Comment 23•10 years ago
|
||
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•10 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+
Comment 25•10 years ago
|
||
Seth, could it be that this (or one of the other related bugs) also fixes bug 993325?
Sebastian
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•10 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 29•10 years ago
|
||
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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8463652 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 31•10 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•10 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•10 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 | ||
Comment 34•10 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•10 years ago
|
||
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•10 years ago
|
Attachment #8461771 -
Attachment is obsolete: true
Attachment #8461771 -
Flags: review?(tnikkel)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8463780 -
Flags: review?(dholbert)
Assignee | ||
Updated•10 years ago
|
Attachment #8461777 -
Attachment is obsolete: true
Attachment #8461777 -
Flags: review?(dholbert)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8463781 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8461780 -
Attachment is obsolete: true
Attachment #8461780 -
Flags: review?(tnikkel)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8463782 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8461783 -
Attachment is obsolete: true
Attachment #8461783 -
Flags: review?(tnikkel)
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8463783 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8461785 -
Attachment is obsolete: true
Attachment #8461785 -
Flags: review?(tnikkel)
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8463784 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8461788 -
Attachment is obsolete: true
Attachment #8461788 -
Flags: review?(tnikkel)
Assignee | ||
Comment 41•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8461792 -
Attachment is obsolete: true
Assignee | ||
Comment 42•10 years ago
|
||
Updated to include all of your feedback except merging the calculations. (See comment 33.)
Attachment #8463786 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Attachment #8461803 -
Attachment is obsolete: true
Attachment #8461803 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 43•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8461832 -
Attachment is obsolete: true
Assignee | ||
Comment 44•10 years ago
|
||
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•10 years ago
|
Attachment #8461918 -
Attachment is obsolete: true
Attachment #8461918 -
Flags: review?(roc)
Assignee | ||
Comment 45•10 years ago
|
||
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•10 years ago
|
Attachment #8461920 -
Attachment is obsolete: true
Updated•10 years ago
|
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+
Comment 47•10 years ago
|
||
(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 48•10 years ago
|
||
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•10 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 50•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8463783 -
Flags: review?(tnikkel) → review+
Comment 51•10 years ago
|
||
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 52•10 years ago
|
||
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•10 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 54•10 years ago
|
||
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 55•10 years ago
|
||
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+
Comment 56•10 years ago
|
||
(r=me on part 3, with comments 52 & 55 addressed)
Assignee | ||
Comment 57•10 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•10 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)
Updated•10 years ago
|
Attachment #8463779 -
Flags: review?(tnikkel) → review+
Comment 59•10 years ago
|
||
(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•10 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.
Comment 61•10 years ago
|
||
(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•10 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•10 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•10 years ago
|
||
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•10 years ago
|
Attachment #8461759 -
Attachment is obsolete: true
Attachment #8461759 -
Flags: review?(tnikkel)
Assignee | ||
Comment 65•10 years ago
|
||
Addressed all the review comments.
Assignee | ||
Updated•10 years ago
|
Attachment #8463780 -
Attachment is obsolete: true
Comment 66•10 years ago
|
||
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+
Comment 67•10 years ago
|
||
(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?
Updated•10 years ago
|
Attachment #8463781 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 68•10 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•10 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 70•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Attachment #8464453 -
Attachment is obsolete: true
Assignee | ||
Comment 72•10 years ago
|
||
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•10 years ago
|
Attachment #8463782 -
Attachment is obsolete: true
Attachment #8463782 -
Flags: review?(tnikkel)
Assignee | ||
Comment 73•10 years ago
|
||
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•10 years ago
|
Attachment #8463789 -
Attachment is obsolete: true
Assignee | ||
Comment 74•10 years ago
|
||
This rebases this part against the changes in part 3.
Assignee | ||
Updated•10 years ago
|
Attachment #8463790 -
Attachment is obsolete: true
Assignee | ||
Comment 75•10 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•10 years ago
|
||
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•10 years ago
|
Attachment #8465013 -
Attachment is obsolete: true
Attachment #8465013 -
Flags: review?(tnikkel)
Updated•10 years ago
|
Attachment #8465153 -
Flags: review?(tnikkel) → review+
Comment 77•10 years ago
|
||
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•10 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)
Updated•10 years ago
|
Attachment #8463784 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 79•10 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 | ||
Comment 80•10 years ago
|
||
Rebase.
Assignee | ||
Updated•10 years ago
|
Attachment #8463652 -
Attachment is obsolete: true
Assignee | ||
Comment 81•10 years ago
|
||
Comment 82•10 years ago
|
||
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.
Comment 83•10 years ago
|
||
I filed bug 1057881 on fixing the dowwnscale-svg-1d.html failure.
Comment 84•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 85•10 years ago
|
||
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
Comment 86•10 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•10 years ago
|
||
sorry spam
status-firefox34:
affected → ---
status-firefox35:
affected → ---
status-firefox36:
affected → ---
status-firefox37:
affected → ---
tracking-firefox35:
? → ---
tracking-firefox36:
? → ---
tracking-firefox37:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•