Bug 98989 – [CSS Shaders] Add CustomFilterRenderer to reuse this class by Accelerated Compositing.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

Extract CustomFilterRenderer class from the rendering part of FECustomFilter. FECustomFilter now plays a role in extending FilterEffect and delegates rendering css shaders to CustomFilterRenderer.

CustomFilterRenderer does not know Filter and FilterEffect. We can create a CustomFilterRenderer instance with only CustomFilterGlobalContext and CustomFilterOperation. It means that Accelerated Compositing can create the CustomFilterRenderer instance if Accelerated Compositing has CustomFilterGlobalContext and CustomFilterOperation, and it is possible if we refactor CustomFilterGlobalContext little.

This patch prepares to enable CSS Shaders on Accelerated Compositing.

[Dongseong Hwang]

Created attachment 168138 [details]
Patch

[Dean Jackson]

Alex + Max may have comments here.

[Dongseong Hwang]

(In reply to comment #2)
> Alex + Max may have comments here.

Thanks. I'm looking forward!

[Alexandru Chiculita]

Comment on attachment 168138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168138&action=review

Looks good! Thanks for the patch.

> Source/WebCore/ChangeLog:16
> +        CustomFilterGlobalContext and CustomFilterOperation, and it is possible if we

I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it.

> Source/WebCore/ChangeLog:21
> +        No new tests. Covered by css1/filters/custom

nit: typo css1 -> css3

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:55
> +PassRefPtr<CustomFilterRenderer> CustomFilterRenderer::create(CustomFilterGlobalContext* customFilterGlobalContext, CustomFilterOperation* operation)

This layer should not use the CustomFilterOperation directly. I would rather keep the same constructor as the FECustomFilter had before.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148
> +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size)

