Bug 108897 - [Cairo] Canvas-shadow behavior is not being as expected
Summary: [Cairo] Canvas-shadow behavior is not being as expected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 115020
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-04 20:59 PST by Rashmi Shyamasundar
Modified: 2013-05-20 09:39 PDT (History)
14 users (show)

See Also:


Attachments
Actual and expected Outputs (63.37 KB, application/octet-stream)
2013-02-04 20:59 PST, Rashmi Shyamasundar
no flags Details
Smaller test-case to reproduce the canvas-shadow issue (23.64 KB, application/octet-stream)
2013-02-04 21:01 PST, Rashmi Shyamasundar
no flags Details
Actual and expected Outputs in .zip format (63.97 KB, application/x-zip-compressed)
2013-02-06 02:18 PST, Rashmi Shyamasundar
no flags Details
Smaller test-case to reproduce the canvas-shadow issue in .zip format (23.88 KB, application/x-zip-compressed)
2013-02-06 02:19 PST, Rashmi Shyamasundar
no flags Details
Image after cairo_fill in GraphicsContextCairo::drawPathShadow (14.76 KB, image/png)
2013-03-19 04:30 PDT, Rashmi Shyamasundar
no flags Details
Image written by adding a cairo_fill(), for debugging (6.03 KB, image/png)
2013-03-19 04:41 PDT, Rashmi Shyamasundar
no flags Details
After cairo_append_path in drawPathShadow() in GraphicsContextCiaro.cpp (6.11 KB, image/png)
2013-03-19 04:49 PDT, Rashmi Shyamasundar
no flags Details
"4_image_after_fill" - Image after cairo_fill in shadowAndFillCurrentCairoPath() in GraphicsContextCairo.cpp (6.11 KB, image/png)
2013-03-19 04:51 PDT, Rashmi Shyamasundar
no flags Details
GraphicsContextCairo_fill.cpp (37.07 KB, text/plain)
2013-03-19 05:51 PDT, Rashmi Shyamasundar
no flags Details
Patch (1.81 KB, patch)
2013-04-01 01:47 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2013-04-09 03:21 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2013-04-16 23:23 PDT, Rashmi Shyamasundar
no flags Details | Formatted Diff | Diff
PlatformContextCairo.cpp - check destRect for negative parameters (12.84 KB, text/plain)
2013-04-25 02:52 PDT, Rashmi Shyamasundar
no flags Details
testImageStretch.cpp - Compare BILNEAR and NEAREST filters (1.54 KB, text/plain)
2013-05-06 01:14 PDT, Rashmi Shyamasundar
no flags Details
swatch-yellow.png - Source image for creating pattern (84 bytes, image/png)
2013-05-06 01:22 PDT, Rashmi Shyamasundar
no flags Details
bilinear.png - Image generated using bilinear filter (5.82 KB, image/png)
2013-05-06 01:24 PDT, Rashmi Shyamasundar
no flags Details
nearest.png - Image generated using nearest-neighbour filter (5.22 KB, image/png)
2013-05-06 01:25 PDT, Rashmi Shyamasundar
no flags Details
testExtendPad.png - Image generated using CAIRO_EXTEND_PAD (1.91 KB, image/png)
2013-05-06 04:11 PDT, Rashmi Shyamasundar
no flags Details
testExtendNone.png - Image generated using CAIRO_EXTEND_NONE (1.51 KB, image/png)
2013-05-06 04:13 PDT, Rashmi Shyamasundar
no flags Details
testExtendModes.cpp - Cairo program to test the pattern-extend-modes (1.38 KB, image/png)
2013-05-06 04:15 PDT, Rashmi Shyamasundar
no flags Details
Patch (6.20 KB, patch)
2013-05-09 02:46 PDT, Rashmi Shyamasundar
mrobinson: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (595.75 KB, application/zip)
2013-05-09 03:11 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rashmi Shyamasundar 2013-02-04 20:59:11 PST
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.
Comment 1 Rashmi Shyamasundar 2013-02-04 21:01:42 PST
Created attachment 186536 [details]
Smaller test-case to reproduce the canvas-shadow issue
Comment 2 Alexey Proskuryakov 2013-02-05 10:48:07 PST
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?
Comment 3 Rashmi Shyamasundar 2013-02-06 02:11:34 PST
This issue is seen on webkit-efl port.
Comment 4 Rashmi Shyamasundar 2013-02-06 02:18:38 PST
Created attachment 186799 [details]
Actual and expected Outputs in .zip format
Comment 5 Rashmi Shyamasundar 2013-02-06 02:19:21 PST
Created attachment 186800 [details]
Smaller test-case to reproduce the canvas-shadow issue in .zip format
Comment 6 Alexey Proskuryakov 2013-02-06 11:23:15 PST
Thank you!

