Understanding and fixing flickering on GTK >= 3.10

[Colomban Wendling]

Hi,

https://sourceforge.net/p/scintilla/bugs/1567/ reports a important
flickering issue when using GTK >= 3.9.2.

TL;DR: it's complicated, but there is a patch linked at the bottom.


## Why does something happen on GTK 3.9.2?

This is due to a change in the way GTK 3.9.2 performs drawing, which
basically makes gtk_widget_set_double_buffered() useless.

In practice, now GTK draws the whole app on one single buffer (Cairo
context), from the bottom-up (from the root window to the deepest
children). This means that the area of the buffer on which we draw will
*always* be invalidated first, because the parent container will have
had drawn its background already. Before this change, we had our own
buffer no one else would mess with.


## Why does this make Scintilla flicker?

When we receive a draw request, we first try to honor it, and if
everything goes well, we do -- and everybody's happy.

But if the area is not large enough for the redraw we have to do,
instead of gently finishing the current draw and then invalidating a
larger area, we try to perform an optimization by abandoning the pain as
soon as we realize it won't be enough. This typically happens when
folding changes.

This used not to be a problem, since we just pained over the existing
buffer: at worse we had inconsistent display before the next scheduled
paint kicked in, it was hardly possible to notice. But now the area
scheduled for repaint it unconditionally pain blank first, we see the
blank area flashing.


## What can we do about this?

Well. First, we could hope for GTK to revert this change since it broke
some applications (at least us), but I doubt this is realistic.

Then, we could try to always paint the whole requested area. This even
sounds more sensible, because relying on single buffering is kind of out
of the time, nowadays most (if not all?) display APIs expect the client
to sensibly answer the draw requests fitting the model.

To properly do this, we have several solutions, with various degrees of
quality and complexity.

1) ScintillaBase::Paint() could be modified never to abandon a paint.
This would remove what looks like an optimization, and would affect all
platforms. I guess this is not an option.

2) ScintillaGTK could have pixel caching. This can possibly be the best
solution, as it allows to paint e.g. on tiles on free time, and simply
"blit" them when the actual draw occurs. It could reduce the drawing
time since the real drawing code could possibly have lot less to draw
(only the parts that actually changed), and could possibly improve
display reactivity (as we would take virtually no time performing the
visible draw).

3) Instead of scheduling a new draw when the pain area is not large
enough, extend the clip manually. This is kind of dirty and requires
non-obvious API, but fortunately Cairo has it: cairo_reset_clip().
This can have better overall performance than we have now (no
re-scheduling), but could make one draw longer (since the full redraw
would happen immediately).

4) Maybe something else??? What do other platform layers do?


Well. I guess 1) is not an option. I would think 2) is the way to go,
but probably requires quite some work. 3) is a bit of a hack, but is
pretty easy, and does work (at least on X11?).


## Proposed solution

I have attached a proposed patch using solution 3 to the bug:
https://sourceforge.net/p/scintilla/bugs/_discuss/thread/6d86168e/8182/attachment/0001-Avoid-flickering-on-some-redraws-with-GTK-3-9-2.patch
Note that most of the changes in ScintillaGTK::DrawTextThis() are
indentation changes because of the added outer `for` loop, most of the
code is the same (you can check the diff ignoring space changes).


What do you think?

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> 1) ScintillaBase::Paint() could be modified never to abandon a paint.
> This would remove what looks like an optimization, and would affect all
> platforms. I guess this is not an option.

One quick option is to set paintingAllText to true before calling Paint. This stops AbandonPaint from abandoning the paint although it means there is no way for the GTK+ code to detect it needs to redraw more.

A more complete approach may be to make AbandonPaint virtual and implement it for GTK+, by setting a redraw-needed flag on ScintillaGTK instead of changing the paintState on Editor.

Neil

[Neil Hodgson]

Colomban Wendling:

> But if the area is not large enough for the redraw we have to do,
> instead of gently finishing the current draw and then invalidating a
> larger area, we try to perform an optimization by abandoning the pain as
> soon as we realize it won't be enough. This typically happens when
> folding changes.

...

> 3) Instead of scheduling a new draw when the pain area is not large
> enough, extend the clip manually. This is kind of dirty and requires
> non-obvious API, but fortunately Cairo has it: cairo_reset_clip().
> This can have better overall performance than we have now (no
> re-scheduling), but could make one draw longer (since the full redraw
> would happen immediately).

The basic reason for paint abandoning is that most GUI toolkits do not allow expanding the area to be drawn inside the paint handler. If it is possible to expand the area then this can be done before calling Paint.

On Cocoa viewWillDraw is called before calling drawRect and the Scintilla implementation of viewWillDraw calls the lexer to update styles and wrap lines - see ScintillaCocoa::WillDraw. This change appears to have eliminated all paint abandonment on Cocoa except for rare occasions with word wrap on.

However, WillDraw invalidates areas through the normal Window::InvalidateRectangle and Window::InvalidateAll calls. If these do not work inside a paint handler to extend the painting area, the sites where they are called could be redirected to ScintillaGTK, possibly through some more virtual methods that behave differently when called inside painting (modify clip) or outside (call gtk_widget_queue_draw*).

Neil

[Colomban Wendling]

Le 23/04/2014 01:06, Neil Hodgson a écrit :
> Colomban Wendling:
>
>> 1) ScintillaBase::Paint() could be modified never to abandon a
>> paint. This would remove what looks like an optimization, and would
>> affect all platforms. I guess this is not an option.
>
> One quick option is to set paintingAllText to true before calling
> Paint. This stops AbandonPaint from abandoning the paint although it
> means there is no way for the GTK+ code to detect it needs to redraw
> more.

Quick testing seem to suggest this works quite well. Apparently it
doesn't miss obvious areas, e.g. highlights matching brace and updates
folding margin -- all seems to look OK.

Attached a quick patch doing this if someone wants to test.

BTW, the paint propagation to wText is always needed, so this could
probably be committed no matter what we end up to fix the problem with
abandoned paint.

> A more complete approach may be to make AbandonPaint virtual and
> implement it for GTK+, by setting a redraw-needed flag on
> ScintillaGTK instead of changing the paintState on Editor.

That could work indeed, IIUC it would mean the paint inside
::DrawTextThis() is never actually abandoned, yet we can still schedule
a FullPaint(), right?


