Bug 34681 - [Qt] the above website does not render properly when in compositing mode
Summary: [Qt] the above website does not render properly when in compositing mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Noam Rosenthal
URL: http://media.24ways.org/2009/15/spinn...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-06 12:30 PST by Noam Rosenthal
Modified: 2010-02-09 02:08 PST (History)
2 users (show)

See Also:


Attachments
GraphicsLayerQt: masks are now being handled outside the layer's paintEvent (10.78 KB, patch)
2010-02-06 18:58 PST, Noam Rosenthal
ariya.hidayat: review-
Details | Formatted Diff | Diff
Masks in GraphicsLayerQt; +added in-code comments (10.62 KB, patch)
2010-02-07 23:08 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace (10.65 KB, patch)
2010-02-07 23:53 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace +even more comments! (10.81 KB, patch)
2010-02-08 11:45 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Masks in GraphicsLayerQt; +removed unnecessary assert/whitepsace/funny English (10.44 KB, patch)
2010-02-08 12:53 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-02-06 12:30:54 PST
This is due to a wrong path for rendering compositing masks - the opacity effect doesn't get updated when the mask is updated.
Comment 1 Noam Rosenthal 2010-02-06 18:58:17 PST
Created attachment 48302 [details]
GraphicsLayerQt: masks are now being handled outside the layer's paintEvent
Comment 2 Ariya Hidayat 2010-02-07 22:13:46 PST
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 :)
Comment 3 Noam Rosenthal 2010-02-07 22:29:45 PST
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.
Comment 4 Noam Rosenthal 2010-02-07 22:55:28 PST
Disregard the last remark - the BackgroundColor optimization is separate and not actually necessary.
Comment 5 Noam Rosenthal 2010-02-07 23:08:47 PST
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 :)
Comment 6 Noam Rosenthal 2010-02-07 23:53:14 PST
Created attachment 48323 [details]
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace

oops, found some whitespace, saving a re-re-review
Comment 7 Ariya Hidayat 2010-02-08 09:40:10 PST
> 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())?
Comment 8 Ariya Hidayat 2010-02-08 09:42:38 PST
> 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.
Comment 9 Noam Rosenthal 2010-02-08 10:34:04 PST
> > 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 :)
Comment 10 Noam Rosenthal 2010-02-08 11:17:13 PST
btw, the QPixmap::fill(Qt::transparent) is important to make the pixmap have "hasAlpha". any better way of doing that?
Comment 11 Ariya Hidayat 2010-02-08 11:29:24 PST
> 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 :)
Comment 12 Noam Rosenthal 2010-02-08 11:45:51 PST
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
Comment 13 Kenneth Rohde Christiansen 2010-02-08 12:09:07 PST
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?
Comment 14 Noam Rosenthal 2010-02-08 12:53:22 PST
Created attachment 48355 [details]
Masks in GraphicsLayerQt; +removed unnecessary assert/whitepsace/funny English
Comment 15 Noam Rosenthal 2010-02-08 22:41:50 PST
I saw that it failed the commit bot due to ChangeLog... do I need to resubmit?
Comment 16 WebKit Commit Bot 2010-02-09 02:08:06 PST
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>
Comment 17 WebKit Commit Bot 2010-02-09 02:08:12 PST
All reviewed patches have been landed.  Closing bug.