Bug 60439 – [Texmap][Qt] Upstream texture-mapper changes from Qt's WebKit2 branch

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

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

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 92707 [details]
Patch 1: TextureMapperPlatformLayer API changes

[Noam Rosenthal]

Created attachment 92708 [details]
Patch 2: TextureMapper API changes

[Noam Rosenthal]

Created attachment 92709 [details]
Patch 3: pro/pri files build fixes

[Noam Rosenthal]

Created attachment 92710 [details]
Patch 4: Enable 3D in the build scripts and settings

[Noam Rosenthal]

Created attachment 92711 [details]
Patch 5: Qt backend changes

[Noam Rosenthal]

Created attachment 92712 [details]
Patch 6: Media player changes

[Noam Rosenthal]

Created attachment 92713 [details]
Patch 7: WebGL changes

[Noam Rosenthal]

Created attachment 92714 [details]
Patch 8: GL backend

[Noam Rosenthal]

Created attachment 92715 [details]
Patch 9: TextureMapperNode refactoring (the big patch)

[Noam Rosenthal]

Created attachment 92716 [details]
Patch 10: Glue layer

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Noam Rosenthal]

Created attachment 92717 [details]
Patch 2: TextureMapper API changes

[WebKit Review Bot]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Noam Rosenthal]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Noam Rosenthal]

Created attachment 92720 [details]
Patch 10: Glue layer

[Early Warning System Bot]

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

[Noam Rosenthal]

Created attachment 92721 [details]
Patch 10: Glue layer

[Noam Rosenthal]

Created attachment 92722 [details]
Patch 4: Enable 3D in the build scripts and settings

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

> > 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.

[Noam Rosenthal]

> > 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.

[Noam Rosenthal]

(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

[Noam Rosenthal]

> > +    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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Created attachment 92740 [details]
Patch 9: TextureMapperNode refactoring (the big patch)

Fixed Kenneth's prior comments

[Noam Rosenthal]

Created attachment 92741 [details]
Patch 10: Glue layer

Fixed based on Kenneth's comments (mainly moved TextureMapperNodeClientQt to an OwnPtr)

[WebKit Review Bot]

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.

[Noam Rosenthal]

Created attachment 92742 [details]
Patch 10: Glue layer

[Noam Rosenthal]

Created attachment 92753 [details]
Patch 4: Enable 3D in the build scripts and settings

[Noam Rosenthal]

Created attachment 92754 [details]
Patch 4: Enable 3D in the build scripts and settings

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

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

[Noam Rosenthal]

Created attachment 92902 [details]
Patch: build fix for plugins

[Noam Rosenthal]

Created attachment 92911 [details]
Patch: add accelerated animation support to TextureMapper

This is the animation portion of the original patch (9)

[Kenneth Rohde Christiansen]

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.

[Noam Rosenthal]

> > 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.

[Noam Rosenthal]

Created attachment 93021 [details]
Patch 9: TextureMapperNode refactoring

Fixed more of Kenneth's comments.

[Kenneth Rohde Christiansen]

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

[Noam Rosenthal]

Committed r86264: <http://trac.webkit.org/changeset/86264>

[Noam Rosenthal]

Committed r86265: <http://trac.webkit.org/changeset/86265>

[Noam Rosenthal]

Committed r86266: <http://trac.webkit.org/changeset/86266>

[WebKit Review Bot]

http://trac.webkit.org/changeset/86265 might have broken Qt Linux Release minimal

[Noam Rosenthal]

Committed r86268: <http://trac.webkit.org/changeset/86268>

[Noam Rosenthal]

Committed r86269: <http://trac.webkit.org/changeset/86269>

[Noam Rosenthal]

Committed r86276: <http://trac.webkit.org/changeset/86276>

[Noam Rosenthal]

(In reply to comment #57)
> http://trac.webkit.org/changeset/86265 might have broken Qt Linux Release minimal

Fixed later on.

[Benjamin Poulain]

(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 :)

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Added bug https://bugs.webkit.org/show_bug.cgi?id=65473 to track the BGRA issue.