Bug 39582 – [Cairo] Enhance the performance of shadowed elements by tiling the shadow area

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dirk Schulze]

-webkit-box-shadow can slow down srcolling or interacting with a page a lot on Gtk. We do the shadow on our own, since Cairo doesn't provide filters. The current implementation blurs a image in the size of the object (maybe clipped to the viewport, but I guess not). This causes much senseless calcluations, since most lines look the same. We also recalculate the shadow on scrolling, a further performance problem.
And now, that box-shadow is supported by more and more browsers, this feature gets used more regulary. See the link above, an example of a german newspaper.

I have a proposal for this problem. We don't take the size of the object, but the smallest possible rect to draw the shadow and give it to the blur-filter. We take the resulting image of the filter output and split it into 9 parts (we don't need 9 images, but we need to calculate the sizes of the parts).

 1 | 2 | 3
-----------
 4 |    | 5
-----------
 6 | 7 | 8

After that we use Image::drawPattern to draw the shadow of the target in the real size. This idea is similar to the CSS property border-image, also see: http://www.css3.info/preview/border-image/

It is neccessary to check sideeffects for inset shadows.

I don't have the time to implement it atm :-( and hope that someone else could try it.

[Alejandro G. Castro]

(In reply to comment #0)
>
> [...]
>
> It is neccessary to check sideeffects for inset shadows.
> 
> I don't have the time to implement it atm :-( and hope that someone else could try it.

Hi krit, very nice analysis, I've spent sometime checking the problem after the bug 38915 appeared,  I'm not very experienced about this topics but my conclusions after checking it a little bit are:

   - Avoid getters/setters and use arrays directly, I measured 20% win easy patch, but not much real difference in the scrolling
   - Add a skip region, we do not want to blur all the image when we want a shadow, not sure if this is happening now
   - Use alpha-only for shadows instead of all the channels?

I have time checking if we can fix this, so I can try it, do you think last point make senses? I've checked the information in the filters page (http://www.w3.org/TR/SVG/filters.html#feGaussianBlur) and if I understand correctly in most cases alpha only is enough.

[Alejandro G. Castro]

*** Bug 38915 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

(In reply to comment #1)
> (In reply to comment #0)
> >
> > [...]
> >
> > It is neccessary to check sideeffects for inset shadows.
> > 
> > I don't have the time to implement it atm :-( and hope that someone else could try it.
> 
> Hi krit, very nice analysis, I've spent sometime checking the problem after the bug 38915 appeared,  I'm not very experienced about this topics but my conclusions after checking it a little bit are:
> 
>    - Avoid getters/setters and use arrays directly, I measured 20% win easy patch, but not much real difference in the scrolling
>    - Add a skip region, we do not want to blur all the image when we want a shadow, not sure if this is happening now
>    - Use alpha-only for shadows instead of all the channels?
> 
> I have time checking if we can fix this, so I can try it, do you think last point make senses? I've checked the information in the filters page (http://www.w3.org/TR/SVG/filters.html#feGaussianBlur) and if I understand correctly in most cases alpha only is enough.

The current implementation does only blur the alpha channel. Otherwise we would be 3-4 times slower :-P

The clipping is one solution, but can have some bad sideeffects. Imagine a rect that you want to blur. We create an ImageBuffer, large enough, so that we have enough place for the rect itself and the resulting blurred edges on the sides. If we clip the rect and the image to the viewPort size, the actual visible size of the screen, the edge on the bottom (if the rect is clipped on the bottom) gets also blured. Of course that can be fixed and it will be more efficient than the current implementation, but to make the shadowImage as small as possible and therefore the number of pixels that need to get blurred as small as possible would be the fastest way.

[Dirk Schulze]

Created attachment 56870 [details]
Test Case large box with box-shadow 

Not sure why, but my attachements are not added when I create a bug.

This is a simple div-box with -webkit-box-shadow

note, that every pixel (at least every pixel you can see of the box) gets blurred. That is unnecessary, since most pixels are overlayed by the box, and also don't change it's value after the blurring.
Some stutters are noticeable on srolling down and up.

[Alejandro G. Castro]

(In reply to comment #3)
>
> [...]
> 
> The clipping is one solution, but can have some bad sideeffects. Imagine a rect 
> that you want to blur. We create an ImageBuffer, large enough, so that we have 
> enough place for the rect itself and the resulting blurred edges on the sides. 
> If we clip the rect and the image to the viewPort size, the actual visible size 
> of the screen, the edge on the bottom (if the rect is clipped on the bottom) 
> gets also blured. Of course that can be fixed and it will be more efficient 
> than the current implementation, but to make the shadowImage as small as 
> possible and therefore the number of pixels that need to get blurred as small 
> as possible would be the fastest way.

I understand, thanks for the comments, I'll try your proposal to create a smaller image and use it as a patter then.

[Alejandro G. Castro]

I've done some tests these days checking the proposed solution but it has one issue that I think can not be solved generally. Basically when blur deviation is big enough the blur converts the squares in a kind-of blur ellipse and we can not use it to do the tiling anymore, that means that the blurred rect size will depend on the deviation and the piece of shadow we get for the tiling could not be correct (rounded corners could not be reduced easily either). Or another option would be to do this when devitation is under a value. Not sure what function describes that relationship between the deviation and the size. What do you think Dirk?

I was thinking that instead of reducing the shape we could reduce the blurring area. Currently, as Dirk said, it is being used the whole size of the boxes, we can clip it with the viewport, and we could even use a part of the surface depending on the offsets of the shadows and the deviation, doing 4 blur operations:

    1
---------
 |    |
4|    |2
 |    |
------|
   3  |

Or just 2 in case the offset is big enough compared to the offset, this could be used for other shapes.

Anyway, checking the solution I found a couple of things that could help us:

1- The one proposed before, use the data instead the getters and setters in the blur function, I meassured 20% performance win with this. http://pastebin.com/NNR005sm

2- In case of alpha-only, avoid getpremultiplied/putpremultiplied and use the alpha information directly from the imagedata, maybe we would have to add API to get this data. Those parts of the process represent almost 40% of the time with the patch of the point 1.

[Dirk Schulze]

(In reply to comment #6)
> I've done some tests these days checking the proposed solution but it has one issue that I think can not be solved generally. Basically when blur deviation is big enough the blur converts the squares in a kind-of blur ellipse and we can not use it to do the tiling anymore, that means that the blurred rect size will depend on the deviation and the piece of shadow we get for the tiling could not be correct (rounded corners could not be reduced easily either). Or another option would be to do this when devitation is under a value. Not sure what function describes that relationship between the deviation and the size. What do you think Dirk?
This is not the normal case and we can't do anything against it, if the user takes this huge deviations. Rounded corners would need bigger pieces for 1,3,6,8 but shouldn't be a problem to determine the sizes.
We should also have a algorihm to determine the size of a blurred object already. We need it to get the smallest size for the temporary ImageBuffer right now.

> 
> I was thinking that instead of reducing the shape we could reduce the blurring area. Currently, as Dirk said, it is being used the whole size of the boxes, we can clip it with the viewport, and we could even use a part of the surface depending on the offsets of the shadows and the deviation, doing 4 blur operations:
> 
>     1
> ---------
>  |    |
> 4|    |2
>  |    |
> ------|
>    3  |
> 
> Or just 2 in case the offset is big enough compared to the offset, this could be used for other shapes.
How can this be faster than using a smaller shape area? You just need to determine the size of the rounded corners after the blurring, and calculate the size size of the shape, so that you have at least 1 pixel for the straight edge for every side. The most blurring operations have a kernel-size < 20 px, so that in most cases the resulting tempImageBuffer won't be bigger than ~ 100x100 pixel (I bet the avarage case will be smaller than 30x30 pixel).
After that, you cut the tempImage into different pieces and fill the area of the original shadow with different patterns. Just like CSS's border-image is doing it (http://www.css3.info/preview/border-image/). You even don't need to rewrite the code, we already have the code. See Image::drawPattern(...)

> 
> Anyway, checking the solution I found a couple of things that could help us:
> 
> 1- The one proposed before, use the data instead the getters and setters in the blur function, I meassured 20% performance win with this. http://pastebin.com/NNR005sm
Sounds reasonable, but I'm not sure if there were security reasons not to do it that way. Oliver can you take a look at this please?

> 
> 2- In case of alpha-only, avoid getpremultiplied/putpremultiplied and use the alpha information directly from the imagedata, maybe we would have to add API to get this data. Those parts of the process represent almost 40% of the time with the patch of the point 1.
Cairo stores all values in cairo_surface_t as premultiplied. That means, getpremultiplied/putpremultiplied are just copying the values from the surface to the CanvasPixelArray. How do you want to get the values without using  getpremultiplied/putpremultiplied? I just know one way, implementing it for cairo again instead of reusing already existing code and optimize the realy relevant part: making the blurred area as small as possible.

[Alejandro G. Castro]

Created attachment 59100 [details]
Patch showing an option to avoid get and put

After talking to krit about the issues I found I've created another bug in order to fix the kernel_size calculations (bug 40793), with that patch we could check the size of the minimum rectangle using the blurRadius. Anyway I also wanted to show the idea of how to avoid the pre/put process, this patch is just a rough test I did to check if it makes sense. In that case we would have to clean the code, do this just in case the image is an alpha image and review the API of ImageBuffer for all the platforms. It used both optimizations: arrays and avoid initial put/get operations of the result buffer doing the operations directly with the result buffers data. As krit said we would have to check also the security issues, probably check carefully all the input variables.

I've tested this patch with identi.ca and it makes the difference in my computer, it is not perfect though (try it and give some feedback), oprofile graphics show we are saving almost 40% of the time (over the 20% we win with the array change).

I would say that if this patch makes sense we can use this one and create the tiling proposal over it, that way we would have a faster solution for these cases.

[Alejandro G. Castro]

Created attachment 63077 [details]
Proposed patch

This is the tiling operation implemented, I'm not adding the review until the submit the other 3 patches that are still pending. I'm going to upload a bundle with all the patches to simplify the testing of all the patch-set. Comments are more than welcome, the patch maybe need a little review but you can check the idea ;).

[Alejandro G. Castro]

Created attachment 63079 [details]
Updated patch for gauss operations without put and get

[Alejandro G. Castro]

Created attachment 63080 [details]
Patch-set with all the patches, for testing purposes

With all the patches the identi.ca webpage performs well in my laptop, oprofile does not report the blur operation as one of the main issues. Check it and send feedback.

[Alejandro G. Castro]

Created attachment 63741 [details]
Proposed patch

Finished the complete specific calculation and added some doc to the method.

[Simon Fraser (smfr)]

Comment on attachment 63741 [details]
Proposed patch

> diff --git a/WebCore/platform/graphics/cairo/FontCairo.cpp b/WebCore/platform/graphics/cairo/FontCairo.cpp
> index d14b371..e0d6c2c 100644
> --- a/WebCore/platform/graphics/cairo/FontCairo.cpp
> +++ b/WebCore/platform/graphics/cairo/FontCairo.cpp
> @@ -114,7 +114,7 @@ void Font::drawGlyphs(GraphicsContext* context, const SimpleFontData* font, cons
>              cairo_restore(shadowCr);
>          }
>          cairo_translate(cr, 0.0, -extents.height);
> -        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, radius);
> +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, radius, true);

This is a great example of why boolean parameters are bad. I've no idea what 'true' means when reading this code.
An enum is better.

[Alejandro G. Castro]

(In reply to comment #13)
> >              cairo_restore(shadowCr);
> >          }
> >          cairo_translate(cr, 0.0, -extents.height);
> > -        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, radius);
> > +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, radius, true);
> 
> This is a great example of why boolean parameters are bad. I've no idea what 'true' means when reading this code.
> An enum is better.

