Bug 24289

Created attachment 28173 [details] first try This is the first try to solve the problem. We save the QPixmap create a new layer to draw on and wait to the call of the right gc->reset() (we have to count every save/restore to be sure). On our restore, we take the QPixmap of the layer and mask it with the saved mask image. Unfortunately image.setMask(maskImage.mask()) just creates a 1bit Bitmap. So we can't do half transparent things with it.

Created attachment 28174 [details] second try The second try. On calling clipToImageBuffer, we start a new layer and draw the mask to the context and use the composite operator SourceIn. We started a new Layer before to not affect the other drawings beside the masking process. After setting the compositeOperator, we start a new (second) Layer for the masked object. On the right call of restore, we draw both layers to the context. Only problem: SourceIn seems not to have no affect on the new created layer. The GraphicsContext draws the mask to the context, but doesn't consider to the masked object. You'll just see the mask.

Comment on attachment 28174 [details] second try Just a quick review before I give it a try. > + [QT] lacks putImageData support in Canvas Should be Qt. > + if (!m_data->countSavesDuringMasking.empty()) Let us use isEmpty() to make the code more Qt-like. > + QRect qrect(QRect(rect.x(), rect.y(), rect.width(), rect.height())); Is it possible to convert FloatRect to QRectF first, then finally to QRect? > + TransparencyLayer * maskLayer = new TransparencyLayer(m_data->p(), qrect); Coding style with *. > + p->setCompositionMode(QPainter::CompositionMode_SourceOver); Should we save the composition mode and restore it again, instead of setting it to SourceOver? > +void ImageBuffer::putImageData(ImageData* source, const IntRect& sourceRect, const IntPoint& destPoint) I guess this is not necessary anymore because it is already implemented?

(In reply to comment #3) > (From update of attachment 28174 [details]) > Just a quick review before I give it a try. > > > + [QT] lacks putImageData support in Canvas > > Should be Qt. > > > + if (!m_data->countSavesDuringMasking.empty()) > > Let us use isEmpty() to make the code more Qt-like. > > > + QRect qrect(QRect(rect.x(), rect.y(), rect.width(), rect.height())); > > Is it possible to convert FloatRect to QRectF first, then finally to QRect? > > > + TransparencyLayer * maskLayer = new TransparencyLayer(m_data->p(), qrect); > > Coding style with *. > > > + p->setCompositionMode(QPainter::CompositionMode_SourceOver); > > Should we save the composition mode and restore it again, instead of setting it > to SourceOver? > > > +void ImageBuffer::putImageData(ImageData* source, const IntRect& sourceRect, const IntPoint& destPoint) > > I guess this is not necessary anymore because it is already implemented? Shall I upload a new patch with the corrections of above? It doesn't work anyway even if it should (in my understanding). But the things you pointed out are correct. The style-issues as well as your assumptions about putImageData and to save the operator. I would prefer the first patch (with fixed style issues), but qt just supports 1-Bit masks.

Yes, please update the patch so that I can give it a try. Could you also mention which test file in LayoutTests is suitable for easy testing? As for workaround the 1-bit mask, I guess I will pull one or two tricks from my sleeve :)

Created attachment 39348 [details] second try of second try This is the update of the second patch with the comments of above. Goal was to draw everthing after the clipToImageBuffer into a new layer and use the stored mask with the new layer together with the composite operator source-in. Test: svg/W3C/masking-mask-01-b.svg svg/W3C/masking-intro-01-f.svg

I am also thinking now of implementing this function for Qt. If I understand correctly, clipToImageBuffer combines a layer and an alpha channel from two images: imagebuffer (RGBA) pixel (i,j): (rrggbbAA) layer pixmap (RGBA) pixel (i,j): (RRGGBBaa) after the combination, the resulting (i,j) pixel will be (RRGGBBAA), and the other bytes are discarded. I feel clipToImageBuffer is too Mac platform specific. Since it is combined with context->save() and context->restore(), would be better the following commands: context->beginClipToImageBuffer() context->endClipToImageBuffer() Other platforms could use layers to emulate clipToImage behaviour. In Qt, I would create QImage-s, and access pixel data through QImage::scanLine()

