After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 332266 - gdk_draw_layout fails for coordinates >= 2^21
gdk_draw_layout fails for coordinates >= 2^21
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: X11
2.8.x
Other All
: Normal normal
: ---
Assigned To: Behdad Esfahbod
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2006-02-23 02:36 UTC by Jody Goldberg
Modified: 2011-02-04 16:10 UTC
See Also:
GNOME target: ---
GNOME version: 2.13/2.14


Attachments
implement behad's suggested kludge. (801 bytes, patch)
2006-02-23 04:05 UTC, Jody Goldberg
none Details | Review
Use a matrix when scaling would overflow (4.01 KB, patch)
2006-03-22 20:18 UTC, Jody Goldberg
none Details | Review
Move device_transform of path to before floating->fixed conversion (8.91 KB, patch)
2006-06-29 17:55 UTC, Carl Worth
none Details | Review
proposed pango api (15.62 KB, patch)
2006-07-07 05:05 UTC, Behdad Esfahbod
needs-work Details | Review
patch (2.79 KB, patch)
2007-01-05 03:28 UTC, Behdad Esfahbod
committed Details | Review
Patch to use pango_matrix_transform_rectangle (4.37 KB, patch)
2007-01-05 05:12 UTC, Behdad Esfahbod
none Details | Review

Description Jody Goldberg 2006-02-23 02:36:38 UTC
Please describe the problem:
gdk_draw_layout takes the gint pixel coordinates and multiplies by PANGO_SCALE
(2^10) before passing to pango_renderer_draw_layout.

Steps to reproduce:
Attempting to use gnumeric with > 134,000 rows generates y coordinates larger
than 2^21 (default is 17 pixels/row at 100% zoom).

Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Jody Goldberg 2006-02-23 02:39:05 UTC
We'll likely need some pango api extension to handle this.
Comment 2 Behdad Esfahbod 2006-02-23 02:48:57 UTC
Guess this can be easily solved in gdkpango.c already.  There is code like this:

  /* When we have a matrix, we do positioning by adjusting the matrix, and
   * clamp just pass x=0, y=0 to the lower levels. We don't want to introduce
   * a matrix when the caller didn't provide one, however, since that adds
   * lots of floating point arithmetic for each glyph.
   */
  matrix = pango_context_get_matrix (pango_layout_get_context (layout));
  if (matrix)
    {
      PangoMatrix tmp_matrix;
      GdkRectangle rect;

      get_rotated_layout_bounds (layout, &rect);

      tmp_matrix = *matrix;
      tmp_matrix.x0 += x - rect.x;
      tmp_matrix.y0 += y - rect.y;
      pango_renderer_set_matrix (renderer, &tmp_matrix);

      x = 0;
      y = 0;
    }
  else
    pango_renderer_set_matrix (renderer, NULL);


It just needs to use a matrix if (x,y) are out of range too...  The range will be something like PANGO_PIXELS (G_MAXINT)...
Comment 3 Behdad Esfahbod 2006-02-23 02:51:43 UTC
While around there, seems to me like the same code in gdk_draw_layout_line_with_colors can use this patch:

  if (matrix)
    {
      PangoMatrix tmp_matrix;

      tmp_matrix = *matrix;
-     tmp_matrix.x0 = x;
-     tmp_matrix.y0 = y;
+     tmp_matrix.x0 += x;
+     tmp_matrix.y0 += y;
      pango_renderer_set_matrix (renderer, &tmp_matrix);

      x = 0;
      y = 0;
    }
Comment 4 Jody Goldberg 2006-02-23 04:04:15 UTC
behad : Your suggestion works, but the performance significantly degraded.  I supposed anyone foolish enough to start scrolling around at rows > 134,000 deserves what they get (let's not discuss the wisdom of using a spreadsheet for that magnitude of data) but it's not an ideal solution.  At a minimum the gdk interface should document that coords must be < 2^21.
Comment 5 Jody Goldberg 2006-02-23 04:05:53 UTC
Created attachment 59980 [details] [review]
implement behad's suggested kludge.

It's not pretty but it gets the job done.
Comment 6 Behdad Esfahbod 2006-02-23 04:08:34 UTC
Jody: Is it with cairo backend?  I think (but not sure) that with cairo, using a matrix or not doesn't make any difference performance-wise.  A similar patch should be applied to gdk_draw_layout_line_with_colors too...
Comment 7 Morten Welinder 2006-02-23 15:19:04 UTC
Behdad: compare http://bugzilla.gnome.org/show_bug.cgi?id=316597#c4
Comment 8 Matthias Clasen 2006-03-16 13:53:55 UTC
so, what we need is

a) a complete patch that covers all functions where
  we use pango scaling