I agree, I'll replace it.

[Alejandro G. Castro]

Created attachment 64004 [details]
Proposed patch

Replaced the boolean with the enum as suggested.

[Ariya Hidayat]

Adding myself.

[Alejandro G. Castro]

Created attachment 64196 [details]
Proposed patch for pixel operations

This patch converts all the operations with array objects in operations with char arrays, avoiding the copy of the data from ImageBuffers to ImageDatas. Even with the tiling patch we need this to get a good performance.

I've tried to create the APIs required for the patch in the other ports but probably they are not correct, actually not sure if what we are doing is possible in Qt for instance, namely, get a reference to the pixel information inside a QPixmap, maybe we need a copy in that case. The other option I guess would be to do the operation inside the ImageBuffer class of each port.

Comments more than welcome ;).

[Ariya Hidayat]

Comment on attachment 64196 [details]
Proposed patch for pixel operations

> +        void dirty();

Probably better to use explicit verb, e.g. "markDirty".

[Alejandro G. Castro]

Created attachment 64199 [details]
Patch-set with all the patches, for testing purposes

[Dirk Schulze]

Comment on attachment 64004 [details]
Proposed patch

A great patch, but maybe a bit more complicated than it should be.

> +#if PLATFORM(CAIRO)
> +    // Decide if the shadow buffer should be blitted or released (required for tiling)
> +    enum ResultBufferOp {
> +        Blit,
> +        Release
> +    };
> +#endif
Can you add a better naming schema? I have no idea what ResultBufferOp means and Blit or Relase are not very helpful (I even don't know what Blit means :-P)

> +/*
> +  This function uses tiling to improve the performance of the shadow
> +  drawing of rounded rectangles. The code basically does the following
> +  steps:
> +
> +     1. Calculate the minimum rectangle size required to create the
> +     tiles
> +
> +     2. If that size is smaller than the real rectangle render the new
> +     small rectangle and its shadow, in other case render the shadow
> +     of the real rectangle.
> +
> +     3. Calculate the sizes and positions of the tiles and their
> +     destinations and use drawPattern to render the final shadow. The
> +     code divides the rendering in 8 tiles:
> +
> +        1 | 2 | 3
> +       -----------
> +        4 |   | 5
> +       -----------
> +        6 | 7 | 8
> +
> +     The corners are directly copied from the small rectangle to the
> +     real one and the side tiles are 1 pixel width, we use them as
> +     tiles to cover the destination side. The corner tiles are bigger
> +     than just the side of the rounded corner, we need to increase it
> +     because the modifications caused by the corner over the blur
> +     effect.
> + */
> +void GraphicsContext::drawFastShadow(GraphicsContext* context, GraphicsContextPrivate* gcp, const IntRect& r, const FloatSize& topLeftRadius, const FloatSize& topRightRadius, const FloatSize& bottomLeftRadius, const FloatSize& bottomRightRadius, ColorSpace colorSpace)
> +{
> +#if ENABLE(FILTERS)
> +    FloatSize shadowSize;
> +    FloatSize shadowOffset;
...
> +        destRect.setX(shadowRect.x() + rect.width() - topRightRadius.width());
> +        destRect.setY(shadowRect.y() + rect.height() - bottomRightRadius.height());
> +        destRect.setWidth(tileRect.width());
> +        destRect.setHeight(tileRect.height());
> +        phase.setX(destRect.x() - tileRect.x());
> +        phase.setY(destRect.y() - tileRect.y());
> +
> +        shadowBuffer->image()->drawPattern(context, tileRect, patternTransform,
> +                                           phase, colorSpace, CompositeSourceOver, destRect);
> +
> +        // bottom left corner
> +        tileRect.setX(0);
> +        tileRect.setY(smallBufferSize.height() - 2 * radius - bottomRightRadius.height());
> +        tileRect.setWidth(2 * radius + bottomLeftRadius.width());
> +        tileRect.setHeight(2 * radius + bottomLeftRadius.height());
> +        destRect.setX(shadowRect.x());
> +        destRect.setY(shadowRect.y() + rect.height() - bottomRightRadius.height());
> +        destRect.setWidth(tileRect.width());
> +        destRect.setHeight(tileRect.height());
> +        phase.setX(destRect.x() - tileRect.x());
> +        phase.setY(destRect.y() - tileRect.y());
> +
> +        shadowBuffer->image()->drawPattern(context, tileRect, patternTransform,
> +                                           phase, colorSpace, CompositeSourceOver, destRect);
> +
> +    } else {
> +        // Create suitably-sized ImageBuffer to hold the shadow.
> +        OwnPtr<ImageBuffer> shadowBuffer = ImageBuffer::create(shadowBufferSize);
> +
> +        // Draw shadow into a new ImageBuffer.
> +        cairo_t* shadowContext = shadowBuffer->context()->platformContext();
> +        copyContextProperties(cr, shadowContext);
> +        cairo_translate(shadowContext, -rect.x() + radius, -rect.y() + radius);
> +        cairo_new_path(shadowContext);
> +        cairo_append_path(shadowContext, path);
> +
> +        setPlatformFill(context, shadowContext, gcp);
> +
> +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, radius, Blit);
> +    }
> +#endif
> +}
This is pretty hard to understand. first,there shouldn't be that much code in an if clause. I suggest to test for the else, move the content of the else in this if and return earlier. the rest don't need to be checked again:

