Created attachment 86643[details]
Texture mapper based implementation of AC for WK2.
1st version for comments.
1. GL implementation doesn't work (and switched off).
2. Needs extra cleanup.
(In reply to comment #2)
> I'm going to preemptively r- this.
I even didn't ask for review ;)
> Noam should be the one upstreaming this.
We've discussed this in email with Noam. I'll do the dirty job of merging and making it work on desktop. As Benjamin have suggested on irc, it will go under Noam's name.
Guys, this should be about 5 different patches with very detailed explanations about the changes, the reasons for them and how things are supposed to work, and also with the will and ability to fix bugs in it if they come up. If that's not currently possible, please wait until I can get to upstreaming this.
(In reply to comment #4)
> Guys, this should be about 5 different patches with very detailed explanations about the changes, the reasons for them and how things are supposed to work, and also with the will and ability to fix bugs in it if they come up. If that's not currently possible, please wait until I can get to upstreaming this.
I agree 100%. I also think this bug would be a nice place to comment on how this approach relates to the other work that's going on in trunk regarding AC, to spread some AC knowledge.
there's plenty of unrelated changes in this mega patch including layout, drawing area, geolocation and touch event items.
virtual void invalidateFixedLayoutSize() const { }
virtual void showAnchorActivationCandidate(const Vector<IntRect>&) { }
bool ChromeClientQt::tabsToLinks() const
void ChromeClientQt::requestGeolocationPermissionForFrame and alikes
QGraphicsWKView::prepareScaleChange and alikes
void QGraphicsWKViewPrivate::clearVisualFeedback and alikes
void QWKPagePrivate::createDrawingArea(DrawingAreaType areaType)
void TiledDrawingAreaProxy::updateWebView() and alikes
WebPageProxy::didChangeDrawingAreaType(uint64_t areaID) and alikes
TiledDrawingArea::scheduleTileUpdate() and alikes
etc
Comment on attachment 96896[details]
Patch 1: WebLayerTreeInfo
View in context: https://bugs.webkit.org/attachment.cgi?id=96896&action=review> Source/WebKit2/Shared/WebLayerTreeInfo.cpp:36
> + encoder->encode(CoreIPC::In(id, name, parent, children, flags, replica, mask));
> + encoder->encode(CoreIPC::In(pos, size, transform, opacity, anchorPoint, childrenTransform, contentsRect));
Any reason why these are encoded separately?
> Source/WebKit2/Shared/WebLayerTreeInfo.h:58
> + FloatPoint pos;
Dont we normally use location() in WebCore ?
> Source/WebKit2/Shared/WebLayerTreeInfo.h:64
> + FloatSize size;
> + TransformationMatrix transform;
> + TransformationMatrix childrenTransform;
> + IntRect contentsRect;
> + float opacity;
how is size different from contentsRect.size() ?
> Source/WebKit2/Shared/WebLayerTreeInfo.h:86
> + Vector<WebLayerInfo> layers;
> + Vector<WebLayerID> deletedLayers;
Both are called layers, though one is a vector of layer ids and the one of of layer infos. A bit confusing
Comment on attachment 96897[details]
Patch 2: WebGraphicsLayer
View in context: https://bugs.webkit.org/attachment.cgi?id=96897&action=review> Source/WebKit2/ChangeLog:11
> + Added WebGraphicsLayer, a subclass of WebCore::GraphicsLayer that serializes the
> + state of the layer tree to the UI process WebLayerTreeInfo.
> + For now this patch doesn't serialize the animation information, a feature that will be upstreamed
> + later on.
You line length quite different per line :-)
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:44
> +static HashMap<WebLayerID, WebGraphicsLayer*>& layerByIDTable()
I believe we normally would call this a map and not a table
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:46
> + static HashMap<WebLayerID, WebGraphicsLayer*> sLayerByIDTable;
I have never seen the sLayer... style, gLayer or s_layer or similar is the most used way to declare statics. But as it is internal anyway, why not just call it globalMap or so.
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:85
> + static WebLayerID sID = 1000;
Add comment why you start at 1000
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:263
> +bool WebGraphicsLayer::addAnimation(const KeyframeValueList& valueList, const IntSize& boxSize, const Animation* anim, const String& keyframesName, double timeOffset)
> +{
> + return false;
> +}
Maybe belongs in the header?
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:283
> +}
> +void WebGraphicsLayer::setMaskLayer(GraphicsLayer* layer)
some missing new line
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:288
> + notifyChange();
> +}
> +void WebGraphicsLayer::setReplicatedByLayer(GraphicsLayer* layer)
other places as well
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:391
> + static const float gDimension = 1024.0;
Shouldnt this be define at the top or so?
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:404
> + WebGraphicsLayer* layerWK2 = toWebGraphicsLayer(layer);
Why not just call it layer? I dont see what the WK2 adds
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:418
> +static void collectCompositingInfoRecursive(GraphicsLayer* rootLayer, WebLayerTreeInfo& outInfo, Vector<WebGraphicsLayer*>& outLayers)
REcursively?
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:431
> +void WebGraphicsLayer::sendLayersToUI(WebCore::GraphicsLayer* rootLayer, WebPage* webPage)
ToUIProcess?
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:449
> + WebGraphicsLayer* layerWK2 = layers[i];
layer*
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> + void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
this is a header, please just write animationName and leave out the /* */s
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> > + void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
>
> this is a header, please just write animationName and leave out the /* */s
I'm sorry, but check-webkit-style script insists on not having parameter names in .h files - just parameter types. Removing /* */ will make it cry ;)
(In reply to comment #15)
> > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> > > + void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
> >
> > this is a header, please just write animationName and leave out the /* */s
>
> I'm sorry, but check-webkit-style script insists on not having parameter names in .h files - just parameter types. Removing /* */ will make it cry ;)
Well it should be fixed then, as that should be totally ok in header files. Also the script is just educational.
Or did our style guide change?
(In reply to comment #13)
> (From update of attachment 96896[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96896&action=review
>
> > Source/WebKit2/Shared/WebLayerTreeInfo.cpp:36
> > + encoder->encode(CoreIPC::In(id, name, parent, children, flags, replica, mask));
> > + encoder->encode(CoreIPC::In(pos, size, transform, opacity, anchorPoint, childrenTransform, contentsRect));
>
> Any reason why these are encoded separately?
You can't encode more than 10 arguments.
>
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:58
> > + FloatPoint pos;
>
> Dont we normally use location() in WebCore ?
No, we use pos. See GraphicsLayer.h
>
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:64
> > + FloatSize size;
> > + TransformationMatrix transform;
> > + TransformationMatrix childrenTransform;
> > + IntRect contentsRect;
> > + float opacity;
>
> how is size different from contentsRect.size() ?
Very different. A layer with a directly composited image can show an image at 10,10,20,20, but clip its children to 0,0,100,100.
>
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:86
> > + Vector<WebLayerInfo> layers;
> > + Vector<WebLayerID> deletedLayers;
>
> Both are called layers, though one is a vector of layer ids and the one of of layer infos. A bit confusing
OK, will modify to deletedLayerIDs.
(In reply to comment #16)
> (In reply to comment #15)
> > > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> > > > + void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
> > >
> > > this is a header, please just write animationName and leave out the /* */s
> >
> > I'm sorry, but check-webkit-style script insists on not having parameter names in .h files - just parameter types. Removing /* */ will make it cry ;)
>
> Well it should be fixed then, as that should be totally ok in header files. Also the script is just educational.
>
> Or did our style guide change?
These function defs are actually directly copied from GraphicsLayer.h
Comment on attachment 96962[details]
Patch 2: WebGraphicsLayer
View in context: https://bugs.webkit.org/attachment.cgi?id=96962&action=review> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:50
> +static const float gTileDimension = 1024.0;
I would put this above the hash map
> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:87
> + static WebLayerID sID = 1000;
Add comment why we start at 1000.
Comment on attachment 97035[details]
Patch 5: LayerTreeContext/LayerTreeHost
View in context: https://bugs.webkit.org/attachment.cgi?id=97035&action=review
Great that you are doing the upstreaming No'am!
> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:223
> + static_cast<DrawingAreaImpl*>(m_webPage->drawingArea())->layerHostDidFlushLayers();
is there no toDrawing... method?
Comment on attachment 96915[details]
Patch 4: Expose webViewVisibleRect, so that we can avoid rendering texture outside of our view.
View in context: https://bugs.webkit.org/attachment.cgi?id=96915&action=review> Source/WebKit2/UIProcess/PageClient.h:104
> + virtual WebCore::IntRect webViewVisibleRect() const = 0;
why is this not just called visibleContentRect? we are using that nomenclature elsewhere in webcore. Or why cant the existing method from FrameView be used?
> > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:223
> > + static_cast<DrawingAreaImpl*>(m_webPage->drawingArea())->layerHostDidFlushLayers();
>
> is there no toDrawing... method?
No, there isn't :)
Maybe we should add one in a different patch.
(In reply to comment #27)
> (From update of attachment 96915[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96915&action=review
>
> > Source/WebKit2/UIProcess/PageClient.h:104
> > + virtual WebCore::IntRect webViewVisibleRect() const = 0;
>
> why is this not just called visibleContentRect? we are using that nomenclature elsewhere in webcore.
Because it's in view coordinates, not taking into account zoom/scroll.
How about visibleContentsRectInViewCoordinates?
> Or why cant the existing method from FrameView be used?
Because FrameView is in the WebProcess only. This is a UI process function that should respond to scrolling/zooming immediately.
Comment on attachment 96915[details]
Patch 4: Expose webViewVisibleRect, so that we can avoid rendering texture outside of our view.
View in context: https://bugs.webkit.org/attachment.cgi?id=96915&action=review>>> Source/WebKit2/UIProcess/PageClient.h:104
>>> + virtual WebCore::IntRect webViewVisibleRect() const = 0;
>>
>> why is this not just called visibleContentRect? we are using that nomenclature elsewhere in webcore. Or why cant the existing method from FrameView be used?
>
> Because it's in view coordinates, not taking into account zoom/scroll.
> How about visibleContentsRectInViewCoordinates?
Actually it is the UI updating the actualVisibleContentRect, so you should have access to that from the UI side.
Maybe this is more like the viewportContentsWindow ?
> Actually it is the UI updating the actualVisibleContentRect, so you should have access to that from the UI side.
Again, the transform is different. This was kind of tricky to figure out exactly which Rect we needed, and that's how we ended up with this one.
btw, TiledDrawingAreaProxy already has a webViewVisibleRect function, that's where the name came from.
>
> Maybe this is more like the viewportContentsWindow ?
viewportClipRect? The word contents is a bit confusing because it's actually about the viewport and not about the contents.
(In reply to comment #33)
> > Actually it is the UI updating the actualVisibleContentRect, so you should have access to that from the UI side.
> Again, the transform is different. This was kind of tricky to figure out exactly which Rect we needed, and that's how we ended up with this one.
>
> btw, TiledDrawingAreaProxy already has a webViewVisibleRect function, that's where the name came from.
Yeah I know and I never liked it there either :-)
> > Maybe this is more like the viewportContentsWindow ?
> viewportClipRect? The word contents is a bit confusing because it's actually about the viewport and not about the contents.
It is only used for clipping? viewportRect or similar would be fine I guess
(In reply to comment #40)
> (In reply to comment #31)
> > (From update of attachment 97035[details] [details])
> > Clearing flags on attachment: 97035
> >
> > Committed r88799: <http://trac.webkit.org/changeset/88799>
>
> I had to rollout this patch, because it made webprocess crash.
> Guys, you should have run layout tests before landing ...
>
> Rollout patch landed in http://trac.webkit.org/changeset/88926
Very interesting.
The change is completely on UI side and new API is not used yet.
Do you have any pointers what to investigate? What tests have crash?
(In reply to comment #41)
> Very interesting.
> The change is completely on UI side and new API is not used yet.
>
> Do you have any pointers what to investigate? What tests have crash?
x86-32 Linux Qt Release WebKit2 buildbot is your friend:
http://build.webkit.sed.hu/waterfall
(In reply to comment #42)
> (In reply to comment #41)
> > Very interesting.
> > The change is completely on UI side and new API is not used yet.
> >
> > Do you have any pointers what to investigate? What tests have crash?
>
> x86-32 Linux Qt Release WebKit2 buildbot is your friend:
> http://build.webkit.sed.hu/waterfall
Oh, sorry, looked at wrong patch.
Is AC enabled on WK2 Qt?
This code will crash (if enabled) until https://bugs.webkit.org/show_bug.cgi?id=60621 is committed. DrawingAreaImpl send null image handles in UpdateInfo.
AC should be disabled on Qt Webkit2 until all the patches are done.
(In reply to comment #43)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > Very interesting.
> > > The change is completely on UI side and new API is not used yet.
> > >
> > > Do you have any pointers what to investigate? What tests have crash?
> >
> > x86-32 Linux Qt Release WebKit2 buildbot is your friend:
> > http://build.webkit.sed.hu/waterfall
>
> Oh, sorry, looked at wrong patch.
> Is AC enabled on WK2 Qt?
> This code will crash (if enabled) until https://bugs.webkit.org/show_bug.cgi?id=60621 is committed. DrawingAreaImpl send null image handles in UpdateInfo.
> AC should be disabled on Qt Webkit2 until all the patches are done.
Slava, can you post a patch that disables AC in the WK2 settings?
(In reply to comment #47)
> Attachment 97192[details] was posted by a committer and has review+, assigning to Noam Rosenthal for commit.
Thank you. Waiting for other patches to be reviewed before committing, as there are interdependencies.
Comment on attachment 97231[details]
Patch 6: Glue LayerTreeHost with DrawingAreaProxy
View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:41
> class DrawingAreaProxyImpl : public DrawingAreaProxy {
I am surprised this is implemented in DrawingAreaProxyImpl since that means the implementation is not available for the tiled proxy. Shouldn't accelerated compositing be independent of the kind of proxy and implemented directly in DrawingAreaProxy?
(In reply to comment #50)
> (From update of attachment 97231[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review
>
> > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:41
> > class DrawingAreaProxyImpl : public DrawingAreaProxy {
>
> I am surprised this is implemented in DrawingAreaProxyImpl since that means the implementation is not available for the tiled proxy. Shouldn't accelerated compositing be independent of the kind of proxy and implemented directly in DrawingAreaProxy?
Slava, was there a reason for us not putting this in DrawingAreaProxy?
(In reply to comment #51)
> (In reply to comment #50)
> > (From update of attachment 97231[details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review
> >
> > > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:41
> > > class DrawingAreaProxyImpl : public DrawingAreaProxy {
> >
> > I am surprised this is implemented in DrawingAreaProxyImpl since that means the implementation is not available for the tiled proxy. Shouldn't accelerated compositing be independent of the kind of proxy and implemented directly in DrawingAreaProxy?
>
> Slava, was there a reason for us not putting this in DrawingAreaProxy?
I have implementation for tiled proxy. As I remember, we agreed to put it later, because layout tests work with DrawingAreaProxy only.
Code is here: https://gitorious.org/~ostapru/webkit/sls-webkit/blobs/wk2_merge6/Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp
It just few lines to make it work with TiledDrawingArea because it uses the same LayerTreeHostProxy.
Comment on attachment 97231[details]
Patch 6: Glue LayerTreeHost with DrawingAreaProxy
View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review
Quick review. This patch needs some cleaning.
> Source/WebKit2/UIProcess/DrawingAreaProxy.h:97
> + virtual void updateWebView() { }
> + virtual WebCore::IntRect viewportVisibleRect() const { return WebCore::IntRect(); }
> + virtual WebCore::IntRect contentsRect() const = 0;
> + virtual float contentsScale() const = 0;
> + virtual bool backingStoreReady() const { return false; }
Let's refactor that to share as much as possible between the two proxies.
What can be implemented in DrawingAreaProxy should be implemented here, including the LayerTreeHostProxy. The methods that are specific to a view should be pure virtual.
> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:83
> + DrawingAreaProxy* drawingAreaProxy() { return this; }
What the fuck???
> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> + WebPageProxy* webPageProxy() const { return m_webPageProxy; }
Wait, what?
> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:62
> + GraphicsLayer* layerByID(WebLayerID id) { return id ? m_layers.get(id).layer : 0; }
It is a bit mysterious why the id 0 cannot be associated with a layer. Usually the IDs of WK2 can wrap around when it overflows and the 0 is not a special value.
Out of curiousity, where are those IDs defined?
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> +GraphicsLayer* LayerTreeHostProxy::createLayer()
> +{
> + return new GraphicsLayerTextureMapper(this);
> +}
This is an open invitation for leaks, the return type should be PassOwnPtr. :)
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:52
> + if (!m_layers.contains(id))
> + continue;
> + delete m_layers.get(id).layer;
> + m_layers.remove(id);
You are doing 3 lookup of the exact same instance. It is O(1) each time but that is not free.
Use HashMap::find() to keep a reference on the value through the iterator.
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:58
> + for (LayerMap::iterator it = m_layers.begin(); it != m_layers.end(); ++it) {
We usually use a local variable for the end iterator since iterators are not necessarily free to instanciate.
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:60
> + if (it->second.layer != layer)
> + continue;
This pattern is weird and will get very ineffective if there can be many layer.
What is the call site for this method? It seems it should take an ID as a parameter instead of the layer. Alternatively, a second hashmap could be maintained for the painting case.
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:87
> +LayerTreeHostProxy::~LayerTreeHostProxy()
> +{
> +}
I think you can skip writing this :)
Comment on attachment 97319[details]
Patch 5: LayerTreeContext/LayerTreeHost with AC disabled by default until null handle problem fixed.
This doesn't compile anymore; and it would have to be re-tested with Qt5.
>
> > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> > + WebPageProxy* webPageProxy() const { return m_webPageProxy; }
>
> Wait, what?
What exactly is the issue with this? We need to expose the proxy so that the layerTreeHostProxy can access it.
> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> > +GraphicsLayer* LayerTreeHostProxy::createLayer()
> > +{
> > + return new GraphicsLayerTextureMapper(this);
> > +}
>
> This is an open invitation for leaks, the return type should be PassOwnPtr.
No can do. I'm saving the result in a HashMap; OwnPtrs are not copyable. Any other suggestion? I can start RefPtring everything but it's a mess.
(In reply to comment #58)
> >
> > > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> > > + WebPageProxy* webPageProxy() const { return m_webPageProxy; }
> >
> > Wait, what?
> What exactly is the issue with this? We need to expose the proxy so that the layerTreeHostProxy can access it.
It is private...
Exposing an attribute through a private accessor? ;)
> > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> > > +GraphicsLayer* LayerTreeHostProxy::createLayer()
> > > +{
> > > + return new GraphicsLayerTextureMapper(this);
> > > +}
> >
> > This is an open invitation for leaks, the return type should be PassOwnPtr.
> No can do. I'm saving the result in a HashMap; OwnPtrs are not copyable. Any other suggestion? I can start RefPtring everything but it's a mess.
That is a common pattern in WebKit, you can still use OwnPtr.
You keep it as a OwnPtr as far as you can, and manage the memory manually when you use it in the HashMap.
Created attachment 103834[details]
Patch: Enable accelerated compositing for QDesktopWebView
As a step towards enabling AC with tiling, we first of all want to finalize the cross-process aspect of accelerated compositing on Qt WebKit2.
This approach uses LayerTreeHost when the page is in compositing mode, deferring all rendering at that point to TextureMapper.
2011-03-23 10:43 PDT, Viatcheslav Ostapenko
2011-06-12 17:40 PDT, Noam Rosenthal
2011-06-12 17:47 PDT, Noam Rosenthal
2011-06-12 20:15 PDT, Noam Rosenthal
2011-06-12 20:49 PDT, Noam Rosenthal
2011-06-13 09:39 PDT, Noam Rosenthal
2011-06-13 09:43 PDT, Noam Rosenthal
2011-06-13 11:04 PDT, Noam Rosenthal
2011-06-13 16:57 PDT, Noam Rosenthal
2011-06-14 16:00 PDT, Noam Rosenthal
2011-06-14 16:31 PDT, Noam Rosenthal
2011-06-14 16:38 PDT, Noam Rosenthal
2011-06-14 22:16 PDT, Noam Rosenthal
2011-06-15 10:33 PDT, Viatcheslav Ostapenko
noam: commit-queue-
2011-08-12 15:55 PDT, Noam Rosenthal
2011-08-12 15:56 PDT, Noam Rosenthal
2011-08-12 16:18 PDT, Noam Rosenthal
2011-08-12 20:27 PDT, Noam Rosenthal