GNOME Bugzilla – Bug 655087
CoreGraphics error "clip: empty path" creating new window on Lion
Last modified: 2018-04-15 00:30:43 UTC
With GTK 2.24.x on MacOS Lion, when showing a new window, CoreGraphic logs an error: Jul 22 00:34:16 saturn.local mono[26379] <Error>: clip: empty path. I obtained the following trace in gdb (not sure why it doesn't have line numbers):
+ Trace 227839
Ok, had a problem with my CFLAGS. Here's a trace with symbols: 0 0x919d171b in CGPostError ()
+ Trace 227842
Okay, so this problem is because _gdk_quartz_gc_update_cg_contex()'s have_clip_region code-path ends up getting n_rects == 0 and then calls CGContextClipToRects() will no rects. Easy fix is just to make sure it has rectangles to clip, else short-cut and return. Not sure if that is the correct solution or not, but it does fix the warning.
I already lifted a similar fix from gtk-osx-build and applied it to my patchset (https://github.com/mhutch/bockbuild/blob/master/packages/patches/gtk/gdkwindow-quartz.patch), but this seems to be slightly different. Looking more closely at the stacktrace, perhaps it's specific to the murrine engine?
Cool, that patch looks to do what I did.
Well, it's in gdk_window_impl_quartz_begin_paint_region which isn't hit in this trace.
just for the record, this bug is not new with Lion. We got it from within Ardour on Tiger and other versions of OS X, until we chaotically and not very carefully changed some of our own drawing code (in cairo) to something else (slower). we never tracked down the origin, but i don't believe that this is Lion specific.
(In reply to comment #4) > Cool, that patch looks to do what I did. But it's already in gtk-2-24-quartz, and the warning is still emitted, so that's not it.
However, this nearly identical patch is: diff --git a/gdk/quartz/gdkgc-quartz.c b/gdk/quartz/gdkgc-quartz.c index 21cc598..1845ed7 100644 --- a/gdk/quartz/gdkgc-quartz.c +++ b/gdk/quartz/gdkgc-quartz.c @@ -463,6 +463,10 @@ _gdk_quartz_gc_update_cg_context (GdkGC *gc, gdk_region_get_rectangles (_gdk_gc_get_clip_region (gc), &rects, &n_rects); + /* No rects, nothing to do */ + if (!n_rects) + return; + if (n_rects == 1) cg_rects = ▭ else at least in Gtk2. I haven't yet found the equivalent function in Gtk3.
Created attachment 192593 [details] [review] Fix empty path warning for Gtk3 Figured it out. There were two changes.
GTK2 patch works great, thanks!
(In reply to comment #2) > Okay, so this problem is because _gdk_quartz_gc_update_cg_contex()'s > have_clip_region code-path ends up getting n_rects == 0 and then calls > CGContextClipToRects() will no rects. > > Easy fix is just to make sure it has rectangles to clip, else short-cut and > return. Not sure if that is the correct solution or not, but it does fix the > warning. While it does fix the warning, I doubt this is the correct fix. For example, gdk_quartz_draw_rectangle() will happily go on drawing the rectangle using CoreGraphics calls after the call to _gdk_quartz_gc_update_cg_context() regardless whether it has done the actual clipping or not. In this case it will not have done the clipping because the path is empty, so the drawing will happen when it should not.
(In reply to comment #8) > However, this nearly identical patch is: > > diff --git a/gdk/quartz/gdkgc-quartz.c b/gdk/quartz/gdkgc-quartz.c > index 21cc598..1845ed7 100644 > --- a/gdk/quartz/gdkgc-quartz.c > +++ b/gdk/quartz/gdkgc-quartz.c > @@ -463,6 +463,10 @@ _gdk_quartz_gc_update_cg_context (GdkGC > *gc, > gdk_region_get_rectangles (_gdk_gc_get_clip_region (gc), > &rects, &n_rects); > > + /* No rects, nothing to do */ > + if (!n_rects) > + return; > + > if (n_rects == 1) > cg_rects = ▭ > else For the problem I have with this patch, see my above comment. > at least in Gtk2. I haven't yet found the equivalent function in Gtk3. This function is not in GTK+ 3.x because GdkGC does not exist anymore in GTK+ 3.
(In reply to comment #9) > Created an attachment (id=192593) [details] [review] > Fix empty path warning for Gtk3 > > Figured it out. There were two changes. The patch seems to cover up the real issue. I have reproduced the issue on a Tiger machine by asserting on n_rects == 0 manually. It appears that in gdk_window_show_internal() (when called from gdk_window_map), gdk_window_raise_internal() is called before recompute_visible_regions(). gdk_window_raise_internal() will, on Quartz, call orderFront, which triggers displayIfNeeded, which will result in an expose event. This expose event is handled while window->clip_region_with_children is still empty. This results in gdk_window_impl_quartz_begin_paint_region() creating the empty impl->paint_clip_region we are seeing. window->clip_region_with_children is updated in recompute_visible_regions(). So, in fact, the expose is being triggered too early. Handling the expose event does not make much sense because the window is not (yet) visible. I will attach a GDK patch which will refrain GDK from emitting an expose event which it knows is useless because window->clip_region_with_children is empty. It would be great if you could test this on Lion to see if it helps the problem. Then I will talk with the GDK maintainer to see if this is acceptable for them.
Created attachment 192598 [details] [review] Don't emit expose events when window->clip_region_with_children is empty Patch is against GTK+3.
Comment on attachment 192598 [details] [review] Don't emit expose events when window->clip_region_with_children is empty Yes, this seems to work... at least it prevents the warning message. Is there a good test in tests that will show it not attempting to draw?
I backported it to 2.24 and it doesn't work, assuming my backport is correct: http://mjhutchinson.com/files/temp/gdkemptypath.patch
FYI, here is a trace with full symbols:
+ Trace 227881
Oh, looks like bugzilla is stripping the arguments from the traces :/
(In reply to comment #16) > I backported it to 2.24 and it doesn't work, assuming my backport is correct: > http://mjhutchinson.com/files/temp/gdkemptypath.patch The patch will definitely need more work on 2.x to handle the other possible code paths which can trigger this empty path issue. Everything using _gdk_quartz_gc_update_cg_context () is affected.
Here's where we end up setting an empty clip region on the gc: (gdb) bt
+ Trace 227886
The fix seems to be to be to check that the clip region isn't empty first: diff --git a/gdk/gdkwindow.c b/gdk/gdkwindow.c index f058570..c8b791b 100644 --- a/gdk/gdkwindow.c +++ b/gdk/gdkwindow.c @@ -3744,7 +3744,7 @@ start_draw_helper (GdkDrawable *drawable, impl = private->impl; } - if (clip) + if (clip && !gdk_region_empty (clip)) _gdk_gc_add_drawable_clip (gc, clip_region_tag, clip, /* If there was a clip origin set appart from the I'll attach the complete patch next. I should probably call attention to the relationship to this comment in gtk_window_realize(): /* This is a bad hack to set the window background. */ gtk_window_paint (widget, NULL);
Created attachment 192645 [details] [review] Backported Gtk3 patch + don't set empty clip on gc
Review of attachment 192645 [details] [review]: This seems to work on Gtk2 -- at least the warning is gone.
Kris, both your patch for Gtk3 and Jeffrey's for Gtk2 prevent the warning. Can we push them onto their respective branches?
(In reply to comment #23) > Kris, both your patch for Gtk3 and Jeffrey's for Gtk2 prevent the warning. Can > we push them onto their respective branches? Sorry for the slight delay -- I just got back from a quick vacation. I need to talk to a GDK maintainer for these patches and I haven't been able to find him on IRC yet. However, we will likely meet in person at the DesktopSummit which starts on Saturday, so I will discuss this issue there.
(In reply to comment #20) - if (clip) + if (clip && !gdk_region_empty (clip)) _gdk_gc_add_drawable_clip (gc, clip_region_tag, clip, /* If there was a clip origin set appart from the I am not sure if this is correct. If the clip is empty, nothing should be drawn. The above patch will not set the clip in this case, which will cause the entire area to be drawn.
So if the clip exists but is empty, start_draw_helper should return NULL?
It has been suggested to me that the begin_paint_count handling in gdk_window_impl_quartz_get_context() might not be needed anymore in GTK+3, since Cairo is handling all drawing & clipping. A quick test with this removed did not reveal immediate problems. I need to properly investigate whether we can really remove this code. This does not help the case for GTK+2 however. For GTK+2, it has been stressed that we must try to only patch the Quartz backend and not the GDK core to avoid regressions problems (since GTK+2 is in low-maintenance mode).
Well, that's rather narrow minded. After all, your complaint about drawing with an empty clip applies to all backends, not just quartz. So I guess we have to call gdk_region_get_rectangles (_gdk_gc_get_clip_region (gc), &rects, &n_rects); and check n_rects > 0 in each of the gdk_quartz_draw_foo functions. I'll try that out later.
Created attachment 193504 [details] [review] Stop draw if there are no rects I think addresses your concerns about the patch in comment 8: It makes _gdk_quartz_gc_update_cg_context return FALSE if it either gets no context or if there are no rectangles in the region, TRUE otherwise. All calls to update_cg_context() check that return value and release the CGContextRef and return if it's FALSE.
Comment on attachment 193504 [details] [review] Stop draw if there are no rects As an added bonus, this seems to fix a likely memory leak reported against Gnucash, which I think was caused by returning without releasing context.
(In reply to comment #28) > Well, that's rather narrow minded. After all, your complaint about drawing with > an empty clip applies to all backends, not just quartz. Well, there are two things here: 1) The drawing with the empty clip likely does not appear on other backends, because the windowing systems backing those do not immediately do a redraw like Cocoa is doing. 2) For GTK+3, the expose event will be handled, but because an empty clip is set in Cairo, Cairo will not do any drawing. In other words, we want Cairo to deal with this empty clip (as we've pushed all drawing code to drawing) and keep the code in GDK as generic as possible.
Comment on attachment 193504 [details] [review] Stop draw if there are no rects I quite like this patch! It's fine for 2-24. For GTK+3, I will look into seeing if it is possible to eliminate the begin_paint_count ASAP.
Comment on attachment 193504 [details] [review] Stop draw if there are no rects http://git.gnome.org/browse/gtk+/commit/?h=gtk-2-24&id=a90d9da9e99dd96d3f7cc64a83da66a95e223e09
i have no hard data, but i have a hunch that this patch (in whatever form it is used in john's gtk-osx modulesets) may be responsible for a regression. when creating certain GdkWindows now (i've seen it for a small dialog and also for the GnomeCanvas window in ardour), the first display of the window draws as a blank white rectangle. this was not the case in gtk 2.22 which was the last version of GTK that we were using for building ardour. i'm not expecting anyone to put in a big effort to debug this, but if there is anything about how the patch operates that might explain this behaviour, please do comment.
i've confirmed that its the patch in https://raw.github.com/jralls/gtk-osx-build/master/patches/gdk-quartz-norects.patch that causes this behaviour.
Paul, is this a "flicker", where the empty window is getting shown momentarily and then a properly rendered one is displayed, or do you have to force a redraw to get the window rendered correctly? The patch aborts the drawing command if there's no rect found to draw into. It's possible that it has revealed the problem Kris described in #13. I'll have to study that a bit.
its a flicker AFAICT. the window is still displayed correctly without any code changes. in the case of the dialog i saw, i have reason to think there were any more redraws queued by GTK or the app.
oops, that last comment should have said "i have NO reason to think .."
> It's possible that it has revealed the problem Kris described in #13. I'll have > to study that a bit. It does appear that the sequence Kris enumerates in #13 is in place in Gtk2. Unfortunately I'm not seeing the problem, which makes it a bit hard to test. Maybe you could do a test of some sort before calling NSWindow orderFront: in gdk_window_quartz_raise()?
ok, here's what i have noted so far: 1) a patch like Kristian's in attachment 14 [details] and Michael's backport in 16 is necessary to stop the unnecessary exposes. The patch cited in 35 has no impact on this at all, but at the same time, the 14/16 patches do not relate to the separate bug where a CGContext gets an empty clip path. 2) the patch cited in #35 is wrong. its wrong because the purpose of gc_update_cg_context() is not *just* to set the clip region of a CGContext, its also to update any other possible property of a CGContext. In addition, there is no reason to ever return FALSE from this function because whether or not there is a clip region set doesn't change the logic of whatever happens after a call to gc_update_cg_context(). I think that somehow we got sidetracked by the n_rects == 0 issue and failed to notice that this condition only affects whether or not to set a clip region or not, and nothing else. 3) I've done a minimal modification that: 1) applies the patch from 16 to GTK+ 2.24 (avoid unnecessary exposes) 2) in gc_update_cg_context(), only call CGContextClipToRects() if the clip region is not empty (i.e. the reverse of the current logic) 3) always return TRUE from gc_update_cg_context() a correct patch will revert to a void return from gc_update_cg_context(). i need to get this tested on Lion (which i don't have) before i can confirm that it works as fully as intended.
one potentially deeper issue is that CG does not appear to allow unsetting a clip path/rect/mask. this means that if the GDK GC is modified to have no clip region, there is no way to propagate this information into the CGContext. i don't know if this is a real problem or not.
there's another location where CGContextClipToRect(s) is called: gdk_window_impl_quartz_get_context (). i've locally changed that to only make the CG call if n_rects != 0 although its much less likely to ever happen there because the region in question is almost certainly not empty (true if the no-expose-when-clip-region-empty patch is used).
(In reply to comment #40) > ok, here's what i have noted so far: > > 1) a patch like Kristian's in attachment 14 [details] and Michael's backport in 16 is > necessary to stop the unnecessary exposes. The patch cited in 35 has no impact > on this at all, but at the same time, the 14/16 patches do not relate to the > separate bug where a CGContext gets an empty clip path. > > 2) the patch cited in #35 is wrong. its wrong because the purpose of > gc_update_cg_context() is not *just* to set the clip region of a CGContext, its > also to update any other possible property of a CGContext. In addition, there > is no reason to ever return FALSE from this function because whether or not > there is a clip region set doesn't change the logic of whatever happens after a > call to gc_update_cg_context(). I think that somehow we got sidetracked by the > n_rects == 0 issue and failed to notice that this condition only affects > whether or not to set a clip region or not, and nothing else. > > 3) I've done a minimal modification that: > > 1) applies the patch from 16 to GTK+ 2.24 (avoid unnecessary exposes) > 2) in gc_update_cg_context(), only call CGContextClipToRects() if the clip > region is not empty (i.e. the reverse of the current logic) > 3) always return TRUE from gc_update_cg_context() > > a correct patch will revert to a void return from gc_update_cg_context(). i > need to get this tested on Lion (which i don't have) before i can confirm that > it works as fully as intended. We've already been down that road: 1) Jeffery already submitted a patch in attachment 192645 [details] [review] that backports attachement 192598. Kris shot it down (in comment 27) because the GDK maintainer wants the change to be only in quartz. 2) The reason for update_cg_context to return a boolean is to abort the drawing if there are no rectangles to clip to, because Kris objected to allowing drawing to continue in that case. In such an instance there's no need to continue to set the rest of the graphics context because drawing will be aborted. 3) If update_cg_context should always return true, then it should just be reverted to void. If you want something tested on Lion you're going to have to attach a patch.
(In reply to comment #41) > one potentially deeper issue is that CG does not appear to allow unsetting a > clip path/rect/mask. this means that if the GDK GC is modified to have no clip > region, there is no way to propagate this information into the CGContext. i > don't know if this is a real problem or not. From the Quartz 2D Drawing Guide: "The clipping area is part of the graphics state. To restore the clipping area to a previous state, you can save the graphics state before you clip, and restore the graphics state after you’re done with clipped drawing." Should we have a stack of graphics states that we pop when we want to clear clip regions? (I haven't yet found how to save and restore graphics states, but there's sure to be a way.) It appears to me that at least part of the problem is that priv->have_clip_region is true, but the clip region has no rects. If that weren't the case then there wouldn't be any calling of CGContextClipToRects() with no rects.
1) i understood that. the point is that the patch is still relevant, even if it never makes it into GDK for 2.X. 2) If Kris is correct about "If the clip is empty, nothing should be drawn.", then i don't see this documented for either GDK or CoreGraphics. I don't see any requirement to set a clip region/mask on a GC before using it. 3) right, i've done that (mostly by reverting your patch). whether this is correct depends on the final analysis of the issue in (2) above. i would submit a patch, but my editing environment right now would generate a mal-formed one (from a whitespace perspective), so i'm holding off on it. we have various lion testers around that are trying my patch(es) as i write. once i can generate a well-formed patch, i'll attach it.
re: clip restore - this is fundamentally in conflict with the X/GDK model. in X/GDK you can do this: set clip region set stipple set color draw change clip region draw in CoreGraphics, you have to: save state set clip region set stipple set color draw restore state reset clip region set stipple set color draw i can't see any way around this. CG just doesn't allow an operation that sets the clip region back to "null", whereas X/GDK does.
"It appears to me that at least part of the problem is that priv->have_clip_region is true, but the clip region has no rects. If that weren't the case then there wouldn't be any calling of CGContextClipToRects() with no rects." this seems right to me (though the needless expose call also triggers the same thing as i noted above in gdk_window_impl_quartz_get_context (). i'll take a look and see if i can understand why have_clip_region gets set this way.
john, excellent observation. it appears that the root problem of the gc_update_cg_context() part of this is fixed with this change. i've confirmed with additional debugging the empty region path through this really does get called, and corresponds precisely to the times when we would have seen the empty clip path message from CG. --- gdkgc-quartz.c~ 2011-08-19 11:13:31.000000000 -0400 +++ gdkgc-quartz.c 2011-08-19 13:00:37.000000000 -0400 @@ -289,7 +289,10 @@ private->have_clip_mask = FALSE; } - private->have_clip_region = region != NULL; + if (region == NULL || gdk_region_empty (region)) { + private->have_clip_region = FALSE; + else + private->have_clip_region = TRUE;
oops, make that: --- gtk+-2.24.0/gdk/quartz/gdkgc-quartz.c~ 2011-08-19 11:13:31.000000000 -0400 +++ gtk+-2.24.0/gdk/quartz/gdkgc-quartz.c 2011-08-19 13:19:22.000000000 -0400 @@ -289,7 +289,10 @@ private->have_clip_mask = FALSE; } - private->have_clip_region = region != NULL; + if (region == NULL || gdk_region_empty (region)) + private->have_clip_region = FALSE; + else + private->have_clip_region = TRUE;
Which could be said more tersely as: --- gtk+-2.24.0/gdk/quartz/gdkgc-quartz.c~ 2011-08-19 11:13:31.000000000 -0400 +++ gtk+-2.24.0/gdk/quartz/gdkgc-quartz.c 2011-08-19 13:19:22.000000000 -0400 @@ -289,7 +289,7 @@ private->have_clip_mask = FALSE; } - private->have_clip_region = region != NULL; + private->have_clip_region = region != NULL && !gdk_region_empty (region); if (reset_origin) { (Or !(region == NULL || gdk_region_empty (region)) Much cleaner and less hacky than attachment 193504 [details] [review]. But this still doesn't do anything for the premature expose, does it? Is there something convenient we can test in gdk_quartz_window_raise that would tell us that we need to call recompute_visible_regions() before orderFront:?
you're right that this doesn't address the premature expose. that seems much harder to tackle, and also the current code doesn't cause any actual misbehaviour, so i'm reluctant to put much effort into a scheme to deal with the expose issue.
(In reply to comment #49) > > - private->have_clip_region = region != NULL; > + if (region == NULL || gdk_region_empty (region)) > + private->have_clip_region = FALSE; > + else > + private->have_clip_region = TRUE; What I am still wondering about is: if the region is empty, why would the caller not pass NULL? Or, put differently, is region == NULL equal to region != NULL && gdk_region_empty (region)? I think they are not equal, because one could say that: - region == NULL means no clip region is set, so draw without clip. - region != NULL && gdk_region_empty (region) means a clip region is set, but is empty, so nothing should be drawn. In the scenario I described in comment 13, the second case did exist. Because the window was not visible at the time the expose event came in, window->clip_region_with_children was empty which resulted in an empty clip mask.
kris - can you point to any docs or code that indicate the presence or absence of clip region in a GC has some implications for drawing?
This fragment, in _gdk_gc_set_drawable_clip(), lends credibility to Kris's belief that an empty clip region means "don't draw anything" (clip everything?): /* Its quite common to expose areas that are completely in or outside * the region, so we try to avoid allocating bitmaps that are just fully * set or completely unset. */ overlap = gdk_region_rect_in (region, &r); if (overlap == GDK_OVERLAP_RECTANGLE_PART) { /* The region and the mask intersect, create a new clip mask that includes both areas */ priv->old_clip_mask = g_object_ref (priv->clip_mask); new_mask = gdk_pixmap_new (priv->old_clip_mask, w, h, -1); tmp_gc = _gdk_drawable_get_scratch_gc ((GdkDrawable *)new_mask, FALSE); gdk_gc_set_foreground (tmp_gc, &black); gdk_draw_rectangle (new_mask, tmp_gc, TRUE, 0, 0, -1, -1); _gdk_gc_set_clip_region_internal (tmp_gc, region, TRUE); /* Takes ownership of region */ gdk_draw_drawable (new_mask, tmp_gc, priv->old_clip_mask, 0, 0, 0, 0, -1, -1); gdk_gc_set_clip_region (tmp_gc, NULL); gdk_gc_set_clip_mask (gc, new_mask); g_object_unref (new_mask); } else if (overlap == GDK_OVERLAP_RECTANGLE_OUT) { /* No intersection, set empty clip region */ GdkRegion *empty = gdk_region_new (); gdk_region_destroy (region); priv->old_clip_mask = g_object_ref (priv->clip_mask); priv->clip_region = empty; _gdk_windowing_gc_set_clip_region (gc, empty, FALSE); } else { /* Completely inside region, don't set unnecessary clip */ gdk_region_destroy (region); return;
(In reply to comment #53) > kris - can you point to any docs or code that indicate the presence or absence > of clip region in a GC has some implications for drawing? It's very likely fair to say that GDK follows the X model. In fact, gc_set_clip_region() triggers a call to XSetClipRectangles() internally (_gdk_x11_gc_flush()). The manual for XSetClipRectangles() reads: " The XSetClipRectangles() function changes the clip-mask in the specified GC to the specified list of rectangles and sets the clip origin. The output is clipped to remain contained within the rectangles. ... The rectangles should be nonintersecting, or the graphics results will be undefined. Note that the list of rectangles can be empty, which effectively disables output. This is the opposite of passing None as the clip-mask in XCreateGC(), XChangeGC(), and XSetClipMask(). " (from: http://tronche.com/gui/x/xlib/GC/convenience-functions/XSetClipRectangles.html) This says an empty list of rectangles disables output; passing None (a NULL GdkRegion pointer) is the opposite and thus enables all output.
Getting this started again, I guess the biggest issue we are seeing are the unnecessary exposes. On GTK+ 2, with the patch applied (like in current master), the window first draws white, before the actual content is drawn. For me, this is similar in current GTK+ 3 master. (I have to say that I can only observe the white flash using Quartz Debug). If I revert the patch on the GTK+ 2 version, I see a black rectangle flash instead of white, before the actual window content is drawn. My feeling is that the bug already existed before applying the patch, though it flashed in black which is perhaps less noticeable? Then there's the issue brought up by Paul in comment 40: > 2) the patch cited in #35 is wrong. its wrong because the purpose of > gc_update_cg_context() is not *just* to set the clip region of a CGContext, its This is true. > also to update any other possible property of a CGContext. In addition, there > is no reason to ever return FALSE from this function because whether or not > there is a clip region set doesn't change the logic of whatever happens after a > call to gc_update_cg_context(). I think that somehow we got sidetracked by the > n_rects == 0 issue and failed to notice that this condition only affects > whether or not to set a clip region or not, and nothing else. The alternative would be to ditch the return value added to _gdk_quartz_gc_update_cg_context() and instead do the following in the drawing functions: _gdk_quartz_gc_update_cg_context (...); if (_gdk_quartz_gc_empty_clip_region (gc)) { gdk_quartz_drawable_release_context (drawable, context); return; } So if the clip region on the gc is empty, don't perform any drawing. However, the code will more or less be the same compared to the code with the return value for _gdk_quartz_cg_update_cg_context().
(In reply to comment #27) > It has been suggested to me that the begin_paint_count handling in > gdk_window_impl_quartz_get_context() might not be needed anymore in GTK+3, > since Cairo is handling all drawing & clipping. A quick test with this removed > did not reveal immediate problems. I need to properly investigate whether we > can really remove this code. For GTK+ 3 I still plan to remove this code. In GTK+ 2, we had to set the clip on the CGContext after acquiring the context in gdk_window_impl_quartz_get_context(), since the context could be used for drawing through the old GdkGC interface. In GTK+ 3, we create the context in _get_context() solely to create a Cairo surface (gdk_quartz_create_cairo_surface(), called by gdk_quartz_ref_cairo_surface()). All drawing and clipping from there on is handled through Cairo. My impression is thus that we can eliminate this code. I will attach the patch that I plan to commit to the GTK+ 3 branches.
Created attachment 199777 [details] [review] Patch to eliminate empty clip error for GTK+ 3
Comment on attachment 199777 [details] [review] Patch to eliminate empty clip error for GTK+ 3 Let's comment the gutted functions with something like /* Not necessary, cairo does this. See Bug 655087 */ so that we remember why they're empty a year or two from now. It looks to me, though, that quartz is the only implementation of begin_paint and end_paint, so maybe we could just get rid of the interface altogether. But does this fix the extra expose?
Meanwhile, back at 2.24, I don't really see much difference between your _gdk_quartz_gc_update_cg_context (...); if (_gdk_quartz_gc_empty_clip_region (gc)) { gdk_quartz_drawable_release_context (drawable, context); return; } and what I did in a90d9da unless there's a difference between nrect == 0 and gdk_quartz_gc_empty_clip_region() (highly likely!). The other thing is that in our somewhat private email discussion of a month ago, Paul said that - private->have_clip_region = region != NULL; + private->have_clip_region = region != NULL && !gdk_region_empty (region); in _gdk_windowing_gc_set_clip_region() *does* fix the bogus expose. It's not clear to me that it fixes the empty clip region in all cases, so I'm in favor of doing both.
(In reply to comment #59) > (From update of attachment 199777 [details] [review]) > Let's comment the gutted functions with something like /* Not necessary, cairo > does this. See Bug 655087 */ so that we remember why they're empty a year or > two from now. But that's what git annotate is for ... > But does this fix the extra expose? No I don't think so, my impression so far is that the extra expose is a separate issue that already existed before we started to fix this bug.
(In reply to comment #60) > Meanwhile, back at 2.24, I don't really see much difference between your > _gdk_quartz_gc_update_cg_context (...); > [snip] > and what I did in a90d9da That's exactly what I said in comment 56 :) : "" However, the code will more or less be the same compared to the code with the return value for _gdk_quartz_cg_update_cg_context(). "" > The other thing is that in our somewhat private email discussion of a month > ago, Paul said that > - private->have_clip_region = region != NULL; > + private->have_clip_region = region != NULL && !gdk_region_empty (region); > in _gdk_windowing_gc_set_clip_region() *does* fix the bogus expose. It's not > clear to me that it fixes the empty clip region in all cases, so I'm in favor > of doing both. I will check that, but I think it will just change the bogus expose to draw a black rectangle instead of a white one.
(In reply to comment #61) > (In reply to comment #59) > > (From update of attachment 199777 [details] [review] [details]) > > Let's comment the gutted functions with something like /* Not necessary, cairo > > does this. See Bug 655087 */ so that we remember why they're empty a year or > > two from now. > > But that's what git annotate is for ... Umm, yeah, as long as everyone remembers to use it. Safer to put in the comment. Better yet, let's just get rid of the functions instead of gutting them. The calls in gdkwindow.c check the pointers before using them, so there's no need to have empty placeholder functions. > > > But does this fix the extra expose? > > No I don't think so, my impression so far is that the extra expose is a > separate issue that already existed before we started to fix this bug. Well, whether it existed before or not we still need to fix it, and it seems silly to open a new bug when we already know what to do, at least for 3.x.
(In reply to comment #63) > (In reply to comment #61) > > But that's what git annotate is for ... > > Umm, yeah, as long as everyone remembers to use it. Safer to put in the > comment. I would expect developers to remember that. Otherwise there's loads more code to annotate with comments ... > Better yet, let's just get rid of the functions instead of gutting them. The > calls in gdkwindow.c check the pointers before using them, so there's no need > to have empty placeholder functions. Sure. > Well, whether it existed before or not we still need to fix it, and it seems > silly to open a new bug when we already know what to do, at least for 3.x. Yes we need to fix it. The main point is that I am not (yet) convinced whether it is a proper fix, because I think that it changes the white flash ("unnecessary expose") to a black flash. I still need to re-test that.
and .. the white flash is back after mitch has committed a bunch of changes to gtk-2-24. this time, the patches i applied on my local tree(s) to fix it no longer have any effect. this is ... irritating.
ok, so here's the scoop. kris commited 92ea94af5f1a4d0970628b58997192ccf74cab36 which attempts to fix corruption in NSViews with a CALayer set (CA => CoreAnimation) when drawRect is called before the window has been told that it is mapped. this occurs when we call showAndMakeKey on the window before calling _gdk_quartz_events_send_map_event(). his commit fixes that case by using some global drawing methods to fill the rect with the current system window background color, and then returns immediately. there are two problems with this commit as it relates to the regression discussed here. (1) doing this fill for NSViews that are *not* using CALayers results in an initial white flash in the window when it is first shown. You can see this more easily by changing the fill from windowBackgroundColor to redColor and then running gtk-demo (or any other gtk app) (2) this isn't really a problem, but the commit does not solve the problem being discussed above as it pertains to empty clip regions. I've spent a good part of today experimenting with this all in current gtk-2-24 and the takeaway summary is that a small modification to kris' commit is needed to stop it interfering with non-CA-using views, and then in addition we need the small fix discussed between john, kris and myself that ended up with the lines in #60. kris believed that this should not be necessary - i think it is, largely because of the different way that the initial show/map events are delivered on OS X compared to X11. This is the small change to kris' commit: --- a/gdk/quartz/GdkQuartzView.c +++ b/gdk/quartz/GdkQuartzView.c @@ -86,7 +86,7 @@ if (NSEqualRects (rect, NSZeroRect)) return; - if (!GDK_WINDOW_IS_MAPPED (gdk_window)) + if (!GDK_WINDOW_IS_MAPPED (gdk_window) && ((gdk_quartz_osx_version() > GDK_OSX_LEOPARD) && [self wantsLayer])) { /* If the window is not yet mapped, clip_region_with_children * will be empty causing the usual code below to draw nothing. the version test is included because wantsLayer isn't a recognized selector before 10.5.
meh, it should of course be >= LEOPARD not > LEOPARD: diff --git a/gdk/quartz/GdkQuartzView.c b/gdk/quartz/GdkQuartzView.c index 77ff5d7..3997a7e 100644 --- a/gdk/quartz/GdkQuartzView.c +++ b/gdk/quartz/GdkQuartzView.c @@ -86,7 +86,7 @@ if (NSEqualRects (rect, NSZeroRect)) return; - if (!GDK_WINDOW_IS_MAPPED (gdk_window)) + if (!GDK_WINDOW_IS_MAPPED (gdk_window) && ((gdk_quartz_osx_version() >= GDK_OSX_LEOPARD) && [self wantsLayer])) { /* If the window is not yet mapped, clip_region_with_children * will be empty causing the usual code below to draw nothing.
That looks ok to me, kris?
(In reply to comment #66) > ok, so here's the scoop. kris commited 92ea94af5f1a4d0970628b58997192ccf74cab36 > which attempts to fix corruption in NSViews with a CALayer set (CA => > CoreAnimation) when drawRect is called before the window has been told that it > is mapped. this occurs when we call showAndMakeKey on the window before calling > _gdk_quartz_events_send_map_event(). his commit fixes that case by using some > global drawing methods to fill the rect with the current system window > background color, and then returns immediately. Correct, the thing is that if you get a drawRect and don't draw something, then for the initial drawRect you end up with undefined behavior. For CA applications this is essentially bad because this manifests itself as a display of uninitialized memory. For non-CA applications this wasn't as bad, I *think* I either got a black or transparent window (can't remember). > (2) this isn't really a problem, but the commit does not solve the problem > being discussed above as it pertains to empty clip regions. I would have to go through the above discussion again. Last time I looked at the code, the situation is that GDK/GTK+ is not ready to draw yet (things are not mapped and setup) but we do get a drawRect already. We have to draw something, but cannot go through the usual GDK code path. > lines in #60. kris believed that this should not be necessary - i think it is, > largely because of the different way that the initial show/map events are > delivered on OS X compared to X11. Exactly. I will double check these two lines in the weekend to see if it can help us. Otherwise we have, I think, two options: 1) Live with the current situation, which will incur a flash for CA applications and we commit your patch below to alleviate for non-CA applications. 2) Figure out if it's possible to avoid the drawRect: call *before* the toplevel is actually mapped in the GDK sense. If this is possible, then the code to draw a "temporary" window background can be removed altogether.
FWIW, I *tried* re-ordering gdk_quartz_events_send_map_event() and showAndMakeKey, and it did not stop quartz from calling drawRect before mapped. unfortunately i am developing mostly on a tiger-based system that lacks the libc support for stacktracing from within the program, and have other needs right now that prevent me from building a debug version of gtk. otherwise i'd know precisely where (and maybe why) drawRect is called "so early".
it might also be better to use a version of the drawRect patch that looks more like this if (!GDK_WINDOW_IS_MAPPED (gdk_window) && [self respondsToSelect:@selector(wantsLayer)] && [self wantsLayer]) which then makes it totally independent of OS X version nonsense.
(In reply to comment #70) > FWIW, I *tried* re-ordering gdk_quartz_events_send_map_event() and > showAndMakeKey, and it did not stop quartz from calling drawRect before mapped. > > unfortunately i am developing mostly on a tiger-based system that lacks the > libc support for stacktracing from within the program, and have other needs > right now that prevent me from building a debug version of gtk. otherwise i'd > know precisely where (and maybe why) drawRect is called "so early". A first clue for this might be in comment 13. The first call to displayIfNeeded is done by orderFront called from gdk_window_quartz_raise(). This trace looks as follows (just tried again):
+ Trace 231248
If I comment out the call to orderFront in gdk_window_quartz_raise(), I do not get a white or grey flash. Actually, drawRect is no longer called when the GDK toplevel is not yet mapped. I am not sure yet how to turn this into a proper patch. Perhaps we can have a special case (since people don't really want us to touch generic GDK) for new toplevel windows where we delay the raise and combine it with GDK mapping the window? (In reply to comment #71) > it might also be better to use a version of the drawRect patch that looks more > like this > > if (!GDK_WINDOW_IS_MAPPED (gdk_window) && [self > respondsToSelect:@selector(wantsLayer)] && [self wantsLayer]) > > which then makes it totally independent of OS X version nonsense. From what I recall we started to prefer checking the version instead of (or before) checking whether the object responds to a given selector because "respondsToSelector" can be slow.
Been experimenting for some time, here are my observations: (In reply to comment #66) > (1) doing this fill for NSViews that are *not* using CALayers results in an > initial white flash in the window when it is first shown. You can see this more > easily by changing the fill from windowBackgroundColor to redColor and then > running gtk-demo (or any other gtk app) This was not introduced by the cited patch. Doing this fill actually gives you a light grey, while NOT doing this gives you white. More on that below. > - if (!GDK_WINDOW_IS_MAPPED (gdk_window)) > + if (!GDK_WINDOW_IS_MAPPED (gdk_window) && ((gdk_quartz_osx_version() > > GDK_OSX_LEOPARD) && [self wantsLayer])) > { So, if you look at this patch in isolation (so without considering comment 60), then the only difference this makes is whether you get white on the first expose or light grey. Light grey is produced by the windowBackgroundColor. Now, where does the white come from? It turns out that if you draw nothing in drawRect, you get a white area. This has been verified in GDK by simply always returning immediately in drawRect. But has also been verified in a stand-alone Cocoa test with a custom view w/o drawRect method or a drawRect method which is empty. > lines in #60. kris believed that this should not be necessary - i think it is, > largely because of the different way that the initial show/map events are > delivered on OS X compared to X11. What the patch in comment 60 does, is actually allowing GDK to draw even though the window is officially seen not mapped. This means that GDK draws its default window background, which is a darker grey than Cocoa's default window background. (I am guessing Cocoa's default window background looks like it is white when it only appears for a very short period). So, if you look at this patch after applying the above patch, the difference you get is whether you get a white or darker grey flash. I still believe the patch in comment 60 is not correct. Due to this patch, the have_clip_region clause is skipped in gdk_quartz_gc_update_cg_context(), so no clip region is set and drawing may proceed when the provided region clip is an empty one. Earlier in this bug we have discussed that a set clip region which is empty means that no drawing should be done. The patch in comment 60 violates this. In summary, it seems it is all about the colors. Do you get a white, grey or dark grey flash? ;) However, the real fix is to eliminate the bogus expose, I think we figured how this could be done in the previous comment. I believe that when that is fixed, neither the comment 66 or comment 60 patches are necessary.
its not about the colors. any application that uses a non-default bg for a window will end up with a flash with the code in the state that it is right now in gtk-2-24, because one way or another quartz and/or GDK will draw a default bg before GDK thinks there is anything else to draw. on the next draw for the window, it will be filled in with the correct bg for that application. the lines from #60 stop this from happening by getting GDK to draw the *correct* bg on the first call to drawRect. i agree that a better way to stop it from happening is to prevent drawRect being called before the window is mapped (assuming that it being mapped prevents the clip region from being empty). but this appears to require an almost equally hacky solution as simply causing the unmapped-drawRect to use the correct bg fill.
(In reply to comment #74) > the lines from #60 stop this from happening by getting GDK to draw the > *correct* bg on the first call to drawRect. But also gets GDK to draw outside the requested clip in other cases. > i agree that a better way to stop it from happening is to prevent drawRect > being called before the window is mapped (assuming that it being mapped > prevents the clip region from being empty). but this appears to require an > almost equally hacky solution as simply causing the unmapped-drawRect to use > the correct bg fill. Another solution is to make the !MAPPED handling code in drawRect actually draw the background color as set by the GTK+/GDK theme instead of the Cocoa default. This may also be hacky, but perhaps less hacky than messing around with raise or changing clipping behavior. How does that sound?
(In reply to comment #75) > (In reply to comment #74) > > the lines from #60 stop this from happening by getting GDK to draw the > > *correct* bg on the first call to drawRect. > > But also gets GDK to draw outside the requested clip in other cases. how so? > > > > i agree that a better way to stop it from happening is to prevent drawRect > > being called before the window is mapped (assuming that it being mapped > > prevents the clip region from being empty). but this appears to require an > > almost equally hacky solution as simply causing the unmapped-drawRect to use > > the correct bg fill. > > > Another solution is to make the !MAPPED handling code in drawRect actually draw > the background color as set by the GTK+/GDK theme instead of the Cocoa default. > This may also be hacky, but perhaps less hacky than messing around with raise > or changing clipping behavior. > > > How does that sound? like an improvement.
(In reply to comment #75) > Another solution is to make the !MAPPED handling code in drawRect actually draw > the background color as set by the GTK+/GDK theme instead of the Cocoa default. > This may also be hacky, but perhaps less hacky than messing around with raise > or changing clipping behavior. Turns out this is not a hack, but already happening in gtk_window_realize() by the gtk_style_set_background calls. These happen before gtk_window_paint() which is marked with a "bad hack" comment, so the bad hack uses the correct color for the window background. The attached patch actually implements gdk_window_quartz_set_background(). So, this function is called to set the background color and after that the "bad hack" will trigger the "lets draw a background color since we are clipped" code. As a result, the background color according to the style is drawn. I think this is the behavior we are looking for. Paul, could you test this patch? If it then gives you a flash in the correct background color, I can commit this and close the bug.
Created attachment 232356 [details] [review] patch which implements gdk_window_quartz_set_background
Kristian, sorry that this has taken me a month to test, but super-busy with ardour3 release stuff. I can confirm that this removes the white flash and replaces it with a "bg" flash in the correct widget's color, similar to X11. Please comment on this bug report when you commit it.
Kristian, unfortunately, with further testing, this patch does NOT fix the white-bg-flash. Will do some more investigating as/when I have time.
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new