In texImage2D and texSubImage2D, the status of the Image is extracted and kept in the function GraphicsContext3D::getImageData() and has some inconvenience. There are no interface and guaranteed way to use the status of the Image outside of GraphicsContext3D::getImageData() safely. For example, you can not get the address of the Image data and operate it outside the scope of the function and there is at least a memory copy existed. The lifetime of some key status is constrained and only kept in in getImageData(). This prevents some potential optimizations and affect the flexibility of the code. It can be solved by adding a ImageLocker class to abstract the status of the Image. Then the lifetime and validity are determined by the lifetime of the object. It provides flexibility on code refactoring and optimizations.
Created attachment 176669 [details] Patch
Comment on attachment 176669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176669&action=review Thanks for doing this restructuring. It looks pretty good overall but a few changes are needed. Please upload a revised patch fixing these issues and I'll gladly r+ it. Please mark it cq? if you want it submitted to the commit queue (I strongly recommend you go through the commit queue since it touches nearly all the ports). Also, please don't forget to regenerate the ChangeLog if you rename the methods and classes as I suggested. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3629 > + void* imagePixelData = const_cast<void*>(imageLocker.imagePixelData()); Please fix the signature of texImage2DBase to take const void* instead of casting away const here. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3639 > + } It's WebKit policy to keep patches focused and not introduce unrelated changes like this optimization. I would prefer you do it in a second patch, but since it's pretty trivial I won't argue if you want to do it here. Same for texSubImage2DImpl below. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3879 > + void* imagePixelData = const_cast<void*>(imageLocker.imagePixelData()); Please fix the signature of texSubImage2DBase to take const void* instead of casting away const here. > Source/WebCore/platform/graphics/GraphicsContext3D.h:879 > + // flags. Returns true upon success. This comment is now out of date; please update it. In particular, ignoreGammaAndColorProfile is only given to the ImageLocker, and AlphaOp is now passed here directly. I think this method should also be renamed to "packImageData". Suggested text: Packs the contents of the given image, passed in |pixels|, into the passed Vector, according to the given format and type, and obeying the flipY and AlphaOp flags. Returns true upon success. > Source/WebCore/platform/graphics/GraphicsContext3D.h:882 > + class ImageLocker { Upon seeing how this class is used, I think it should be named ImageExtractor. See further comments below. > Source/WebCore/platform/graphics/GraphicsContext3D.h:883 > + public: Indent by two spaces here. > Source/WebCore/platform/graphics/GraphicsContext3D.h:885 > + bool getImageData(bool premultiplyAlpha, bool ignoreGammaAndColorProfile); getImageData shouldn't be public. Having it be public means it could be called multiple times and I don't think the implementations are prepared to handle that. Please change the constructor to take "Image* image, bool premultiplyAlpha, bool ignoreGammaAndColorProfile" and call getImageData (or whatever it ends up being called) inside the constructor. Set a flag called m_succeeded (e.g., "m_succeeded = getImageData(...)"), add a "bool succeeded()" method returning that flag, and in WebGLRenderingContext, call succeeded() instead of getImageData. Also, please document explicitly that each platform needs to implement the destructor and getImageData. The constructor should be the first public method in the class; please move it up. Also, I think getImageData should now be renamed to something else, like extractImage -- it doesn't behave like a getter any more. > Source/WebCore/platform/graphics/GraphicsContext3D.h:887 > + ImageLocker(Image* image) {m_image = image;}; Please move the body of the constructor into GraphicsContext3D.cpp, since it will become non-trivial. > Source/WebCore/platform/graphics/GraphicsContext3D.h:888 > + const void* imagePixelData() {return m_imagePixelData;}; Please add spaces inside the braces and get rid of the ';'s at the end of the lines here and below. > Source/WebCore/platform/graphics/GraphicsContext3D.h:894 > + private: Indent by two spaces here. > Source/WebCore/platform/graphics/GraphicsContext3D.h:902 > + CGImageRef cgImage; These three members need to use the m_ naming convention. > Source/WebCore/platform/graphics/GraphicsContext3D.h:917 > + private: Indentation is wrong here. Back up "private" by two spaces.
Created attachment 176925 [details] Patch
Kenneth, thanks for your comments and suggestions. I had made changes accordingly and submitted it to commit queue. Please help to add more reviewers if needed.
Comment on attachment 176925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176925&action=review Looks great. Thank you for pushing through this change. A couple of minor nits which are not blockers for committing in my opinion. If you wouldn't mind, please clean them up in a subsequent patch. Submitting this to the commit queue for you. r=me > Source/WebCore/platform/graphics/GraphicsContext3D.h:877 > + // according to the given format and type, and obeying the flipY and premultiplyAlpha flags. Typo: premultiplyAlpha -> AlphaOp > Source/WebCore/platform/graphics/GraphicsContext3D.h:900 > + // Return true upon success Please use complete sentences in comments (end each sentence with a period).
Comment on attachment 176925 [details] Patch Clearing flags on attachment: 176925 Committed r136282: <http://trac.webkit.org/changeset/136282>
All reviewed patches have been landed. Closing bug.