Bug 23526

Created attachment 27009 [details] example code This is an example, how a platform independent clipToImageBuffer could look like. We give clipToImageBuffer to ImageBuffers, one for the mask, the other is the masked object. The replacment is not complete. Gradients are still a bit bugy and no work has been done on -webkit-background-clip. But it demonstrates that it is possible with this design.

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

Created attachment 27215 [details] clipToImageBuffer via CompositeSourceIn Added support for clipToImageBuffer by using the composite operator CompositeSourceIn. This is a easy way to support it, with a minumum of code changes to WebKit. We need a new Layer for a composite operator, to ensure that we don't influence the drawing before and after the masking. Therefore I added a new method closeClipToImageBuffer.

Comment on attachment 27215 [details] clipToImageBuffer via CompositeSourceIn we need an implementation without closeClipToImageBuffer

Created attachment 27361 [details] clipToImageBuffer for cairo An clipToImageBuffer implemetation for Cairo. I'm using composite operators here, because on cairo we need the masked object before the mask. It doesn't work the other way arround like on CG.

Comment on attachment 27361 [details] clipToImageBuffer for cairo r=me -- i've tried to make sure i understand what this patch is doing, as it doesn't appear there are any active cairo/canvas/gtk reviewers around :-/

Comment on attachment 27361 [details] clipToImageBuffer for cairo I think we can make this more clear. The problem here is that in CG you can set a mask image on the context, and then CG will automatically apply it when necessary. In Cairo, masking is an immediate operation. GraphicsContext expects masking to function lazily like CG does. So we have to fix GraphicsContextCairo to save off the mask image, and apply the mask at the last moment possible (when popping the context). I think we could do this clearer by saving the mask image on the GraphicsContextState and applying it during m_data->restore(); You could write a new applyMaskImage() function on m_data which does the actual apply using the stored image. the mask image would not inherit between graphics context states. Does that seem sane?

