Bug 60439 - [Texmap][Qt] Upstream texture-mapper changes from Qt's WebKit2 branch
: [Texmap][Qt] Upstream texture-mapper changes from Qt's WebKit2 branch
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
: Qt
Depends on: 68808
Blocks: 47068
  Show dependency treegraph
 
Reported: 2011-05-07 10:23 PDT by Noam Rosenthal
Modified: 2011-09-26 09:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch 1: TextureMapperPlatformLayer API changes (4.36 KB, patch)
2011-05-07 19:07 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 2: TextureMapper API changes (6.69 KB, patch)
2011-05-07 19:07 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 3: pro/pri files build fixes (2.57 KB, patch)
2011-05-07 19:08 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 4: Enable 3D in the build scripts and settings (3.70 KB, patch)
2011-05-07 19:09 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 5: Qt backend changes (7.27 KB, patch)
2011-05-07 19:10 PDT, Noam Rosenthal
kenneth: review+
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch 6: Media player changes (7.18 KB, patch)
2011-05-07 19:11 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 7: WebGL changes (6.73 KB, patch)
2011-05-07 19:11 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 8: GL backend (38.47 KB, patch)
2011-05-07 19:12 PDT, Noam Rosenthal
kenneth: review+
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch 9: TextureMapperNode refactoring (the big patch) (101.21 KB, patch)
2011-05-07 19:13 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 10: Glue layer (22.88 KB, patch)
2011-05-07 19:13 PDT, Noam Rosenthal
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch 2: TextureMapper API changes (6.42 KB, patch)
2011-05-07 19:28 PDT, Noam Rosenthal
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch 10: Glue layer (21.99 KB, patch)
2011-05-07 19:38 PDT, Noam Rosenthal
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch 10: Glue layer (22.32 KB, patch)
2011-05-07 19:52 PDT, Noam Rosenthal
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch 10: Glue layer (22.64 KB, patch)
2011-05-07 20:05 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 4: Enable 3D in the build scripts and settings (3.15 KB, patch)
2011-05-07 20:16 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 9: TextureMapperNode refactoring (the big patch) (101.35 KB, patch)
2011-05-08 12:49 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 10: Glue layer (24.44 KB, patch)
2011-05-08 12:50 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 10: Glue layer (24.41 KB, patch)
2011-05-08 12:58 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 4: Enable 3D in the build scripts and settings (3.66 KB, patch)
2011-05-08 19:20 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 4: Enable 3D in the build scripts and settings (3.66 KB, patch)
2011-05-08 19:28 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
92740: Patch 9: TextureMapperNode refactoring (the big patch) (74.79 KB, patch)
2011-05-09 18:16 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch: build fix for plugins (1.03 KB, patch)
2011-05-09 18:20 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch: add accelerated animation support to TextureMapper (24.79 KB, patch)
2011-05-09 20:01 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch 9: TextureMapperNode refactoring (74.37 KB, patch)
2011-05-10 15:10 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2011-05-07 10:23:12 PDT
Over the last few months several fixes were made to TextureMapper in a branch, including performance improvements, better integration API, internal animation support and more.
This is an attempt to upstream all these changes.

