The default bug view has changed. See this FAQ.

[Azure] Implement push/pop clip in Cairo backend

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 620171 [details] [diff] [review]
push/pop clip patch
Attachment #620171 - Flags: review?(joe)
(Assignee)

Comment 2

5 years ago
Push to try: https://tbpl.mozilla.org/?tree=Try&rev=19591f43adcf

Reference push: https://tbpl.mozilla.org/?tree=Try&rev=14eeb79a91ca



Both have pretty much the same fails, so I assume I haven't broken anything, but I haven't checked which tests pass/fail. I've tested manually and clipping on Azure/Cairo canvas works properly.
Comment on attachment 620171 [details] [diff] [review]
push/pop clip patch

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

This might be a naive question as I don't know the cairo backend that well, but is there no other state that we need to ensure is not affected by this save/restore pair?
Comment on attachment 620171 [details] [diff] [review]
push/pop clip patch

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +625,5 @@
> +  if (aPath->GetBackendType() != BACKEND_CAIRO) {
> +    return;
> +  }
> +
> +  PathCairo* path = const_cast<PathCairo*>(static_cast<const PathCairo*>(aPath));

You need to call WillChange(aPath) here, because we're going to set aPath to be our current path.

@@ +635,5 @@
>  
>  void
>  DrawTargetCairo::PushClipRect(const Rect& aRect)
>  {
> +  cairo_save(mContext);

Similarly, you need to call WillChange() here. However!

We actually run into a problem in WillChange(), in that we're going to change the path from under the PathObserver, but we don't have code that handles the case of aPath = NULL.

So, you should do one of two things:

1) implement aPath = NULL meaning "forget about your current path". If you do this, you'll need to check all the callers of WillChange (remember implicit callers!) to make sure that it won't cause unintended problems.
2) Create a new function called something like PathWillChange() to do the above, and only call it in the relevant areas.

I leave it up to you to decide which makes most sense.
Attachment #620171 - Flags: review?(joe) → review-
(In reply to Bas Schouten (:bas) from comment #3)
> Comment on attachment 620171 [details] [diff] [review]
> push/pop clip patch
> 
> Review of attachment 620171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This might be a naive question as I don't know the cairo backend that well,
> but is there no other state that we need to ensure is not affected by this
> save/restore pair?

The Cairo path object handles things for us pretty well. What did you have in mind?
(Assignee)

Comment 6

5 years ago
(In reply to Bas Schouten (:bas) from comment #3)
> Comment on attachment 620171 [details] [diff] [review]
> push/pop clip patch
> 
> Review of attachment 620171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This might be a naive question as I don't know the cairo backend that well,
> but is there no other state that we need to ensure is not affected by this
> save/restore pair?

Save/restore also affects the transform, but the way Cairo is used here won't affect us.
(Assignee)

Comment 7

5 years ago
(In reply to Joe Drew (:JOEDREW!) from comment #4)
> Comment on attachment 620171 [details] [diff] [review]
> push/pop clip patch
> 
> Review of attachment 620171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCairo.cpp
> @@ +625,5 @@
> > +  if (aPath->GetBackendType() != BACKEND_CAIRO) {
> > +    return;
> > +  }
> > +
> > +  PathCairo* path = const_cast<PathCairo*>(static_cast<const PathCairo*>(aPath));
> 
> You need to call WillChange(aPath) here, because we're going to set aPath to
> be our current path.
> 
> @@ +635,5 @@
> >  
> >  void
> >  DrawTargetCairo::PushClipRect(const Rect& aRect)
> >  {
> > +  cairo_save(mContext);
> 
> Similarly, you need to call WillChange() here. However!
> 
> We actually run into a problem in WillChange(), in that we're going to
> change the path from under the PathObserver, but we don't have code that
> handles the case of aPath = NULL.
> 
> So, you should do one of two things:
> 
> 1) implement aPath = NULL meaning "forget about your current path". If you
> do this, you'll need to check all the callers of WillChange (remember
> implicit callers!) to make sure that it won't cause unintended problems.
> 2) Create a new function called something like PathWillChange() to do the
> above, and only call it in the relevant areas.
> 
> I leave it up to you to decide which makes most sense.

I'm not exactly clear, but I think this has already been implemented. If so, then I just need to make a minor change to WillChange() (in the new patch) and it's all good. If I'm wrong, I'll have another go.
(Assignee)

Comment 8

5 years ago
Created attachment 620972 [details] [diff] [review]
push/pop clip patch
Attachment #620171 - Attachment is obsolete: true
Attachment #620972 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #620972 - Flags: review? → review?(joe)
(Assignee)

Comment 9

5 years ago
Created attachment 620981 [details] [diff] [review]
push/pop clip patch
Attachment #620972 - Attachment is obsolete: true
Attachment #620972 - Flags: review?(joe)
Attachment #620981 - Flags: review?(joe)
Comment on attachment 620981 [details] [diff] [review]
push/pop clip patch

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +645,5 @@
>  
>  void
>  DrawTargetCairo::PopClip()
>  {
> +  cairo_restore(mContext);

Add a comment above this saying "cairo_restore doesn't affect the path, so we don't need to do a WillChange()" or something to that effect.
Attachment #620981 - Flags: review?(joe) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 621784 [details] [diff] [review]
push/pop clip patch

added comment as requested, carrying r=joe
Attachment #620981 - Attachment is obsolete: true
Attachment #621784 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [autoland:-p all -u all]
Whiteboard: [autoland:-p all -u all] → [autoland-try:-p all -u all]

Updated

5 years ago
Whiteboard: [autoland-try:-p all -u all] → [autoland-in-queue]
https://hg.mozilla.org/integration/mozilla-inbound/rev/404ef622ffd3
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [autoland-in-queue]
Target Milestone: --- → mozilla15
Comment on attachment 621784 [details] [diff] [review]
push/pop clip patch

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +639,5 @@
>  {
> +  WillChange();
> +  cairo_save(mContext);
> +  cairo_rectangle(mContext, aRect.X(), aRect.Y(), aRect.Width(), aRect.Height());
> +  cairo_clip_preserve(mContext);

Seems to me that you need a cairo_new_path(mContext) here
(Assignee)

Comment 14

5 years ago
Should I just change the cairo_clip_preserve to cairo_clip?

Or do you mean before this code?

And now I realise I am not so sure of the semantics here - do we leave the cairo path (in the context) clear after an operation, or do we clear it before we start an operation?
https://hg.mozilla.org/mozilla-central/rev/404ef622ffd3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 16

5 years ago
Autoland Patchset:
	Patches: 621784
	Branch: mozilla-central => try
Patch 621784 could not be applied to mozilla-central.
patching file gfx/2d/DrawTargetCairo.cpp
Hunk #1 FAILED at 610
Hunk #2 FAILED at 762
2 out of 2 hunks FAILED -- saving rejects to file gfx/2d/DrawTargetCairo.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
(Assignee)

Comment 17

5 years ago
Need to make some changes to the patch and rebase, then try again to land.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Your patch already landed; if you need to change anything, please file a new bug.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

5 years ago
Perhaps I misinterpreted comment 16, I thought that meant the patch could not be applied. Has hunk 2 of the patch landed or not? And if not, should it be a new bug?
Your patch failed to apply, because it was already applied.
(Assignee)

Updated

5 years ago
Blocks: 756007
You need to log in before you can comment on or make changes to this bug.