Anyway, although IIUC both above solutions mean painting some area
twice, it's indeed probably ways better than abusing the clip, and
easier than adding a pixel cache.


Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> Quick testing seem to suggest this works quite well. Apparently it
> doesn't miss obvious areas, e.g. highlights matching brace and updates
> folding margin -- all seems to look OK.
>
> Attached a quick patch doing this if someone wants to test.

Still seeing some missing redraws although not as much.

Just turning on paintingAllText may leave styling that overflows the current line not appearing. Test this in a C++ file by entering /* to start a multi-line comment. Various effects may appear such as the comment appearing on the current line but not on subsequent lines. Also try removing the * and the comment should stop on both the current line and subsequent lines. This can be sensitive to where the comment is added and removed, so try several places if you don’t see a failure.

Neil

[Neil Hodgson]

Colomban Wendling:

> That could work indeed, IIUC it would mean the paint inside
> ::DrawTextThis() is never actually abandoned, yet we can still schedule
> a FullPaint(), right?

The paint would now always complete although that may mean that the display temporarily shows some incorrect old pixels along with those correctly updated. While some mechanism should be able to schedule a FullPaint, its possible that the failures we are seeing are because the call to gtk_widget_queue_draw in FullPaint is being ignored as it is occurring during the processing of the “draw” signal. The area queued to draw must be cleared at some point - perhaps (not having examined the GTK+ implementation) it is being cleared after the return from “draw”.

Neil

[Nils Nordman]

I tried it out as well, and it seems to work most of the time (tried
Ubuntu 14.04 and 11.10). I did manage to encounter Neil's case where
text was not redrawn correctly when adding and removing multi-line
comments. I reproduced that only once however, and now it seems that I
can't for the life of me make it happen again. I'm not using Scintilla's
lexers though, so perhaps it's easier to reproduce in that case.

Cheers,

Nils

[Colomban Wendling]

Le 23/04/2014 07:01, Neil Hodgson a écrit :
>
> Still seeing some missing redraws although not as much.
>
> Just turning on paintingAllText may leave styling that overflows the
> current line not appearing. Test this in a C++ file by entering /* to
> start a multi-line comment. Various effects may appear such as the
> comment appearing on the current line but not on subsequent lines.
> Also try removing the * and the comment should stop on both the
> current line and subsequent lines. This can be sensitive to where the
> comment is added and removed, so try several places if you don't see
> a failure.

I understand what could happen, and I am actually surprised not to see
it. But I tested again, both with Geany and SciTE and I never was able
to trigger an incomplete redraw (I tested multiline comments, adding an
removing braces to change folding, etc.).

Colomban

[Colomban Wendling]

Le 23/04/2014 01:28, Neil Hodgson a écrit :
> Colomban Wendling:
>
>> But if the area is not large enough for the redraw we have to do,
>> instead of gently finishing the current draw and then invalidating
>> a larger area, we try to perform an optimization by abandoning the
>> pain as soon as we realize it won't be enough. This typically
>> happens when folding changes.
> ...
>> 3) Instead of scheduling a new draw when the pain area is not
>> large enough, extend the clip manually. This is kind of dirty and
>> requires non-obvious API, but fortunately Cairo has it:
>> cairo_reset_clip(). This can have better overall performance than
>> we have now (no re-scheduling), but could make one draw longer
>> (since the full redraw would happen immediately).
>
> The basic reason for paint abandoning is that most GUI toolkits do
> not allow expanding the area to be drawn inside the paint handler.

Yes, OK, but most toolkits also don't really allow not to paint the
whole invalidated area, so we should (do?) suffer from flickering on
most platforms, shouldn't we?

> If it is possible to expand the area then this can be done before
> calling Paint.
>
> On Cocoa viewWillDraw is called before calling drawRect and the
> Scintilla implementation of viewWillDraw calls the lexer to update
> styles and wrap lines - see ScintillaCocoa::WillDraw. This change
> appears to have eliminated all paint abandonment on Cocoa except for
> rare occasions with word wrap on.
>
> However, WillDraw invalidates areas through the normal
> Window::InvalidateRectangle and Window::InvalidateAll calls. If these
> do not work inside a paint handler to extend the painting area, the
> sites where they are called could be redirected to ScintillaGTK,
> possibly through some more virtual methods that behave differently
> when called inside painting (modify clip) or outside (call
> gtk_widget_queue_draw*).

OK, I see, it performs most of the computing that would lead to an
abandoned paint early, to avoid the real Paint() call to abandon.
Doesn't this have some performance impact by doing some computations
twice, or are these properly cached so the calls in Paint() would be
mostly no-ops? Also, if we end up doing this on all platforms, couldn't
we have something like Editor::PreparePaint() that would do the
preparation (as done in ScintillaCocoa::WillDraw()), and return whether
the area was large enough -- or better, the area that would need repainting?

Anyway, I tried to mimic what ScintillaCocoa::WillDraw() does in the
early stage of ScintillaGTK::DrawTextThis(), and it seems to work like
charm. I replaced the Redraw() in it with the clip expansion to avoid
queuing a full paint, so if I didn't miss anything it should always
succeed, and only hack the clip when absolutely needed. Patch attached.

This seems like a gentle and more clever version of the clip resetting,
and seems to work great, even when heavily reflowing lines (e.g. by
resizing the window).


However, with this the clip is actually expanded only with wrapping on,
so this would mean most of the flickering upon changes is not due to
clipping triggering an abandon, but to something else abandoning?? I
didn't yet trace this, but it seems a bit strange that simply preparing
a few computations would solve the issue, that would mean that Paint()
is actually abandoning somewhere it should not, doesn't it?


Colomban

[Colomban Wendling]

Le 23/04/2014 20:20, Colomban Wendling a écrit :
> {...]

>
> However, with this the clip is actually expanded only with wrapping on,
> so this would mean most of the flickering upon changes is not due to
> clipping triggering an abandon, but to something else abandoning?? I
> didn't yet trace this, but it seems a bit strange that simply preparing
> a few computations would solve the issue, that would mean that Paint()
> is actually abandoning somewhere it should not, doesn't it?

Hum. I dug a bit, and what used to abandon was, as I would have
expected before, the first Editor::CheckForChangeOutsidePaint() inside
Editor::NotifyModified().

However, even if I set the `paintState` to `painting` before calling
Document::EnsureStyledTo() in ScintillaGTK::PrepareDrawText(), it
doesn't abandon, nor does it on the following Paint() call.

I don't get why it wouldn't abandon in the prepare code, but would have
had in Paint() if it wasn't for the prepare code, but in the end it seem
to mean it indeed does some magic.

I guess I am missing something, though.

Colomban

[Neil Hodgson]

Colomban Wendling:

> BTW, the paint propagation to wText is always needed, so this could
> probably be committed no matter what we end up to fix the problem with
> abandoned paint.

While this may improve drawing, it may just be covering up issues by drawing twice. I’m seeing 2 identical calls into DrawTextThis when the propagation is performed. This is slowing performance greatly so that holding down the down arrow key leads to key events backing up and then continuing after the key is released.

> I understand what could happen, and I am actually surprised not to see
> it. But I tested again, both with Geany and SciTE and I never was able
> to trigger an incomplete redraw (I tested multiline comments, adding an
> removing braces to change folding, etc.).

I’m seeing this quite often although there could be more drawing problems on this setup which is in a VirtualBox VM.

> Yes, OK, but most toolkits also don't really allow not to paint the
> whole invalidated area, so we should (do?) suffer from flickering on
> most platforms, shouldn't we?

Back when Scintilla started, GUI toolkits were quite happy to leave the pixels unchanged if there were no graphics calls by the widget code. Windows still works like this.

> OK, I see, it performs most of the computing that would lead to an
> abandoned paint early, to avoid the real Paint() call to abandon.
> Doesn't this have some performance impact by doing some computations
> twice, or are these properly cached so the calls in Paint() would be
> mostly no-ops?

The preparation work done is mostly styling and folding so progress is retained.

> Also, if we end up doing this on all platforms, couldn't
> we have something like Editor::PreparePaint() that would do the
> preparation (as done in ScintillaCocoa::WillDraw()), and return whether
> the area was large enough -- or better, the area that would need repainting?

The mechanism may have to be different on each platform. I still don’t know what the effect of calling gtk_widget_queue_draw_area inside the “draw” handler for the same widget is. On Cocoa viewWillDraw is separate from drawRect and is allowed to change the invalid area which is subsequently used by drawRect. GTK+ does not have an equivalent of viewWillDraw so we are left experimenting. Perhaps on GTK+, we will need to accumulate our own invalid area, then post an event with this area to idle time processing (so not inside “draw”) where gtk_widget_queue_draw_area can then be called.

> This seems like a gentle and more clever version of the clip resetting,
> and seems to work great, even when heavily reflowing lines (e.g. by
> resizing the window).

While it may have improved things, I am still seeing drawing failures.

> I don't get why it wouldn't abandon in the prepare code, but would have
> had in Paint() if it wasn't for the prepare code, but in the end it seem
> to mean it indeed does some magic.

The paintingAllText flag is also taken into effect for abandoning and it isn’t set for prepare.

Neil

[Neil Hodgson]

Thought I should show the sorts of drawing failures I am seeing.

http://scintilla.org/GTKBadCaret.png is from stopping the caret blink (caret.period=0 in SciTE) and then holding down the up key for a while. Not all old carets are cleaned up.

http://scintilla.org/GTKBadDraw.png shows a problem while scrolling.

This is with the draw propagation active and paint abandonment turned off. It looks as though some draw events are missing or some drawing just doesn’t make it to the screen.

AbandonPaint is now virtual so an implementation can be added in ScintillaGTK that continues drawing like the attached patch.

Neil

[Neil Hodgson]

Installed Ubuntu 14.04 on a physical machine and am not seeing the drawing failures from before. Blaming VirtualBox for now.

Still not understanding everything but want to improve situation with a combination of avoiding the main source of flashing by modifying double buffering and draw propagation (check_on_gtk_3_8.patch) together with avoiding paint abandonment (AbandonPaint.patch). The combined change is attached

Neil

[Colomban Wendling]

I'll be testing this soon, I didn't find time to do it in the past few
days but should manage to do it soon.

Your AbandonPaint patch looks sensible to me, at least in theory, as it
would do exactly what's needed, e.g. not abandon a paint and not abuse
the drawing model.

However, you said before that the paint propagation resulted in
slowness, and although I didn't see it at first (probably because my
machine is fast enough), I was able to see such lags when moving cursor
down under high computer load. I didn't debug that yet, but maybe there
is a problem. I'll check your all this and your patch in the coming
hours/days.

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> However, you said before that the paint propagation resulted in
> slowness, and although I didn't see it at first (probably because my
> machine is fast enough), I was able to see such lags when moving cursor
> down under high computer load. I didn't debug that yet, but maybe there
> is a problem. I'll check your all this and your patch in the coming
> hours/days.

I observed the double paint with your 0001-Avoid-flickering-on-some-redraws-with-GTK-3-9-2.patch or 0001-Avoid-flickering-on-some-redraws-with-GTK-3-9-2_v2.patch by placing a trace in the drawing method showing the paint area and time stamp. Double paint does not appear to occur with other variants of paint propagation.

Neil

[Nils Nordman]

Hi,

I've used this patch for a couple of days, and it seems to work fine. I
previously used Colomban's last(?) patch,
"0001-Avoid-flickering-on-some-redraws-with-GTK-3-9-2_v2.patch", which
seemed to work fine for me as well. However, with Colomban's patch I
received an error report [1] for Gtk 3.4.2, where the editor would
segfault seemingly due to a Cairo error.

Cheers,

Nils

[1]: https://github.com/nilnor/howl/issues/28

[Neil Hodgson]

Nils Nordman:

> I've used this patch for a couple of days, and it seems to work fine. I previously used Colomban's last(?) patch, "0001-Avoid-flickering-on-some-redraws-with-GTK-3-9-2_v2.patch", which seemed to work fine for me as well.

I’ve seen one crash recently but that occurred while modifying the system scaling factor and the crash was somewhere in scroll bar handling.

> However, with Colomban's patch I received an error report [1] for Gtk 3.4.2, where the editor would segfault seemingly due to a Cairo error.

That looks like a re-entrance error which could indicate that reseting the clip area inside drawing is dangerous.

Neil

[Neil Hodgson]

Flicker.patch has now been committed as
http://sourceforge.net/p/scintilla/code/ci/3b46bfe4c3add450859fca006520c019a4289c65/

Neil

[Colomban Wendling]

Le 14/05/2014 15:22, Neil Hodgson a écrit :
> Flicker.patch has now been committed as
> http://sourceforge.net/p/scintilla/code/ci/3b46bfe4c3add450859fca006520c019a4289c65/

Sorry for not having tested this earlier. I just did and it indeed
seems to work great (apart when wrapping lines) thanks a lot!

Attached a trivial patch that fixes the line wrapping flickering issue.

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> Attached a trivial patch that fixes the line wrapping flickering issue.

OK, committed as
http://sourceforge.net/p/scintilla/code/ci/1f5fc8f0ebb50cae33af33a76331ad961839b6f2/

Neil

[Colomban Wendling]

Le 14/05/2014 15:22, Neil Hodgson a écrit :

> Flicker.patch has now been committed as
> http://sourceforge.net/p/scintilla/code/ci/3b46bfe4c3add450859fca006520c019a4289c65/

Hum, apparently enabling double buffering on < 3.9.2 breaks (some)
refreshing. I have no idea why yet, but a Mint user reported this, and
I can reproduce with both 3.6 and 3.8. Forwarding draw on these version
inside ScintillaGTK::DrawThis() doesn't help.

To see it, e.g. open a file in SciTE and close it. Untitled file is now
open, but it displays a lot of the previous file in it.

I suggest we only enable double buffering on >= 3.9.2 rather than on all
3.x, at least until we understand and fix this issue.

I must say it puzzles me why this could happen with the new
non-abandoning stuff, but well, there is a problem.

Also, we probably actually want *runtime* GTK version checking for these
flickering fixes, because the code paths should be different depending
on the runtime use, not the version against which it was compiled. If
we don't do that, a Scintilla compiled against e.g. GTK 3.8 will start
failing if running against 3.10.

Attached patch does both runtime checks and re-enable single-buffering
on GTK3 < 3.9.2.

Regards,
Colomban

[Lex Trotman]

Confirmed patch seems to work on GTK 3.6.0 and 3.8.2 which were
otherwise unusable.

Cheers
Lex

> --
> You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-inter...@googlegroups.com.
> To post to this group, send email to scintilla...@googlegroups.com.
> Visit this group at http://groups.google.com/group/scintilla-interest.
> For more options, visit https://groups.google.com/d/optout.

[Andreas Tscharner]

On 19.05.2014 03:24, Colomban Wendling wrote:

[snip]

>
> Attached patch does both runtime checks and re-enable single-buffering
> on GTK3 < 3.9.2.

Did you intentionally rename the macro?
#if GTK_CHECK_VERSION(3,9,2) -> #if GTK_CHECK_VERSION(3,0,0)

Best regards
Andreas
--
Andreas Tscharner <sterne...@gmail.com>
----------------------------------------------------------------------
"Intruder on level one. All Aliens please proceed to level one."
-- Call in "Alien: Resurrection"

[Neil Hodgson]

Colomban Wendling:

> ]Attached patch does both runtime checks and re-enable single-buffering
> on GTK3 < 3.9.2.

Seems I should delay 3.4.2 (which was going to happen in 15 hours) until this has been examined for a couple of days.

Neil

[Lex Trotman]

On 19 May 2014 18:06, Andreas Tscharner <sterne...@gmail.com> wrote:
> On 19.05.2014 03:24, Colomban Wendling wrote:
>
> [snip]
>
>>
>> Attached patch does both runtime checks and re-enable single-buffering
>> on GTK3 < 3.9.2.
>
>
> Did you intentionally rename the macro?
> #if GTK_CHECK_VERSION(3,9,2) -> #if GTK_CHECK_VERSION(3,0,0)

I think the idea is that whichever version of gtk3 its compiled on, it
does a runtime test for the version being used on the runtime system
(which need not be the same as the version on the build system).

Cheers
Lex

>
> Best regards
> Andreas
> --
> Andreas Tscharner <sterne...@gmail.com>
> ----------------------------------------------------------------------
> "Intruder on level one. All Aliens please proceed to level one."
> -- Call in "Alien: Resurrection"
>
>

[Colomban Wendling]

Le 19/05/2014 11:25, Lex Trotman a écrit :
> On 19 May 2014 18:06, Andreas Tscharner <sterne...@gmail.com> wrote:
>> On 19.05.2014 03:24, Colomban Wendling wrote:
>>
>> [snip]
>>
>>>
>>> Attached patch does both runtime checks and re-enable single-buffering
>>> on GTK3 < 3.9.2.
>>
>>
>> Did you intentionally rename the macro?
>> #if GTK_CHECK_VERSION(3,9,2) -> #if GTK_CHECK_VERSION(3,0,0)
>
> I think the idea is that whichever version of gtk3 its compiled on, it
> does a runtime test for the version being used on the runtime system
> (which need not be the same as the version on the build system).

Exactly.

[Colomban Wendling]

Okay. I will be testing on GTK 3.12 myself, and if Lex sticks with it
we should at least also have him testing with < 3.9.2.

Colomban

[Neil Hodgson]

Colomban Wendling:

I think the idea is that whichever version of gtk3 its compiled on, it
does a runtime test for the version being used on the runtime system
(which need not be the same as the version on the build system).

Exactly.

   The logic here is a little difficult to understand particularly since gtk_check_version is returning NULL for success and NULL is normally falsy.

   Committed the change as

http://sourceforge.net/p/scintilla/code/ci/3ddc94b6dec54cbc01c5f32f67b65cd4dab2bd0e/

   VirtualBox has a new release 4.3.12 which includes "• 3D support: several fixes, including better support for Ubuntu 14.04” and has improved display updating. With Ubuntu 14.04 (GTK+ 3.10.8), I am not seeing Scintilla work on my laptop native installation and then fail on a Virtual Box instance as often. Other elements of SciTE such as the standard File Open dialog and other applications such as FireFox still show incomplete drawing on Virtual Box.

   Scrolling that continues after letting go of the down arrow key has come back but isn’t as noticeable as before. While this is unpleasant, its less of a problem than the other effects so should not delay 3.4.2. Putting some traces in shows that the window is scrolled by blit, so only the newly scrolled-in lines should need painting, but then the whole window is repainted. AbandonPaint isn’t occurring during scrolling.

   Neil

[Nils Nordman]

On 05/20/2014 02:16 AM, Neil Hodgson wrote:
> Colomban Wendling:
>
>>> I think the idea is that whichever version of gtk3 its compiled on, it
>>> does a runtime test for the version being used on the runtime system
>>> (which need not be the same as the version on the build system).
>>
>> Exactly.
>
> The logic here is a little difficult to understand particularly
> since gtk_check_version is returning NULL for success and NULL is
> normally falsy.
>
> Committed the change as
> http://sourceforge.net/p/scintilla/code/ci/3ddc94b6dec54cbc01c5f32f67b65cd4dab2bd0e/

Trouble in paradise I'm afraid. I upgraded to Scintilla 3.4.2 some time
ago, and then again received a bug report for older Gtk versions; a seg
fault due to "_cairo_surface_begin_modification: Assertion `!
surface->finished' failed.". This worked fine with Neil's fix, but broke
with the run time check patch. Particularly, the disabling of double
buffering exposes this (does not happen with double buffering on).

I've now reproduced this on a Ubuntu 12.04 virtual box, Gtk version
3.4.2. I see the seg fault only when line wrapping is enabled and
wrapping is actually taking place, in my case using SC_WRAP_WORD.

Relevant backtrace:

#8 0xb70c846d in cairo_fill_preserve () from
/usr/lib/i386-linux-gnu/libcairo.so.2
#9 0xb70c8490 in cairo_fill () from /usr/lib/i386-linux-gnu/libcairo.so.2
#10 0x080de006 in Editor::PaintSelMargin(Surface*, PRectangle&) ()
#11 0x080e6109 in Editor::Paint(Surface*, PRectangle) ()
#12 0x080c3a83 in ScintillaGTK::DrawTextThis(_cairo*) ()

Regards,

Nils

[Lex Trotman]

Wrapping works ok on GTK 3.6.0 and 3.8.2, maybe there is a change
between 3.4 and 3.6.

Cheers
Lex

[Nils Nordman]

On 06/03/2014 01:17 PM, Lex Trotman wrote:
> On 3 June 2014 20:08, Nils Nordman <ni...@nordman.org> wrote:
>> I've now reproduced this on a Ubuntu 12.04 virtual box, Gtk version 3.4.2. I
>> see the seg fault only when line wrapping is enabled and wrapping is
>> actually taking place, in my case using SC_WRAP_WORD.

>>[..]

>
> Wrapping works ok on GTK 3.6.0 and 3.8.2, maybe there is a change
> between 3.4 and 3.6.

Yes, something must must be different in that case. I also verified the
issue with a Gtk-3 build of Scite yesterday (open a file, adjust window
size to clip lines, toggle wrap -> seg fault).

Regards,

Nils

[Colomban Wendling]

Le 03/06/2014 12:08, Nils Nordman a écrit :
> [...]

>
> Trouble in paradise I'm afraid. I upgraded to Scintilla 3.4.2 some time
> ago, and then again received a bug report for older Gtk versions; a seg
> fault due to "_cairo_surface_begin_modification: Assertion `!
> surface->finished' failed.". This worked fine with Neil's fix, but broke
> with the run time check patch. Particularly, the disabling of double
> buffering exposes this (does not happen with double buffering on).

That's weird, especially since before the recent changes double
buffering was *always* disabled.

> I've now reproduced this on a Ubuntu 12.04 virtual box, Gtk version
> 3.4.2. I see the seg fault only when line wrapping is enabled and
> wrapping is actually taking place, in my case using SC_WRAP_WORD.

I'm afraid I can't reproduce, I just tested a clean SciTE GTK3 clone on
a Debian Stable machine (GTK 3.4.2) and it seems to work just great. It
reflows well, no drawing artifacts, etc.

> Relevant backtrace:
>
> #8 0xb70c846d in cairo_fill_preserve () from
> /usr/lib/i386-linux-gnu/libcairo.so.2
> #9 0xb70c8490 in cairo_fill () from /usr/lib/i386-linux-gnu/libcairo.so.2
> #10 0x080de006 in Editor::PaintSelMargin(Surface*, PRectangle&) ()
> #11 0x080e6109 in Editor::Paint(Surface*, PRectangle) ()
> #12 0x080c3a83 in ScintillaGTK::DrawTextThis(_cairo*) ()

Could you give a backtrace from Scintilla/SciTE tip with debugging
information (-g and -O0) so we can see exactly where it breaks?
(PaintSelMargin() is huge).

Regards,
Colomban

[Nils Nordman]

On 06/04/2014 07:18 PM, Colomban Wendling wrote:
> Le 03/06/2014 12:08, Nils Nordman a écrit :
>> [...]
>>
>> Trouble in paradise I'm afraid. I upgraded to Scintilla 3.4.2 some time
>> ago, and then again received a bug report for older Gtk versions; a seg
>> fault due to "_cairo_surface_begin_modification: Assertion `!
>> surface->finished' failed.". This worked fine with Neil's fix, but broke
>> with the run time check patch. Particularly, the disabling of double
>> buffering exposes this (does not happen with double buffering on).
>
> That's weird, especially since before the recent changes double
> buffering was *always* disabled.

I might have been a bit unclear, but prior to upgrading to 3.4.2 I used
Neil's last before addition runtime checks
(https://groups.google.com/d/msg/scintilla-interest/NXDnrvYE2U8/IVcy6clHMPoJ).
That patch turned off double buffering only for Gtk < 3. The later
patch, including the runtime checks, modified this to turn off double
buffering for Gtk < 3.9.2.

> I'm afraid I can't reproduce, I just tested a clean SciTE GTK3 clone on
> a Debian Stable machine (GTK 3.4.2) and it seems to work just great. It
> reflows well, no drawing artifacts, etc.

That's weird, with wrapping on and in effect?

> Could you give a backtrace from Scintilla/SciTE tip with debugging
> information (-g and -O0) so we can see exactly where it breaks?
> (PaintSelMargin() is huge).

Built with DEBUG defined:

#9 0xb7073490 in cairo_fill () from /usr/lib/i386-linux-gnu/libcairo.so.2
#10 0x0817c79a in SurfaceImpl::Copy (this=0x9aaeb48, rc=..., from=...,
surfaceSource=...)
at PlatGTK.cxx:839
#11 0x08154852 in Editor::PaintSelMargin (this=0x9ba5cc8,
surfWindow=0x9aaeb48, rc=...)
at ../src/Editor.cxx:2124
#12 0x0815c3a2 in Editor::Paint (this=0x9ba5cc8,
surfaceWindow=0x9aaeb48, rcArea=...)
at ../src/Editor.cxx:3556
#13 0x0812b56d in ScintillaGTK::DrawTextThis (this=0x9ba5cc8,
cr=0xb7123500) at ScintillaGTK.cxx:2418
#14 0x0812b64c in ScintillaGTK::DrawText (cr=0xb7123500,
sciThis=0x9ba5cc8) at ScintillaGTK.cxx:2442

(This is with a clean Scintilla 3.4.2 release, and latest Scite source
tarball).

Cheers,

Nils

[Neil Hodgson]

This crash reproduces for me on Ubuntu 12.04, GTK+ 3.4.2. The crash does not occur with revision 5102 and appears with 5103.

Colomban Wendling:

> I'm afraid I can't reproduce, I just tested a clean SciTE GTK3 clone on
> a Debian Stable machine (GTK 3.4.2) and it seems to work just great. It
> reflows well, no drawing artifacts, etc.

One possible difference with Ubuntu that has caused problems in the past is the overlay scroll bar used by Ubuntu.

Neil

[Nils Nordman]

On 06/11/2014 03:26 AM, Neil Hodgson wrote:
>
> Colomban Wendling:
>
>> I'm afraid I can't reproduce, I just tested a clean SciTE GTK3 clone on
>> a Debian Stable machine (GTK 3.4.2) and it seems to work just great. It
>> reflows well, no drawing artifacts, etc.
>
> One possible difference with Ubuntu that has caused problems in the past is the overlay scroll bar used by Ubuntu.

Right you are. Turning off overlay scrollbars (gsettings set
org.gnome.desktop.interface ubuntu-overlay-scrollbars false) makes the
problem go away.

Cheers,

Nils

[Nils Nordman]

To follow up the problem goes away with overlay scrollbars disabled
(i.e. nog seg fault), but the following then shows on the console instead:

*** BUG ***
In pixman_region32_init_rect: Invalid rectangle passed
Set a breakpoint on '_pixman_log_error' to debug

Nils

[Neil Hodgson]

Nils Nordman:

> To follow up the problem goes away with overlay scrollbars disabled (i.e. nog seg fault), but the following then shows on the console instead:
>
> *** BUG ***
> In pixman_region32_init_rect: Invalid rectangle passed
> Set a breakpoint on '_pixman_log_error' to debug

I’m not seeing that and a breakpoint on _pixman_log_error never hits.

The previous Ubuntu scroll bar bug was a little indirect: changes to scroll bars during painting caused the drawing surface to be invalid. Here is the change set:
https://sourceforge.net/p/scintilla/code/ci/c39df2a9f97adbc79d63419f9e749bfbd59dfd90/

Neil

[Nils Nordman]

On 06/12/2014 01:12 AM, Neil Hodgson wrote:
>> *** BUG ***
>> In pixman_region32_init_rect: Invalid rectangle passed
>> Set a breakpoint on '_pixman_log_error' to debug
>
> I'm not seeing that and a breakpoint on _pixman_log_error never hits.

Huh, might be something specific to my setup then (virtual box). At any
rate I chased this a little bit to see if it was related to the seg
faults, which it did not seem to be (eliminated the BUG printout but
still got the original seg fault; had a confusing debugging session
though, details at the end for reference).

> The previous Ubuntu scroll bar bug was a little indirect: changes to scroll bars during painting caused the drawing surface to be invalid. Here is the change set:
> https://sourceforge.net/p/scintilla/code/ci/c39df2a9f97adbc79d63419f9e749bfbd59dfd90/

Thanks, I'll see if I can get some time to dive into Scintilla and this
issue properly sometime soonish.

Nils

- Weird pixman debug log debugging session:

Breakpoint 1, _pixman_log_error (function=0xb70f96a8
"pixman_region32_init_rect",
message=0xb70f9547 "Invalid rectangle passed") at
../../pixman/pixman-utils.c:297
297 ../../pixman/pixman-utils.c: No such file or directory.
(gdb) bt
#0 _pixman_log_error (function=0xb70f96a8 "pixman_region32_init_rect",
message=0xb70f9547 "Invalid rectangle passed") at
../../pixman/pixman-utils.c:297
#1 0xb70c2db1 in pixman_region32_init_rect (region=0x8571f60, x=0, y=0,
width=496, height=4294967284)
at ../../pixman/pixman-region.c:389
#2 0xb7992893 in cairo_region_create_rectangle () from
/usr/lib/i386-linux-gnu/libcairo.so.2
#3 0xb7dde184 in gtk_widget_queue_draw_area () from
/usr/lib/i386-linux-gnu/libgtk-3.so.0
#4 0x0817e124 in Window::InvalidateRectangle (this=0x84fcb9c, rc=...)
at PlatGTK.cxx:1297
#5 0x0814dc98 in Editor::Redraw (this=0x84fcb98) at ../src/Editor.cxx:662
#6 0x0814c7a0 in Editor::InvalidateStyleRedraw (this=0x84fcb98) at
../src/Editor.cxx:344
#7 0x081724e6 in Editor::WndProc (this=0x84fcb98, iMessage=2069,
wParam=0, lParam=0)
at ../src/Editor.cxx:8859
#8 0x0814b4a7 in ScintillaBase::WndProc (this=0x84fcb98, iMessage=2069,
wParam=0, lParam=0)
at ../src/ScintillaBase.cxx:1033
#9 0x0812757c in ScintillaGTK::WndProc (this=0x84fcb98, iMessage=2069,
wParam=0, lParam=0)
at ScintillaGTK.cxx:1011
#10 0x0812c2ad in ScintillaGTK::DirectFunction (ptr=139447192,
iMessage=2069, wParam=0, lParam=0)
at ScintillaGTK.cxx:2825
#11 0x080f0d4d in GUI::ScintillaWindow::Call(unsigned int, unsigned
long, long) ()
#12 0x0811fade in SciTEBase::ReadProperties() ()
#13 0x080e3dec in SciTEGTK::ReadProperties() ()
#14 0x0810f3f9 in SciTEBase::Open(FilePath, SciTEBase::OpenFlags) ()
#15 0x08104892 in SciTEBase::ProcessCommandLine(std::string&, int) ()
#16 0x080f0a89 in SciTEGTK::Run(int, char**) ()
#17 0x080e1d83 in main ()
(gdb) frame 5
#5 0x0814dc98 in Editor::Redraw (this=0x84fcb98) at ../src/Editor.cxx:662
662 wMain.InvalidateRectangle(rcClient);
(gdb) info locals
rcClient = {left = 0, top = 0, right = 496, bottom = -12}

Consistent with the huge weight in frame #1, due to conversion from
signed to unsigned. However, adding debug printouts just made for
confusion, with the output being:

GetClientRectangle <-- Editor::Redraw, before GetClientRectangle()
a x: 0, y: 0, width: 509, height: 1 <-- PlatGtk: Window::GetPosition(),
allocation after gtk_widget_get_allocation
left: 0, top: 0, right: 509, bottom: 1 <-- PlatGtk:
Window::GetPosition(), PRectangle before return
GetClientRectangle done <-- Editor::Redraw, after GetClientRectangle()
left: 0, top: 0, right: 496, bottom: -12 <-- Editor::Redraw, resulting
rcClient

I'm guessing the call chain is something different than what I expected
(GetClientRectancle -> GetPosition), or there's some other devilry
involved. Just skipping the InvalidateRectangle for negative heights got
rid of the pixman warning, but had no effect on the seg fault. End of
session.

[Neil Hodgson]

Nils Nordman:

> #1 0xb70c2db1 in pixman_region32_init_rect (region=0x8571f60, x=0, y=0, width=496, height=4294967284)
> at ../../pixman/pixman-region.c:389
> #2 0xb7992893 in cairo_region_create_rectangle () from /usr/lib/i386-linux-gnu/libcairo.so.2
> #3 0xb7dde184 in gtk_widget_queue_draw_area () from /usr/lib/i386-linux-gnu/libgtk-3.so.0
> #4 0x0817e124 in Window::InvalidateRectangle (this=0x84fcb9c, rc=...) at PlatGTK.cxx:1297
> #5 0x0814dc98 in Editor::Redraw (this=0x84fcb98) at ../src/Editor.cxx:662

Shouldn’t be getting a negative height - this height is the window height - scrollbar height and SciTE should never allow the window to get that short. Tried changing an app to allow very small windows but couldn’t hit the warning. Could be the window manager only allowing a very small window in some circumstances. There could be a test for empty rectangles in Window::InvalidateRectangle but it doesn’t appear needed.

> GetClientRectangle <-- Editor::Redraw, before GetClientRectangle()
> a x: 0, y: 0, width: 509, height: 1 <-- PlatGtk: Window::GetPosition(), allocation after gtk_widget_get_allocation
> left: 0, top: 0, right: 509, bottom: 1 <-- PlatGtk: Window::GetPosition(), PRectangle before return
> GetClientRectangle done <-- Editor::Redraw, after GetClientRectangle()
> left: 0, top: 0, right: 496, bottom: -12 <-- Editor::Redraw, resulting rcClient

That is saying that the height of the horizontal scroll bar is 13. The scroll bars look after their own redrawing so are excluded from the invalidation area.

Neil

[Nils Nordman]

On 06/13/2014 01:08 AM, Neil Hodgson wrote:
> Nils Nordman:
>
>> #1 0xb70c2db1 in pixman_region32_init_rect (region=0x8571f60, x=0, y=0, width=496, height=4294967284)
>> at ../../pixman/pixman-region.c:389
>> #2 0xb7992893 in cairo_region_create_rectangle () from /usr/lib/i386-linux-gnu/libcairo.so.2
>> #3 0xb7dde184 in gtk_widget_queue_draw_area () from /usr/lib/i386-linux-gnu/libgtk-3.so.0
>> #4 0x0817e124 in Window::InvalidateRectangle (this=0x84fcb9c, rc=...) at PlatGTK.cxx:1297
>> #5 0x0814dc98 in Editor::Redraw (this=0x84fcb98) at ../src/Editor.cxx:662
>
> Shouldn't be getting a negative height - this height is the window height - scrollbar height and SciTE should never allow the window to get that short. Tried changing an app to allow very small windows but couldn't hit the warning. Could be the window manager only allowing a very small window in some circumstances. There could be a test for empty rectangles in Window::InvalidateRectangle but it doesn't appear needed.

This only occurs when initializing and not after the initial display is
done, so I'm guessing that if you don't see it then you're not going to
see it at all.

Meanwhile, I had a look at the original seg fault, and am attaching a
minimal patch that gets rid of it by updating paintState to abandoned
when scrollbars are modified.

On a related note, should not paintState still be set to abandoned in
ScintillaGTK::AbandonPaint()?

Regards,

Nils

[Neil Hodgson]

Nils Nordman:

> Meanwhile, I had a look at the original seg fault, and am attaching a minimal patch that gets rid of it by updating paintState to abandoned when scrollbars are modified.

That is exactly reverting Columban Wendling’s patch from May 15 which was to avoid flicker caused by not continuing to draw.
https://sourceforge.net/p/scintilla/code/ci/1f5fc8f0ebb50cae33af33a76331ad961839b6f2/

> On a related note, should not paintState still be set to abandoned in ScintillaGTK::AbandonPaint()?

The reason for not setting paintState to paintAbandoned is that stops drawing which leads to a flash.

If the surface has been destroyed by Ubuntu-specific scroll bar code then possibly we need to detect Ubuntu or the Ubuntu scroll bar code. There might be a call to examine surface->finished and then set paintState to paintAbandoned since we really can’t do any more painting. Have Ubuntu published documentation on their scroll bars? The source code has to be published somewhere.

Neil

[Matthew Brush]

$ mkdir ubuntu-hacks && cd ubuntu-hacks
$ apt-get source libgtk+-3-0 overlay-scrollbar-gtk3

Then dig around in there you should see their patch set. There's a patch
in former package that exposes the scrollbars' Gdk windows so latter
GTK+ "plugin" module can "work".

Cheers,
Matthew Brush

[Colomban Wendling]

Le 11/06/2014 03:26, Neil Hodgson a écrit :
> This crash reproduces for me on Ubuntu 12.04, GTK+ 3.4.2. The crash
> does not occur with revision 5102 and appears with 5103.

Just FTR, it is the combination of non-abandonement on scrollbar changes
(http://sourceforge.net/p/scintilla/code/ci/1f5fc8f0ebb50cae33af33a76331ad961839b6f2/)
and single buffering
(http://sourceforge.net/p/scintilla/code/ci/3ddc94b6dec54cbc01c5f32f67b65cd4dab2bd0e/)

> Colomban Wendling:
>
>> I'm afraid I can't reproduce, I just tested a clean SciTE GTK3
>> clone on a Debian Stable machine (GTK 3.4.2) and it seems to work
>> just great. It reflows well, no drawing artifacts, etc.
>
> One possible difference with Ubuntu that has caused problems in the
> past is the overlay scroll bar used by Ubuntu.

Indeed testing on Ubuntu 12.04 with overlay scrollbars shows the crash.

I tried to understand what was going on, but I have absolutely no clue.
About why it happens, I can only guess that for some reason Ubuntu's
the overlay scrollbars finish the current surface (leading to the
!surface->finished assertion failure), and that with double buffering
it's no real problem because it invalidates the currently displayed
buffer rather than the one on which we draw.

I however don't currently have any idea what we could do to avoid this
(but a super ugly hack like checking if one of the overlay scrollbars'
symbol is defined and take action -- don't beat me please, I know it's
completely absurd).

I tried to take a look at the source code for overlay scrollbars, but
I'm afraid it's far too complex for me to find what could lead to this
weird issue.

So… for now I'm afraid I have no idea to suggest. I'll try to keep
thinking on this, but I don't know.


Regards,
Colomban

[Nils Nordman]

On 06/13/2014 01:38 PM, Neil Hodgson wrote:
> Nils Nordman:
>
>> Meanwhile, I had a look at the original seg fault, and am attaching a minimal patch that gets rid of it by updating paintState to abandoned when scrollbars are modified.
>
> That is exactly reverting Columban Wendling's patch from May 15 which was to avoid flicker caused by not continuing to draw.
> https://sourceforge.net/p/scintilla/code/ci/1f5fc8f0ebb50cae33af33a76331ad961839b6f2/

Ah, I see, I'm catching up with the previous discussion now. I see also
that original addition of the code was to work around precisely this
issue, or so it seems:

https://groups.google.com/forum/#!topic/scintilla-interest/gOULMoJcDXk

Thanks,

Nils

[Colomban Wendling]

Le 12/06/2014 01:12, Neil Hodgson a écrit :
> Nils Nordman:
>
>> To follow up the problem goes away with overlay scrollbars disabled
>> (i.e. nog seg fault), but the following then shows on the console
>> instead:
>>
>> *** BUG *** In pixman_region32_init_rect: Invalid rectangle passed
>> Set a breakpoint on '_pixman_log_error' to debug
>
> I'm not seeing that and a breakpoint on _pixman_log_error never
> hits.

Me neither, testing with Ubuntu 12.04 running inside virtualbox. Also,
Valgrind doesn't report memory access errors at this point.

Regards,
Colomban

[Matthew Brush]

One way would be to take the scrollbars out of Scintilla's GtkContainer
and let the parent container (ex. GtkScrolledWindow) manage them in the
usual way. Since the ScintillaGTK class mostly deals with adjustments, I
doubt this would be very difficult, although any existing UIs that use
the widget without packing in a scroll container would stop having
scrollbars altogether next time they update :)

Cheers,
Matthew Brush

[Neil Hodgson]

The finished field on cairo_surface_t is hidden from client code so can not be directly tested. When finished is set, the surface status remains CAIRO_STATUS_SUCCESS until a drawing operation is attempted. Many drawing operations, such as fill, call _cairo_surface_begin_modification which simply asserts out when finished is set.

Some operations, however, check for finished instead of asserting, and then set the surface status, sometimes to CAIRO_STATUS_NO_MEMORY and sometimes to CAIRO_STATUS_SURFACE_FINISHED. Once the surface status is not CAIRO_STATUS_SUCCESS, warnings are shown instead of assertions allowing the application to continue, albeit with some cheerful "Gtk-WARNING **: drawing failure" messages on stderr.

Looking through the Cairo source code reveals some functions that check for finished that can be used. cairo_surface_has_show_text_glyphs is a getter so can be used without other side effects but is only available on Cairo 1.8+ (unsure what the minimum version of Cairo that needs support is). cairo_surface_set_device_offset is available on all versions of Cairo and while it changes the state, if the values are first retrieved through cairo_surface_get_device_offset there should be no effect.

With Ubuntu overlay scrollbars, the problem occurs when filling or copying a rectangle. Since these calls are made many times adding in extra checks may decrease performance. A single call at the start of the next drawing code (near the start of PaintSelMargin) would be better. While a new call to check for errors could be added to Surface, this would require all downstream platforms to be updated. Including the check inside the Initialised method and calling surfWindow->Initialised() near the top of PaintSelMargin appears to work. Here is the changed method:

bool SurfaceImpl::Initialised() {
if (inited && context) {
if (cairo_status(context) == CAIRO_STATUS_SUCCESS) {
cairo_surface_t *psurfContext = cairo_get_target(context);
if (psurfContext) {
//cairo_surface_has_show_text_glyphs(sst);
double xo;
double yo;
cairo_surface_get_device_offset(psurfContext, &xo, &yo);
cairo_surface_set_device_offset(psurfContext, xo, yo);
//fprintf(stderr, "Initialised cairo status = %d surf = %d %g %g\n”,
// cairo_status(context), cairo_surface_status(psurfContext), xo, yo);
}
}
}
return inited;
}

Neil

[Neil Hodgson]

A similar change has been committed as
https://sourceforge.net/p/scintilla/code/ci/7ef4eca592d64380ad33e03a022440ae2d4df66f/

The change uses cairo_surface_has_show_text_glyphs as this has less potential for side effects than setting the device offset. However it is only available from Cairo 1.8 (September 2008) so, if anyone needs to support older versions of Cairo, it could be wrapped in a version check.

Neil