Skia can have textures attached to the GraphicsContext, so we can leverage that to avoid making copies from the CPU to the GPU.
Created attachment 163967 [details] Patch V1
Dean, if you want you can review only the non-Skia part and I will remove the Skia file and open a different bug for it. I need those FECustomFilter changes for https://bugs.webkit.org/show_bug.cgi?id=96668 . I've just put it all in one single patch to make it easier to understand why all those changes are needed.
Comment on attachment 163967 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=163967&action=review I'm happy with the non-Skia parts. As you mentioned, since I'm not a Skia person either someone from there should r+ that part, or split it off. > Source/WebCore/ChangeLog:11 > + There is a comment in FECustomFilterSkia.cpp explaining how this works. After the refactory FECustomFilter takes typo "refactoring" > Source/WebCore/ChangeLog:12 > + an input texture and outputs to a destionation texture. I've used Skia to draw the input ImageBuffer to a typo "destination" > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:2 > - * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved. > + * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved. I think you want to keep 2011 in there as well. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:181 > + bool needsInputTexture = programNeedsInputTexture(); > + if (needsInputTexture && (filterDrawType == NEEDS_INPUT_TEXTURE) && !ensureInputTexture()) You can move the programNeedsInputTexture() into the conditional since you never use needsInputTexture again. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-174 > -#if !PLATFORM(BLACKBERRY) // BlackBerry defines its own Texture class. > - // Do not draw the filter if the input image cannot fit inside a single GPU texture. > - if (m_inputTexture->tiles().numTilesX() != 1 || m_inputTexture->tiles().numTilesY() != 1) > - return false; > -#endif I'm a little confused here. Where did this code move to? Does createTexture duplicate this? > Source/WebCore/platform/graphics/filters/skia/FECustomFilterSkia.cpp:16 > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER âAS ISâ AND ANY You have some copy and paste encoding issue here.
(In reply to comment #3) > (From update of attachment 163967 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163967&action=review > Thanks Dean! > I'm happy with the non-Skia parts. As you mentioned, since I'm not a Skia person either someone from there should r+ that part, or split it off. I will try to find someone today. > > > Source/WebCore/ChangeLog:11 > > + There is a comment in FECustomFilterSkia.cpp explaining how this works. After the refactory FECustomFilter takes > > typo "refactoring" I wish check webkit style had a spell checker builtin. :) > > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:2 > > - * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved. > > + * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved. > > I think you want to keep 2011 in there as well. Ok. > > > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:181 > > + bool needsInputTexture = programNeedsInputTexture(); > > + if (needsInputTexture && (filterDrawType == NEEDS_INPUT_TEXTURE) && !ensureInputTexture()) > > You can move the programNeedsInputTexture() into the conditional since you never use needsInputTexture again. Ok. > > > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-174 > > -#if !PLATFORM(BLACKBERRY) // BlackBerry defines its own Texture class. > > - // Do not draw the filter if the input image cannot fit inside a single GPU texture. > > - if (m_inputTexture->tiles().numTilesX() != 1 || m_inputTexture->tiles().numTilesY() != 1) > > - return false; > > -#endif > > I'm a little confused here. Where did this code move to? Does createTexture duplicate this? I've changed the m_inputTexture to just use the texture directly, so this part of the code moved into FECustomFilter::resizeContextIfNeeded where I check for the width and height of the texture. This way Blackberry could also enable the FECustomFilter. > > Source/WebCore/platform/graphics/filters/skia/FECustomFilterSkia.cpp:16 > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER âAS ISâ AND ANY > > You have some copy and paste encoding issue here. Ok.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 163967 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=163967&action=review > > > Thanks Dean! > > > I'm happy with the non-Skia parts. As you mentioned, since I'm not a Skia person either someone from there should r+ that part, or split it off. > > I will try to find someone today. I've extracted all non-Skia related changes to https://bugs.webkit.org/show_bug.cgi?id=96801
Created attachment 164218 [details] Patch V2 Removed the FECustomFilter changes as those were integrated in a different patch.
Comment on attachment 164218 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=164218&action=review The changes look reasonable, but I am no familiar with the Gr* classes. Like Dean suggested, you should ask some Skia folks to give a final review. > Source/WebCore/platform/graphics/filters/skia/FECustomFilterSkia.cpp:103 > + GraphicsContext context(&inputPlatformContextSkia); I am a bit confused. This is platform dependent code, why do you use the layer GraphicsContext instead of SkPaint?
Comment on attachment 164218 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=164218&action=review Thanks for taking a look Dirk. I think Stephen White would be the best person to review this patch. >>> Source/WebCore/platform/graphics/filters/skia/FECustomFilterSkia.cpp:103 >>> + GraphicsContext context(&inputPlatformContextSkia); >> >> I am a bit confused. This is platform dependent code, why do you use the layer GraphicsContext instead of SkPaint? > > I am a bit confused. This is platform dependent code, why do you use the layer GraphicsContext instead of SkPaint? I'm drawing a platform independent type "Image", so I don't want to duplicate the code that's already written for that. Is there a better way to extract a SkBitmap out of the Image object?
Comment on attachment 164218 [details] Patch V2 Skia-specific, so removing review flags.
Skia-specific, so closing the bug.