> +    if (...) {
> +        // Create suitably-sized ImageBuffer to hold the shadow.
> +        OwnPtr<ImageBuffer> shadowBuffer = ImageBuffer::create(shadowBufferSize);
> +
> +        // Draw shadow into a new ImageBuffer.
> +        cairo_t* shadowContext = shadowBuffer->context()->platformContext();
> +        copyContextProperties(cr, shadowContext);
> +        cairo_translate(shadowContext, -rect.x() + radius, -rect.y() + radius);
> +        cairo_new_path(shadowContext);
> +        cairo_append_path(shadowContext, path);
> +
> +        setPlatformFill(context, shadowContext, gcp);
> +
> +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, radius, Blit);
>          return;
> +    }
> ....



> +        tileRect.setX(0);
> +        tileRect.setY(smallBufferSize.height() - 2 * radius - bottomRightRadius.height());
> +        tileRect.setWidth(2 * radius + bottomLeftRadius.width());
> +        tileRect.setHeight(2 * radius + bottomLeftRadius.height());
This should be set at once:
   tileRect = FloatRect(0, smallBufferSize.height() - 2 * radius - bottomRightRadius.height(), ...)
Of course you can use multiple line for that.

You should use the logic of FloatRect/Size/Point much more in your code. and also it's funtcions. E.g. inflateX and inflateY extends the rect. Please use them to enlarge your rect to respect the blurring.
Don't mix up the enlargement of your rects with the offset. You can apply the blur radius to the with and position of the rects and move the rect by the shadowOffset.
Some multiplications are done multiple times (radius * 2) can you store them in a new variable like radiusTwice and so on?

