Summary: | [Texmap][Qt] Upstream texture-mapper changes from Qt's WebKit2 branch | ||
---|---|---|---|
Product: | WebKit | Reporter: | Noam Rosenthal <noam> |
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | abarth, benjamin, eric, kenneth, webkit.review.bot, yael |
Priority: | P2 | Keywords: | Qt |
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | 68808 | ||
Bug Blocks: | 47068 | ||
Attachments: |
Description
Noam Rosenthal
2011-05-07 10:23:12 PDT
I've divided the patches to 10: 1. Changes to TextureMapperPlatformLayer API 2. Changes to TextureMapper API 3. Build changes 4. Allow 3D by default 5. Qt backend 6. media player fixes 7. WebGL fixes 8. GL backend 9. The "meaty" changes to TextureMapperNode 10. Glue layer (PageClientQt and friends). If more granularity is needed, I can try though it's kind of hard to separate the code into pieces like that. In any case, the history is available in the webkit/qtwebkit-webkit2-dev branch on gitorious so we wouldn't gain much from splitting it more. Created attachment 92707 [details]
Patch 1: TextureMapperPlatformLayer API changes
Created attachment 92708 [details]
Patch 2: TextureMapper API changes
Created attachment 92709 [details]
Patch 3: pro/pri files build fixes
Created attachment 92710 [details]
Patch 4: Enable 3D in the build scripts and settings
Created attachment 92711 [details]
Patch 5: Qt backend changes
Created attachment 92712 [details]
Patch 6: Media player changes
Created attachment 92713 [details]
Patch 7: WebGL changes
Created attachment 92714 [details]
Patch 8: GL backend
Created attachment 92715 [details]
Patch 9: TextureMapperNode refactoring (the big patch)
Created attachment 92716 [details]
Patch 10: Glue layer
Comment on attachment 92708 [details] Patch 2: TextureMapper API changes Attachment 92708 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8653103 Comment on attachment 92711 [details] Patch 5: Qt backend changes Attachment 92711 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8640116 Created attachment 92717 [details]
Patch 2: TextureMapper API changes
Attachment 92717 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:33: "TextureMapperQt.h" already included at Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:24 [build/include] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 92714 [details] Patch 8: GL backend Attachment 92714 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8655112 Comment on attachment 92716 [details] Patch 10: Glue layer Attachment 92716 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8645092 Created attachment 92718 [details]
Patch 10: Glue layer
The failed patches are due to a non-protected incude inside PageClientQt. That's rectified in Patch 10.
Comment on attachment 92717 [details] Patch 2: TextureMapper API changes Attachment 92717 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8645095 Comment on attachment 92718 [details] Patch 10: Glue layer Attachment 92718 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8653113 Created attachment 92720 [details]
Patch 10: Glue layer
Comment on attachment 92720 [details] Patch 10: Glue layer Attachment 92720 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8660112 Created attachment 92721 [details]
Patch 10: Glue layer
Created attachment 92722 [details]
Patch 4: Enable 3D in the build scripts and settings
Comment on attachment 92707 [details] Patch 1: TextureMapperPlatformLayer API changes View in context: https://bugs.webkit.org/attachment.cgi?id=92707&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:56 > -typedef TextureMapperPlatformLayer PlatformLayer; > +typedef const TextureMapperPlatformLayer PlatformLayer; So what is someone does "const PlatformLayer"? Wouldn't it be better to leave out the const? Comment on attachment 92708 [details] Patch 2: TextureMapper API changes View in context: https://bugs.webkit.org/attachment.cgi?id=92708&action=review > Source/WebCore/ChangeLog:11 > + 1. an entry/exit point for painting (beginPaint/endPaint) An* > Source/WebCore/platform/graphics/texmap/TextureMapper.h:97 > + virtual void drawTexture(const BitmapTexture&, const FloatRect& target, const TransformationMatrix& modelViewMatrix = TransformationMatrix(), float opacity = 1.0f, const BitmapTexture* maskTexture = 0) = 0; im not sure that the modelView adds value, it actually just make it more confusing to me > Source/WebCore/platform/graphics/texmap/TextureMapper.h:108 > + void setViewportSize(const IntSize& s) { m_viewportSize = s; } I think writing size makes more sense Comment on attachment 92722 [details] Patch 4: Enable 3D in the build scripts and settings View in context: https://bugs.webkit.org/attachment.cgi?id=92722&action=review > Source/WebKit/qt/Api/qwebsettings.cpp:177 > + settings->setAcceleratedCompositingFor3DTransformsEnabled(true); > + settings->setAcceleratedCompositingForAnimationEnabled(true); > + settings->setAcceleratedCompositingForVideoEnabled(true); why true, and not value? Comment on attachment 92711 [details] Patch 5: Qt backend changes View in context: https://bugs.webkit.org/attachment.cgi?id=92711&action=review > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:103 > QPainter* painter = m_currentSurface ? &m_currentSurface->m_painter : m_painter; You are using this in different places? make an inline private method for it? > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:113 > + QPainter* painter = m_currentSurface ? &m_currentSurface->m_painter : m_painter; like here > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:119 > + return IntSize(m_painter->device()->width(), m_painter->device()->height()); What if there is a currentSurface? Comment on attachment 92712 [details] Patch 6: Media player changes View in context: https://bugs.webkit.org/attachment.cgi?id=92712&action=review r+ if you fix my comments > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:615 > +void MediaPlayerPrivateQt::paintToTextureMapper(TextureMapper* textureMapper, > + const FloatRect& targetRect, > + const TransformationMatrix& matrix, > + float opacity, > + BitmapTexture*) const Please put on one line before committing! > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.h:115 > + virtual void paintToTextureMapper(TextureMapper*, > + const FloatRect& targetRect, > + const TransformationMatrix&, > + float opacity, > + BitmapTexture* mask) const; One line please Comment on attachment 92713 [details] Patch 7: WebGL changes View in context: https://bugs.webkit.org/attachment.cgi?id=92713&action=review How useful is this fallback path? > Source/WebCore/ChangeLog:11 > + No new tests. Tests in LayoutTests/compositing cover this. compositing? webgl tests are there? > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:182 > + virtual void paintToTextureMapper(TextureMapper*, > + const FloatRect& target, > + const TransformationMatrix&, > + float opacity, > + BitmapTexture* mask) const; one line please Comment on attachment 92721 [details] Patch 10: Glue layer View in context: https://bugs.webkit.org/attachment.cgi?id=92721&action=review > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:69 > + if (m_frame->d) Does the frame not always have a private? You dont test for this elsewhere > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:293 > + textureMapperNodeClient = new TextureMapperNodeClientQt(page->mainFrame(), layer); Can't we use an OwnPtr? > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:305 > + delete textureMapperNodeClient; > + textureMapperNodeClient = 0; OwnPtr? Comment on attachment 92714 [details] Patch 8: GL backend View in context: https://bugs.webkit.org/attachment.cgi?id=92714&action=review Already mostly reviewed in person with No'am at the WebKit contributors meeting > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:169 > + GLuint findOrCreate(ImageUid image, bool& found) I would use ImageUID imageID or similar > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:346 > -" gl_FragColor = vec4(color.rgb * Opacity, color.a * Opacity); \n" > +" gl_FragColor = vec4(color.rgba * Opacity); \n" For consistency reasons you should add more spaces before the \n > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:420 > + drawTexture(textureGL.id(), textureGL.isOpaque(), textureGL.relativeSize(), targetRect, matrix, opacity, mask, false); What is false here? pls make that clear /* flip */ > Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:91 > +typedef uint64_t ImageUid; I think ImageUID would be a better type Comment on attachment 92715 [details] Patch 9: TextureMapperNode refactoring (the big patch) View in context: https://bugs.webkit.org/attachment.cgi?id=92715&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:133 > + void setOpacity(float o) { m_opacity = o; } I prefer value over o > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:184 > + TransformationMatrix target, replica, forDescendants, local, base, perspective; We normally would declare these on separate lines > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:321 > +inline void deleteOwnedPtr(TextureMapperNode* ptr) > +{ > + delete ptr; > +} Huh? why is this necessary? > > Source/WebCore/platform/graphics/texmap/TextureMapper.h:97
> > + virtual void drawTexture(const BitmapTexture&, const FloatRect& target, const TransformationMatrix& modelViewMatrix = TransformationMatrix(), float opacity = 1.0f, const BitmapTexture* maskTexture = 0) = 0;
>
> im not sure that the modelView adds value, it actually just make it more confusing to me
Otherwise it's a check-webkit-style error. Apparently check-webkit-style doesn't like TransformationMatrix& matrix = TransformationMatrix(). But I can put it in and declare it a false positieve.
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:321
> > +inline void deleteOwnedPtr(TextureMapperNode* ptr)
> > +{
> > + delete ptr;
> > +}
>
> Huh? why is this necessary?
Oops... Not necessary. Will remove.
(In reply to comment #31) > (From update of attachment 92721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92721&action=review > > > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:69 > > + if (m_frame->d) Fair enough > Does the frame not always have a private? You dont test for this elsewhere > > > Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:293 > > + textureMapperNodeClient = new TextureMapperNodeClientQt(page->mainFrame(), layer); > > Can't we use an OwnPtr? OK > > + return IntSize(m_painter->device()->width(), m_painter->device()->height());
>
> What if there is a currentSurface?
This is the size that's used to decide what size a new surface should be. It's not affected by the existence of a current surface.
(In reply to comment #30) > (From update of attachment 92713 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92713&action=review > > How useful is this fallback path? It shows WebGL inside CSS when we're in a QWebView. This is copied from the non-textureMapper version. > > > Source/WebCore/ChangeLog:11 > > + No new tests. Tests in LayoutTests/compositing cover this. > > compositing? webgl tests are there? Yes. Created attachment 92740 [details]
Patch 9: TextureMapperNode refactoring (the big patch)
Fixed Kenneth's prior comments
Created attachment 92741 [details]
Patch 10: Glue layer
Fixed based on Kenneth's comments (mainly moved TextureMapperNodeClientQt to an OwnPtr)
Attachment 92741 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/WebCoreSupport/PageClientQt.h:49: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebKit/qt/WebCoreSupport/PageClientQt.h:49: The parameter name "layer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebKit/qt/WebCoreSupport/PageClientQt.h:51: The parameter name "textureMapper" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 3 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 92742 [details]
Patch 10: Glue layer
Created attachment 92753 [details]
Patch 4: Enable 3D in the build scripts and settings
Created attachment 92754 [details]
Patch 4: Enable 3D in the build scripts and settings
Comment on attachment 92742 [details] Patch 10: Glue layer View in context: https://bugs.webkit.org/attachment.cgi?id=92742&action=review > Source/WebKit/qt/WebCoreSupport/PageClientQt.h:50 > + void scroll(float dx, float dy); scrollBy ? > Source/WebKit/qt/WebCoreSupport/PageClientQt.h:52 > + virtual ~TextureMapperNodeClientQt(); Why not place it next to the ctor? > Source/WebKit/qt/WebCoreSupport/PageClientQt.h:57 > +protected: Are you inheriting from this class? Comment on attachment 92740 [details] Patch 9: TextureMapperNode refactoring (the big patch) View in context: https://bugs.webkit.org/attachment.cgi?id=92740&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:82 > + virtual bool addAnimation(const KeyframeValueList&, const IntSize& /*boxSize*/, const Animation*, const String& /*keyframesName*/, double /*timeOffset*/); No need to do /*boxSize*/, just write boxSize > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:68 > + void markTextureUsage(BitmapTexture* texture) mark usage? shouldnt it have a bool then? what about markTextureUsed ? So this method finds the texture, removes it and prepends it? I guess we need to find a better name for the method > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:162 > +#if TEXTURE_MAPPER_MINIMAL_COMPUTATIONS would we add all this TEXTURE_MAPPER_MINIMAL_COMPUTATIONS in a separate patch? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:270 > +static const float gTileDimension = 1024.0; Maybe add to the top? in an empty namespace? Created attachment 92900 [details]
92740: Patch 9: TextureMapperNode refactoring (the big patch)
Divided patch 9 to 4 patches:
- the main patch
- animations
- minimal-computations
- cache
Created attachment 92902 [details]
Patch: build fix for plugins
Created attachment 92911 [details]
Patch: add accelerated animation support to TextureMapper
This is the animation portion of the original patch (9)
Comment on attachment 92911 [details] Patch: add accelerated animation support to TextureMapper View in context: https://bugs.webkit.org/attachment.cgi?id=92911&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:674 > +void TextureMapperNode::syncCompositingStateInternal(GraphicsLayerTextureMapper* graphicsLayer, bool traverseDescendants, TextureMapper* textureMapper) wouldn't it makes sense to have the traverseDescentants after the textureMapper? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:682 > + m_state.maskLayer->syncCompositingStateInternal(toGraphicsLayerTextureMapper(graphicsLayer->maskLayer()), false, textureMapper); I dislike the Internal part... then it is better to find a more proper name > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:684 > + if (m_state.maskLayer->m_size.isEmpty()) > + m_state.maskLayer->m_size = m_size; why only when it is empty? comment? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:688 > + m_state.replicaLayer->syncCompositingStateInternal(toGraphicsLayerTextureMapper(graphicsLayer->replicaLayer()), false, textureMapper); can the false be replaced by an enum? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:692 > + bool visibleRectDirty = true; IsDirty > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:843 > + return; why not just break; ? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:66 > + static PassRefPtr<TextureMapperAnimation> create(const KeyframeValueList& values) { return adoptRef(new TextureMapperAnimation(values)); } I would put that one at the top. > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:674 > > +void TextureMapperNode::syncCompositingStateInternal(GraphicsLayerTextureMapper* graphicsLayer, bool traverseDescendants, TextureMapper* textureMapper) > > wouldn't it makes sense to have the traverseDescentants after the textureMapper? OK > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:682 > > + m_state.maskLayer->syncCompositingStateInternal(toGraphicsLayerTextureMapper(graphicsLayer->maskLayer()), false, textureMapper); > > I dislike the Internal part... then it is better to find a more proper name Actually it seems unnecessary now. I should put everything into syncCompositing state. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:684 > > + if (m_state.maskLayer->m_size.isEmpty()) > > + m_state.maskLayer->m_size = m_size; > > why only when it is empty? comment? It's a default. Sometimes the mask layer has its own size. Will add a comment. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:688 > > + m_state.replicaLayer->syncCompositingStateInternal(toGraphicsLayerTextureMapper(graphicsLayer->replicaLayer()), false, textureMapper); > > can the false be replaced by an enum? Yes. Created attachment 93021 [details]
Patch 9: TextureMapperNode refactoring
Fixed more of Kenneth's comments.
Comment on attachment 93021 [details] Patch 9: TextureMapperNode refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=93021&action=review Great stuff, nice to finally get our delta upstreamed > Source/WebCore/ChangeLog:14 > + 1. Support the new TextureMapperPlatformLayer for media & WebGL. > + 2. Use a pool for intermediate surfaces, to avoid constant allocating/freeing of textures. > + 3. Divide computation operations to different smaller functions. > + 4. Get rid of scissor/clip layers, use transformed clip instead. > + 5. Allow tiling for big layers. In the future it would be nice with these as separate patches, but as we have gone thru these patches together in real life, I accept it this time :-) > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:395 > + if (m_state.replicaLayer && !options.isSurface) > + m_currentContent.media->paintToTextureMapper(options.textureMapper, targetRect, > + TransformationMatrix(m_transforms.target).multiply(m_state.replicaLayer->m_transforms.local), > + opacity * m_state.replicaLayer->m_opacity, > + replicaMaskTexture ? replicaMaskTexture.get() : maskTexture.get()); That if needs braces Committed r86264: <http://trac.webkit.org/changeset/86264> Committed r86265: <http://trac.webkit.org/changeset/86265> Committed r86266: <http://trac.webkit.org/changeset/86266> http://trac.webkit.org/changeset/86265 might have broken Qt Linux Release minimal Committed r86268: <http://trac.webkit.org/changeset/86268> Committed r86269: <http://trac.webkit.org/changeset/86269> Committed r86276: <http://trac.webkit.org/changeset/86276> (In reply to comment #57) > http://trac.webkit.org/changeset/86265 might have broken Qt Linux Release minimal Fixed later on. (In reply to comment #60) > Committed r86276: <http://trac.webkit.org/changeset/86276> In this commit, was there a review for "Patch 12/12: Enable accelerated animations in texture-mapper. The entire interpolation"? I don't see it. The following is changed without explanation and I don't see it in the comments: -GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, m_dirtyRect.x(), m_dirtyRect.y(), m_dirtyRect.width(), m_dirtyRect.height(), GL_RGBA, GL_UNSIGNED_BYTE, m_buffer->data())) +GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, m_dirtyRect.x(), m_dirtyRect.y(), m_dirtyRect.width(), m_dirtyRect.height(), GL_BGRA, GL_UNSIGNED_BYTE, m_buffer->data())) The annoying part being GL_BGRA does not exist on OpenGL ES (see http://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexImage2D.xml) so this does not compile on ARM anymore. The use of the format BGRA is kind of mysterious since the buffer is a RGBA32PremultimpliedBuffer. The format + the fact that the original data is premultiplied makes BitmapTextureGL::endPaint() pretty mysterious without explanations :) (In reply to comment #62) > (In reply to comment #60) > > Committed r86276: <http://trac.webkit.org/changeset/86276> > > In this commit, was there a review for > "Patch 12/12: Enable accelerated animations in texture-mapper. The entire interpolation"? I don't see it. Some of this was reviewed offline. > > > The following is changed without explanation and I don't see it in the comments: > -GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, m_dirtyRect.x(), m_dirtyRect.y(), m_dirtyRect.width(), m_dirtyRect.height(), GL_RGBA, GL_UNSIGNED_BYTE, m_buffer->data())) > +GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, m_dirtyRect.x(), m_dirtyRect.y(), m_dirtyRect.width(), m_dirtyRect.height(), GL_BGRA, GL_UNSIGNED_BYTE, m_buffer->data())) > > > The annoying part being GL_BGRA does not exist on OpenGL ES (see http://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexImage2D.xml) so this does not compile on ARM anymore. > > The use of the format BGRA is kind of mysterious since the buffer is a RGBA32PremultimpliedBuffer. > The format + the fact that the original data is premultiplied makes BitmapTextureGL::endPaint() pretty mysterious without explanations :) We need to add a software based RGBA->BGRA for platforms that don't support it, or put it in the shader. Added bug https://bugs.webkit.org/show_bug.cgi?id=65473 to track the BGRA issue. |