Summary: | [CSS Shaders][Skia] Add FECustomFilterSkia.cpp to implement a faster way to run the custom filters | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||
Component: | Layout and Rendering | Assignee: | Alexandru Chiculita <achicu> | ||||||
Status: | RESOLVED WONTFIX | ||||||||
Severity: | Normal | CC: | darin, dino, senorblanco, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 85086, 96692 | ||||||||
Attachments: |
|
Description
Alexandru Chiculita
2012-09-12 17:07:34 PDT
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 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. |