It's great that you try to reuse the drawPattern code.

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1303
 +          if (offsetHeight > min (bottomLeftRadius.height(), bottomRightRadius.height()))
no space between min (

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1296
 +      if (offsetWidth > 0)
use if (offsetWidth)
The same in other lines.

[Alejandro G. Castro]

(In reply to comment #18)
> (From update of attachment 64196 [details])
> > +        void dirty();
> 
> Probably better to use explicit verb, e.g. "markDirty".

Good point, I'll change it.

[Alejandro G. Castro]

Thanks for the detailed review.

(In reply to comment #20)
>
> [...]
>
> Can you add a better naming schema? I have no idea what ResultBufferOp means and Blit or Relase are not very helpful (I even don't know what Blit means :-P)
> 

Yeah, naming is always the hardest, it tried to be Result Buffer Operation, I know, it sucks :). http://en.wikipedia.org/wiki/Bit_blit. I'll try to get something better.

>
> [...]
>
> > +}
> This is pretty hard to understand. first,there shouldn't be that much code in an if clause. I suggest to test for the else, move the content of the else in this if and return earlier. the rest don't need to be checked again:
> 
> > +        tileRect.setX(0);
> > +        tileRect.setY(smallBufferSize.height() - 2 * radius - bottomRightRadius.height());
> > +        tileRect.setWidth(2 * radius + bottomLeftRadius.width());
> > +        tileRect.setHeight(2 * radius + bottomLeftRadius.height());
> This should be set at once:
>    tileRect = FloatRect(0, smallBufferSize.height() - 2 * radius - bottomRightRadius.height(), ...)
> Of course you can use multiple line for that.
> 
> You should use the logic of FloatRect/Size/Point much more in your code. and also it's funtcions. E.g. inflateX and inflateY extends the rect. Please use them to enlarge your rect to respect the blurring.
> Don't mix up the enlargement of your rects with the offset. You can apply the blur radius to the with and position of the rects and move the rect by the shadowOffset.
> Some multiplications are done multiple times (radius * 2) can you store them in a new variable like radiusTwice and so on?
> 
> It's great that you try to reuse the drawPattern code.
> 
> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1303
>  +          if (offsetHeight > min (bottomLeftRadius.height(), bottomRightRadius.height()))
> no space between min (
> 
> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1296
>  +      if (offsetWidth > 0)
> use if (offsetWidth)
> The same in other lines.

I agree, good points, I'll check them.

[Alejandro G. Castro]

(In reply to comment #20)
> (From update of attachment 64004 [details])
>
> [...]
>
> A great patch, but maybe a bit more complicated than it should be.
> This should be set at once:
>    tileRect = FloatRect(0, smallBufferSize.height() - 2 * radius - bottomRightRadius.height(), ...)
> Of course you can use multiple line for that.
>

I just used the sets because with FloatRect we are creating a new object each time, but not sure which option is better wrt performance or if it make sense, I agree with you creating the objects is clearer, I¡ll change them.

> You should use the logic of FloatRect/Size/Point much more in your code. and also it's funtcions. E.g. inflateX and inflateY extends the rect. Please use them to enlarge your rect to respect the blurring.

I think in this case (if it is the case I'm checking :) is not an inflate but a size change, we do not want to move the location.

> Don't mix up the enlargement of your rects with the offset. You can apply the blur radius to the with and position of the rects and move the rect by the shadowOffset.
> 

Yeah, I'll review all the calculations to make them clearer

> 
> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1296
>  +      if (offsetWidth > 0)
> use if (offsetWidth)

In this case we have to use > 0 because it is not just a boolean check, in the else we want to check the case where offsetWidth is less than 0.

Thanks for the comments, I'll try to have a patch ready, I will also use the new drawPattern added by Hyatt to avoid the copies of the image.

[Alejandro G. Castro]

Created attachment 64747 [details]
Proposed patch

Include all the suggestions. Still not for review until we land the other two patches blocking this one.

[Alejandro G. Castro]

Created attachment 64754 [details]
Proposed patch for pixel operations

Added ariya's suggestions for this patch.

[Alejandro G. Castro]

Created attachment 64755 [details]
Patch-set with all the patches, for testing purposes

Updated patchset.

[Alejandro G. Castro]

Created attachment 64827 [details]
Proposed patch

Rebased and fixed problem with big offsets.

[Alejandro G. Castro]

Created attachment 65277 [details]
Proposed patch

After checking the options about the offset with Ariya he proposed using the blur radius and fill with the solid color the other parts of the figure. With this modification we do not need to calculate the offset anymore so we can land this patch. Adding review flag.

[Alejandro G. Castro]

I've added a new bug with the FEGaussianBlur pixel operations patch: bug 44528. It does not depend on this one.

[simontol]

I'm experiencing pretty the same issue on http://www.google.com/reader, using webkitGTK browsers (Epiphany or Midori on Linux)  scrolling is very slow.
No issue with QTwebkit browsers (Arora, Rekonq, Konqueror+webkit).
Do I have to open a new bug?

[Alejandro G. Castro]

(In reply to comment #30)
> I'm experiencing pretty the same issue on http://www.google.com/reader, using webkitGTK browsers (Epiphany or Midori on Linux)  scrolling is very slow.
> No issue with QTwebkit browsers (Arora, Rekonq, Konqueror+webkit).
> Do I have to open a new bug?

It is this issue, there are shadows in the style, a new bug is not necessary, thanks for reporting it.

[Alejandro G. Castro]

Created attachment 65927 [details]
Proposed patch

Update the patch with the current HEAD.

[Alejandro G. Castro]

Created attachment 66044 [details]
Proposed patch

Some improvements thanks to mrobinson's review the pixel tests he added:
  - Fixed issues with the tiles destination
  - Fixed problems when there are transformations
  - Fixed pointers ownership, no need to the resultRelease function

[Alejandro G. Castro]

Created attachment 66059 [details]
Proposed patch

- Fixed Changelog
- Clean smallBufferSize calculation
- Removed leak of the cairo path

[Alejandro G. Castro]

Created attachment 66061 [details]
Proposed patch

Upload the real last version :).

[Martin Robinson]

Comment on attachment 66061 [details]
Proposed patch

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1285
> +
I think I prefer to switch this calculation around a bit to make it clearer. Consider:

// Find the extra space needed from the curve of the corners.
int extraWidthFromCornerRadii = max(topLeftRadius.width(), bottomLeftRadius.width()) + tileWidth + radiusTwice + max(topRightRadius.width(), bottomRightRadius.width();
int extraHeightFromCornerRadii = max(topLeftRadius.height(), topRightRadius.height()) + tileWidth + radiusTwice + max(bottomLeftRadius.height(), bottomRightRadius.height();

// The length of a side of the buffer is the enough space for two blur radii,
// the radii of the corners, and then 1 pixel to draw the side tiles.
IntSize smallBufferSize = IntSize((blurRadius * 2) + 1 + extraWidthFromCornerRadii,
                                  (blurRadius * 2) + 1 + extraHeightFromCornerRadii));

Notice that I renamed radius to blurRadius.

[Alejandro G. Castro]

(In reply to comment #36)
> // Find the extra space needed from the curve of the corners.
> int extraWidthFromCornerRadii = max(topLeftRadius.width(), bottomLeftRadius.width()) + tileWidth + radiusTwice + max(topRightRadius.width(), bottomRightRadius.width();
> int extraHeightFromCornerRadii = max(topLeftRadius.height(), topRightRadius.height()) + tileWidth + radiusTwice + max(bottomLeftRadius.height(), bottomRightRadius.height();
> 
> // The length of a side of the buffer is the enough space for two blur radii,
> // the radii of the corners, and then 1 pixel to draw the side tiles.
> IntSize smallBufferSize = IntSize((blurRadius * 2) + 1 + extraWidthFromCornerRadii,
>                                   (blurRadius * 2) + 1 + extraHeightFromCornerRadii));
> 
> Notice that I renamed radius to blurRadius.

I like it, I'm going to add it.

[Alejandro G. Castro]

Created attachment 66129 [details]
Proposed patch

Some more reviews of the code style, now fillRect also uses the function. At this point we are not supporting rotations, we will check how to fix aliasing problems in those cases in a following bug.

[Dirk Schulze]

Comment on attachment 66129 [details]
Proposed patch

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:630
> +    if ((transform.isIdentityOrTranslationOrFlipped())) {
> +        cairo_t* cr = context->platformContext();;
Avoid multiple braces and muliple semicolons.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:633
> +        FloatSize corner(0, 0);
No need for (0, 0), just use FloatSize corner;

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1330
> +    OwnPtr<ImageBuffer> smallBuffer = ImageBuffer::create(smallBufferSize);
You should check if we realy got an imagebuffer.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1340
> +    OwnPtr<ImageBuffer> shadowBuffer = ImageBuffer::create(smallBufferSize);
dito

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1378
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
can be in one line

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1388
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
> +                              phase, colorSpace, CompositeSourceOver, destRect);
dito

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1397
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
> +                              phase, colorSpace, CompositeSourceOver, destRect);
dito

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1405
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
> +                              phase, colorSpace, CompositeSourceOver, destRect);
dito

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1413
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
> +                              phase, colorSpace, CompositeSourceOver, destRect);
dito

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1421
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
> +                              phase, colorSpace, CompositeSourceOver, destRect);
dito

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1429
> +    shadowBuffer->drawPattern(this, tileRect, patternTransform,
> +                              phase, colorSpace, CompositeSourceOver, destRect);
dito

[Alejandro G. Castro]

Thanks for the review :)

(In reply to comment #39)
> (From update of attachment 66129 [details])
> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:630
> > +    if ((transform.isIdentityOrTranslationOrFlipped())) {
> > +        cairo_t* cr = context->platformContext();;
> Avoid multiple braces and muliple semicolons.
> 
> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:633
> > +        FloatSize corner(0, 0);
> No need for (0, 0), just use FloatSize corner;
> 
> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1330
> > +    OwnPtr<ImageBuffer> smallBuffer = ImageBuffer::create(smallBufferSize);
> You should check if we realy got an imagebuffer.
> 

Good catch this one, there are more places int he code without this check, I'll change it in the patch and create a new bug for this.

[Alejandro G. Castro]

Created attachment 66175 [details]
Patch

[Alejandro G. Castro]

Created attachment 66179 [details]
Proposed patch

I reviewed the commens, I'll add the pixel tests to the patch when committing to avoid git/svn binary problems.

[Martin Robinson]

Comment on attachment 66179 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66179&action=prettypatch

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:628
> +    // drawTiledShadow still does not work with rotations
Please add a period to the end of this sentence before landing.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1442
> +    // drawTiledShadow still does not work with rotations
Same here.

This looks good to me. Eventually much of this should be separated out into another class. We should share ContextShadow. I think I'll start on that today. We need to ensure that shadows for non-rectangular shapes are fast as well.

[Alejandro G. Castro]

landed r66607

Thanks everybody for the help adding all the patches ;).

[WebKit Review Bot]

http://trac.webkit.org/changeset/66608 might have broken SnowLeopard Intel Release (Tests)

[Martin Robinson]

*** Bug 53888 has been marked as a duplicate of this bug. ***