Closed Bug 784573 Opened 12 years ago Closed 12 years ago

[Azure] Cairo path transforms broken

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Tracking Flags:
Tracking Status
firefox17 --- fixed
firefox18 --- fixed

People

(Reporter: ajones, Assigned: ajones)

References

(
URL
)

Details

Attachments

(4 files, 10 obsolete files)

Ugly lines on the left of the canvas
373.75 KB, image/png
Details
Part 2: RAII matrix helper v3
1.17 KB, patch
joe
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
Part 4: Added reftest and removed todo v3
3.54 KB, patch
joe
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
Part 3: Fix cairo path transforms v5
22.88 KB, patch
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
Paths don't draw correctly using Azure/Cairo if they need to move between user space and device space. This shows up on asteroidbench test 4 as a bunch of unwanted lines across the canvas when drawing enemy bullets.
It would probably make sense to just combine these patches.
Attachment #654512 - Flags: review?(joe)
Attachment #654512 - Attachment is obsolete: true
Attachment #654512 - Flags: review?(joe)
Attachment #654514 - Flags: review?(joe)
Attached patch Part 3: RAII matrix helper (obsolete) — — Splinter Review
Attachment #654515 - Flags: review?(joe)
Attachment #654514 - Attachment description: 654512: Part2: Fixed some test failures and did more clean up v2 → Part2: Fixed some test failures and did more clean up v2
Attachment #654513 - Attachment description: reftest to make sure the problem stays fixed → Part 1: reftest to make sure the problem stays fixed
Attachment #654515 - Attachment description: RAII matrix helper → Part 3: RAII matrix helper
Attachment #654120 - Attachment description: Fix cairo path transforms → Part 1.5: Fix cairo path transforms
Attachment #654514 - Attachment description: Part2: Fixed some test failures and did more clean up v2 → Part 2: Fixed some test failures and did more clean up v2
Attachment #654513 - Attachment is obsolete: true
Attachment #654513 - Flags: review?(joe)
Comment on attachment 654845 [details] [diff] [review] Part 4: Added reftest and removed todo v2 Review of attachment 654845 [details] [diff] [review]: ----------------------------------------------------------------- This should be part n+1 because it depends on the fixes in the previous n patches. ::: layout/reftests/canvas/784573-1.html @@ +1,1 @@ > +<!DOCTYPE html> Can you add a comment saying what this is testing (that is, device space vs user space paths). Also, <!-- Any copyright is dedicated to the public domain. -->
Attachment #654845 - Flags: review?(joe) → review+
Comment on attachment 654120 [details] [diff] [review] Part 1.5: Fix cairo path transforms Review of attachment 654120 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks OK, but I need more explanation for why things are correct before I r+. ::: gfx/2d/PathCairo.cpp @@ +25,5 @@ > + cairo_new_path(mContext); > + > + if (mDrawTarget) { > + mDrawTarget->SetPathObserver(this); > + } How do we get around needing to have a separate context from the draw target, as below? @@ +40,5 @@ > + , mFillRule(aPathContext->mFillRule) > +{ > + Matrix inverse = aTransform; > + inverse.Invert(); > + mTransform = aPathContext->mTransform * inverse; Can you please add a comment explaining why you're doing this? @@ +48,3 @@ > > // If we don't have an identity transformation, we need to have a separate > // context from the draw target, because we can't set a transformation on its Shouldn't we always have to duplicate the context and path here? IIRC CairoPathContexts don't know how to share contexts between each other. @@ +98,5 @@ > cairo_fill_rule_t rule = cairo_get_fill_rule(mContext); > > // Duplicate the context. > cairo_surface_t* surf = cairo_get_target(mContext); > + cairo_matrix_t matrix; Extra space at the end of this line. @@ +104,3 @@ > cairo_destroy(mContext); > mContext = cairo_create(surf); > + cairo_set_matrix(mContext, &matrix); Also here. @@ +263,5 @@ > > TemporaryRef<Path> > PathBuilderCairo::Finish() > { > + RefPtr<PathCairo> path = new PathCairo(mPathContext); We should really file a follow-up bug for the fact that this will duplicate the context unnecessarily. ::: gfx/2d/PathCairo.h @@ +37,5 @@ > class CairoPathContext : public RefCounted<CairoPathContext> > { > public: > // Construct a CairoPathContext and set it to be the path observer of > + // aDrawTarget with an empty path. aDrawTarget. Also, modify its path in-place to be empty. @@ +42,4 @@ > CairoPathContext(cairo_t* aCtx, DrawTargetCairo* aDrawTarget, > + FillRule aFillRule, const Matrix& aTransform); > + > + // Create a CairoPathContext which is a cope of a previous context with "copy" @@ +42,5 @@ > CairoPathContext(cairo_t* aCtx, DrawTargetCairo* aDrawTarget, > + FillRule aFillRule, const Matrix& aTransform); > + > + // Create a CairoPathContext which is a cope of a previous context with > + // applying a new transform. "an optional transform"? @@ +85,5 @@ > operator cairo_t* () const { return mContext; } > > private: // methods > CairoPathContext(const CairoPathContext&) MOZ_DELETE; > + bool ClassInvariant(); this is never used
Attachment #654120 - Flags: review?(joe) → review-
Comment on attachment 654514 [details] [diff] [review] Part 2: Fixed some test failures and did more clean up v2 Review of attachment 654514 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks like it would be improved by folding it with patch 1.5. ::: gfx/2d/PathCairo.cpp @@ +23,5 @@ > cairo_reference(mContext); > cairo_set_fill_rule(mContext, GfxFillRuleToCairoFillRule(mFillRule)); > + > + aDrawTarget->SetPathObserver(this); > + mDrawTarget = aDrawTarget; It looks like you do this dance on purpose. Can you explain why in a comment? @@ +250,5 @@ > + cairo_get_matrix(*mPathContext, &saveMatrix); > + > + cairo_matrix_t matrix; > + GfxMatrixToCairoMatrix(mPathContext->GetTransform(), matrix); > + cairo_set_matrix(*mPathContext, &matrix); When do these get out of sync? Shouldn't this always be the case? ::: gfx/2d/PathCairo.h @@ +43,2 @@ > > + // Transform an existing path These comments seem worse than the previous ones. @@ +60,5 @@ > // our context. > void ForgetDrawTarget(); > > // Create a duplicate context, and copy this path to that context. Optionally, > // the new context can be transformed. Update this comment.
Attachment #654514 - Flags: review?(joe) → review-
Comment on attachment 654515 [details] [diff] [review] Part 3: RAII matrix helper Review of attachment 654515 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/HelpersCairo.h @@ +206,5 @@ > > return CAIRO_FILL_RULE_WINDING; > } > > +class CairoTempMatrix { Can you document this class? Also put { open braces on their own line for the class and functions.
Attachment #654515 - Flags: review?(joe) → review+
Attachment #654120 - Attachment is obsolete: true
Attachment #654514 - Attachment is obsolete: true
Attachment #655515 - Flags: review?(joe)
Comment on attachment 655515 [details] [diff] [review] Part 0: Fix cairo path transforms v2 Review of attachment 655515 [details] [diff] [review]: ----------------------------------------------------------------- Some unanswered questions (some left over from previous reviews) lead me to still need to see a follow-up before r+. ::: gfx/2d/DrawTargetCairo.h @@ +168,5 @@ > cairo_t* mContext; > cairo_surface_t* mSurface; > IntSize mSize; > std::vector<SourceSurfaceCairo*> mSnapshots; > + mutable CairoPathContext* mPathObserver; Ownership rules deserve a comment here or on SetPathObserver. Seeing a bare pointer makes my tummy hurt. ::: gfx/2d/PathCairo.cpp @@ +22,5 @@ > { > cairo_reference(mContext); > cairo_set_fill_rule(mContext, GfxFillRuleToCairoFillRule(mFillRule)); > > + aDrawTarget->SetPathObserver(this); Note: You didn't answer my earlier question, "How do we get around needing to have a separate context from the draw target, as below?", but I think I have figured it out: the non-copy CairoPathContext constructor is only ever called when creating a new path, correct? @@ +35,5 @@ > + , mFillRule(aPathContext->mFillRule) > +{ > + cairo_reference(mContext); > + > + // Canvas is telling us what transform we would apply from device space to Don't talk about canvas here - it is one of the users of the DrawTarget API, but not the only one. @@ +75,5 @@ > cairo_path_destroy(path); > + > + // Transform the context. > + GfxMatrixToCairoMatrix(mTransform, matrix); > + cairo_set_matrix(mContext, &matrix); You set the cairo context's matrix to mContext's old matrix, and then set it to mTransform. Why? When do those two get out of sync? @@ +86,5 @@ > +{ > + if (mDrawTarget) { > + // null the draw target so that we don't try to copy the path when we get > + // a PathWillChange notification back. > + DrawTargetCairo* drawTarget = mDrawTarget; Seems like most of this can just be elided, with some comments explaining it and an unconditional mDrawTarget = nullptr; @@ +250,5 @@ > inverse.Invert(); > Point transformed = inverse * aPoint; > > + // ContainsPoint can be called after the DrawTarget has been notified about > + // a change in transform but before the path has been notified by Canvas. In s/Canvas/DrawTarget/ perhaps? References to canvas don't really make sense in backend code. Perhaps s/Canvas/user code/? @@ +300,5 @@ > > void > PathCairo::CopyPathTo(cairo_t* aContext, DrawTargetCairo* aDrawTarget) > { > + mPathContext->CopyPathTo(aContext); Why do you do this unconditionally? Was the previous optimization incorrect? And, if mPathContext->GetContext() == aContext, isn't this going to do useless work? ::: gfx/2d/PathCairo.h @@ +37,5 @@ > class CairoPathContext : public RefCounted<CairoPathContext> > { > public: > + // Construct a new empty CairoPathContext that uses the given draw target and > + // it's cairo context. Using the existing context may save having to copy the its @@ +62,5 @@ > // our context. > void ForgetDrawTarget(); > > // Create a duplicate context, and copy this path to that context. Optionally, > // the new context can be transformed. Please fix this comment. @@ +98,5 @@ > > // This constructor, called with a CairoPathContext*, implicitly takes > // ownership of the path, and therefore makes aPathContext copy out its path > // regardless of whether it has a pointer to a DrawTargetCairo. > // The path currently set on aPathContext is not changed. s/aPathContext/aContext/g
Attachment #655515 - Flags: review?(joe) → review-
Try run for fa656a8c5699 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=fa656a8c5699 Results (out of 17 total builds): exception: 17 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-fa656a8c5699
Status: NEW → ASSIGNED
Attachment #655515 - Attachment is obsolete: true
Attachment #655524 - Attachment is obsolete: true
Attachment #655888 - Flags: review?(joe)
Attachment #654845 - Attachment description: Part 1: Added reftest and removed todo v2 → Part 4: Added reftest and removed todo v2
(In reply to Joe Drew (:JOEDREW!) from comment #15) > @@ +300,5 @@ > > > > void > > PathCairo::CopyPathTo(cairo_t* aContext, DrawTargetCairo* aDrawTarget) > > { > > + mPathContext->CopyPathTo(aContext); > > Why do you do this unconditionally? Was the previous optimization incorrect? > And, if mPathContext->GetContext() == aContext, isn't this going to do > useless work? CairoPathContext::CopyPathTo() does this check anyway so it's duplicated. The previous code was deciding whether to subscribe to the DrawTarget more than to skip the call. Subscribing to the DrawTarget is a coin toss as to whether it'll save time or create the need for another copy.
Try run for 36ddd2e5b9b6 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=36ddd2e5b9b6 Results (out of 2 total builds): failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-36ddd2e5b9b6
Try run for aace50d4c079 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=aace50d4c079 Results (out of 2 total builds): failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-aace50d4c079
Fuzzed reftest on D2D
Attachment #654845 - Attachment is obsolete: true
Attachment #656640 - Flags: review?(joe)
Attachment #655888 - Flags: review?(joe) → review+
Comment on attachment 655817 [details] [diff] [review] Part 3: Fix cairo path transforms v3 Review of attachment 655817 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetCairo.cpp @@ -924,5 @@ > - // We're about to logically change our transformation. Our current path will > - // need to change, because Cairo stores paths in device space. > - if (mPathObserver) { > - mPathObserver->MatrixWillChange(aTransform); > - } As discussed on IRC, we can't remove this, because it'll break a perfectly reasonable use case: [4:39 PM] <joe> PathBuilder* pb = dt->CreatePathBuilder() [4:39 PM] <joe> pb->AddPoints() // or whatever [4:39 PM] <joe> dt->SetMatrix(something) [4:39 PM] <joe> pb->AddPoints() [4:39 PM] <joe> those points will be transformed by the new matrix Instead, maybe only copy out the path when the path is changed after the matrix is changed?
Attachment #655817 - Flags: review?(joe) → review-
Attachment #656640 - Flags: review?(joe) → review+
Attachment #655817 - Attachment is obsolete: true
Try run for 2ff039275a33 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2ff039275a33 Results (out of 24 total builds): success: 24 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-2ff039275a33
Comment on attachment 657157 [details] [diff] [review] Part 3: Fix cairo path transforms v4 Review of attachment 657157 [details] [diff] [review]: ----------------------------------------------------------------- OK! Looks good with some small fixes. Hooray! ::: gfx/2d/PathCairo.cpp @@ +61,5 @@ > mContext = cairo_create(surf); > > + // Set the matrix to match the source context so that the path is copied in > + // device space. > + cairo_set_matrix(mContext, &matrix); Ooh, better use a CairoTempMatrix here, so we're reset to identity afterwards. @@ +88,5 @@ > if (mDrawTarget) { > // The context we point to is going to change from under us. To continue > // using this path, we need to copy it to a new context. > DuplicateContextAndPath(); > + mDrawTarget = nullptr; Better to leave this as a call to ForgetDrawTarget(). @@ +100,3 @@ > // cairo_copy_path gives us a user-space copy of the path, so we don't have > // to worry about transformations here. > + CairoTempMatrix tempMatrix(mContext, aTransform); This line contradicts the comment above - better remove the comment :) @@ +217,5 @@ > +PathBuilderCairo::PrepareForWrite() > +{ > + if (mPathContext->refCount() != 1) { > + mPathContext = new CairoPathContext(*mPathContext); > + } Better add a comment saying that only PathBuilder and PathCairo maintain references to CairoPathContext, not DrawTarget, so this will ensure that we're duplicating if we're sharing a context. ::: gfx/2d/PathCairo.h @@ +42,5 @@ > + // path later. > + CairoPathContext(cairo_t* aCtx, DrawTargetCairo* aDrawTarget); > + > + // Copy the path and supply a new fill rule. > + CairoPathContext(CairoPathContext& aPathContext); This does not seem to supply a new fill rule. @@ +86,4 @@ > PathBuilderCairo(cairo_t* aCtx, DrawTargetCairo* aDrawTarget, FillRule aFillRule); > > + // Creates a path builder out of an existing CairoPathContext with a new fill > + // rule and transform. The fill Unfinished sentence in this comment.
Attachment #657157 - Flags: review?(joe) → review+
(In reply to Joe Drew (:JOEDREW! \o/) from comment #26) > ::: gfx/2d/PathCairo.cpp > @@ +61,5 @@ > > mContext = cairo_create(surf); > > > > + // Set the matrix to match the source context so that the path is copied in > > + // device space. > > + cairo_set_matrix(mContext, &matrix); > > Ooh, better use a CairoTempMatrix here, so we're reset to identity > afterwards. We swap out the matrix every time so it doesn't matter what we set it to. We could set it to identity but it would serve no purpose.
Made changes as requested.
Attachment #657157 - Attachment is obsolete: true
Try run for 51a9cd52811b is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=51a9cd52811b Results (out of 16 total builds): exception: 15 success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-51a9cd52811b
Try run for 73be938dae67 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=73be938dae67 Results (out of 110 total builds): exception: 63 success: 42 warnings: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-73be938dae67
Try run for f5efca0c6882 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f5efca0c6882 Results (out of 194 total builds): exception: 1 success: 168 warnings: 21 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f5efca0c6882
Try run for 23bf9f4a005b is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=23bf9f4a005b Results (out of 2 total builds): failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-23bf9f4a005b
Try run for f5efca0c6882 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f5efca0c6882 Results (out of 196 total builds): exception: 1 success: 169 warnings: 21 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f5efca0c6882
Try run for f5efca0c6882 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f5efca0c6882 Results (out of 197 total builds): exception: 1 success: 170 warnings: 21 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f5efca0c6882
Try run for 0fcdb2834678 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0fcdb2834678 Results (out of 47 total builds): success: 45 warnings: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-0fcdb2834678
Try run for 0fcdb2834678 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0fcdb2834678 Results (out of 49 total builds): success: 47 warnings: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-0fcdb2834678
Blocks: 803658
Bug 803658 suggests we need this fix on beta? If so, please request beta approval.
Comment on attachment 655888 [details] [diff] [review] Part 2: RAII matrix helper v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): 773460 enable azure canvas User impact if declined: broken canvas transforms Testing completed (on m-c, etc.): in m-c for several weeks before aurora uplift Risk to taking this patch (and alternatives if risky): May cause other canvas path breakages or may not fix everything. Low risk on content because content doesn't use Azure Cairo backend. String or UUID changes made by this patch: none
Attachment #655888 - Flags: approval-mozilla-beta?
Attachment #656640 - Flags: approval-mozilla-beta?
Attachment #657715 - Flags: approval-mozilla-beta?
Attachment #655888 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #656640 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #657715 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1124549
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: