Bug 108897 – [Cairo] Canvas-shadow behavior is not being as expected

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Rashmi Shyamasundar]

Created attachment 186535 [details]
Actual and expected Outputs

The layout-test fast/canvas/canvas-createPattern-fillRect-shadow.html is failing. 
The image drawn on the canvas is not being as expected. This failure is not being caught in the pixel-checks.

Attached is a smaller test-case (canvas-test-pattern-shadow.html) that can be used to reproduce the same issue.
Attached are the expected and actual outputs of this smaller test-case.

[Rashmi Shyamasundar]

Created attachment 186536 [details]
Smaller test-case to reproduce the canvas-shadow issue

[Alexey Proskuryakov]

What platform is this with?

Can you please attach the archives in a commonly recognized format, which we wouldn't need to download an unarchiver for?

[Rashmi Shyamasundar]

This issue is seen on webkit-efl port.

[Rashmi Shyamasundar]

Created attachment 186799 [details]
Actual and expected Outputs in .zip format

[Rashmi Shyamasundar]

Created attachment 186800 [details]
Smaller test-case to reproduce the canvas-shadow issue in .zip format

[Alexey Proskuryakov]

Thank you!

Mac port appears to be unaffected.

[Rashmi Shyamasundar]

The below link can be used to run the test case on any port - 
http://jsfiddle.net/wE8cr/

[Dirk Schulze]

I(In reply to comment #7)
> The below link can be used to run the test case on any port - 
> http://jsfiddle.net/wE8cr/

The result on the image is definitely wrong. The result on chromium on safari are correct. Can you give a bit more feedback please? Do you see that just on EFL? Does EFL use Cairo Graphics? In Software or GL? Does it happen in WebKitGtk as well?

[Rashmi Shyamasundar]

EFL and GTK use Cairo-graphics. This issue is being reproducible on EFL and GTK.
This issue is not seen in Windows and Qt ports.

[Philip Rogers]

(In reply to comment #9)
> EFL and GTK use Cairo-graphics. This issue is being reproducible on EFL and GTK.
> This issue is not seen in Windows and Qt ports.

Rashmi, do you know the regression range for this?

[Dirk Schulze]

(In reply to comment #10)
> (In reply to comment #9)
> > EFL and GTK use Cairo-graphics. This issue is being reproducible on EFL and GTK.
> > This issue is not seen in Windows and Qt ports.
> 
> Rashmi, do you know the regression range for this?

This is no regression IIRC. Shadows just don't work properly on images for Cairo. Problem is either in ShadowBlur or GrapicsContextCairo.

[Rashmi Shyamasundar]

Created attachment 193787 [details]
Image after cairo_fill in GraphicsContextCairo::drawPathShadow

[Rashmi Shyamasundar]

Created attachment 193790 [details]
Image written by adding a cairo_fill(), for debugging

[Rashmi Shyamasundar]

Created attachment 193792 [details]
After cairo_append_path in drawPathShadow() in GraphicsContextCiaro.cpp

Image written by adding a cairo_fill(), for debugging

[Rashmi Shyamasundar]

Created attachment 193793 [details]
"4_image_after_fill" - Image after cairo_fill in shadowAndFillCurrentCairoPath() in GraphicsContextCairo.cpp

[Rashmi Shyamasundar]

Created attachment 193803 [details]
GraphicsContextCairo_fill.cpp

The attached file - "GraphicsContextCairo_fill.cpp" contains the code I have added to GraphicsContextCairo.cpp, in order to generate the images which I have earlier attached - 

Please look for the following names in GraphicsContextCairo_fill.cpp - 
1. "shadow_after_fill.png" - https://bugs.webkit.org/attachment.cgi?id=193787
2. "before_append_path.png" - https://bugs.webkit.org/attachment.cgi?id=193790
3. "after_append_path.png" - https://bugs.webkit.org/attachment.cgi?id=193792
4. "image_after_fill.png" - https://bugs.webkit.org/attachment.cgi?id=193793

[Rashmi Shyamasundar]

I see that, the image drawn on cairoShadowContext [refer function drawPathShadow() in GraphicsContextCairo.cpp] is actually the image itself and not its shadow. Is this correct?

cairo_surface_write-to_png() called on this cairoShadowContext, is generating the image - https://bug-108897-attachments.webkit.org/attachment.cgi?id=193787.

[Rashmi Shyamasundar]

This issue can solved by making the below change in PlatformContextCairo::drawSurfaceToContext() -

REPLACE the line

"cairo_pattern_set_extend(pattern.get(), CAIRO_EXTEND_PAD);"

WITH

"cairo_pattern_set_extend(pattern.get(), CAIRO_EXTEND_NONE);"

[Rashmi Shyamasundar]

Created attachment 195942 [details]
Patch

[Martin Robinson]

It actually should be PAD there, otherwise canvas will break. :) Can you explain why turning off the extend there fixes the issue? Also, can you check if my patch at https://bugs.webkit.org/show_bug.cgi?id=58309 improves things?

[Dirk Schulze]

Comment on attachment 195942 [details]
Patch

The default PAD is just for gradients. And there is definitely makes sense to support filling inner focal radius and repeating gradients. Are you sure that this does not add regressions? Also, can you add a test please?

[Benjamin Poulain]

Comment on attachment 195942 [details]
Patch

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

No clue about the patch's correctness, but the ChangeLog is not good enough.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should remove this line; No ChangeLog with "OOPS" should land.

> Source/WebCore/ChangeLog:11
> +        In PlatformContextCairo::drawSurfaceToContext(), 
> +        extend mode for the pattern should be CAIRO_EXTEND_NONE

It is not enough explaining what you did, you have to explain why you are doing the change:

You need to explain why the code was wrong, what errors it was causing, and why the new code is correct.

[Rashmi Shyamasundar]

Question:. Why should the extend mode be "CAIRO_EXTEND_NONE"?
Ans: As described in the cairo manual,
http://cairographics.org/manual/cairo-cairo-pattern-t.html#cairo-extend-t :

"The default extend mode is CAIRO_EXTEND_NONE for surface patterns and CAIRO_EXTEND_PAD for gradient patterns."

In the function PlatformContextCairo::drawSurfaceToContext(), a pattern is being created from the source surface and this pattern is drawn onto the destination context. There is no gradient involved. Hence the extend mode for filling the pattern should be "CAIRO_EXTEND_NONE".

This does not add any regression.

There is already a layout-test highlighting this issue -
fast/canvas/canvas-createPattern-fillRect-shadow.html

In this test case, without this patch, the "shadowColor"(red) mentioned in html, will be seen in the original image on canvas, wherever the image is transparent.
With this patch, this issue is solved.

The steps used for drawing a shadow is - 

1. Create a cairo surface/context called shadow-context and set this context to translate to shadowOffsetX and shadowOffsetY. Draw the original image on the shadow-context. 

The translation is handled in ShadowBlur::beginShadowLayer(). Drawing the original image on the context is done in drawPathShadow() in GraphicsContextCairo.cpp. 

2. Fill the shadow-context with "shadowColor", by setting the global composite operator to be "CompositeSourceIn". This will ulitmately result in changing the color of the image in shadowContext(destination image), to shadowColor. This is done in ShadowBlur::blurAndColorShadowBuffer().

3. Copy the shadow-context to the original canvas-context, using the global composite operator - "CompositeSourceOver".

Root cause for the issue:-

In step #2, while filling the shadow-context with "shadowColor", if we use the extend mode "CAIRO_EXTEND_PAD" in PlatformContextCairo::drawSurfaceToContext(), the original image area is also filled with the shadowColor.

[Dirk Schulze]

(In reply to comment #23)
> Question:. Why should the extend mode be "CAIRO_EXTEND_NONE"?
> Ans: As described in the cairo manual,
> http://cairographics.org/manual/cairo-cairo-pattern-t.html#cairo-extend-t :
> 
> "The default extend mode is CAIRO_EXTEND_NONE for surface patterns and CAIRO_EXTEND_PAD for gradient patterns."
> 
> In the function PlatformContextCairo::drawSurfaceToContext(), a pattern is being created from the source surface and this pattern is drawn onto the destination context. There is no gradient involved. Hence the extend mode for filling the pattern should be "CAIRO_EXTEND_NONE".
> 
> This does not add any regression.
> 
> There is already a layout-test highlighting this issue -
> fast/canvas/canvas-createPattern-fillRect-shadow.html
> 
> In this test case, without this patch, the "shadowColor"(red) mentioned in html, will be seen in the original image on canvas, wherever the image is transparent.
> With this patch, this issue is solved.
> 
> The steps used for drawing a shadow is - 
> 
> 1. Create a cairo surface/context called shadow-context and set this context to translate to shadowOffsetX and shadowOffsetY. Draw the original image on the shadow-context. 
> 
> The translation is handled in ShadowBlur::beginShadowLayer(). Drawing the original image on the context is done in drawPathShadow() in GraphicsContextCairo.cpp. 
> 
> 2. Fill the shadow-context with "shadowColor", by setting the global composite operator to be "CompositeSourceIn". This will ulitmately result in changing the color of the image in shadowContext(destination image), to shadowColor. This is done in ShadowBlur::blurAndColorShadowBuffer().
> 
> 3. Copy the shadow-context to the original canvas-context, using the global composite operator - "CompositeSourceOver".
> 
> Root cause for the issue:-
> 
> In step #2, while filling the shadow-context with "shadowColor", if we use the extend mode "CAIRO_EXTEND_PAD" in PlatformContextCairo::drawSurfaceToContext(), the original image area is also filled with the shadowColor.

That sounds reasonable for me. However, benjamin is right and the patch also lacks some tests.

[Martin Robinson]

(In reply to comment #23)
> Question:. Why should the extend mode be "CAIRO_EXTEND_NONE"?
> Ans: As described in the cairo manual,
> http://cairographics.org/manual/cairo-cairo-pattern-t.html#cairo-extend-t :
> 
> "The default extend mode is CAIRO_EXTEND_NONE for surface patterns and CAIRO_EXTEND_PAD for gradient patterns."
> 
> In the function PlatformContextCairo::drawSurfaceToContext(), a pattern is being created from the source surface and this pattern is drawn onto the destination context. There is no gradient involved. Hence the extend mode for filling the pattern should be "CAIRO_EXTEND_NONE".

Not sure I understand this part of your explanation. The Cairo documentation you point to talks about the default extend mode, but in this code we are overriding the default. We should be considering what the correct behavior of WebKit is here, not merely duplicating the default settings in Cairo. Perhaps I've misinterpreted what you said though...

[Martin Robinson]

(In reply to comment #23)

> In step #2, while filling the shadow-context with "shadowColor", if we use the extend mode "CAIRO_EXTEND_PAD" in PlatformContextCairo::drawSurfaceToContext(), the original image area is also filled with the shadowColor.

So is the issue that during shadow coloring, area outside the shadow region are incorrect filled? Perhaps we should also be clipping the CompositeSourceIn to the interesting area of the shadow-generating surface.

[Rashmi Shyamasundar]

Created attachment 197019 [details]
Patch

[Rashmi Shyamasundar]

A layout test is added in my latest patch, with more description in the Changelog.

[Rashmi Shyamasundar]

Root cause for the issue :-

Correcting the statement I made in comment - https://bugs.webkit.org/show_bug.cgi?id=108897#c23

The root cause for this issue lies in step #3, and not step#2. (Please refer https://bugs.webkit.org/show_bug.cgi?id=108897#c23 for the steps involved in drawing a shadow).

step #3: Copy the shadow-context to the original canvas-context.

This copy is achieved through the below sequence of functions :-

3a. In the funciton ShadowBlur::endShadowLayer(), context->drawImageBuffer() is called in order to copy the image from shadowContext to the original graphics context.

3b. GraphicsContext::drawImageBuffer() in turn calls ImageBuffer::draw() which in turn calls GraphicsContext::drawImage(), which in turn calls BitmapImage::draw(), which in turn calls PlatformContextCairo::drawSurfaceToContext().

3c. In the function PlatformContextCairo::drawSurfaceToContext(), a pattern is created from the source surface and the destination context is filled with this pattern.

In the step #3c, since we have created a surface pattern and not a gradient pattern, the extend mode for filling the pattern should be CAIRO_EXTEND_NONE and not CAIRO_EXTEND_PAD.

[Rashmi Shyamasundar]

The patch https://bugs.webkit.org/show_bug.cgi?id=58309 by Mr. Martin Robinson does not solve this issue.

[Dirk Schulze]

Comment on attachment 197019 [details]
Patch

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

Nearly done! Just one last snippet. Rest looks good to me.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Please add what you changed here, instead of LayoutTests/ChangeLog

> LayoutTests/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Here you should just say what you test, no implementation details.

[Dirk Schulze]

Comment on attachment 197019 [details]
Patch

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

> LayoutTests/ChangeLog:19
> +        * fast/canvas/green.png: Added.

Oh, I did not see that. There should not be a PNG. A DRT test is enough here. Remove the PNG from the patch. I wonder why you added it in the first place.

[Martin Robinson]

(In reply to comment #29)

> In the step #3c, since we have created a surface pattern and not a gradient pattern, the extend mode for filling the pattern should be CAIRO_EXTEND_NONE and not CAIRO_EXTEND_PAD.

The question I have is why the region painted (destRect) is larger than the actual shadowed region? It seems that a full fix for this would actually just ensure that destRect is the proper size, no?

[Rashmi Shyamasundar]

Created attachment 198470 [details]
Patch

[Rashmi Shyamasundar]

I see that "destRect" has the expected values(that is the shadowed region) in the function PlatformContextCairo::drawSurfaceToContext().

[Martin Robinson]

Any reason why this doesn't include any expectations updates for the original test that you mentioned (fast/canvas/canvas-createPattern-fillRect-shadow.html)? I don't see it skipped or marked as a failure and I don't see any updates to results in the patch.

[Rashmi Shyamasundar]

The purpose of the test - fast/canvas/canvas-createPattern-fillRect-shadow.html is actually to verify the shadowBlur. 

I guess the test to verify the color and size of the shadow-region and the original-image-region can be kept separate. The new test case I have added - "canvas-image-shadow.html" tests this.

Do you think I should add more tests to canvas-createPattern-fillRect-shadow.html, for verifying color and size of the shadow/original-image region?

[Martin Robinson]

(In reply to comment #37)
> The purpose of the test - fast/canvas/canvas-createPattern-fillRect-shadow.html is actually to verify the shadowBlur. 
> 
> I guess the test to verify the color and size of the shadow-region and the original-image-region can be kept separate. The new test case I have added - "canvas-image-shadow.html" tests this.
> 
> Do you think I should add more tests to canvas-createPattern-fillRect-shadow.html, for verifying color and size of the shadow/original-image region?

I'm just curious because you say in the original comment that the test (The layout-test fast/canvas/canvas-createPattern-fillRect-shadow.html is failing) is failing, but it's not skipped and you do not update the results here. I also don't see it failing on the WebKitEFL bots.

[Rashmi Shyamasundar]

The layout-test fast/canvas/canvas-createPattern-fillRect-shadow.html has pixel-checks to check if the shadow-blur is as expected. And, there is no issue with the shadow-blur. Hence the test is passing.

The original image area is not included in the pixel-checks in canvas-createPattern-fillRect-shadow.html.

The issue being fixed in this patch, is that some part of the original area also has the shadow-color instead of the original-image-color. Hence, I have added a separate test to verify the color in the original-image-area.

[Martin Robinson]

Comment on attachment 198470 [details]
Patch

Thanks! I'll land this fixing some nits in the test.

[Martin Robinson]

Committed r148923: <http://trac.webkit.org/changeset/148923>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 115020

[Zan Dobersek]

(In reply to comment #42)
> Re-opened since this is blocked by bug 115020

Rolled out due to the patch causing failures in various CSS and Canvas tests, both on EFL and GTK.
Confirmed it locally before rolling it out.

Here's the first build in which the failures occur (previous builds were failing at compilation, hence no previous results):
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r148935%20(37100)/results.html

Here's the (perhaps only partial) list of regressions:
canvas/philip/tests/2d.drawImage.9arg.destsize.html
canvas/philip/tests/2d.drawImage.floatsource.html
css2.1/20110323/border-conflict-element-001d.htm
css2.1/20110323/c541-word-sp-001.htm
css2.1/20110323/floats-001.html
css2.1/20110323/margin-collapse-clear-012.htm
css2.1/20110323/margin-collapse-clear-013.htm
css2.1/20110323/margin-collapse-clear-014.htm
css2.1/20110323/margin-collapse-clear-015.htm
fast/canvas/DrawImageSinglePixelStretch.html
fast/canvas/drawImage-with-negative-source-destination.html

Here's the dashboard results for these tests:
http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=canvas%2Fphilip%2Ftests%2F2d.drawImage.9arg.destsize.html%2Ccanvas%2Fphilip%2Ftests%2F2d.drawImage.floatsource.html%2Ccss2.1%2F20110323%2Fborder-conflict-element-001d.htm%2Ccss2.1%2F20110323%2Fc541-word-sp-001.htm%2Ccss2.1%2F20110323%2Ffloats-001.html%2Ccss2.1%2F20110323%2Fmargin-collapse-clear-012.htm%2Ccss2.1%2F20110323%2Fmargin-collapse-clear-013.htm%2Ccss2.1%2F20110323%2Fmargin-collapse-clear-014.htm%2Ccss2.1%2F20110323%2Fmargin-collapse-clear-015.htm%2Cfast%2Fcanvas%2FDrawImageSinglePixelStretch.html%2Cfast%2Fcanvas%2FdrawImage-with-negative-source-destination.html

[Rashmi Shyamasundar]

The below failures are being caused because of negative parameters of destination rect. It can be solved by appropriately them to positive parameters.

fast/canvas/drawImage-with-negative-source-destination.html
css2.1/20110323/margin-collapse-clear-012.htm
css2.1/20110323/margin-collapse-clear-013.htm
css2.1/20110323/margin-collapse-clear-014.htm
css2.1/20110323/margin-collapse-clear-015.htm

Currently in PlatformContextCairo::drawSurfaceToContext(), only the srcRect parameters are being checked for negative values. The same check has to be added for destRect as well.

The below piece of code resolves the above failures -

// We need to account for negative destination dimensions by flipping the rectangle.
    FloatRect finalDestRect = destRect;
    if (destRect.width() < 0) {
        finalDestRect.setX(destRect.x() + destRect.width());
        finalDestRect.setWidth(std::fabs(destRect.width()));
    }
    if (destRect.height() < 0) {
        finalDestRect.setY(destRect.y() + destRect.height());
        finalDestRect.setHeight(std::fabs(destRect.height()));
    }

I have attached the file PlatformContextCairo.cpp, with the above code inserted in the function drawSurfaceToContext().

[Rashmi Shyamasundar]

Created attachment 199646 [details]
PlatformContextCairo.cpp - check destRect for negative parameters

[Martin Robinson]

(In reply to comment #44)
> The below failures are being caused because of negative parameters of destination rect. It can be solved by appropriately them to positive parameters.
> 
> fast/canvas/drawImage-with-negative-source-destination.html
> css2.1/20110323/margin-collapse-clear-012.htm
> css2.1/20110323/margin-collapse-clear-013.htm
> css2.1/20110323/margin-collapse-clear-014.htm
> css2.1/20110323/margin-collapse-clear-015.htm
> 
> Currently in PlatformContextCairo::drawSurfaceToContext(), only the srcRect parameters are being checked for negative values. The same check has to be added for destRect as well.
> 

I do not understand the interaction of your change and mine. How did they combine to cause these new failures?

[Rashmi Shyamasundar]

The root cause for the below failures is the filtering algorithm(CAIRO_FILTER_BILINEAR) being used in PlatformCaontextCairo::drawSurfaceToContext().

The failures will be resolved if we use "CAIRO_FILTER_NEAREST" or "CAIRO_FILTER_FAST".

[Rashmi Shyamasundar]

Below are the failures for which I mentioned a fix in my previous comment

1.	css2.1/20110323/border-conflict-element-001d.htm
2.	css2.1/20110323/c541-word-sp-001.htm
3.	css2.1/20110323/floats-001.html
4.	canvas/philip/tests/2d.drawImage.9arg.destsize.html
5.	canvas/philip/tests/2d.drawImage.floatsource.html

[Martin Robinson]

(In reply to comment #47)
> The root cause for the below failures is the filtering algorithm(CAIRO_FILTER_BILINEAR) being used in PlatformCaontextCairo::drawSurfaceToContext().
> 
> The failures will be resolved if we use "CAIRO_FILTER_NEAREST" or "CAIRO_FILTER_FAST".

You'll need to explain why reducing the quality of the scaling filter fixes the tests. I think we want images to be scaled with high quality filters.

[Rashmi Shyamasundar]

There seems to be an issue with the bilinear filtering technique being used in cairo. The filter types "CAIRO_FILTER_GOOD", "CAIRO_FILTER_BEST" and "CAIRO_FILTER_BILINEAR" use the bilinear technique. "CAIRO_FILTER_NEAREST" and "CAIRO_FILTER_FAST" use the nearest-neighbour technique and its working fine.

I have attached a simple program - "testImageStretch.cpp", which shows the usage of "BILINEAR" and "NEAREST NEIGHBOUR" filtering techniques in cairo.

I have also attached 3 images - 
1. swatch-yellow.png - Source Image for creating the pattern
2. bilinear.png - Image stretched using BILINEAR filter
3. nearest.png - Image stretched using "nearest" filter

We can clearly see that, the border pixels in "bilinear.png" are not the same as the color in the source image - "swatch-yellow.png".

[Rashmi Shyamasundar]

Created attachment 200641 [details]
testImageStretch.cpp - Compare BILNEAR and NEAREST filters

[Rashmi Shyamasundar]

Created attachment 200643 [details]
swatch-yellow.png - Source image for creating pattern

[Rashmi Shyamasundar]

Created attachment 200644 [details]
bilinear.png - Image generated using bilinear filter

[Rashmi Shyamasundar]

Created attachment 200645 [details]
nearest.png - Image generated using nearest-neighbour filter

[Rashmi Shyamasundar]

Attached is a program - "testExtendModes.cpp", demonstrating the difference between the pattern-extend-modes - "CAIRO_EXTEND_NONE" and "CAIRO_EXTEND_PAD". This difference justifies that, we need to use "CAIRO_EXTEND_NONE" in the function PlatformContextCairo::drawSurfaceToContext().

If we use the extend mode "CAIRO_EXTEND_PAD" then, the corresponding cairo_fill() will completely fill all the cairo-rectangles, irrespective of the pattern size. If we use "CAIRO_EXTEND_NONE" then, the cairo_fill() fills only one cairo_rectangle with the pattern, preserving the pattern size; That is, the pattern is NOT repeated to fill the complete rectangle.

[Rashmi Shyamasundar]

Created attachment 200655 [details]
testExtendPad.png - Image generated using CAIRO_EXTEND_PAD

[Rashmi Shyamasundar]

Created attachment 200656 [details]
testExtendNone.png - Image generated using CAIRO_EXTEND_NONE

[Rashmi Shyamasundar]

Created attachment 200657 [details]
testExtendModes.cpp - Cairo program to test the pattern-extend-modes

[Martin Robinson]

(In reply to comment #55)
> Attached is a program - "testExtendModes.cpp", demonstrating the difference between the pattern-extend-modes - "CAIRO_EXTEND_NONE" and "CAIRO_EXTEND_PAD". This difference justifies that, we need to use "CAIRO_EXTEND_NONE" in the function PlatformContextCairo::drawSurfaceToContext().

I don't think it does really. We don't want to scale images with a low quality scaling filter. You'll either need to explain why this change does not affect scaled images on pages or explain why it doesn't decrease the quality of their scaling.

Instead of these changes, which are invasive and seem to be cascading, perhaps you should instead try to fix the target rect size? Earlier you said that the target rect size was correct, but it's clear that the rect is large enough that the padding is affecting it. Why not make it small enough such that the padding does not have any affect? Perhaps it is too difficult to calculate that size, but "It's too difficult to calculate that size, because we are only estimating" is not something I've heard yet. You only said it was the right size. I still don't understand how that could be the case.

[Rashmi Shyamasundar]

Dear Mr. Robinson, 
Thank you very much for your comments. From the discussion we have had so far, I realize that this issue can be solved in 2 ways -

1. CAIRO_EXTEND_PAD - As you said, if the rect is properly defined then, "CAIRO_EXTEND_PAD" will take care of scaling the source image to the required destination-rect-size. 

In this current shadow issue, the destination-rect is properly defined in PlatformContextCairo::drawPatternToCairoContext(). But, on this context, another rect(for drawing the original image, not the shadow) has already been defined in GraphicsContext::fillRect(). So, when cairo_fill() is called in PlatformContextCairo::drawPatternToCairoContext(), both the rects get filled with the shadow. 

Hence we need to make sure that only the shadow-rect is part of the cairo-path when we are drawing the shadow. This can be achieved with the below lines of code in PlatformContextCairo::drawPatternToCairoContext().

static void drawPatternToCairoContext(cairo_t* cr, cairo_pattern_t* pattern, const FloatRect& destRect, float alpha)
{
    cairo_translate(cr, destRect.x(), destRect.y());
    cairo_set_source(cr, pattern);
    cairo_save(cr);     //Save cairo-state 
    cairo_new_path(cr); //Clear the cairo-path
    cairo_rectangle(cr, 0, 0, destRect.width(), destRect.height());

    if (alpha < 1) {
        cairo_clip(cr);
        cairo_paint_with_alpha(cr, alpha);
    } else
        cairo_fill(cr);
    cairo_restore(cr); //Restore the cairo-state and hence the cairo-path
}

I ran all the canvas layouttests, with the above fix. Its working fine. I have not run all other layouttests. Is there a way to quickly compare the layout test results, with and without the fix?

2. CAIRO_EXTEND_NONE - In this case, we will have to depend on the cairo-matrix and hence the cairo-filters for scaling the source image to the destination-rect size. 

There seems to be an issue with the bilinear filter in cairo. Please check https://bugs.webkit.org/show_bug.cgi?id=108897#c50 . Please open the images (mentioned in comment#50), using "Image Viewer" on linux which shows the image with a checker-box background. This background will highlight the issue.

Do you think I can raise a bug on Cairo-bilinear-filter, using this sample cairo program?

[Martin Robinson]

(In reply to comment #60)

> static void drawPatternToCairoContext(cairo_t* cr, cairo_pattern_t* pattern, const FloatRect& destRect, float alpha)
> {
>     cairo_translate(cr, destRect.x(), destRect.y());
>     cairo_set_source(cr, pattern);
>     cairo_save(cr);     //Save cairo-state 
>     cairo_new_path(cr); //Clear the cairo-path
>     cairo_rectangle(cr, 0, 0, destRect.width(), destRect.height());
> 
>     if (alpha < 1) {
>         cairo_clip(cr);
>         cairo_paint_with_alpha(cr, alpha);
>     } else
>         cairo_fill(cr);
>     cairo_restore(cr); //Restore the cairo-state and hence the cairo-path
> }

This seems like the real bug now. :) I do not believe the path is saved and restored when you call cairo_save and cairo_restore. Where is the original part of the path defined?

> I ran all the canvas layouttests, with the above fix. Its working fine. I have not run all other layouttests. Is there a way to quickly compare the layout test results, with and without the fix?

You're going to need to be very careful, because I do not think the original path is on the context when you call cairo_restore. Try to figure out what sets the path.

> There seems to be an issue with the bilinear filter in cairo. Please check https://bugs.webkit.org/show_bug.cgi?id=108897#c50 . Please open the images (mentioned in comment#50), using "Image Viewer" on linux which shows the image with a checker-box background. This background will highlight the issue.

It looks like a bug because the left side of the image seems to be sampling outside the original swatch. That's what you are trying to direct me to look at, right? If this happens with the bilinear filter and not the nearest neighbor filter, it seems like a bug in Cairo for sure.

[Rashmi Shyamasundar]

Created attachment 201176 [details]
Patch

[Rashmi Shyamasundar]

Dear Mr. Robinson, 
As discussed earlier, the path on cairoContext should be cleared before copying the shadow from shadowContext to the original cairoContext. This can be done in drawPathShadow() defined in GraphicsContextCairo.cpp . Please review this patch. 

P.S -

The path on original cairoContext is getting added in the below funciton.

void GraphicsContext::fillRect(const FloatRect& rect)
{
    if (paintingDisabled())
        return;

    cairo_t* cr = platformContext()->cr();
    cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height());
    shadowAndFillCurrentCairoPath(this);
}

[Build Bot]

Comment on attachment 201176 [details]
Patch

Attachment 201176 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/298253

New failing tests:
editing/style/5065910.html

[Build Bot]

Created attachment 201177 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Rashmi Shyamasundar]

Dear Mr. Martin Robinson,

According to the log from build bot, the below tests are failing on mac - 
1. editing/execCommand/copy-without-selection.html
2. editing/pasteboard/pasting-empty-html-falls-back-to-text.html
3. editing/pasteboard/copy-two-pasteboard-types-both-work.html
3. editing/style/5065910.html

But, these tests are passing when I run them on my mac, after building webkit, with my patch.

Can I re-submit the same patch to re-check the failures?

[Martin Robinson]

(In reply to comment #66)

> Can I re-submit the same patch to re-check the failures?

Sorry. I'll land this one tomorrow.

[Martin Robinson]

Committed r150370: <http://trac.webkit.org/changeset/150370>