b) as jody says, some documentation on the acceptable range
  of coordinates 
Comment 9 Behdad Esfahbod 2006-03-16 18:57:07 UTC
It's more like one or the other.  With a), there's no limit on coordinates anymore, at the cost of paying for matrix operations, so we may want to document that, but I don't see it essential at all.
Comment 10 Jody Goldberg 2006-03-22 20:18:30 UTC
Created attachment 61790 [details] [review]
Use a matrix when scaling would overflow
Comment 11 Morten Welinder 2006-03-22 20:44:36 UTC
We could go so far as to export that utility function from pango instead
of copying it all over the place.
Comment 12 Behdad Esfahbod 2006-03-30 04:54:31 UTC
Makes sense.  If we want to be complete, we should add the following six functions:

  pango_layout_get_rotated_size
  pango_layout_get_rotated_pixel_size
  pango_layout_get_rotated_extents
  pango_layout_get_rotated_pixel_extents
  pango_layout_line_get_rotated_extents
  pango_layout_line_get_rotated_pixel_extents

Any thoughts, Owen?  Like for example, the _size ones are not after all a good idea?
Comment 13 Matthias Clasen 2006-03-30 16:04:19 UTC
since the _size functions are just trivial wrappers around the others,
I'd leave them out. 
As for pixel_extents, would it not make more sense to expose a function
to convert a rectangle from pango units to device units ?
Comment 14 Behdad Esfahbod 2006-03-30 22:48:15 UTC
Ok, good.  Any ide how it should work? The current versions look suspicious to me (bug 329547).  Should they round?  floor? what?
Comment 15 Behdad Esfahbod 2006-04-11 20:41:56 UTC
Answering myself.  I think the most important property that such a function should hold is that touching non-overlaping rectangles should still touch and not overlap after rounded using such a functions.  That means the same function should be applied to the start and end coordinates (and not start and length coordinates!)  As for the function itself, looking around gdkpango and pango itself, some use PANGO_PIXELS that rounds, others use / PANGO_SCALE which floors.  Lets go for rounding.  I'll cook a patch for Pango.
Comment 16 Morten Welinder 2006-04-13 18:36:57 UTC
The layoutline version should not correct for rotated bounding box.

Also, you cannot do

          tmp_matrix = PANGO_MATRIX_INIT;

but must do...

	  static const PangoMatrix unit = PANGO_MATRIX_INIT;
	  tmp_matrix = unit;
Comment 17 Jody Goldberg 2006-06-28 14:43:43 UTC
This just got alot more serious.
It looks like cairo is now enforcing a limit of 2^15

