Bug 101801 – [Texmap][CSS Shaders] Reuse the precompiled shader for custom filters in TextureMapperGL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Alexandru Chiculita]

The shader is always recompiled before paint. Add code to cache the shader at least until next paint.

[Alexandru Chiculita]

Created attachment 174817 [details]
Patch V1

[Noam Rosenthal]

Comment on attachment 174817 [details]
Patch V1

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

Great progress, see comments.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:152
> +            WebCustomFilterProgramProxy* customFilterProgramProxy = static_cast<WebCustomFilterProgramProxy*>(program->platformCompiledProgram()->client());

This seems a bit messy... I am sure we have enough information in LayerTreeCoordinator to know which shaders we have to send.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:159
> +            if (!customFilterProgramProxy->wasSentToUIProcess()) {

I would prefer if we sent all the new programs to the Ui process in LayerTreeCoordinator rather than here with an if condition.

> Source/WebKit2/Shared/CoordinatedGraphics/WebCustomFilterProgramProxy.h:75
> +    bool wasSentToUIProcess() const  { return m_wasSentToUIProcess; }

Sent you went through the trouble of creating this class, maybe it should also do its own encoding/decoding rather than leave it to CoordinatedGraphicsArgumentCoders.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:292
> +    injectCachedCustomFilterPrograms(const_cast<FilterOperations&>(filters));

Would be cleaner to create a local copy and inject to it rather than const_cast

[Alexandru Chiculita]

(In reply to comment #2)
> (From update of attachment 174817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174817&action=review
> 
> Great progress, see comments.

Thanks Noam, your comments make sense. I will integrate the feedback and upload again.

[Alexandru Chiculita]

Created attachment 174832 [details]
Patch V2

Integrated the feedback.

[Noam Rosenthal]

Comment on attachment 174832 [details]
Patch V2

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

Very nice!
Some pre-landing nits below...

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:270
> +            int programId = 0;
> +            if (!decoder->decode(programId))

programID

> Source/WebKit2/Shared/CoordinatedGraphics/WebCustomFilterOperation.h:50
> +    int programId() const { return m_programId; }

programID()
and m_programID
(WebKit style)

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:313
> +        }
> +        WebCustomFilterOperation* customOperation = static_cast<WebCustomFilterOperation*>(operation);

Add line break

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:319
> +        if (iter == m_customFilterPrograms.end()) {
> +            ASSERT_NOT_REACHED();
> +            continue;
> +        }

ASSERT(iter != m_customFilterPrograms.end())

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:338
> +    if (iter == m_customFilterPrograms.end()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

Ditto

[Noam Rosenthal]

btw we should probably call checkCustomFilterProgramProxies for filter operations inside an animation, otherwise those might not be rendered correctly.

[Alexandru Chiculita]

(In reply to comment #6)
> btw we should probably call checkCustomFilterProgramProxies for filter operations inside an animation, otherwise those might not be rendered correctly.

Hm.. didn't know the filter animations are accelerated on Texture Mapper. Will do.

[Alexandru Chiculita]

Created attachment 174835 [details]
Patch V3

I've added the animations fix, so I've set the CQ+ flag again.

@Noam: is there an automatic way to test the composited animations?

[Noam Rosenthal]

(In reply to comment #8)
> @Noam: is there an automatic way to test the composited animations?

Unfortunately not really quite yet, we would need good support for taking several pixel snapshots for the same page and that's tricky.

[WebKit Review Bot]

Comment on attachment 174835 [details]
Patch V3

Clearing flags on attachment: 174835

Committed r135056: <http://trac.webkit.org/changeset/135056>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.