bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Make DrawTargetCG's transform handling better

RESOLVED FIXED in Firefox 41

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

(Blocks: 1 bug)

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8600098 [details] [diff] [review]
Make DrawTargetCG's transform handling better
Attachment #8600098 - Flags: review?(mstange)
Comment on attachment 8600098 [details] [diff] [review]
Make DrawTargetCG's transform handling better

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

This breaks the UnboundnessFixer because the return value of CGContextGetClipBoundingBox is in user space, and with this patch we enter UnboundnessFixer::Check with a different transform, and the clip bounds will be misinterpreted.
Attachment #8600098 - Flags: review?(mstange) → review-
(Assignee)

Updated

3 years ago
Attachment #8600118 - Flags: review?(mstange)
Comment on attachment 8600118 [details] [diff] [review]
patch

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

This also breaks the mSavedClipBounds checks, so let's just remove those completely.

::: gfx/2d/DrawTargetCG.cpp
@@ +284,5 @@
>          // If we're entirely clipped out or if the drawing operation covers the entire clip then
>          // we don't need to create a temporary surface.
>          if (CGRectIsEmpty(mClipBounds) ||
>              (maskBounds && maskBounds->Contains(CGRectToRect(mClipBounds)))) {
>            return baseCg;

You also need to reset the transform in this error case.
Attachment #8600118 - Flags: review?(mstange) → review+
Comment on attachment 8600118 [details] [diff] [review]
patch

Matt Woodrow pointed out that DrawSurfaceWithShadow and CopySurface take parameters in dest space, so those would be treated as user space with this patch.

It seems reviewing this patch involves looking at all the code that's *not* touched by it.
Attachment #8600118 - Flags: review+ → review-
(Assignee)

Comment 5

3 years ago
Created attachment 8600388 [details] [diff] [review]
Make DrawTargetCG's transform handling better v3
Attachment #8600098 - Attachment is obsolete: true
Attachment #8600118 - Attachment is obsolete: true
Attachment #8600388 - Flags: review?(mstange)
Comment on attachment 8600388 [details] [diff] [review]
Make DrawTargetCG's transform handling better v3

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

::: gfx/2d/DrawTargetCG.cpp
@@ +936,5 @@
> +DrawTargetCG::SetTransform(const Matrix &aTransform)
> +{
> +  mTransform = aTransform;
> +  CGContextSetCTM(mCg, CGAffineTransformMake(1, 0, 0, -1, 0, mSize.height));
> +  CGContextConcatCTM(mCg, GfxMatrixToCGAffineTransform(aTransform));

Would it be faster to multiply the transforms first and then setting the result in one go?
Attachment #8600388 - Flags: review?(mstange) → review+
(Assignee)

Updated

3 years ago
Assignee: nobody → jmuizelaar
https://hg.mozilla.org/mozilla-central/rev/434bb2c9dad6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.