(In reply to comment #7) > I am also thinking now of implementing this function for Qt. If I understand > correctly, clipToImageBuffer combines a layer and an alpha channel from two > images: > > imagebuffer (RGBA) pixel (i,j): (rrggbbAA) > layer pixmap (RGBA) pixel (i,j): (RRGGBBaa) The alpha channel is not replaced. The alpha of the layer gets multiplied with the alpha of the imgaBuffer IIRC. > > after the combination, the resulting (i,j) pixel will be (RRGGBBAA), and the > other bytes are discarded. I feel clipToImageBuffer is too Mac platform > specific. Since it is combined with context->save() and context->restore(), > would be better the following commands: > > context->beginClipToImageBuffer() > context->endClipToImageBuffer() > > Other platforms could use layers to emulate clipToImage behaviour. Yes, thats what I tried at https://bugs.webkit.org/show_bug.cgi?id=23526#c3 . Not sure, but I think this concept failed, because the endClipToImageBuffer may don't get reached. Another problem was the confusing concept of the old SVG Masker code. It could be possible with the new masker code. clipToImageBuffer is used in * CSS3 text clip * SVG masking: rendering/RenderSVGResourceMasker.cpp * SVG clipping: rendering/RenderSVGResourceClipper.cpp both, masker and clipper, are called in SVGRenderSupport.cpp. To use your begin/end concept, you have to go the same way as filter and create a masker / clipper before calling SVGRenderSupport.cpp:prepareToRenderSVGContent. And give the masker / clipper to prepareToRenderSVGContent and finishRenderSVGContent. Not realy a clean way, but possible. * SVG Filters: FEComposite.cpp and SourceAlpha.cpp shouldn't be a big problem * Canvas to fill texts with stroke and fill (only on Cg)

> The alpha channel is not replaced. The alpha of the layer gets multiplied with > the alpha of the imgaBuffer IIRC. So it is practically the same as the light map in texture mapping. > Yes, thats what I tried at https://bugs.webkit.org/show_bug.cgi?id=23526#c3 . > Not sure, but I think this concept failed, because the endClipToImageBuffer may > don't get reached. Another problem was the confusing concept of the old SVG > Masker code. It could be possible with the new masker code. I support your solution. If I could be any help, I would gladly join to you improve pianting in WebKit. We might need to refactor many things in WebKit, but I am not afraid of challenges :)

(In reply to comment #9) > > The alpha channel is not replaced. The alpha of the layer gets multiplied with > > the alpha of the imgaBuffer IIRC. > > So it is practically the same as the light map in texture mapping. > > > Yes, thats what I tried at https://bugs.webkit.org/show_bug.cgi?id=23526#c3 . > > Not sure, but I think this concept failed, because the endClipToImageBuffer may > > don't get reached. Another problem was the confusing concept of the old SVG > > Masker code. It could be possible with the new masker code. > > I support your solution. If I could be any help, I would gladly join to you > improve pianting in WebKit. We might need to refactor many things in WebKit, > but I am not afraid of challenges :) Sorry. But I am busy at the moment. So I can't work on that. Another solution might be to store the maskImage, count save and restores and use the masking operation at the right time. Just like one of the patches above tries to do it, like the patch on bug 23526 tries it and like Skia already does it.

Created attachment 54414 [details] proposed patch Save counting is not yet implemented. And the implementation reuses the existing layer frame buffers.

> Another solution might be to store the maskImage, count save and restores and > use the masking operation at the right time. Just like one of the patches above > tries to do it, like the patch on bug 23526 tries it and like Skia already does > it. Is it true, that only 1 clipToImageBuffer is allowed? Seems to me.

(In reply to comment #12) > > Another solution might be to store the maskImage, count save and restores and > > use the masking operation at the right time. Just like one of the patches above > > tries to do it, like the patch on bug 23526 tries it and like Skia already does > > it. > > Is it true, that only 1 clipToImageBuffer is allowed? Seems to me. No. And IIRC SVG's clipPath may make use of multiple clipToImageBuffer calls.

> No. And IIRC SVG's clipPath may make use of multiple clipToImageBuffer calls. Excellent. It was worth to follow the general (and more difficult) path. Anyway, I probably still need to implement some kind of save/restore counters, isn't it? I hope eventually we can use beginClipToImage and endClipToImage calls to avoid such code hacks.

(In reply to comment #14) > > No. And IIRC SVG's clipPath may make use of multiple clipToImageBuffer calls. > > Excellent. It was worth to follow the general (and more difficult) path. > Anyway, I probably still need to implement some kind of save/restore counters, > isn't it? > > I hope eventually we can use beginClipToImage and endClipToImage calls to avoid > such code hacks. Sure, just upload a patch if you have a good implementation.

(In reply to comment #11) > Created an attachment (id=54414) [details] > proposed patch > > Save counting is not yet implemented. And the implementation reuses the > existing layer frame buffers. IIRC, TransparencyLayer is used in SVG to handle opacity. That has nothing to do with clipToImageBuffer. So a alpha mask would be wrong here. We need all channels.

> IIRC, TransparencyLayer is used in SVG to handle opacity. That has nothing to > do with clipToImageBuffer. So a alpha mask would be wrong here. We need all > channels. I keep all chanels, if I understand you correctly. It is a stack composed of transparency layers and clipToImageBuffer layers. All have their image. For TransparencyLayer an opacity, for clipToImageBuffer an alpha-mask channel is stored, respectively. If you would implement an stack, the effect would be the same from the image storage view: Common stack: Separated stack: [Transp Layer 2] [Transp Layer 2] [clipToIm Layer 2] [clipToIm Layer 2] [Transp Layer 1] [Transp Layer 1] [clipToIm Layer 1] [clipToIm Layer 1]

Created attachment 54561 [details] final patch (I hope so) The patch fixes 21 additional SVG related layout tests. Since these pixel results are not in the trunk (yet), I don't need to attach them to the patch. I saw other wrong SVG filters in Qt and I plan to fix them. I like SVG more and more.

The parts that concern me the most about this patch are that a) It requires an expensive conversion from a QPixmap to a QImage (inside clipToImageBuffer) and b) By painting onto a QImage a completely different paint engine is used, which may produce different output and certainly won't utilize any hardware acceleration. I wish there was a way to do the masking without falling back to software rendering ;(

> I wish there was a way to do the masking without falling back to software > rendering ;( I would prefer to move the ImageBuffer to QImage, since it suits far better for its purpose, and that would avoid conversions. Although your concern about using a different painting engine is right, mixed environments could make surprising results. Simon, what is your proposal? I would prefer a slow implementation over a no implementation :) but you might have other preferences. I could also use QPixmap for the layer, and do more (expensive) conversions, which would avoid the use of two raster engines for painting.

Dirk's patches use composition modes (DestinationIn / SourceIn), which is the HW accelerated way to do masks; see GraphicsLayerQt.cpp. I'm sure this can be applied to your fix though if it works like it is I'm fine with it.

Created attachment 54690 [details] porter duff based version

(In reply to comment #22) > Created an attachment (id=54690) [details] > porter duff based version I would like to know, if this implementatin already works in pracitce. On my tries, I did not get SVG Masking working, or at least it did not look like it should look like. Does your patch work with http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/masking-mask-01-b.html and following pages?

> I would like to know, if this implementatin already works in pracitce. On my > tries, I did not get SVG Masking working, or at least it did not look like it > should look like. Does your patch work with > http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/masking-mask-01-b.html > and following pages? What OS do you use? Did you try passing -graphicsystem raster option to QtLauncher?

(In reply to comment #24) > > I would like to know, if this implementatin already works in pracitce. On my > > tries, I did not get SVG Masking working, or at least it did not look like it > > should look like. Does your patch work with > > http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/masking-mask-01-b.html > > and following pages? > > What OS do you use? Did you try passing -graphicsystem raster option to > QtLauncher? I was working on linux and did not pass any options. But does it mean, your patch won't work without any options? I did not check your patch. But maybe you can test the link of above on your build?

> I was working on linux and did not pass any options. But does it mean, your > patch won't work without any options? I did not check your patch. But maybe you > can test the link of above on your build? I think you remember when I told you about that Qt does not render anything on a QPixmap filled with transparent color. I discussed this problem with the Qt experts, and they told me that x11 implementation of QPixmap is not "perfect". I should use this option on Linux or use a different OS :D This option is accepted by all Qt based program, since it is an internal (secret) option. I can only check my build tomorrow. If you still have a Qt based build, you can try that option now.

> I would like to know, if this implementatin already works in pracitce. On my > tries, I did not get SVG Masking working, or at least it did not look like it > should look like. Does your patch work with > http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/masking-mask-01-b.html > and following pages? Well, it works on some examples, and not on others. The link itself is only partly work. I still feel the patch is a step ahead, but feel free to give an r- if you think it is not. This works for example: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-example-01-b.html

(In reply to comment #27) > > I would like to know, if this implementatin already works in pracitce. On my > > tries, I did not get SVG Masking working, or at least it did not look like it > > should look like. Does your patch work with > > http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/masking-mask-01-b.html > > and following pages? > > Well, it works on some examples, and not on others. The link itself is only > partly work. I still feel the patch is a step ahead, but feel free to give an > r- if you think it is not. > > This works for example: > > http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-example-01-b.html I did not upload other patches I made, because of this problem. In one of my patches, the masking-mask-01-b seems to work, but the text 'SVG' was not filled with green and different opacity values, but with black and different opacity values. After this breakdown, I concentrated on the beginMask/endMask concept. This causes realy poor changes on the SVG code path (I mentioned it in one of the comments above). At the end, your patch is the right direction. The Qt guys should look at the results, maybe they know why we don't see the expected results. But we shouldn't land this patch until this problem is resolved. It's bad to have a clipToImageBuffer implementation that may, or may not work. I'll give you a r- for this reason. Ask the Qt guys for help and if this issue needs to be fixed on the Qt-side or somehow on the webkit side.

Created attachment 55108 [details] porter duff based version again This patch is mainly for Tronical to review (basically the same as before, just an updated Changelog)

(In reply to comment #29) > Created an attachment (id=55108) [details] > porter duff based version again > > This patch is mainly for Tronical to review (basically the same as before, just > an updated Changelog) How do you plan to solve the last issues?

> How do you plan to solve the last issues? Discussing (#qtwebkit channel) the issue with Tronical, who is a Qt expert. He agrees with this solution, and happy about avoiding direct pixel manipulation. Just because the Qt X11 port has issues, the patch itself is good.

Comment on attachment 55108 [details] porter duff based version again WebCore/platform/graphics/qt/GraphicsContextQt.cpp:208 + QPixmap* alphaMask; QPixmap is actually a pointer to a QPixmapData object. Storing a QPixmap pointer is a double indirection, and later in the patch we allocate a QPixmap on the heap. That's not necessary. I think the code would be simpler and faster when using a QPixmpap by value here. WebCore/ChangeLog:11 + inTransparencyLayer() is removed as it is unnecessary. This is not correct. When building QtWebKit for Windows we compile WebCore/plugins/PluginViewWin.cpp, which calls this function. Otherwise this looks good to me. Less pixel manipulation. yay :)

Created attachment 55366 [details] next patch

Comment on attachment 55366 [details] next patch layout tests are passed

Created attachment 56364 [details] another patch

Comment on attachment 56364 [details] another patch We discussed the patch online and this new patch incorporates mine and Simon's comments. r=me

Landed in 59668: http://trac.webkit.org/changeset/59668 Closing bug.

Revision r59668 cherry-picked into qtwebkit-2.0 with commit ef200e2a3259185c99bfa2cedc67abb1dc0e256b

Follow up fix landed in r59713

<cherry-pick-from-backport: r59713>

Revision r59713 cherry-picked into qtwebkit-2.0 with commit d25f3446f1156215d57695bc3a99322e1fc7f365

I unskipped tests fixed by this bug: http://trac.webkit.org/changeset/78999