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
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Qt
: 68808
: 47068
  Show dependency treegraph
 
Reported: 2011-05-07 10:23 PST by
Modified: 2011-09-26 09:39 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-07 10:23:12 PST
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 From 2011-05-07 19:06:05 PST -------
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 From 2011-05-07 19:07:07 PST -------
Created an attachment (id=92707) [details]
Patch 1: TextureMapperPlatformLayer API changes
------- Comment #3 From 2011-05-07 19:07:34 PST -------
Created an attachment (id=92708) [details]
Patch 2: TextureMapper API changes
------- Comment #4 From 2011-05-07 19:08:10 PST -------
Created an attachment (id=92709) [details]
Patch 3: pro/pri files build fixes
------- Comment #5 From 2011-05-07 19:09:22 PST -------
Created an attachment (id=92710) [details]
Patch 4: Enable 3D in the build scripts and settings
------- Comment #6 From 2011-05-07 19:10:22 PST -------
Created an attachment (id=92711) [details]
Patch 5: Qt backend changes
------- Comment #7 From 2011-05-07 19:11:14 PST -------
Created an attachment (id=92712) [details]
Patch 6: Media player changes
------- Comment #8 From 2011-05-07 19:11:41 PST -------
Created an attachment (id=92713) [details]
Patch 7: WebGL changes
------- Comment #9 From 2011-05-07 19:12:11 PST -------
Created an attachment (id=92714) [details]
Patch 8: GL backend
------- Comment #10 From 2011-05-07 19:13:09 PST -------
Created an attachment (id=92715) [details]
Patch 9: TextureMapperNode refactoring (the big patch)
------- Comment #11 From 2011-05-07 19:13:38 PST -------
Created an attachment (id=92716) [details]
Patch 10: Glue layer
------- Comment #12 From 2011-05-07 19:17:01 PST -------
(From update of attachment 92708 [details])
Attachment 92708 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8653103
------- Comment #13 From 2011-05-07 19:25:07 PST -------
(From update of attachment 92711 [details])
Attachment 92711 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8640116
------- Comment #14 From 2011-05-07 19:28:12 PST -------
Created an attachment (id=92717) [details]
Patch 2: TextureMapper API changes
------- Comment #15 From 2011-05-07 19:31:19 PST -------
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 From 2011-05-07 19:32:35 PST -------
(From update of attachment 92714 [details])
Attachment 92714 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8655112
------- Comment #17 From 2011-05-07 19:38:28 PST -------
(From update of attachment 92716 [details])
Attachment 92716 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8645092
------- Comment #18 From 2011-05-07 19:38:49 PST -------
Created an attachment (id=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 From 2011-05-07 19:42:38 PST -------
(From update of attachment 92717 [details])
Attachment 92717 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8645095
------- Comment #20 From 2011-05-07 19:48:35 PST -------
(From update of attachment 92718 [details])
Attachment 92718 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8653113
------- Comment #21 From 2011-05-07 19:52:25 PST -------
Created an attachment (id=92720) [details]
Patch 10: Glue layer
------- Comment #22 From 2011-05-07 20:01:17 PST -------
(From update of attachment 92720 [details])
Attachment 92720 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8660112
------- Comment #23 From 2011-05-07 20:05:13 PST -------
Created an attachment (id=92721) [details]
Patch 10: Glue layer
------- Comment #24 From 2011-05-07 20:16:43 PST -------
Created an attachment (id=92722) [details]
Patch 4: Enable 3D in the build scripts and settings
------- Comment #25 From 2011-05-08 04:33:05 PST -------
(From update of attachment 92707 [details])
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 From 2011-05-08 04:37:42 PST -------
(From update of attachment 92708 [details])
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 From 2011-05-08 04:40:47 PST -------
(From update of attachment 92722 [details])
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 From 2011-05-08 04:46:54 PST -------
(From update of attachment 92711 [details])
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 From 2011-05-08 04:49:04 PST -------
(From update of attachment 92712 [details])
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 From 2011-05-08 04:55:21 PST -------
(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?

> 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 From 2011-05-08 05:43:08 PST -------
(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)

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 From 2011-05-08 09:15:32 PST -------
(From update of attachment 92714 [details])
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 From 2011-05-08 09:21:13 PST -------
(From update of attachment 92715 [details])
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 From 2011-05-08 09:50:29 PST -------
> > 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 From 2011-05-08 09:51:09 PST -------
> > 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 From 2011-05-08 09:51:52 PST -------
(In reply to comment #31)
> (From update of attachment 92721 [details] [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 From 2011-05-08 10:26:33 PST -------
> > +    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 From 2011-05-08 10:37:51 PST -------
(In reply to comment #30)
> (From update of attachment 92713 [details] [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 From 2011-05-08 12:49:01 PST -------
Created an attachment (id=92740) [details]
Patch 9: TextureMapperNode refactoring (the big patch)

Fixed Kenneth's prior comments
------- Comment #40 From 2011-05-08 12:50:14 PST -------
Created an attachment (id=92741) [details]
Patch 10: Glue layer

Fixed based on Kenneth's comments (mainly moved TextureMapperNodeClientQt to an OwnPtr)
------- Comment #41 From 2011-05-08 12:52:53 PST -------
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 From 2011-05-08 12:58:34 PST -------
Created an attachment (id=92742) [details]
Patch 10: Glue layer
------- Comment #43 From 2011-05-08 19:20:57 PST -------
Created an attachment (id=92753) [details]
Patch 4: Enable 3D in the build scripts and settings
------- Comment #44 From 2011-05-08 19:28:30 PST -------
Created an attachment (id=92754) [details]
Patch 4: Enable 3D in the build scripts and settings
------- Comment #45 From 2011-05-09 01:10:48 PST -------
(From update of attachment 92742 [details])
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 From 2011-05-09 01:43:40 PST -------
(From update of attachment 92740 [details])
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 From 2011-05-09 18:16:10 PST -------
Created an attachment (id=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 From 2011-05-09 18:20:21 PST -------
Created an attachment (id=92902) [details]
Patch: build fix for plugins
------- Comment #49 From 2011-05-09 20:01:57 PST -------
Created an attachment (id=92911) [details]
Patch: add accelerated animation support to TextureMapper

This is the animation portion of the original patch (9)
------- Comment #50 From 2011-05-10 02:33:43 PST -------
(From update of attachment 92911 [details])
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 From 2011-05-10 07:57:03 PST -------
> > 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 From 2011-05-10 15:10:18 PST -------
Created an attachment (id=93021) [details]
Patch 9: TextureMapperNode refactoring

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

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