Bug 102414 – [Texmap][CSS Shaders] Make the CustomFilterValidatedProgram maintain the platform compiled program

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

[Alexandru Chiculita]

ValidatedCustomFilterOperation already contains a reference to a CustomFilterValidatedProgram which is cached and reused across multiple repaints and RenderLayers. It also has a mechanism to keep a reference to a platform compiled shader. We will reuse this to maintain the "compiled shaders" in the WK2 LayerTreeCoordinator.cpp.

[Alexandru Chiculita]

Created attachment 174496 [details]
Patch V1

[Alexandru Chiculita]

Created attachment 174503 [details]
Patch V2

Rebased.

[Kenneth Rohde Christiansen]

Comment on attachment 174503 [details]
Patch V2

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:118
>        '../platform/graphics/filters',
> +      '../platform/graphics/filters/texmap',
>        '../platform/graphics/filters/skia',

Google doesn't use texture mapper, why is this needed, seems wrong

> Source/WebCore/WebCore.gypi:4955
>              'platform/graphics/efl/IntRectEfl.cpp',
> +            'platform/graphics/filters/texmap/CustomFilterValidatedProgramTextureMapper.cpp',
> +            'platform/graphics/filters/texmap/TextureMapperPlatformCompiledProgram.h',
>              'platform/graphics/filters/CustomFilterArrayParameter.h',
>              'platform/graphics/filters/CustomFilterConstants.h',

Same here

[Alexandru Chiculita]

