Summary: | [Qt] the above website does not render properly when in compositing mode | ||
---|---|---|---|
Product: | WebKit | Reporter: | Noam Rosenthal <noam> |
Component: | WebKit Qt | Assignee: | Noam Rosenthal <noam> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | ariya.hidayat, commit-queue |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | OS X 10.5 | ||
URL: | http://media.24ways.org/2009/15/spinner-alt.html | ||
Attachments: |
Description
Noam Rosenthal
2010-02-06 12:30:54 PST
Created attachment 48302 [details]
GraphicsLayerQt: masks are now being handled outside the layer's paintEvent
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 :) 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. Disregard the last remark - the BackgroundColor optimization is separate and not actually necessary. 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 :)
Created attachment 48323 [details]
Masks in GraphicsLayerQt; +added in-code comments +removed some whitespace
oops, found some whitespace, saving a re-re-review
> 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())?
> 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.
> > 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 :)
btw, the QPixmap::fill(Qt::transparent) is important to make the pixmap have "hasAlpha". any better way of doing that?
> 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 :)
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 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? Created attachment 48355 [details]
Masks in GraphicsLayerQt; +removed unnecessary assert/whitepsace/funny English
I saw that it failed the commit bot due to ChangeLog... do I need to resubmit? 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> All reviewed patches have been landed. Closing bug. |