Bug 24289 - [Qt] lacks clipToImageBuffer()
Summary: [Qt] lacks clipToImageBuffer()
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Zoltan Herczeg
URL:
Keywords: Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2009-03-02 07:15 PST by Dirk Schulze
Modified: 2011-02-18 06:40 PST (History)
9 users (show)

See Also:


Attachments
first try (2.40 KB, patch)
2009-03-02 07:20 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
second try (6.20 KB, patch)
2009-03-02 07:28 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
second try of second try (3.64 KB, patch)
2009-09-10 07:46 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
proposed patch (7.16 KB, patch)
2010-04-27 06:46 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
final patch (I hope so) (7.49 KB, patch)
2010-04-28 06:53 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
porter duff based version (6.29 KB, patch)
2010-04-29 04:30 PDT, Zoltan Herczeg
krit: review-
Details | Formatted Diff | Diff
porter duff based version again (6.37 KB, patch)
2010-05-05 04:49 PDT, Zoltan Herczeg
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
next patch (6.21 KB, patch)
2010-05-07 05:21 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
another patch (6.50 KB, patch)
2010-05-18 06:08 PDT, Zoltan Herczeg
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2009-03-02 07:15:29 PST
Qt lacks support for GraphicsContext::clipToImageBuffer().

clipToImageBuffer gets a rect and an QPixmap. This is the mask image and the size of the masked area.

Before it is possible to use clipToImageBuffer, it needs a call of gc->save(). Everything that is drawn after clipToImageBuffer is the masked object. The masked object is closed be a gc->reset, that belongs to the gc->safe() before the clipToImageBuffer call.
Comment 1 Dirk Schulze 2009-03-02 07:20:23 PST
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.
Comment 2 Dirk Schulze 2009-03-02 07:28:25 PST
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 3 Ariya Hidayat 2009-09-09 19:03:29 PDT
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?
Comment 4 Dirk Schulze 2009-09-10 04:05:43 PDT
(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.
Comment 5 Ariya Hidayat 2009-09-10 04:15:14 PDT
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 :)
Comment 6 Dirk Schulze 2009-09-10 07:46:52 PDT
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
Comment 7 Zoltan Herczeg 2010-04-23 05:57:04 PDT
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()
Comment 8 Dirk Schulze 2010-04-23 10:21:55 PDT
(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)
Comment 9 Zoltan Herczeg 2010-04-23 11:11:52 PDT
> 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 :)
Comment 10 Dirk Schulze 2010-04-23 11:29:10 PDT
(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.
Comment 11 Zoltan Herczeg 2010-04-27 06:46:28 PDT
Created attachment 54414 [details]
proposed patch

Save counting is not yet implemented. And the implementation reuses the existing layer frame buffers.
Comment 12 Zoltan Herczeg 2010-04-27 06:48:15 PDT
> 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.
Comment 13 Dirk Schulze 2010-04-27 07:22:00 PDT
(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.
Comment 14 Zoltan Herczeg 2010-04-27 08:25:50 PDT
> 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.
Comment 15 Dirk Schulze 2010-04-27 08:59:02 PDT
(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.
Comment 16 Dirk Schulze 2010-04-27 09:02:21 PDT
(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.
Comment 17 Zoltan Herczeg 2010-04-27 11:07:38 PDT
> 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]
Comment 18 Zoltan Herczeg 2010-04-28 06:53:08 PDT
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.
Comment 19 Simon Hausmann 2010-04-28 13:58:44 PDT
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 ;(
Comment 20 Zoltan Herczeg 2010-04-28 14:13:11 PDT
> 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.
Comment 21 Noam Rosenthal 2010-04-28 14:19:37 PDT
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.
Comment 22 Zoltan Herczeg 2010-04-29 04:30:25 PDT
Created attachment 54690 [details]
porter duff based version
Comment 23 Dirk Schulze 2010-04-29 11:03:45 PDT
(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?
Comment 24 Zoltan Herczeg 2010-04-29 11:10:48 PDT
> 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?
Comment 25 Dirk Schulze 2010-04-29 11:15:55 PDT
(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?
Comment 26 Zoltan Herczeg 2010-04-29 11:25:10 PDT
> 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.
Comment 27 Zoltan Herczeg 2010-04-29 23:45:34 PDT
> 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
Comment 28 Dirk Schulze 2010-04-30 12:01:09 PDT
(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.
Comment 29 Zoltan Herczeg 2010-05-05 04:49:59 PDT
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)
Comment 30 Dirk Schulze 2010-05-05 05:43:42 PDT
(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?
Comment 31 Zoltan Herczeg 2010-05-05 06:25:08 PDT
> 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 32 Simon Hausmann 2010-05-06 01:13:17 PDT
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 :)
Comment 33 Zoltan Herczeg 2010-05-07 05:21:24 PDT
Created attachment 55366 [details]
next patch
Comment 34 Zoltan Herczeg 2010-05-10 08:08:43 PDT
Comment on attachment 55366 [details]
next patch

layout tests are passed
Comment 35 Zoltan Herczeg 2010-05-18 06:08:34 PDT
Created attachment 56364 [details]
another patch
Comment 36 Kenneth Rohde Christiansen 2010-05-18 06:10:43 PDT
Comment on attachment 56364 [details]
another patch

We discussed the patch online and this new patch incorporates mine and Simon's comments. r=me
Comment 37 Zoltan Herczeg 2010-05-18 06:26:57 PDT
Landed in 59668: http://trac.webkit.org/changeset/59668
Closing bug.
Comment 38 Simon Hausmann 2010-05-18 22:07:51 PDT
Revision r59668 cherry-picked into qtwebkit-2.0 with commit ef200e2a3259185c99bfa2cedc67abb1dc0e256b
Comment 39 Kenneth Rohde Christiansen 2010-05-19 05:38:17 PDT
Follow up fix landed in r59713
Comment 40 Simon Hausmann 2010-05-19 07:14:16 PDT
<cherry-pick-from-backport: r59713>
Comment 41 Simon Hausmann 2010-05-20 01:23:28 PDT
Revision r59713 cherry-picked into qtwebkit-2.0 with commit d25f3446f1156215d57695bc3a99322e1fc7f365
Comment 42 Csaba Osztrogonác 2011-02-18 06:40:20 PST
I unskipped tests fixed by this bug: http://trac.webkit.org/changeset/78999