(In reply to comment #7) > I think we could do this clearer by saving the mask image on the > GraphicsContextState and applying it during m_data->restore(); > You could write a new applyMaskImage() function on m_data which does the actual > apply using the stored image. the mask image would not inherit between > graphics context states. > > Does that seem sane? A new GraphicsContextState is the copy of a previous one, so every content gets copied too. This is impossible for ImageBuffers. So we can't store ImageBuffers into the state, but even references will be copied. That means we still need something that counts the save/restores as well as the number of clipToImageBuffer calls. That is what the current patch is doing (Vector of int). With pushing the context I move the store operation out of webkit into cairo. That makes it possibly clearer. applyMaskImage sounds reasonable, but it doesn't change anything on the general problem. It would just move the code out of restorePlatformState.

The original email from webkit-dev: "I'm trying to implement this CSS value on the Cairo backend and I'm stuck with a problem regarding the Porter-Duff composition operators (why is CAIRO_OPERATOR_DEST_IN unbound?). I've made the following changes to reach this point: 1. I implemented a version of GraphicsContext::clipToImageBuffer starting from this patch: https://bugs.webkit.org/show_bug.cgi?id=23526 , but saving the image mask and the masking rect on a stack and pushing only a group for the masked content. 2. In GraphicsContext::restorePlatformState when the save count is 0 for the current mask, I recover the masked content as a pattern, I clip it using the mask from the stack (I used a new group and operator CAIRO_OPERATOR_IN) and then I should apply the result on the destination surface using CAIRO_OPERATOR_DEST_IN, but bounded to the current mask. The problem here is that CAIRO_OPERATOR_DEST_IN is unbounded so it doesn't take into account the mask, thus affecting all the surface. 3. I modify RenderBoxModelObject::paintFillLayerExtended so that when drawing the background image to use the bgLayer->composite() operator if the clip is TextFillBox. Using CompositeSourceIn can't work because clipToImageBuffer started a new group which uses a different surface for drawing (so there is no destination text to composite with). This way the composition will be done in restorePlatformState. I know this is ugly and that it's highly unlikely to work in this form with SVG masking. For now I just need a hook to make this work. Do you have any other suggestions to make this CSS value work with Cairo?" This is the rendering path I discovered for "-webkit-mask-clip: text" and why it doesn't work with this Cairo patch: 1. The text is rendered on the destination surface. 2. In PaintPhaseMask the filling is achieved using RenderBoxModelObject::paintFillLayerExtended with bgLayer->clip() == TextFillBox. The text is re-rendered in an image to be used a mask for the background. By calling clipToImageBuffer this image is painted on a new group and another one is also pushed for the masked image. 3. When drawing the background, the composite operator used is an IN kind. This operator needs to have the text rendered on the destination surface. By pushing a new group in the previous step, the destination surface becomes empty so the IN operator can't work properly. In the end the background is not painted. I have attached an example. The result should be green text with white gradient stripes (masked by that .png) on red background. Andrei.

Created attachment 59549 [details] -webkit-mask-clip: text fails with this patch

(In reply to comment #10) > Created an attachment (id=59549) [details] > -webkit-mask-clip: text fails with this patch This doesn't pass on chromium either, but IIRC they used the same idea of the patch above. If you already get it managed to save the mask image and the mask rect, why don't you use cairo_mask_surface()? This should solve all ugly problems with setting composite operators!

There are 3 render elements involved: 1. The rendered text 2. The text mask 3. The image mask The paint effect should be: the rendered text is displayed only under the image mask and only under the text mask. What is under the text mask and not under the image mask must be "clear" (as you can see with Safari, the background for the stripes is white, not red). I don't know how to achieve the "clear" effect without using composite operators.

(In reply to comment #12) > There are 3 render elements involved: > 1. The rendered text > 2. The text mask > 3. The image mask > > The paint effect should be: the rendered text is displayed only under the image mask and only under the text mask. What is under the text mask and not under the image mask must be "clear" (as you can see with Safari, the background for the stripes is white, not red). > > I don't know how to achieve the "clear" effect without using composite operators. Thats a bit silly. I don't have Safari here, so I can't test it on my own. But if I apply a clip mask to an object, it should mask the object with all it's content, including the background. Why should the background get removed? Or if we see the object without it's background, the background in your example should be solid red and and the text would be "filled" with a red-green-red gradient.

(In reply to comment #13) > Thats a bit silly. I don't have Safari here, so I can't test it on my own. But if I apply a clip mask to an object, it should mask the object with all it's content, including the background. Why should the background get removed? Or if we see the object without it's background, the background in your example should be solid red and and the text would be "filled" with a red-green-red gradient. Well... on Safari the stripes are white, not red :). This is because the red background is applied first and then the IN operator (the "op" parameter of RenderBoxModelObject::paintFillLayerExtended) I talked about removes some of the text with respect to the text mask. The composition operation doesn't "know" there is a red background and just keeps some of the content with respect to the mask image (the gradient box in our case) and removes the other content (the gradient stripes).

Is it possible, that you can apply a screenshot of Safaris behavior please?

Created attachment 59619 [details] How Safari renders the example As you can see the "masked" content is white because of the composite operator used.

(In reply to comment #16) > Created an attachment (id=59619) [details] > How Safari renders the example > > As you can see the "masked" content is white because of the composite operator used. Thank you for uploading the image. After reading the blog-post, the result is expected. I don't think, that any other platform than CG is able to render it with the current RenderBoxModelObject::paintFillLayerExtended. So we might think about changing the order of rendering somehow, wihtout breaking CG's output :-/

(In reply to comment #17) Checked Skia and Qt. They realy have the same Problem. So we might need to change the concept, not only of RenderBoxModelObject::paintFillLayerExtended, but also of clipToImageBuffer. All platforms use the same hack, similar to the patch of above. We should open a new bug to track this problem, since the issue is not limited to Cairo.

I might talk rubbish... but I think that CAIRO_OPERATOR_DEST_IN may be the solution in cairo's case (I guess Skia has something equivalent). As described on this page http://cairographics.org/operators/ we can use the mask image (the gradient box in example) to select the areas from the text that remain unchanged and remove the other parts. The only problem is that we need this operator to be "bounded", to respect the text mask and not affect the entire surface. I asked on the Cairo mailgroup about why this operator is not bounded and the reply was pretty straightforward "Mostly due to an accident of definitions inherited from XRender I think.". Maybe there is a way to push a bounded version of this operator in Cairo.

(In reply to comment #19) > I might talk rubbish... but I think that CAIRO_OPERATOR_DEST_IN may be the solution in cairo's case (I guess Skia has something equivalent). > As described on this page http://cairographics.org/operators/ we can use the mask image (the gradient box in example) to select the areas from the text that remain unchanged and remove the other parts. The only problem is that we need this operator to be "bounded", to respect the text mask and not affect the entire surface. > I asked on the Cairo mailgroup about why this operator is not bounded and the reply was pretty straightforward "Mostly due to an accident of definitions inherited from XRender I think.". Maybe there is a way to push a bounded version of this operator in Cairo. This should be possible by clipping, but you'll need the boundaries of the text for it.

(In reply to comment #20) I thought about this also and I don't think it's a solution. We need to limit the clip path only to the text. From what I read in documentation this is not achievable in Cairo.

(In reply to comment #21) > (In reply to comment #20) > I thought about this also and I don't think it's a solution. We need to limit the clip path only to the text. From what I read in documentation this is not achievable in Cairo. Oh, you don't mean the boundingBox, but something like this?: http://cairographics.org/manual/cairo-paths.html#cairo-glyph-path

(In reply to comment #22) Ooooh... I skipped that section :). You are right, that function could be what I need to fix this issue using DEST_IN operator. These days I'll try to write a workaround using it and I'll post the results.

Created attachment 83097 [details] Patch

I've uploaded a completed patch for this issue. It does two things: 1. Instead of pushing two groups to emulate the image clip, it pushes one (to isolate the mask) and then calls cairo_mask_surface during restorePlatformState. 2. Cairo does not composite with the previous group when you push a group. To deal with this problem, I blit the contents of the previous group onto the new group after pushing it. This produces the correct rendering with Andrei's test case.

(In reply to comment #25) > I've uploaded a completed patch for this issue. It does two things: > > 1. Instead of pushing two groups to emulate the image clip, it pushes one (to isolate the mask) and then calls cairo_mask_surface during restorePlatformState. > > 2. Cairo does not composite with the previous group when you push a group. To deal with this problem, I blit the contents of the previous group onto the new group after pushing it. This produces the correct rendering with Andrei's test case. Did you run pixel tests on LayoutTests/svg/clip-path/? Or even better LayoutTests/svg/ in general?

(In reply to comment #26) > (In reply to comment #25) > > I've uploaded a completed patch for this issue. It does two things: > > > > 1. Instead of pushing two groups to emulate the image clip, it pushes one (to isolate the mask) and then calls cairo_mask_surface during restorePlatformState. > > > > 2. Cairo does not composite with the previous group when you push a group. To deal with this problem, I blit the contents of the previous group onto the new group after pushing it. This produces the correct rendering with Andrei's test case. > > Did you run pixel tests on LayoutTests/svg/clip-path/? Or even better LayoutTests/svg/ in general? Oops. Forget this comment. Saw the results in your patch.

Comment on attachment 83097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83097&action=review > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1193 > + RefPtr<cairo_surface_t> currentContents(adoptRef(cairo_surface_create_similar(currentTarget, > + CAIRO_CONTENT_COLOR_ALPHA, > + rect.width(), rect.height()))); > + RefPtr<cairo_t> currentContentsContext(adoptRef(cairo_create(currentContents.get()))); > + cairo_surface_flush(currentTarget); > + cairo_set_source_surface(currentContentsContext.get(), currentTarget, -rect.x(), -rect.y()); > + cairo_rectangle(currentContentsContext.get(), 0, 0, rect.width(), rect.height()); > + cairo_fill(currentContentsContext.get()); I think this can be done with a cairo subsurface, all this block could be just: RefPtr<cairo_surface_t> currentContents(adoptRef(cairo_surface_create_for_rectangle(cairo_get_target(cr), rect.x(), rect.y(), rect.width(), rect.height())));

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1185 > + cairo_surface_t* currentTarget = cairo_get_target(cr); This should be cairo_get_group_target(cr); cairo_get_target() will always reference the original surface that was passed to cairo_create(), not the one that's currently painted to. >> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1193 >> + cairo_fill(currentContentsContext.get()); > > I think this can be done with a cairo subsurface, all this block could be just: > RefPtr<cairo_surface_t> currentContents(adoptRef(cairo_surface_create_for_rectangle(cairo_get_target(cr), > rect.x(), rect.y(), > rect.width(), rect.height()))); If that block was meant to get around the problem with self-copies, then no, this cannot be done with subsurfaces, because subsurfaces are just a bit of shim that still proxies all calls to the original surface. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1199 > + cairo_set_source_surface(cr, currentContents.get(), rect.x(), rect.y()); You could just do cairo_set_source_surface(cr, currentTarget, rect.x(), rect.y()); here and avoid the whole currentContents surface, unless I'M not understanding why you do the currentContents thing.

(In reply to comment #29) > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1185 > > + cairo_surface_t* currentTarget = cairo_get_target(cr); > > This should be cairo_get_group_target(cr); > cairo_get_target() will always reference the original surface that was passed to cairo_create(), not the one that's currently painted to. > > >> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1193 > >> + cairo_fill(currentContentsContext.get()); > > > > I think this can be done with a cairo subsurface, all this block could be just: > > RefPtr<cairo_surface_t> currentContents(adoptRef(cairo_surface_create_for_rectangle(cairo_get_target(cr), > > rect.x(), rect.y(), > > rect.width(), rect.height()))); > > If that block was meant to get around the problem with self-copies, then no, this cannot be done with subsurfaces, because subsurfaces are just a bit of shim that still proxies all calls to the original surface. In this case the subsurface it's only used as a source, so it shouldn't affect the original surface, just to avoid having to create a new surface and fill with current contents. > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1199 > > + cairo_set_source_surface(cr, currentContents.get(), rect.x(), rect.y()); > > You could just do cairo_set_source_surface(cr, currentTarget, rect.x(), rect.y()); here and avoid the whole currentContents surface, unless I'M not understanding why you do the currentContents thing.

(In reply to comment #30) > In this case the subsurface it's only used as a source, so it shouldn't affect the original surface, just to avoid having to create a new surface and fill with current contents. > But why not just use the original surface for that?

(In reply to comment #29) > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1185 > > + cairo_surface_t* currentTarget = cairo_get_target(cr); > > This should be cairo_get_group_target(cr); > cairo_get_target() will always reference the original surface that was passed to cairo_create(), not the one that's currently painted to. I'm not sure why, but using cairo_get_group_target(cr) breaks the example attached in comment #10. > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1199 > > + cairo_set_source_surface(cr, currentContents.get(), rect.x(), rect.y()); > > You could just do cairo_set_source_surface(cr, currentTarget, rect.x(), rect.y()); here and avoid the whole currentContents surface, unless I'M not understanding why you do the currentContents thing. indeed, we can use the current target here.

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

Created attachment 83492 [details] Updated patch that uses avoids creation of new surface

(In reply to comment #34) > Created an attachment (id=83492) [details] > Updated patch that uses avoids creation of new surface Thanks for the comments! I've updated the patch to just copy from the original surface. I guess originally I wasn't sure if this was valid, but apparently it is. :)

Comment on attachment 83492 [details] Updated patch that uses avoids creation of new surface View in context: https://bugs.webkit.org/attachment.cgi?id=83492&action=review Looks great to me, I'll leave some notes, as I'm not yet certain whether it's r+, as I'm not too familiar with the Cairo port. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:240 > + if (maskInformation.valid()) { s/valid/isValid/ > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:255 > + cairo_restore(m_data->cr); > + m_data->restore(); Why do you need, to do anything if maskInformation was not valid at this point? If no mask is used at all, you're now doing one cairo_restore/restore more, is that desired/needed? > Source/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:59 > + bool valid() const { return m_maskSurface; } s/valid/isValid/ > Source/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:125 > ContextShadow shadow; > Vector<ContextShadow> shadowStack; > + Vector<ImageMaskInformation> m_maskImageStack; This is inconsistent, either make all of them private (I think this is out of scope for this bug), or just name it maskImageStack as well, and put a FIXME: Make these private. above the ContextShadow line.

Comment on attachment 83492 [details] Updated patch that uses avoids creation of new surface This is so exciting!

Btw. Can we rename the bug report to "[CAIRO] clipToImageBuffer implementation for Cairo ports"? Since the original idea of the bug was to redesign clipToImageBuffer so that all platforms could use it in a native way for the certain platform. All platforms (beside the cairo ports) already have work arounds, so bug title is no longer valid.

Comment on attachment 83492 [details] Updated patch that uses avoids creation of new surface View in context: https://bugs.webkit.org/attachment.cgi?id=83492&action=review > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1209 > + cairo_t* cr = m_data->cr; > + cairo_surface_t* currentTarget = cairo_get_target(cr); > + cairo_surface_flush(currentTarget); > + > + // Pushing a new group ensures that only things painted after this point are clipped. > + cairo_push_group(cr); > + cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE); > + > + cairo_set_source_surface(cr, currentTarget, 0, 0); > + cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height()); > + cairo_fill(cr); Is this faster than cairo-mask-surface? (http://cairographics.org/manual/cairo-cairo-t.html#cairo-mask-surface)

(In reply to comment #38) > Btw. Can we rename the bug report to "[CAIRO] clipToImageBuffer implementation for Cairo ports"? Oops, forgot that clipToImageBuffer was replaced. So maybe "[CAIRO] Support ImageBuffers clip operation on all Cairo ports"?

Created attachment 83662 [details] Patch with reviwer suggestions

(In reply to comment #36) Thanks for the comments! > (From update of attachment 83492 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83492&action=review > > Looks great to me, I'll leave some notes, as I'm not yet certain whether it's r+, as I'm not too familiar with the Cairo port. > > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:240 > > + if (maskInformation.valid()) { > > s/valid/isValid/ Done! > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:255 > > + cairo_restore(m_data->cr); > > + m_data->restore(); > > Why do you need, to do anything if maskInformation was not valid at this point? If no mask is used at all, you're now doing one cairo_restore/restore more, is that desired/needed? I just moved these two lines from the top of the method. We still need to restore the platform state even if no masking operation was started after the call to savePlatformState. restorePlatformState is just the most reasonable time to pop the group and apply the mask since we know this is where it will end. It's still a distinct operation from the save/restore pair though. > > > Source/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:59 > > + bool valid() const { return m_maskSurface; } > > s/valid/isValid/ Done! > > Source/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:125 > > ContextShadow shadow; > > Vector<ContextShadow> shadowStack; > > + Vector<ImageMaskInformation> m_maskImageStack; > > This is inconsistent, either make all of them private (I think this is out of scope for this bug), or just name it maskImageStack as well, and put a FIXME: Make these private. above the ContextShadow line. Changed this to be simply maskImageStack. Some of the members of this class use the m_namingScheme and some don't. You're right that another patch should address that. I've opened https://bugs.webkit.org/show_bug.cgi?id=55150.

(In reply to comment #39) Thanks for the comments. :) > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1209 > + cairo_t* cr = m_data->cr; > + cairo_surface_t* currentTarget = cairo_get_target(cr); > + cairo_surface_flush(currentTarget); > + > + // Pushing a new group ensures that only things painted after this point are clipped. > + cairo_push_group(cr); > + cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE); > + > + cairo_set_source_surface(cr, currentTarget, 0, 0); > + cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height()); > + cairo_fill(cr); Is this faster than cairo-mask-surface? (http://cairographics.org/manual/cairo-cairo-t.html#cairo-mask-surface) Since cairo_mask_surface is immediate it cannot affect anything that's painted later. I use cairo_mask_surface in platformRestoreContext to mask the group, but if I had not pushed a group here, it would mask everything that was painted before pushMaskSurface and everything after. (In reply to comment #40) > > Btw. Can we rename the bug report to "[CAIRO] clipToImageBuffer implementation for Cairo ports"? > Oops, forgot that clipToImageBuffer was replaced. So maybe "[CAIRO] Support ImageBuffers clip operation on all Cairo ports"? Done!

Comment on attachment 83662 [details] Patch with reviwer suggestions Looks great, r=me.

Committed r79592: <http://trac.webkit.org/changeset/79592>

http://trac.webkit.org/changeset/79592 might have broken GTK Linux 32-bit Release

(In reply to comment #46) > http://trac.webkit.org/changeset/79592 might have broken GTK Linux 32-bit Release I Skipped the following 3 SVG tests because they're still not working on 32-bit Release bot. Looks like the mask operation is ok but bot results have 2 decimal places instead of none (50.00 is shown instead of 50) svg/batik/masking/maskRegions.svg svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg svg/zoom/page/zoom-mask-with-percentages.svg

(In reply to comment #47) > (In reply to comment #46) > > http://trac.webkit.org/changeset/79592 might have broken GTK Linux 32-bit Release > > I Skipped the following 3 SVG tests because they're still not working on 32-bit Release bot. Looks like the mask operation is ok but bot results have 2 decimal places instead of none (50.00 is shown instead of 50) > > svg/batik/masking/maskRegions.svg > svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg > svg/zoom/page/zoom-mask-with-percentages.svg Unfortunately a known bug. Side effect of String::format usage in svg/. I'm still on my mission to eliminate String::format, but it's still going to take a while.