I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:474
> +static void orthogonalProjectionMatrix(TransformationMatrix& matrix, float left, float right, float bottom, float top)
> +{
> +    ASSERT(matrix.isIdentity());
> +

I think static methods would better be placed at the beginning of the file.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.h:67
> +    enum CustomFilterDrawType {
> +        NEEDS_INPUT_TEXTURE,
> +        NO_INPUT_TEXTURE

I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory.

For the accelerated compositor you will need a different render method that will take a source texture ID and a destination texture ID, so maybe we could add it back in a different form at that time.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:44
> +FECustomFilter::FECustomFilter(Filter* filter, PassRefPtr<CustomFilterRenderer> renderer)

I think FECustomFilter should create its own CustomFilterRenderer and not be passed one. CustomFilterRenderer would then be just an implementation detail.

[Dongseong Hwang]

(In reply to comment #4)
> (From update of attachment 168138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168138&action=review

Thank you for your review!

> > Source/WebCore/ChangeLog:16
> > +        CustomFilterGlobalContext and CustomFilterOperation, and it is possible if we
> 
> I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it.

Ok. I will make FECustomFilter have no dependency on CustomFilterOperation. I think it is better for FECustomFilter to not know CustomFilterOperation.
IMHO, each GraphicsLayer uses FilterOperation directly. Moreover, TextureMapperLayer and TextureMaperGL heavily use FilterOperation, too.


> > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148
> > +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size)
> 
> I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back.

You're right. The name is poor. I'll more refactor CustomFilterRenderer for the Accelerated Pipeline.


> > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.h:67
> > +    enum CustomFilterDrawType {
> > +        NEEDS_INPUT_TEXTURE,
> > +        NO_INPUT_TEXTURE
> 
> I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory.

Yes, I can and I want :)
I want to file and post another bug removing CustomFilterDrawType which this bug depends on.

I have a question.
There is the CustomFilterProgramType enumeration in CustomFilterProgramInfo.h. Is it also useless?

enum CustomFilterProgramType {
    PROGRAM_TYPE_NO_ELEMENT_TEXTURE,
    PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE
};

CustomFilterProgramType is set only in StyleResolver.

PassRefPtr<CustomFilterOperation> StyleResolver::createCustomFilterOperation(WebKitCSSFilterValue* filterValue)
{
    ...
    CustomFilterProgramType programType = PROGRAM_TYPE_NO_ELEMENT_TEXTURE;
    ...
        if (fragmentShaderOrMixFunction->isWebKitCSSMixFunctionValue()) {
            programType = PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE;
        ...
}

I don't understand why only mix(<uri> ..) needs the DOM element texture. I think just <uri> fragmentShader also needs the DOM element texture.
http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragmentShader-attribute

Could I remove CustomFilterProgramType also?

[Dongseong Hwang]

(In reply to comment #5)
> > I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory.
> I don't understand why only mix(<uri> ..) needs the DOM element texture. I think just <uri> fragmentShader also needs the DOM element texture.
> http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragmentShader-attribute
> 
> Could I remove CustomFilterProgramType also?

I understand my old question is wrong after reading spec: "38.2.1. Fragment Shaders Differences with GLSL." http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragment-shaders-differences-with-glsl

I understand <uri> fragmentShader can not use the DOM element texture.  <uri> fragmentShader can just render a simple shader like "gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);". Am I right?

As you mentioned, we just need to remove CustomFilterDrawType. I'll do that.

[Dongseong Hwang]

Created attachment 169381 [details]
Patch

[Dongseong Hwang]

(In reply to comment #4)
> (From update of attachment 168138 [details])
> I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it.

Done.

> nit: typo css1 -> css3

Done.

> This layer should not use the CustomFilterOperation directly. I would rather keep the same constructor as the FECustomFilter had before.

Done.

> > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148
> > +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size)
> 
> I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back.

The previous patch had made FECustomFilter delegate uploading texture, rendering, managing fbo and reading back pixel to CustomFilterRenderer. However, this patch only delegates rendering css shaders to CustomFilterRenderer because Accelerated Compositing needs CustomFilterRenderer only to render css shaders.
FECustomFilter has jobs:  uploading texture, managing fbo and reading back pixel as it has done. CustomFilterRenderer::paint will be used by both FECustomFilter and Accelerated Compositing.

> I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory.
> 
> For the accelerated compositor you will need a different render method that will take a source texture ID and a destination texture ID, so maybe we could add it back in a different form at that time.

Done.

> I think FECustomFilter should create its own CustomFilterRenderer and not be passed one. CustomFilterRenderer would then be just an implementation detail.

Done.

[Dongseong Hwang]

I enabled CSS Shaders on Texture Mapper in Bug 98990. It is the first enabling CSS Shaders on Accelerated Compositing.

[Alexandru Chiculita]

Comment on attachment 169381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169381&action=review

Thanks! Looks good, I like the new approach where the CustomFilterRenderer is not messing around with textures anymore.

I have a few minor comments. Going this way, it looks like CustomFilterRenderer could be really decoupled from the Global Context and that would simplify a bunch of the other checks.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:76
> +PassRefPtr<CustomFilterRenderer> CustomFilterRenderer::create(CustomFilterGlobalContext* customFilterGlobalContext, const CustomFilterProgramInfo& programInfo, const CustomFilterParameterList& parameters,
> +    unsigned meshRows, unsigned meshColumns, CustomFilterOperation::MeshBoxType meshBoxType, CustomFilterOperation::MeshType meshType)
> +{

I would remove the need to pass the customFIlterGlobalContext at this point. It looks like you only need the context and the validated program. Let's make this class as simple as possible, so that it can be embedded anywhere.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:116
> +    m_contextSize = size;

Can you add a fixme and a new bug to remove m_contextSize from the CustomFilterRenderer? I think we would need something like CustomFilterRendererState that will contain the size and maybe other parameters in the future. We should pass that to bindProgramBuffers instead of storing it.

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:145
> +    // FIXME: Sharing the mesh would just save the time needed to upload it to the GPU, so I assume we could
> +    // benchmark that for performance.
> +    // https://bugs.webkit.org/show_bug.cgi?id=88429
> +    m_mesh = CustomFilterMesh::create(m_context.get(), m_meshColumns, m_meshRows, FloatRect(0, 0, 1, 1), m_meshType);

Move this to a separate method called initializeMeshIfNeeded(), so that it is not guarded by the first return. It should not be an issue, but looks weird to have it in a method called initializeCompiledProgram.

Also, make the call after checking that the compiled program is initialized in prepareForDrawing.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:68
> +    RefPtr<FECustomFilter> feCustomFilter = adoptRef(new FECustomFilter(filter, customFilterGlobalContext, programInfo, parameters, meshRows, meshColumns, meshBoxType, meshType));

It looks like you could remove the dependency on customFilterGlobalContext and just pass in the validated program + the GraphicsContext3D directly. That will simplify a bunch of the checks we do here.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:72
> +    ASSERT(feCustomFilter->m_context.get());

Right now the context is never null, but it might be in the future if shaders are disabled after a GPU reset.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-122
> -    if (!m_context)

At this point there is no guarantee that the m_context is not 0. I think you should put this back in.

[Dongseong Hwang]

Created attachment 170116 [details]
Patch

[Dongseong Hwang]

Comment on attachment 169381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169381&action=review

>> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:76
>> +{
> 
> I would remove the need to pass the customFIlterGlobalContext at this point. It looks like you only need the context and the validated program. Let's make this class as simple as possible, so that it can be embedded anywhere.

Thanks for your careful review!

Done.

>> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:116
>> +    m_contextSize = size;
> 
> Can you add a fixme and a new bug to remove m_contextSize from the CustomFilterRenderer? I think we would need something like CustomFilterRendererState that will contain the size and maybe other parameters in the future. We should pass that to bindProgramBuffers instead of storing it.

Done.
I filed Bug 100107.

>> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:145
>> +    m_mesh = CustomFilterMesh::create(m_context.get(), m_meshColumns, m_meshRows, FloatRect(0, 0, 1, 1), m_meshType);
> 
> Move this to a separate method called initializeMeshIfNeeded(), so that it is not guarded by the first return. It should not be an issue, but looks weird to have it in a method called initializeCompiledProgram.
> 
> Also, make the call after checking that the compiled program is initialized in prepareForDrawing.

Done.

>> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:68
>> +    RefPtr<FECustomFilter> feCustomFilter = adoptRef(new FECustomFilter(filter, customFilterGlobalContext, programInfo, parameters, meshRows, meshColumns, meshBoxType, meshType));
> 
> It looks like you could remove the dependency on customFilterGlobalContext and just pass in the validated program + the GraphicsContext3D directly. That will simplify a bunch of the checks we do here.

Done. You are right. The bunch of the checks are only in createCustomFilterEffect now.

>> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:72
>> +    ASSERT(feCustomFilter->m_context.get());
> 
> Right now the context is never null, but it might be in the future if shaders are disabled after a GPU reset.

Done.

>> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-122
>> -    if (!m_context)
> 
> At this point there is no guarantee that the m_context is not 0. I think you should put this back in.

I put ASSERT(m_context) because we have guarantee that the m_context is not 0. We set m_context in the constructor of FECustomFilter. createCustomFilterEffect in FilterEffectRenderer.cpp dose not create FECustomFilter if the m_context is 0.
Moreover, many methods of FECustomFilter already use the m_context without the checks. IMHO if we put this back in, other folks will be confused when reading here.

[Build Bot]

Comment on attachment 170116 [details]
Patch

Attachment 170116 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14486902

[Dongseong Hwang]

Created attachment 170234 [details]
Patch

[Dongseong Hwang]

(In reply to comment #14)
> Created an attachment (id=170234) [details]
> Patch

I fixed build fail on mac due to
CustomFilterRenderer.cpp:79:173: error: unused parameter 'programInfo' [-Werror,-Wunused-parameter]

[Max Vujovic]

Comment on attachment 170234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170234&action=review

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:79
> +CustomFilterRenderer::CustomFilterRenderer(PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo&, const CustomFilterParameterList& parameters,

Do we need to pass in CustomFilterProgramInfo separately? Can ask for validatedProgram->programInfo() instead?

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:97
> +    return m_validatedProgram->programInfo().programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE;

Like this.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:47
> +FECustomFilter::FECustomFilter(Filter* filter, PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo& programInfo, const CustomFilterParameterList& parameters,

Ditto regarding CustomFilterProgramInfo.

[Dongseong Hwang]

(In reply to comment #16)
> (From update of attachment 170234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170234&action=review
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:79
> > +CustomFilterRenderer::CustomFilterRenderer(PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo&, const CustomFilterParameterList& parameters,
> 
> Do we need to pass in CustomFilterProgramInfo separately? Can ask for validatedProgram->programInfo() instead?

Oops, I was not careful. We don't need CustomFilterProgramInfo in CustomFilterRenderer. Thank you. I'll update.

[Dongseong Hwang]

Created attachment 170249 [details]
Patch

[Dongseong Hwang]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 170234 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170234&action=review
> > 
> > > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:79
> > > +CustomFilterRenderer::CustomFilterRenderer(PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo&, const CustomFilterParameterList& parameters,
> > 
> > Do we need to pass in CustomFilterProgramInfo separately? Can ask for validatedProgram->programInfo() instead?
> 
> Oops, I was not careful. We don't need CustomFilterProgramInfo in CustomFilterRenderer. Thank you. I'll update.

I removed CustomFilterProgramInfo in CustomFilterRenderer. Thank Max.
In addition, I removed unused two headers in CustomFilterRenderer.

[Max Vujovic]

(In reply to comment #19)
> I removed CustomFilterProgramInfo in CustomFilterRenderer. Thank Max.
> In addition, I removed unused two headers in CustomFilterRenderer.

Nice. Thanks Huang :)

[Dongseong Hwang]

(In reply to comment #20)
> (In reply to comment #19)
> > I removed CustomFilterProgramInfo in CustomFilterRenderer. Thank Max.
> > In addition, I removed unused two headers in CustomFilterRenderer.
> 
> Nice. Thanks Huang :)

Thank you, too! :) I'll ask dino to review.

[Dean Jackson]

Comment on attachment 170249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170249&action=review

This looks great. Just two very minor nits. Sorry this means another submission - you'll hopefully be getting commit access very soon :)

> Source/WebCore/ChangeLog:10
> +        rendering css shaders to CustomFilterRenderer.

Super Nit "css" -> "CSS"

> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:196
> +        // The viewport is a box with the size of 1 unit, so we are scalling up here to make sure that translations happen using real pixel

Typo "scaling"

[Dongseong Hwang]

Created attachment 171422 [details]
Patch

[Dongseong Hwang]

(In reply to comment #22)
> (From update of attachment 170249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170249&action=review
> 
> This looks great. Just two very minor nits. Sorry this means another submission - you'll hopefully be getting commit access very soon :)

Thanks for your review! I hope so :)

> Super Nit "css" -> "CSS"
> Typo "scaling"

Done.

I rename CustomFilterRenderer::draw() from CustomFilterRenderer::paint() because Chiculita seems to prefer draw to paint. right? :)

[Alexandru Chiculita]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 170249 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170249&action=review

> I rename CustomFilterRenderer::draw() from CustomFilterRenderer::paint() because Chiculita seems to prefer draw to paint. right? :)

Hm, I don't have a preference, but looking at the code we've used "draw" a lot in this class. Maybe we could change that to "paint" at a later time in order to be more consistent with the rest of WebKit. But that's a different patch.

I can set you the commit-queue flag if needed.

[Dongseong Hwang]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #22)
> Hm, I don't have a preference, but looking at the code we've used "draw" a lot in this class. Maybe we could change that to "paint" at a later time in order to be more consistent with the rest of WebKit. But that's a different patch.

Hi, alex. I just recalled CCCustomFilterRenderer::draw().

> I can set you the commit-queue flag if needed.

If you do, I'll be happy!

[Dean Jackson]

Comment on attachment 171422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171422&action=review

> Source/WebCore/ChangeLog:36
> +        (WebCore::CustomFilterRenderer::paint):

This is wrong now :( Alex, could you land this manually (sorry, I can't do it right now). No need to go through another review.

[Dongseong Hwang]

Comment on attachment 171422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171422&action=review

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::CustomFilterRenderer::paint):
> 
> This is wrong now :( Alex, could you land this manually (sorry, I can't do it right now). No need to go through another review.

Oops. Sorry. Thanks Alex in advance.

[Alexandru Chiculita]

(In reply to comment #28)
> (From update of attachment 171422 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171422&action=review
> 
> >> Source/WebCore/ChangeLog:36
> >> +        (WebCore::CustomFilterRenderer::paint):
> > 
> > This is wrong now :( Alex, could you land this manually (sorry, I can't do it right now). No need to go through another review.
> 
> Oops. Sorry. Thanks Alex in advance.

I'm rebasing and landing it manually. Thanks for the patch!

[Alexandru Chiculita]

Landed manually in http://trac.webkit.org/changeset/132920.

[Dongseong Hwang]

Thank alex, dino and max for review and commit. I'm happy because of contributing to an awesome feature, which is CSS Shaders. :)

[Dean Jackson]

(In reply to comment #31)
> Thank alex, dino and max for review and commit. I'm happy because of contributing to an awesome feature, which is CSS Shaders. :)

You're very welcome. Keep the patches coming :)