Bug 19652 – [CAIRO] wrong drawing of border-image

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Schulze]

The pattern in ImageCairo::drawImage() doesn't use tileRect(). Thats why border-images are drawn wrong in some cases. See: http://www.css3.info/preview/border-image/

[Dirk Schulze]

Created attachment 21816 [details]
border-image

Added support for tileRect() for a correct drawing of border-images. I hope it is effiecent enough.

[Dirk Schulze]

Created attachment 21818 [details]
border-image

for more safety

[Dirk Schulze]

Created attachment 21823 [details]
border-image

cairo_t* cr_surface must be defined before the if clause, to make cairo_destroy(cr_surface). Even if we don't need cr_surface :-(

I changed rectangle and fill with cairo_paint, it's more efficient.

[Mark Rowe (bdash)]

Comment on attachment 21823 [details]
border-image

This call to cairo_destroy can pass an uninitialized value for cr_surface which I doubt is safe:

+    cairo_destroy(cr_surface);

Why does the call to cairo_destroy need to be outside the body of the if block where cr_surface is used?  The naming of cr_surface seems like poor choice too:  both surface and cr_surface are cairo types, and I can't see any particular significance to the cr_ prefix.

[Dirk Schulze]

(In reply to comment #4)
> (From update of attachment 21823 [details] [edit])
> This call to cairo_destroy can pass an uninitialized value for cr_surface which
> I doubt is safe:
> 
> +    cairo_destroy(cr_surface);
> 
> Why does the call to cairo_destroy need to be outside the body of the if block
> where cr_surface is used?

cairo_destroy decreases the reference count. And every acces to image (where the new surface is stored at the end) caused crashes of webkit.

> The naming of cr_surface seems like poor choice too
rigth. How about:
surface -> clippedImage
cr_surface -> clippedImageContext
?

[Dirk Schulze]

Created attachment 22854 [details]
border-image

Renamed the functions and destroy surface after using it.

[Eric Seidel (no email)]

Comment on attachment 22854 [details]
border-image

You can force a FloatSize to an IntSize using IntSize(floatSize);

nativeImageForCurrentFrame is supposed to return a pointer to a cached frame image, so you shouldn't be releasing it.  Does cairo intentionally have a different memory management model for this?  Maybe Images aren't ref-counted in Cairo?

[Dirk Schulze]

Created attachment 23291 [details]
Cairo border-image

cairo_surface's are ref-counted in cairo. Thats why I put auto_ptr<ImageBuffer> above the if-clause. Otherwise the image is cleaned-up to early and I get a ref-count error (ref-count lower than zero).
But I measured the call and it takes only 0.000047s extra time as worst-case (normaly 0.000009s).
I don't know (or understand) what you mean with nativeImageForCurrentFrame().
I need a cairo_suface_t* out of the imageBuffer and nativeImageForCurrentFrame() can give it.
 
I haven't r? the patch, to discuss the changes again.

[Alp Toker]

(In reply to comment #8)
> Created an attachment (id=23291) [edit]
> Cairo border-image
> 
> cairo_surface's are ref-counted in cairo. Thats why I put auto_ptr<ImageBuffer>
> above the if-clause. Otherwise the image is cleaned-up to early and I get a
> ref-count error (ref-count lower than zero).
> But I measured the call and it takes only 0.000047s extra time as worst-case
> (normaly 0.000009s).
> I don't know (or understand) what you mean with nativeImageForCurrentFrame().
> I need a cairo_suface_t* out of the imageBuffer and
> nativeImageForCurrentFrame() can give it.
> 
> I haven't r? the patch, to discuss the changes again.
> 

Maybe you could do something like this (completely untested) instead of using ImageBuffer?

    cairo_pattern_t* pattern;
    if (tileRect.size() == size())
        pattern = cairo_pattern_create_for_surface(image);
    else {
        cairo_push_group(cr);
        //IntRect imageSize = enclosingIntRect(tileRect);
        cairo_set_source_surface(cr, image, -tileRect.x(), -tileRect.y());
        cairo_paint(cr);
        pattern = cairo_pop_group(cr);
    }

[Dirk Schulze]

(In reply to comment #9)
Thanks for your reply. It is a great idea. I tested it with many modifications and spend much time on it, but didn't get it to work.

The problem is, the geometry of the context isn't reset. That means, the dimension of the context is different on every time.
The second problem is: You don't only move the the image by -tileRect.x(), -tileRect.y(), you need a part of it, e.g. if you have a 90x90 surface, you need 5 parts. One would be (30, 0, 30, 30)/(x,y,width,height). I wasn't able to realise it with push_to_group. One thing why i didn't manage it was that a rect with the coordinate 0,0 wasn't placed at the top,left of the context, it was somewhere out of the view area.
I have to admit, that this behavior makes it difficult to understand the sense of push_to_group.

In summary:
I need a context with a height and width of the surface. Otherwise I'll get problems with the pattern. It will give more than just this part of a surface.
If it is possible to get the width and height of the current context and the coordinates of the first pixel in the top/left of the context, you can scale and transform the created image (pop_to_group).
But I don't believe that it will be more affective than just creating a new surface with the height and width given by tileRect and draw the part of the image into it's context.

If you have more suggestions, please post it. It would be great to do it without an new surface or ImageBuffer and an acceptable complexity.

[Eric Seidel (no email)]

Comment on attachment 23291 [details]
Cairo border-image

This would be super-easy to approve if Gtk supported pixel tests. ;)

[Dirk Schulze]

@eseidel: don't upset the gtk gods.

I talked about this problem in #cairo on irc and it was assured me, that this is already the fastest way. 
cairo_pop_to_group does not work here. We still create a surface, but with a undefined size. That means we have no win, it is neither a speed-up, nor more resource affective.

[Eric Seidel (no email)]

Comment on attachment 23291 [details]
Cairo border-image

Please add a note in the changelog pointing to a LayoutTest which will test this once Gtk has pixel tests.  If there is no such test, please add one.

Otherwise this looks sane enough.

[Dirk Schulze]

landed in r44081.