That limits gnumeric to about 2500 rows which is a significant regression.  I'll see if this can be ameliorated in foocanvas but you'll need to check TreeView .
Comment 18 Owen Taylor 2006-06-28 14:59:11 UTC
It used to be that cairo converted to fixed point *after* applying the
"device offset", so it would never handle coordinates outside the 
X coordinate space. Did that change?
Comment 19 Carl Worth 2006-06-29 10:48:21 UTC
(In reply to comment #18)
> It used to be that cairo converted to fixed point *after* applying the
> "device offset", so it would never handle coordinates outside the 
> X coordinate space. Did that change?

Ouch. Yes it did, (as part of some work that mozilla did which pushed the device_offset stuff from the gstate layer down into the surface layer of cairo).
Compare _cairo_gstate_user_to_backend to see the difference.  I'll look into what it would take to bring the necessary parts back up.

Sorry about the regression...

-Carl
Comment 20 Carl Worth 2006-06-29 17:55:43 UTC
Created attachment 68186 [details] [review]
Move device_transform of path to before floating->fixed conversion

Here's a suggested patch to cairo to address this problem.

Please test this and let me know how it works for you.

Thanks,

-Carl
Comment 21 Jody Goldberg 2006-06-29 18:15:15 UTC
cworth #20 : The patch solves the 2^15 limitation for me.  Once it goes in we can downgrade this back to normal.  Thanks.
Comment 22 Carl Worth 2006-06-29 19:58:49 UTC
(In reply to comment #21)
> cworth #20 : The patch solves the 2^15 limitation for me.  Once it goes in we
> can downgrade this back to normal.  Thanks.

Thanks for verifying the fix. I just pushed out a fix for this:

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

-Carl

PS. I'd mark the bug as resolved, but it appears I don't have permission for that.
Comment 23 Jody Goldberg 2006-06-29 20:20:04 UTC
cworth #22 : It's not resolved yet.  We're just back to where we started with a bound of 2^21.
Comment 24 Behdad Esfahbod 2006-07-07 02:23:37 UTC
Jody, can you submit a patch based on Morten's feedback in comment 16?

I'm going to add a few convenience functions in Pango for rotated bounds, etc, but they will only be in the next stable release, so we can fix this bug by duplicating code in the mean time.
Comment 25 Behdad Esfahbod 2006-07-07 05:05:27 UTC
Created attachment 68521 [details] [review]
proposed pango api

Proposed new api for pango.
Comment 26 Kjartan Maraas 2006-12-27 10:50:00 UTC
This didn't go into pango yet afaics. Is it going in at some point?
Comment 27 Matthias Clasen 2006-12-30 03:43:23 UTC
Behdad, did you want to fix this ?
Comment 28 Behdad Esfahbod 2006-12-30 16:53:54 UTC
Yes, I'm working on this.
Comment 29 Behdad Esfahbod 2007-01-04 19:39:13 UTC
Ok, committed the Pango API:

2007-01-04  Behdad Esfahbod  <behdad@gnome.org>

        Part of Bug 332266 – gdk_draw_layout fails for coordinates >= 2^21

        * pango/pango-types.h:
        * pango/pango-matrix.c: New public API:
                pango_matrix_transform_distance()
                pango_matrix_transform_point()
                pango_matrix_transform_rectangle()
                pango_matrix_transform_pixel_rectangle()

        * pango/pango-utils.h:
        * pango/pango-utils.c: New public API:
                pango_units_from_double()
                pango_units_to_double()
                pango_extents_to_pixels()

        * pango/pango-layout.c (pango_layout_get_pixel_extents),
        (pango_layout_line_get_pixel_extents): Use pango_extents_to_pixels().

        * pango/pangocairo-fcfont.c:
        (pango_cairo_fc_font_glyph_extents_cache_init),
        (compute_glyph_extents): Use pango_units_from_double().

        * examples/renderdemo.c (do_output): Use
        pango_matrix_transform_pixel_rectangle();

        * pango/pango.def:
        * docs/pango-sections.txt:
        * docs/tmpl/glyphs.sgml: Update.

So, the get_rotated_layout_bounds() function (in gdkpango and gtklabel) can be replaced with a pango_matrix_transform_rectangle() followed by a pango_extents_to_pixels().  Working on the gtk+ patch.  Test case more than welcome.
Comment 30 Behdad Esfahbod 2007-01-05 03:28:22 UTC
Created attachment 79423 [details] [review]
patch

Actually no new Pango API was needed.  Transformed PangoLineLayout's should not be shifted, cause the x,y you pass on is the position of the baseline-left of the line, no matter what the rotation is.
Comment 31 Behdad Esfahbod 2007-01-05 05:12:51 UTC
Created attachment 79428 [details] [review]
Patch to use pango_matrix_transform_rectangle

Patch to use the Pango API added above.  Doesn't really have anything to do with this bug.  Removes some code.
Comment 32 Behdad Esfahbod 2007-01-05 06:28:39 UTC
2007-01-05  Behdad Esfahbod  <behdad@gnome.org>

        * gdk/gdkprivate.h:
        * gdk/gdkpango.c (gdk_draw_layout_line_with_colors),
        (gdk_draw_layout_with_colors):
        * gdk/gdkwindow.c (gdk_window_draw_glyphs_transformed):
        Avoid overflow when converting coordinates to Pango units. (#332266,
        Jody Goldberg)

Comment 33 Behdad Esfahbod 2007-01-05 06:50:11 UTC
attachment 79428 [details] [review] will be followed in bug 340141.