Bug 34681 – [Qt] the above website does not render properly when in compositing mode

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Noam Rosenthal]

This is due to a wrong path for rendering compositing masks - the opacity effect doesn't get updated when the mask is updated.

[Noam Rosenthal]

Created attachment 48302 [details]
GraphicsLayerQt: masks are now being handled outside the layer's paintEvent

[Ariya Hidayat]

Comment on attachment 48302 [details]
GraphicsLayerQt: masks are now being handled outside the layer's paintEvent

> +class MaskEffectQt : public QGraphicsEffect {

If we still need to support Qt 4.5, better enclose this in #ifdef QT_VERSION for 4.6 and beyond.
And yes, there should be an alternative code path for Qt 4.5 even if it means the test site will not work.

> +        QPixmap srcPixmap = sourcePixmap(Qt::LogicalCoordinates, &offset, QGraphicsEffect::NoPad);
> +        QPixmap pixmap(srcPixmap.size());
> +        pixmap.fill(Qt::transparent);
> +
> +        if (pixmap.isNull())
> +            return;
> +
> +        QPainter pixmapPainter(&pixmap);
> +        pixmapPainter.setRenderHints(painter->renderHints());
> +        pixmapPainter.setCompositionMode(QPainter::CompositionMode_Source);
> +        pixmapPainter.drawPixmap(0, 0, srcPixmap);

That sounds inefficient. Why do you need to create 'pixmap', fill it transparent, then draw those pixels over again with srcPixmap?
I guess, simple QPIxmap pixmap = srcPixmap should work (QPixmap will detach as soon as you start the pixmapPainter).


> +        pixmapPainter.setCompositionMode(QPainter::CompositionMode_DestinationIn);
> +        pixmapPainter.drawPixmap(0, 0, maskPixmap);
> +        pixmapPainter.end();
> +        painter->drawPixmap(offset, pixmap);

Brings me another question: do we need 'pixmap' at all? Can't we just do:

painter->save();
painter->setCompositionMode(QPainter::CompositionMode_Source);
painter->drawPixmap(offset, srcPixmap);
painter->setCompositionMode(QPainter::CompositionMode_DestinationIn);
painter->drawPixmap(0, 0, maskPixmap);
painter->restore();


>  void GraphicsLayerQt::setBackgroundColor(const Color& c)
>  {
> +    if (c == backgroundColor())
> +        return;

Is this necessary for this patch or is it actually a separate optimization?



Otherwise, looks really good :)

[Noam Rosenthal]

Re. 4.5: the whole file GraphicsLayerQt.cpp will not be compiled in 4.5 anyway, see WebCore.pro.

(In reply to comment #2)
> (From update of attachment 48302 [details])
> > +class MaskEffectQt : public QGraphicsEffect {
> 
> If we still need to support Qt 4.5, better enclose this in #ifdef QT_VERSION
> for 4.6 and beyond.

The whole GraphicsLayerQt.cpp file and the rest of the compositing features don't get compiled in 4.5 anyway, they rely on QAnimation and other goodies.

> That sounds inefficient. Why do you need to create 'pixmap', fill it
> transparent, then draw those pixels over again with srcPixmap?
> I guess, simple QPIxmap pixmap = srcPixmap should work (QPixmap will detach as
> soon as you start the pixmapPainter).

It SEEMS inefficient, but it actually a lot more efficient! QPixmap::detach in OpenGL engine calls QPixmap::toImage, meaning copy from video memory to system memory. Painting to a new pixmap in video memory is a lot more efficient. Without this code path I end up with 10% of the total time in copying pixels from video memory to system memory (or so valgrind claims).
See http://bugreports.qt.nokia.com/browse/QTBUG-7986

The website I put as a test-case here runs with ~15fps now, and ~8FPS when I use QGraphicsOpacityEffect, which does the QPixmap detach thingy.
> 
> 
> > +        pixmapPainter.setCompositionMode(QPainter::CompositionMode_DestinationIn);
> > +        pixmapPainter.drawPixmap(0, 0, maskPixmap);
> > +        pixmapPainter.end();
> > +        painter->drawPixmap(offset, pixmap);
> 
> Brings me another question: do we need 'pixmap' at all? Can't we just do:
> 
> painter->save();
> painter->setCompositionMode(QPainter::CompositionMode_Source);
> painter->drawPixmap(offset, srcPixmap);
> painter->setCompositionMode(QPainter::CompositionMode_DestinationIn);
> painter->drawPixmap(0, 0, maskPixmap);
> painter->restore();

Then I'd be setting a DestinationIn composition mode on the whole viewport in case I'm not in a cache-mode - potentially masking things I shouldn't. 
I tried this :)

> 
> 
> >  void GraphicsLayerQt::setBackgroundColor(const Color& c)
> >  {
> > +    if (c == backgroundColor())
> > +        return;
> 
> Is this necessary for this patch or is it actually a separate optimization?
> 
Necessary for this patch. Apparently when I have manage the mask layer in the new way WebCore keeps calling setBackgroundColor with the old color, which constantly updates the layer.

[Noam Rosenthal]

Disregard the last remark - the BackgroundColor optimization is separate and not actually necessary.

[Noam Rosenthal]

Created attachment 48321 [details]
Masks in GraphicsLayerQt; +added in-code comments

Based on Ariya's comments:
- Removed an optimization that's unrelated to the bug
- Added in-code comments in places where I feel my approach is justified :)

[Noam Rosenthal]

Created attachment 48323 [details]
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace

oops, found some whitespace, saving a re-re-review

[Ariya Hidayat]

> The whole GraphicsLayerQt.cpp file and the rest of the compositing features
> don't get compiled in 4.5 anyway, they rely on QAnimation and other goodies.

Ah OK, my bad, I missed that.

> It SEEMS inefficient, but it actually a lot more efficient! QPixmap::detach in
> OpenGL engine calls QPixmap::toImage, meaning copy from video memory to system
> memory. Painting to a new pixmap in video memory is a lot more efficient.
> Without this code path I end up with 10% of the total time in copying pixels
> from video memory to system memory (or so valgrind claims).
> See http://bugreports.qt.nokia.com/browse/QTBUG-7986

Seems that this is only for OpenGL graphics system? I can't imagine this is faster in X11 or raster engine, though.

Also, even if you insist on doing that, is the Qt::transparent fill absolutely necessary? You replace all pixels in that pixmap (due to CompositionMode_Source) right after that.

> Then I'd be setting a DestinationIn composition mode on the whole viewport in
> case I'm not in a cache-mode - potentially masking things I shouldn't. 
> I tried this :)

Even with clipping out QRect(offset, pixmap.size())?

[Ariya Hidayat]

> Even with clipping out QRect(offset, pixmap.size())?

Ignore this, I finally understood why you don't want to mask anything outside the item cache.

[Noam Rosenthal]

> > See http://bugreports.qt.nokia.com/browse/QTBUG-7986
> 
> Seems that this is only for OpenGL graphics system? I can't imagine this is
> faster in X11 or raster engine, though.
Drawing a pixmap 

I would think that detaching a QPixmap and drawing a QPixmap to another one should be similar cost on raster/X11, and definitely not as costly as detaching on OpenGL. Also I'd rather optimize for raster/OpenGL paint engines, which are what we use on embedded, rather than for the X11 paint engine which is slower for everything anyway :)

[Noam Rosenthal]

btw, the QPixmap::fill(Qt::transparent) is important to make the pixmap have "hasAlpha". any better way of doing that?

[Ariya Hidayat]

> I would think that detaching a QPixmap and drawing a QPixmap to another one
> should be similar cost on raster/X11, and definitely not as costly as detaching
> on OpenGL. Also I'd rather optimize for raster/OpenGL paint engines, which are
> what we use on embedded, rather than for the X11 paint engine which is slower
> for everything anyway :)

