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.
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?
(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?
(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.
Created attachment 193792[details]
After cairo_append_path in drawPathShadow() in GraphicsContextCiaro.cpp
Image written by adding a cairo_fill(), for debugging
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.
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);"
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 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 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.
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.
(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.
(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...
(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.
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 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.
(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?
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.
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?
(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.
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.
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().
(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?
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".
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
(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.
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".
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.
(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.
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?
(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.
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);
}
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
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?
2013-02-04 20:59 PST, Rashmi Shyamasundar
2013-02-04 21:01 PST, Rashmi Shyamasundar
2013-02-06 02:18 PST, Rashmi Shyamasundar
2013-02-06 02:19 PST, Rashmi Shyamasundar
2013-03-19 04:30 PDT, Rashmi Shyamasundar
2013-03-19 04:41 PDT, Rashmi Shyamasundar
2013-03-19 04:49 PDT, Rashmi Shyamasundar
2013-03-19 04:51 PDT, Rashmi Shyamasundar
2013-03-19 05:51 PDT, Rashmi Shyamasundar
2013-04-01 01:47 PDT, Rashmi Shyamasundar
2013-04-09 03:21 PDT, Rashmi Shyamasundar
2013-04-16 23:23 PDT, Rashmi Shyamasundar
2013-04-25 02:52 PDT, Rashmi Shyamasundar
2013-05-06 01:14 PDT, Rashmi Shyamasundar
2013-05-06 01:22 PDT, Rashmi Shyamasundar
2013-05-06 01:24 PDT, Rashmi Shyamasundar
2013-05-06 01:25 PDT, Rashmi Shyamasundar
2013-05-06 04:11 PDT, Rashmi Shyamasundar
2013-05-06 04:13 PDT, Rashmi Shyamasundar
2013-05-06 04:15 PDT, Rashmi Shyamasundar
2013-05-09 02:46 PDT, Rashmi Shyamasundar
buildbot: commit-queue-
2013-05-09 03:11 PDT, Build Bot