(In reply to comment #3)
> (From update of attachment 174503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174503&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:118
> >        '../platform/graphics/filters',
> > +      '../platform/graphics/filters/texmap',
> >        '../platform/graphics/filters/skia',
> 
> Google doesn't use texture mapper, why is this needed, seems wrong
> 
> > Source/WebCore/WebCore.gypi:4955
> >              'platform/graphics/efl/IntRectEfl.cpp',
> > +            'platform/graphics/filters/texmap/CustomFilterValidatedProgramTextureMapper.cpp',
> > +            'platform/graphics/filters/texmap/TextureMapperPlatformCompiledProgram.h',
> >              'platform/graphics/filters/CustomFilterArrayParameter.h',
> >              'platform/graphics/filters/CustomFilterConstants.h',
> 
> Same here

Sorry about that. I've searched for TextureMapper related files and found them in the GYP project file too. I thought that there may be projects based on that. I will remove them.

[Alexandru Chiculita]

(In reply to comment #4)

For example in 
http://svn.webkit.org/repository/webkit/trunk/Source/WebCore/WebCore.gypi

search for platform/graphics/texmap/

[Kenneth Rohde Christiansen]

I suggest you git blame that line, it might be a mistake

[Kenneth Rohde Christiansen]

b372bde3 Source/WebCore/WebCore.gypi ([e-mail]               2011-07-22 21:31:04 +0000 3986)             'platform/graphics/texmap/GraphicsLayerTextureMapper.cpp',
b372bde3 Source/WebCore/WebCore.gypi ([e-mail]               2011-07-22 21:31:04 +0000 3987)             'platform/graphics/texmap/GraphicsLayerTextureMapper.h',
ecf8da49 Source/WebCore/WebCore.gypi ([e-mail]       2012-01-09 11:54:06 +0000 3988)             'platform/graphics/texmap/TextureMapper.cpp',
b372bde3 Source/WebCore/WebCore.gypi ([e-mail]               2011-07-22 21:31:04 +0000 3989)             'platform/graphics/texmap/TextureMapper.h',
1571338c Source/WebCore/WebCore.gypi ([e-mail]       2012-02-14 12:53:30 +0000 3990)             'platform/graphics/texmap/TextureMapperBackingStore.cpp',
1571338c Source/WebCore/WebCore.gypi ([e-mail]       2012-02-14 12:53:30 +0000 3991)             'platform/graphics/texmap/TextureMapperBackingStore.h',
5ee5803c Source/WebCore/WebCore.gypi ([e-mail]          2012-02-22 04:02:26 +0000 3992)             'platform/graphics/texmap/TextureMapperGL.cpp',
5ee5803c Source/WebCore/WebCore.gypi ([e-mail]          2012-02-22 04:02:26 +0000 3993)             'platform/graphics/texmap/TextureMapperGL.h',
1ddb153e Source/WebCore/WebCore.gypi ([e-mail]       2012-02-03 14:59:31 +0000 3994)             'platform/graphics/texmap/TextureMapperImageBuffer.cpp',
1ddb153e Source/WebCore/WebCore.gypi ([e-mail]       2012-02-03 14:59:31 +0000 3995)             'platform/graphics/texmap/TextureMapperImageBuffer.h',
a3e29880 Source/WebCore/WebCore.gypi ([e-mail]       2012-02-15 08:26:30 +0000 3996)             'platform/graphics/texmap/TextureMapperLayer.cpp',
a3e29880 Source/WebCore/WebCore.gypi ([e-mail]       2012-02-15 08:26:30 +0000 3997)             'platform/graphics/texmap/TextureMapperLayer.h',
b372bde3 Source/WebCore/WebCore.gypi ([e-mail]               2011-07-22 21:31:04 +0000 3998)             'platform/graphics/texmap/TextureMapperPlatformLayer.h',
5ee5803c Source/WebCore/WebCore.gypi ([e-mail]          2012-02-22 04:02:26 +0000 3999)             'platform/graphics/texmap/TextureMapperShaderManager.cpp',
5ee5803c Source/WebCore/WebCore.gypi ([e-mail]          2012-02-22 04:02:26 +0000 4000)             'platform/graphics/texmap/TextureMapperShaderManager.h',

[Kenneth Rohde Christiansen]

> ecf8da49 Source/WebCore/WebCore.gypi ([e-mail]       2012-01-09 11:54:06 +0000 3988)             'platform/graphics/texmap/TextureMapper.cpp',

Noam, I don't think these should be added to chrome right?

[Noam Rosenthal]

(In reply to comment #8)
> > ecf8da49 Source/WebCore/WebCore.gypi ([e-mail]       2012-01-09 11:54:06 +0000 3988)             'platform/graphics/texmap/TextureMapper.cpp',
> 
> Noam, I don't think these should be added to chrome right?
Probably not :) Let's remove it...

[Alexandru Chiculita]

(In reply to comment #9)
> (In reply to comment #8)
> > > ecf8da49 Source/WebCore/WebCore.gypi ([e-mail]       2012-01-09 11:54:06 +0000 3988)             'platform/graphics/texmap/TextureMapper.cpp',
> > 
> > Noam, I don't think these should be added to chrome right?
> Probably not :) Let's remove it...

Added a bug for that:
https://bugs.webkit.org/show_bug.cgi?id=102425

[Alexandru Chiculita]

Created attachment 174509 [details]
Patch V3

[Noam Rosenthal]

Comment on attachment 174509 [details]
Patch V3

Looks like a step in the right direction. Can't find anything here that I would do different.

[Alexandru Chiculita]

(In reply to comment #12)
> (From update of attachment 174509 [details])
> Looks like a step in the right direction. Can't find anything here that I would do different.

Thanks, Noam!

Looking in the gypi project file I've found lots of other unrelated files like for example the whole platform/graphics/qt/ is in there.

Do you know what's the policy?

[Tony Chang]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 174509 [details] [details])
> > Looks like a step in the right direction. Can't find anything here that I would do different.
> 
> Thanks, Noam!
> 
> Looking in the gypi project file I've found lots of other unrelated files like for example the whole platform/graphics/qt/ is in there.
> 
> Do you know what's the policy?

Normally, we list every file and directory in the .gypi files.  In WebCore.gyp, we exclude files that aren't used:
http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.gyp/WebCore.gyp#L1612

You can see that texmap is already excluded so adding new files to WebCore.gypi is fine.

In theory, the purpose is to make things easy for people not working on Chromium.  You don't have to think about whether to add a file or not, you just add everything.

[kov's GTK+ EWS bot]

Comment on attachment 174509 [details]
Patch V3

Attachment 174509 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14848530

[Alexandru Chiculita]

Just finished a local build on Gtk and it didn't fail.

[Build Bot]

Comment on attachment 174509 [details]
Patch V3

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

New failing tests:
inspector-protocol/nmi-webaudio.html

[Noam Rosenthal]

(In reply to comment #16)
> Just finished a local build on Gtk and it didn't fail.

It's a flaky test, feel free to land when you're ready.

[Alexandru Chiculita]

Comment on attachment 174509 [details]
Patch V3

Added cq+ to land the patch.

[WebKit Review Bot]

Comment on attachment 174509 [details]
Patch V3

Clearing flags on attachment: 174509

Committed r134948: <http://trac.webkit.org/changeset/134948>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.