It's not the detach, it's because instead detach you have to do these 3 steps:
  Create a new pixmap with the same size
  Open a painter
  Draw srcPixmap

before masking it.

I suspect on X11/raster, these steps are faster:
  Create a copy of the pixmap
  Open a painter (implies a detach)
  Mask the pixmap

And according to your test, these are faster on OpenGL:  
  Create a new pixmap (from scratch) with the same size
  (Fill it with transparent pixels)
  Open a painter
  Draw srcPixmap
  Mask the pixmap

Hence, my concerns that your approach will have an impact on non-GL engine.

However, this is just a wild guess. A proper fps benchmark is necessary to justify the worry :)

[Noam Rosenthal]

Created attachment 48348 [details]
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace +even more comments!

Captured the comments thread in in-code comments

[Kenneth Rohde Christiansen]

Comment on attachment 48348 [details]
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace +even more comments!


> +        QPixmap srcPixmap = sourcePixmap(Qt::LogicalCoordinates, &offset, QGraphicsEffect::NoPad);
> +
> +

One newline too much

> +        if (m_state.drawsContent) {
> +            // this is the "expensive" bit. we try to minimize calls to this neck of the woods by proper caching

Better to get to the point, instead of using sophisticated/funny English :-)


> +    Q_ASSERT(this);

I'm not sure if it is better to use WebCore ASSERT or not?

[Noam Rosenthal]

Created attachment 48355 [details]
Masks in GraphicsLayerQt; +removed unnecessary assert/whitepsace/funny English

[Noam Rosenthal]

I saw that it failed the commit bot due to ChangeLog... do I need to resubmit?

[WebKit Commit Bot]

Comment on attachment 48355 [details]
Masks in GraphicsLayerQt; +removed unnecessary assert/whitepsace/funny English

Clearing flags on attachment: 48355

Committed r54541: <http://trac.webkit.org/changeset/54541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.