Mac port appears to be unaffected.
Comment 7 Rashmi Shyamasundar 2013-02-13 23:27:00 PST
The below link can be used to run the test case on any port - 
http://jsfiddle.net/wE8cr/
Comment 8 Dirk Schulze 2013-02-14 16:36:03 PST
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?
Comment 9 Rashmi Shyamasundar 2013-02-14 23:34:50 PST
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.
Comment 10 Philip Rogers 2013-02-15 20:12:55 PST
(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?
Comment 11 Dirk Schulze 2013-02-15 20:59:16 PST
(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.
Comment 12 Rashmi Shyamasundar 2013-03-19 04:30:33 PDT
Created attachment 193787 [details]
Image after cairo_fill in GraphicsContextCairo::drawPathShadow
Comment 13 Rashmi Shyamasundar 2013-03-19 04:41:21 PDT
Created attachment 193790 [details]
Image written by adding a cairo_fill(), for debugging
Comment 14 Rashmi Shyamasundar 2013-03-19 04:49:12 PDT
Created attachment 193792 [details]
After cairo_append_path in drawPathShadow() in GraphicsContextCiaro.cpp

Image written by adding a cairo_fill(), for debugging
Comment 15 Rashmi Shyamasundar 2013-03-19 04:51:40 PDT
Created attachment 193793 [details]
"4_image_after_fill" - Image after cairo_fill in shadowAndFillCurrentCairoPath() in GraphicsContextCairo.cpp
Comment 16 Rashmi Shyamasundar 2013-03-19 05:51:31 PDT
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
Comment 17 Rashmi Shyamasundar 2013-03-20 03:30:36 PDT
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.
Comment 18 Rashmi Shyamasundar 2013-04-01 00:13:47 PDT
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);"
Comment 19 Rashmi Shyamasundar 2013-04-01 01:47:32 PDT
Created attachment 195942 [details]
Patch
Comment 20 Martin Robinson 2013-04-01 08:17:13 PDT
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?
Comment 21 Dirk Schulze 2013-04-01 11:46:30 PDT
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?
Comment 22 Benjamin Poulain 2013-04-01 12:41:45 PDT
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.
Comment 23 Rashmi Shyamasundar 2013-04-01 22:57:59 PDT
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.
Comment 24 Dirk Schulze 2013-04-03 10:57:15 PDT
(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.
Comment 25 Martin Robinson 2013-04-03 11:06:34 PDT
(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...
Comment 26 Martin Robinson 2013-04-03 11:14:38 PDT
(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.
Comment 27 Rashmi Shyamasundar 2013-04-09 03:21:07 PDT
Created attachment 197019 [details]
Patch
Comment 28 Rashmi Shyamasundar 2013-04-09 06:19:25 PDT
A layout test is added in my latest patch, with more description in the Changelog.
Comment 29 Rashmi Shyamasundar 2013-04-09 23:18:58 PDT
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.
Comment 30 Rashmi Shyamasundar 2013-04-10 01:13:25 PDT
The patch https://bugs.webkit.org/show_bug.cgi?id=58309 by Mr. Martin Robinson does not solve this issue.
Comment 31 Dirk Schulze 2013-04-10 16:45:20 PDT
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.
Comment 32 Dirk Schulze 2013-04-10 16:46:50 PDT
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.
Comment 33 Martin Robinson 2013-04-10 16:51:39 PDT
(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?
Comment 34 Rashmi Shyamasundar 2013-04-16 23:23:35 PDT
Created attachment 198470 [details]
Patch
Comment 35 Rashmi Shyamasundar 2013-04-16 23:28:54 PDT
I see that "destRect" has the expected values(that is the shadowed region) in the function PlatformContextCairo::drawSurfaceToContext().
Comment 36 Martin Robinson 2013-04-18 16:10:49 PDT
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.
Comment 37 Rashmi Shyamasundar 2013-04-19 03:58:15 PDT
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?
Comment 38 Martin Robinson 2013-04-19 08:14:15 PDT
(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.
Comment 39 Rashmi Shyamasundar 2013-04-22 01:34:56 PDT
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.
Comment 40 Martin Robinson 2013-04-22 15:21:46 PDT
Comment on attachment 198470 [details]
Patch

Thanks! I'll land this fixing some nits in the test.
Comment 41 Martin Robinson 2013-04-22 16:05:07 PDT
Committed r148923: <http://trac.webkit.org/changeset/148923>
Comment 42 WebKit Commit Bot 2013-04-23 00:44:45 PDT
Re-opened since this is blocked by bug 115020
Comment 43 Zan Dobersek 2013-04-23 00:52:12 PDT
(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
Comment 44 Rashmi Shyamasundar 2013-04-25 02:49:53 PDT
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().
Comment 45 Rashmi Shyamasundar 2013-04-25 02:52:45 PDT
Created attachment 199646 [details]
PlatformContextCairo.cpp - check destRect for negative parameters
Comment 46 Martin Robinson 2013-04-25 09:23:46 PDT
(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?
Comment 47 Rashmi Shyamasundar 2013-05-03 03:44:56 PDT
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".
Comment 48 Rashmi Shyamasundar 2013-05-03 03:46:56 PDT
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
Comment 49 Martin Robinson 2013-05-03 06:26:02 PDT
(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.
Comment 50 Rashmi Shyamasundar 2013-05-06 01:10:21 PDT
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".
Comment 51 Rashmi Shyamasundar 2013-05-06 01:14:06 PDT
Created attachment 200641 [details]
testImageStretch.cpp - Compare BILNEAR and NEAREST filters
Comment 52 Rashmi Shyamasundar 2013-05-06 01:22:38 PDT
Created attachment 200643 [details]
swatch-yellow.png - Source image for creating pattern
Comment 53 Rashmi Shyamasundar 2013-05-06 01:24:05 PDT
Created attachment 200644 [details]
bilinear.png - Image generated using bilinear filter
Comment 54 Rashmi Shyamasundar 2013-05-06 01:25:24 PDT
Created attachment 200645 [details]
nearest.png - Image generated using nearest-neighbour filter
Comment 55 Rashmi Shyamasundar 2013-05-06 04:09:45 PDT
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.
Comment 56 Rashmi Shyamasundar 2013-05-06 04:11:44 PDT
Created attachment 200655 [details]
testExtendPad.png - Image generated using CAIRO_EXTEND_PAD
Comment 57 Rashmi Shyamasundar 2013-05-06 04:13:14 PDT
Created attachment 200656 [details]
testExtendNone.png - Image generated using CAIRO_EXTEND_NONE
Comment 58 Rashmi Shyamasundar 2013-05-06 04:15:02 PDT
Created attachment 200657 [details]
testExtendModes.cpp - Cairo program to test the pattern-extend-modes
Comment 59 Martin Robinson 2013-05-06 08:13:56 PDT
(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.
Comment 60 Rashmi Shyamasundar 2013-05-07 05:55:30 PDT
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?
Comment 61 Martin Robinson 2013-05-07 08:15:39 PDT
(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.
Comment 62 Rashmi Shyamasundar 2013-05-09 02:46:26 PDT
Created attachment 201176 [details]
Patch
Comment 63 Rashmi Shyamasundar 2013-05-09 02:56:11 PDT
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);
}
Comment 64 Build Bot 2013-05-09 03:11:25 PDT
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
Comment 65 Build Bot 2013-05-09 03:11:29 PDT
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
Comment 66 Rashmi Shyamasundar 2013-05-16 05:00:18 PDT
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?
Comment 67 Martin Robinson 2013-05-16 06:20:50 PDT
(In reply to comment #66)

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

Sorry. I'll land this one tomorrow.
Comment 68 Martin Robinson 2013-05-20 09:39:04 PDT
Committed r150370: <http://trac.webkit.org/changeset/150370>