The staging branch is at https://gitorious.org/~ostapru/webkit/sls-webkit, branch merge-contd
Comment 1 Noam Rosenthal 2011-05-07 19:06:05 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.
Comment 2 Noam Rosenthal 2011-05-07 19:07:07 PDT
Created attachment 92707 [details]
Patch 1: TextureMapperPlatformLayer API changes
Comment 3 Noam Rosenthal 2011-05-07 19:07:34 PDT
Created attachment 92708 [details]
Patch 2: TextureMapper API changes
Comment 4 Noam Rosenthal 2011-05-07 19:08:10 PDT
Created attachment 92709 [details]
Patch 3: pro/pri files build fixes
Comment 5 Noam Rosenthal 2011-05-07 19:09:22 PDT
Created attachment 92710 [details]
Patch 4: Enable 3D in the build scripts and settings
Comment 6 Noam Rosenthal 2011-05-07 19:10:22 PDT
Created attachment 92711 [details]
Patch 5: Qt backend changes
Comment 7 Noam Rosenthal 2011-05-07 19:11:14 PDT
Created attachment 92712 [details]
Patch 6: Media player changes
Comment 8 Noam Rosenthal 2011-05-07 19:11:41 PDT
Created attachment 92713 [details]
Patch 7: WebGL changes
Comment 9 Noam Rosenthal 2011-05-07 19:12:11 PDT
Created attachment 92714 [details]
Patch 8: GL backend
Comment 10 Noam Rosenthal 2011-05-07 19:13:09 PDT
Created attachment 92715 [details]
Patch 9: TextureMapperNode refactoring (the big patch)
Comment 11 Noam Rosenthal 2011-05-07 19:13:38 PDT
Created attachment 92716 [details]
Patch 10: Glue layer
Comment 12 Early Warning System Bot 2011-05-07 19:17:01 PDT
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 13 Early Warning System Bot 2011-05-07 19:25:07 PDT
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
Comment 14 Noam Rosenthal 2011-05-07 19:28:12 PDT
Created attachment 92717 [details]
Patch 2: TextureMapper API changes
Comment 15 WebKit Review Bot 2011-05-07 19:31:19 PDT
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 16 Early Warning System Bot 2011-05-07 19:32:35 PDT
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 17 Early Warning System Bot 2011-05-07 19:38:28 PDT
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
Comment 18 Noam Rosenthal 2011-05-07 19:38:49 PDT
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 19 Early Warning System Bot 2011-05-07 19:42:38 PDT
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 20 Early Warning System Bot 2011-05-07 19:48:35 PDT
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
Comment 21 Noam Rosenthal 2011-05-07 19:52:25 PDT
Created attachment 92720 [details]
Patch 10: Glue layer
Comment 22 Early Warning System Bot 2011-05-07 20:01:17 PDT
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
Comment 23 Noam Rosenthal 2011-05-07 20:05:13 PDT
Created attachment 92721 [details]
Patch 10: Glue layer
Comment 24 Noam Rosenthal 2011-05-07 20:16:43 PDT
Created attachment 92722 [details]
Patch 4: Enable 3D in the build scripts and settings
Comment 25 Kenneth Rohde Christiansen 2011-05-08 04:33:05 PDT
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 26 Kenneth Rohde Christiansen 2011-05-08 04:37:42 PDT
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 27 Kenneth Rohde Christiansen 2011-05-08 04:40:47 PDT
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 28 Kenneth Rohde Christiansen 2011-05-08 04:46:54 PDT
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 29 Kenneth Rohde Christiansen 2011-05-08 04:49:04 PDT
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 30 Kenneth Rohde Christiansen 2011-05-08 04:55:21 PDT
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 31 Kenneth Rohde Christiansen 2011-05-08 05:43:08 PDT
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 32 Kenneth Rohde Christiansen 2011-05-08 09:15:32 PDT
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 33 Kenneth Rohde Christiansen 2011-05-08 09:21:13 PDT
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?
Comment 34 Noam Rosenthal 2011-05-08 09:50:29 PDT
> > 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.
Comment 35 Noam Rosenthal 2011-05-08 09:51:09 PDT
> > 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.
Comment 36 Noam Rosenthal 2011-05-08 09:51:52 PDT
(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
Comment 37 Noam Rosenthal 2011-05-08 10:26:33 PDT
> > +    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.
Comment 38 Noam Rosenthal 2011-05-08 10:37:51 PDT
(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.
Comment 39 Noam Rosenthal 2011-05-08 12:49:01 PDT
Created attachment 92740 [details]
Patch 9: TextureMapperNode refactoring (the big patch)

Fixed Kenneth's prior comments
Comment 40 Noam Rosenthal 2011-05-08 12:50:14 PDT
Created attachment 92741 [details]
Patch 10: Glue layer

Fixed based on Kenneth's comments (mainly moved TextureMapperNodeClientQt to an OwnPtr)
Comment 41 WebKit Review Bot 2011-05-08 12:52:53 PDT
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.
Comment 42 Noam Rosenthal 2011-05-08 12:58:34 PDT
Created attachment 92742 [details]
Patch 10: Glue layer
Comment 43 Noam Rosenthal 2011-05-08 19:20:57 PDT
Created attachment 92753 [details]
Patch 4: Enable 3D in the build scripts and settings
Comment 44 Noam Rosenthal 2011-05-08 19:28:30 PDT
Created attachment 92754 [details]
Patch 4: Enable 3D in the build scripts and settings
Comment 45 Kenneth Rohde Christiansen 2011-05-09 01:10:48 PDT
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 46 Kenneth Rohde Christiansen 2011-05-09 01:43:40 PDT
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?
Comment 47 Noam Rosenthal 2011-05-09 18:16:10 PDT
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
Comment 48 Noam Rosenthal 2011-05-09 18:20:21 PDT
Created attachment 92902 [details]
Patch: build fix for plugins
Comment 49 Noam Rosenthal 2011-05-09 20:01:57 PDT
Created attachment 92911 [details]
Patch: add accelerated animation support to TextureMapper

This is the animation portion of the original patch (9)
Comment 50 Kenneth Rohde Christiansen 2011-05-10 02:33:43 PDT
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.
Comment 51 Noam Rosenthal 2011-05-10 07:57:03 PDT
> > 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.
Comment 52 Noam Rosenthal 2011-05-10 15:10:18 PDT
Created attachment 93021 [details]
Patch 9: TextureMapperNode refactoring

Fixed more of Kenneth's comments.
Comment 53 Kenneth Rohde Christiansen 2011-05-11 02:43:05 PDT
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
Comment 54 Noam Rosenthal 2011-05-11 14:03:28 PDT
Committed r86264: <http://trac.webkit.org/changeset/86264>
Comment 55 Noam Rosenthal 2011-05-11 14:06:23 PDT
Committed r86265: <http://trac.webkit.org/changeset/86265>
Comment 56 Noam Rosenthal 2011-05-11 14:09:57 PDT
Committed r86266: <http://trac.webkit.org/changeset/86266>
Comment 57 WebKit Review Bot 2011-05-11 14:13:15 PDT
http://trac.webkit.org/changeset/86265 might have broken Qt Linux Release minimal
Comment 58 Noam Rosenthal 2011-05-11 14:14:52 PDT
Committed r86268: <http://trac.webkit.org/changeset/86268>
Comment 59 Noam Rosenthal 2011-05-11 14:18:25 PDT
Committed r86269: <http://trac.webkit.org/changeset/86269>
Comment 60 Noam Rosenthal 2011-05-11 15:11:32 PDT
Committed r86276: <http://trac.webkit.org/changeset/86276>
Comment 61 Noam Rosenthal 2011-05-11 15:23:25 PDT
(In reply to comment #57)
> http://trac.webkit.org/changeset/86265 might have broken Qt Linux Release minimal

Fixed later on.
Comment 62 Benjamin Poulain 2011-08-01 05:25:30 PDT
(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 :)
Comment 63 Noam Rosenthal 2011-08-01 07:54:17 PDT
(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.
Comment 64 Noam Rosenthal 2011-08-01 08:45:36 PDT
Added bug https://bugs.webkit.org/show_bug.cgi?id=65473 to track the BGRA issue.