Bug 77148 – [Qt][Texmap] Refactor TextureMapper API to use ImageBuffers when possible.

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

[Noam Rosenthal]

TextureMapper's backing-store code (tiling/uploads) is messy and hard to maintain. Time to give it some love.

[Noam Rosenthal]

Created attachment 124186 [details]
Patch 1: Clean up TextureMapper's content-upload code

[Kenneth Rohde Christiansen]

Comment on attachment 124186 [details]
Patch 1: Clean up TextureMapper's content-upload code

View in context: https://bugs.webkit.org/attachment.cgi?id=124186&action=review

It is pretty amazing that you are able to remove so much code

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:615
> +        swizzleRgb(reinterpret_cast<uint32_t*>(qtImage.bits()), qtImage.size());

werent you going to rename this method just jocelyn asked you to?

[Noam Rosenthal]

(In reply to comment #2)
> (From update of attachment 124186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124186&action=review
> 
> It is pretty amazing that you are able to remove so much code
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:615
> > +        swizzleRgb(reinterpret_cast<uint32_t*>(qtImage.bits()), qtImage.size());
> 
> werent you going to rename this method just jocelyn asked you to?

He asked that I add a comment in WebGraphicsLayer what I'm swizzling to/from. Here it should be inferred from the code but I can add a comment as well.

[Jocelyn Turcotte]

Comment on attachment 124186 [details]
Patch 1: Clean up TextureMapper's content-upload code

View in context: https://bugs.webkit.org/attachment.cgi?id=124186&action=review

Went through it and it looks good, just got two nitpicks.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:589
> +static void swizzleRgb(uint32_t* data, const IntSize& size)

It's called swizzleRGB with capitals in ShareableBitmap, maybe it's worth to keep the names consistent.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:123
> +void swizzleRgb(Image*);

It's defined static and used only in TextureMapperGL.cpp.
Does this belong in TextureMapper.h?

[Noam Rosenthal]

Created attachment 124325 [details]
Patch

[Noam Rosenthal]

After some deeper tests with Instruments on Mac, this seems to still create unnecessary image copies on WebKit2.
I've decided to combine it with another patch that switches TextureMapperQt to a cross-platform implementation that uses ImageBuffer instead of QPixmap; Originally I thought of spliitting it to two patches, but a lot of the changes from the first patch don't make sense when the second patch is there...
So I'm working on a patch which moves things around in TextureMapper API and erases a lot of redundant code, but doesn't change any behavior or creates additional copies.

[Noam Rosenthal]

Created attachment 124497 [details]
Patch

Patch might be big but most of it is deletions :)

[WebKit Review Bot]

Attachment 124497 [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/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:60:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:86:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11366468

[Early Warning System Bot]

Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11367458

[Gyuyoung Kim]

Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11359783

[WebKit Review Bot]

Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11354004

[Kenneth Rohde Christiansen]

Comment on attachment 124497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124497&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:420
> +#if PLATFORM(QT)
> +        void concatTransform(const TransformationMatrix&);
> +        void setTransform(const TransformationMatrix&);
> +        TransformationMatrix getTransform() const;
> +#else
> +        void concatTransform(const TransformationMatrix& transform) { concatCTM(transform.toAffine()); }
> +        void setTransform(const TransformationMatrix& transform) { setCTM(transform.toAffine()); }
> +        TransformationMatrix getTransform() const { return TransformationMatrix(getCTM()); }
> +#endif

Maybe a comment here would be nice to know why this is needed for Qt. Isn't it a bit confusing to have both concatTransform and concatCTM?

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:121
> +
> +void BitmapTextureImageBuffer::updateContents(void* data, const IntRect& targetRect, PixelFormat format)
> +{

Maybe a comment here would be helpful for other ports?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:85
> +    enum AccelerationMode { Accelerated, Unaccelerated };

In the case someone adds native directx support some day, would Software, OpenGL make more sense?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:191
> +    // This suld be a reference copy only.

suld*?

[Noam Rosenthal]

Created attachment 124641 [details]
Patch

[Noam Rosenthal]

Created attachment 124643 [details]
Patch

[WebKit Review Bot]

Attachment 124643 [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/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:86:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 124643 [details]
Patch

Attachment 124643 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11366793

[Noam Rosenthal]

Created attachment 124664 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 124664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124664&action=review

Looks good to me, maybe someone else wants to have a look?

> Source/WebCore/ChangeLog:18
> +        Also, Functions that use TransformationMatrix were added to GraphicsContext, to allow for

functions*

> Source/WebCore/platform/graphics/GraphicsContext.cpp:781
> +// These fuctions should be implemented to support software-rendering of 3d transforms.

3D

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:590
> -static void swizzleBGRAToRGBA(uint32_t* data, const IntSize& size)
> +static void swizzleRGB(uint32_t* data, const IntSize& size)

wasnt the old name better?

[Noam Rosenthal]

Created attachment 124715 [details]
Patch

Addressed Kenneth's comments, fixed warnings.

[WebKit Review Bot]

Attachment 124715 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   3b3325c..baf92d92 master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106355 = 3b3325cd5a20bf357b8fdad5c0c1dcb448125ec4
r106356 = baf92d9262faea6b0ee0b2ada47f821fc179c331
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

(In reply to comment #20)
I know it's probably is difficult to split, but this patch is very difficult to follow.
I couldn't find anything to improve after looking at it, though I surely missed something.

[Noam Rosenthal]

(In reply to comment #22)
> (In reply to comment #20)
> I know it's probably is difficult to split, but this patch is very difficult to follow.
It's impossible to split without breaking something :(
However, it's a large patch mostly due to deletions. The main thing the patch actually does is move stuff around.

[Noam Rosenthal]

Anything I still need to explain/modify about this patch?

[Jocelyn Turcotte]

(In reply to comment #23)
> It's impossible to split without breaking something :(
> However, it's a large patch mostly due to deletions. The main thing the patch actually does is move stuff around.

If you say that most of this code was moved, you have my (pretty low value) vote.
I guess that progressively fixing introduced regression, at this stage, would take less efforts than chewing on that patch anyway.

[Noam Rosenthal]

> If you say that most of this code was moved, you have my (pretty low value) vote.
Your vote is high-value :)

> I guess that progressively fixing introduced regression, at this stage, would take less efforts than chewing on that patch anyway.

Also, this patch does not at all affect the mainstream code-path that uses GL, only the software fallback.

[Martin Robinson]

Comment on attachment 124715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124715&action=review

Just a couple questions before I finish the review. Looks nice! I'm glad that we can share the ImageBuffer code with you.

> Source/WebCore/platform/graphics/GraphicsContext.h:413
> +#if ENABLE(3D_RENDERING)

You might want to have this be ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER). Then you could move the empty implementation out of Source/WebCore/platform/graphics/GraphicsContext.cpp.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:193
> +#if ENABLE(3D_RENDERING)
> +    context->concat3DTransform(matrix);
> +#else
> +    context->concatCTM(matrix);
> +#endif

This is repeated a bit. Did you consider moving the #ifdefs into concat3DTransform and the other new methods on GraphicsContext.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:189
> +    {
> +        context->setImageInterpolationQuality(textureMapper->imageInterpolationQuality());
> +        context->setTextDrawingMode(textureMapper->textDrawingMode());
> +        context->translate(-dirtyRect.x(), -dirtyRect.y());
> +        layer->paintGraphicsLayerContents(*context, dirtyRect);
> +        if (m_currentContent.contentType == DirectImageContentType)
> +            context->drawImage(m_currentContent.image.get(), ColorSpaceDeviceRGB, m_state.contentsRect);
> +    }

What is the purpose of this new inner scope?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:191
> +    // This suld be a reference copy only.

suld -> should

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:192
> +    RefPtr<Image> image = imageBuffer->copyImage();

I believe this is a deep copy on Cairo, at least. You may need to pass a copy behavior here. Looks like Cairo has not implemented the shallow copy behavior.

[Noam Rosenthal]

(In reply to comment #27)
> (From update of attachment 124715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124715&action=review
> 
> Just a couple questions before I finish the review. Looks nice! I'm glad that we can share the ImageBuffer code with you.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:413
> > +#if ENABLE(3D_RENDERING)
> 
> You might want to have this be ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER). Then you could move the empty implementation out of Source/WebCore/platform/graphics/GraphicsContext.cpp.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:193
> > +#if ENABLE(3D_RENDERING)
> > +    context->concat3DTransform(matrix);
> > +#else
> > +    context->concatCTM(matrix);
> > +#endif
> 
> This is repeated a bit. Did you consider moving the #ifdefs into concat3DTransform and the other new methods on GraphicsContext.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:189
> > +    {
> > +        context->setImageInterpolationQuality(textureMapper->imageInterpolationQuality());
> > +        context->setTextDrawingMode(textureMapper->textDrawingMode());
> > +        context->translate(-dirtyRect.x(), -dirtyRect.y());
> > +        layer->paintGraphicsLayerContents(*context, dirtyRect);
> > +        if (m_currentContent.contentType == DirectImageContentType)
> > +            context->drawImage(m_currentContent.image.get(), ColorSpaceDeviceRGB, m_state.contentsRect);
> > +    }
> 
> What is the purpose of this new inner scope?
>
Not needed, it was there before because we a GraphicsContext on the stack. I'll remove that.

> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:191
> > +    // This suld be a reference copy only.
> 
> suld -> should
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:192
> > +    RefPtr<Image> image = imageBuffer->copyImage();
> 
> I believe this is a deep copy on Cairo, at least. You may need to pass a copy behavior here. Looks like Cairo has not implemented the shallow copy behavior.

Ah, yes. I need to specify DontCopyBackingStore, which is not really implemented in Qt/GTK. I don't think it's in the scope of this patch though, maybe I should add it as a FIXME?

[Noam Rosenthal]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 124715 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124715&action=review
> > 
> > Just a couple questions before I finish the review. Looks nice! I'm glad that we can share the ImageBuffer code with you.
> > 
> > > Source/WebCore/platform/graphics/GraphicsContext.h:413
> > > +#if ENABLE(3D_RENDERING)
> > 
> > You might want to have this be ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER). Then you could move the empty implementation out of Source/WebCore/platform/graphics/GraphicsContext.cpp.

OK, I don't mind - I just thought that concat3DTransform is not inherently specific to TextureMapper, it's just used by TextureMapper.

[Noam Rosenthal]

Created attachment 125205 [details]
Revised patch based on Martin Robinson's comments.

[Noam Rosenthal]

Created attachment 125206 [details]
Revised patch based on Martin Robinson's comments.

[Martin Robinson]

Comment on attachment 125206 [details]
Revised patch based on Martin Robinson's comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=125206&action=review

Okay. I think I understand enough of this to give an r+ now. Very nice. The only change I request is to move the ImageBuffer implementations to another file (perhaps TextureMapperImageBuffer.cpp/h), to reduce the number of classes per-file. Eventually I think it would be nice to move the BitmapTexture classes to their own files too.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:1338
> +#if ENABLE(3D_RENDERING)

I think you are missing a && USE(TEXTURE_MAPPER) here.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:80
> +class BitmapTextureImageBuffer : public BitmapTexture {
> +    friend class TextureMapperImageBuffer;
> +public:

I think it makes sense to split out the ImageBuffer implementation into a new file.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:194
> +    context->save();
> +    context->setAlpha(opacity);
> +#if ENABLE(3D_RENDERING)
> +    context->concat3DTransform(matrix);
> +#else
> +    context->concatCTM(matrix);
> +#endif
> +    context->drawImageBuffer(image, ColorSpaceDeviceRGB, targetRect);

It seems that if alpha == 1 and the transform is just a 2D translation we could avoid a copy? Perhaps not, but if so it might be a good fast path to add for the future!

[Noam Rosenthal]

Created attachment 125214 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125214 [details]
Patch

Clearing flags on attachment: 125214

Committed r106620: <http://trac.webkit.org/changeset/106620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke the build:

../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp: In member function 'virtual void WebCore::TextureMapperImageBuffer::beginClip(const WebCore::TransformationMatrix&, const WebCore::FloatRect&)':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:58: error: 'const class WebCore::TransformationMatrix' has no member named 'toAffine'
../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp: In member function 'virtual void WebCore::TextureMapperImageBuffer::drawTexture(const WebCore::BitmapTexture&, const WebCore::FloatRect&, const WebCore::TransformationMatrix&, float, const WebCore::BitmapTexture*)':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:98: error: no matching function for call to 'WebCore::GraphicsContext::concatCTM(const WebCore::TransformationMatrix&)'
../../../../Source/WebCore/platform/graphics/GraphicsContext.h:409: note: candidates are: void WebCore::GraphicsContext::concatCTM(const WebCore::AffineTransform&)
I

[Csaba Osztrogonác]

It seems that the false branch of ENABLE(3D_RENDERING) is broken.
( Qt ARM and Windows build is broken because of this bug. )

[Csaba Osztrogonác]

Sorry, but I had to roll it out to make builds work again: http://trac.webkit.org/changeset/106630

Please reland with fixed build.

[Noam Rosenthal]

Created attachment 125313 [details]
Fixed the non-3d-rendering path

[WebKit Review Bot]

Comment on attachment 125313 [details]
Fixed the non-3d-rendering path

Clearing flags on attachment: 125313

Committed r106659: <http://trac.webkit.org/changeset/106659>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because the build is still broken:

In file included from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:21:0:
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h: In static member function 'static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapper::platformCreateAccelerated()':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:124:16: error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::TextureMapper>' requested
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:125:5: warning: control reaches end of non-void function

[Csaba Osztrogonác]

Comment on attachment 125313 [details]
Fixed the non-3d-rendering path

View in context: https://bugs.webkit.org/attachment.cgi?id=125313&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:126
> +#if USE(TEXTURE_MAPPER_GL)
> +    static PassOwnPtr<TextureMapper> platformCreateAccelerated();
> +#else
> +    static PassOwnPtr<TextureMapper> platformCreateAccelerated()
> +    {
> +        return 0;
> +    }
> +#endif

It broke the Qt ARM build:

In file included from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:21:
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h: In static member function 'static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapper::platformCreateAccelerated()':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:124: error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::TextureMapper>' requested
In file included from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:24,
                 from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:21:
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h: In static member function 'static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapper::platformCreateAccelerated()':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:124: error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::TextureMapper>' requested

[Noam Rosenthal]

Created attachment 125346 [details]
Attempt at a patch that doesn't break the ARM bot

[Csaba Osztrogonác]

(In reply to comment #44)
> Created an attachment (id=125346) [details]
> Attempt at a patch that doesn't break the ARM bot

It won't apply to trunk, because your wrong patch is still in it.

[Csaba Osztrogonác]

Additionally Balázs landed a speculative fix - http://trac.webkit.org/changeset/106668