Bug 6955

The patch is definitely not what we want to do as it discards the use of X
server-side glyph caches, so it should have bad performance characteristics.

Is this bug perhaps the same as bug #6617 ?

(In reply to comment #1)
> The patch is definitely not what we want to do as it discards the use of X
> server-side glyph caches, so it should have bad performance characteristics.

Yeah, I worry about the performance too. If we can check faster whether the
character is added correctly, it makes happy.

> Is this bug perhaps the same as bug #6617 ?

Probably, no, this bug is occurred per character, not per transaction. 

Created attachment 5663 [details] [review]
Patch rv2.0

This patch fixes this bug. But I don't know why current code have this line.

That's a much cleaner version of the same patch, so thanks. But it's still got
the same performance problems.

If I'm reading the code correctly, the purpose of the code your patch removes is
to ensure that the glyph data is only sent to the X server once, (and then it's
cached in the X server after that).

The X server glyph cache is client-managed. If glyphs are disappearing, perhaps
the bug is that when cairo removes glyphs from the server cache it is neglecting
to clear the scaled_glyph->surface_private flag?

I'll ask Keith Packard to take a look at this bug and let us know what might be
going on.

Created attachment 5664 [details] [review]
Patch rv3.0

Is this correct?

(In reply to comment #5)
> Created an attachment (id=5664) [edit]
> Patch rv3.0
> 
> Is this correct?

I don't think so. Here's something to try though, (I'm far too lazy to make a
bugzilla attachment out of this). If this fixes the bug for you then I'm pretty
sure it's correct.

Meanwhile, all of this code could use a bit of cleanup, but less at least get
the functionality correct first.

-Carl

diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index bcf2380..aad9f97 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -2173,6 +2173,7 @@ _cairo_xlib_surface_scaled_glyph_fini (c
        XRenderFreeGlyphs (font_private->dpy,
                           font_private->glyphset,
                           &glyph_index, 1);
+       scaled_glyph->surface_private = NULL;
     }
 }

(In reply to comment #6)
> I don't think so. Here's something to try though, (I'm far too lazy to make a
> bugzilla attachment out of this). If this fixes the bug for you then I'm pretty
> sure it's correct.
> 
> Meanwhile, all of this code could use a bit of cleanup, but less at least get
> the functionality correct first.
> 
> -Carl
> 
> diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
> index bcf2380..aad9f97 100644
> --- a/src/cairo-xlib-surface.c
> +++ b/src/cairo-xlib-surface.c
> @@ -2173,6 +2173,7 @@ _cairo_xlib_surface_scaled_glyph_fini (c
>         XRenderFreeGlyphs (font_private->dpy,
>                            font_private->glyphset,
>                            &glyph_index, 1);
> +       scaled_glyph->surface_private = NULL;
>      }
>  }
> 

No, this cannot fix this bug :-(

Created attachment 5676 [details] [review]
Patch rv4.0

O.K. I see why the characters disappears randomly. If there are more 256
characters, the cache table removes randomly at inserting new entry. So, we
must lock the cache table while crating glyphs and drawing the text.

Carl:

Would you check the latest patch?

Created attachment 5717 [details] [review]
Perform freeze/thaw in _cairo_surface_show_glyphs

Thanks for the reminder, and thanks for chasing down the problem.

Here's another patch that simply moves the existing freeze/thaw all the way up
to _cairo_surface_show_glyphs so that any backends implementing show_glyphs
will benefit from the frozen cache.

I've put this patch (as well as the earlier fix I proposed, which I think is
still a valid bug fix, even if we may not have hit that bug yet) on the 6955
branch in my personal cairo tree which you can look at here:

http://gitweb.cairographics.org/?p=users-cworth-cairo;a=shortlog;h=6955

Or you can fetch it into a git clone of cairo with:

git fetch git://git.cairographics.org/~cworth/cairo 6955:6955
git checkout 6955

Please test and let me know how it works.

Thanks,

-Carl

Thanks, your patch fixes this bug too.

I had Keith Packard take a look at the latest patch and he objected to doing the
freeze/thaw at the generic level since many backends will not require it. So he
suggests pushing this fix down into the xlib backend instead.

-Carl

Keith Packard recommended a good way to be able to write a test case for this
bug. I did that with the new test/glyph-cache-pressure. As expected, xlib failed
the test while all the other backends passed.

And, also as expected, just adding a freeze/thaw pair in
_cairo_xlib_surface_sho_glyphs made it pass again. See the commit here:

http://gitweb.cairographics.org/?p=cairo;a=commit;h=7e457cb4c1e69670f27e3e8e134a9e32a8f75788

So I'm glad to say that this problem is officially fixed in cairo 1.1.11 so the
fix will be in place for cairo 1.2.0.

Thanks for all the help on this one,

-Carl