Bug 77148

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

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?

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

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?

Created attachment 124325 [details] Patch

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.

Created attachment 124497 [details] Patch Patch might be big but most of it is deletions :)

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.

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

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

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

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

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*?

Created attachment 124641 [details] Patch

Created attachment 124643 [details] Patch

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.

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

Created attachment 124664 [details] Patch

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?

Created attachment 124715 [details] Patch Addressed Kenneth's comments, fixed warnings.

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.

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

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

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

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

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

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.

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

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

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

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

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!

Created attachment 125214 [details] Patch

Comment on attachment 125214 [details] Patch Clearing flags on attachment: 125214 Committed r106620: <http://trac.webkit.org/changeset/106620>

All reviewed patches have been landed. Closing bug.

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

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

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

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

Comment on attachment 125313 [details] Fixed the non-3d-rendering path Clearing flags on attachment: 125313 Committed r106659: <http://trac.webkit.org/changeset/106659>

All reviewed patches have been landed. Closing bug.

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

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

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

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

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