Bug 96579 – [CSS Shaders][Skia] Add FECustomFilterSkia.cpp to implement a faster way to run the custom filters

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Alexandru Chiculita]

Skia can have textures attached to the GraphicsContext, so we can leverage that to avoid making copies from the CPU to the GPU.

[Alexandru Chiculita]

Created attachment 163967 [details]
Patch V1

[Alexandru Chiculita]

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.

[Dean Jackson]

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.

[Alexandru Chiculita]

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

[Alexandru Chiculita]

(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

[Alexandru Chiculita]

Created attachment 164218 [details]
Patch V2

Removed the FECustomFilter changes as those were integrated in a different patch.

[Dirk Schulze]

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?

[Dirk Schulze]

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?

[Alexandru Chiculita]

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?

[Darin Adler]

Comment on attachment 164218 [details]
Patch V2

Skia-specific, so removing review flags.

[Darin Adler]

Skia-specific, so closing the bug.