Last Comment Bug 747822 - [Azure] Implement push/pop clip in Cairo backend
: [Azure] Implement push/pop clip in Cairo backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Nick Cameron [:nrc]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 694351 756007
  Show dependency treegraph
 
Reported: 2012-04-22 21:34 PDT by Nick Cameron [:nrc]
Modified: 2012-05-16 23:05 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
push/pop clip patch (1.59 KB, patch)
2012-05-01 19:50 PDT, Nick Cameron [:nrc]
joe: review-
Details | Diff | Splinter Review
push/pop clip patch (1.59 KB, patch)
2012-05-03 23:19 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
push/pop clip patch (2.24 KB, patch)
2012-05-04 00:06 PDT, Nick Cameron [:nrc]
joe: review+
Details | Diff | Splinter Review
push/pop clip patch (2.31 KB, patch)
2012-05-07 16:36 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-04-22 21:34:43 PDT

    
Comment 1 Nick Cameron [:nrc] 2012-05-01 19:50:24 PDT
Created attachment 620171 [details] [diff] [review]
push/pop clip patch
Comment 2 Nick Cameron [:nrc] 2012-05-01 19:52:26 PDT
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 3 Bas Schouten (:bas.schouten) 2012-05-02 07:36:43 PDT
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 4 Joe Drew (not getting mail) 2012-05-02 10:17:02 PDT
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.
Comment 5 Joe Drew (not getting mail) 2012-05-02 10:17:47 PDT
(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?
Comment 6 Nick Cameron [:nrc] 2012-05-03 22:49:31 PDT
(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.
Comment 7 Nick Cameron [:nrc] 2012-05-03 23:19:11 PDT
(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.
Comment 8 Nick Cameron [:nrc] 2012-05-03 23:19:51 PDT
Created attachment 620972 [details] [diff] [review]
push/pop clip patch
Comment 9 Nick Cameron [:nrc] 2012-05-04 00:06:06 PDT
Created attachment 620981 [details] [diff] [review]
push/pop clip patch
Comment 10 Joe Drew (not getting mail) 2012-05-07 13:41:13 PDT
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.
Comment 11 Nick Cameron [:nrc] 2012-05-07 16:36:02 PDT
Created attachment 621784 [details] [diff] [review]
push/pop clip patch

added comment as requested, carrying r=joe
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-05-08 15:52:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/404ef622ffd3
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-08 20:06:23 PDT
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
Comment 14 Nick Cameron [:nrc] 2012-05-08 22:13:19 PDT
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?
Comment 15 Ed Morley [:emorley] 2012-05-09 03:44:35 PDT
https://hg.mozilla.org/mozilla-central/rev/404ef622ffd3
Comment 16 Mozilla RelEng Bot 2012-05-11 16:20:03 PDT
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.
Comment 17 Nick Cameron [:nrc] 2012-05-13 03:50:53 PDT
Need to make some changes to the patch and rebase, then try again to land.
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 05:06:56 PDT
Your patch already landed; if you need to change anything, please file a new bug.
Comment 19 Nick Cameron [:nrc] 2012-05-13 13:35:14 PDT
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?
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-05-13 14:06:13 PDT
Your patch failed to apply, because it was already applied.

Note You need to log in before you can comment on or make changes to this bug.