GNOME Bugzilla – Bug 660512
ClutterShaderEffect recompiles the shader for every instance
Last modified: 2011-09-30 10:59:28 UTC
I think most ClutterShaderEffect subclasses just use a single static shader that gets used for every instance of the effect. However the current implementation creates a new shader for every instance. This is enforced by the API because the only way to set the shader source is by calling clutter_shader_effect_set_shader_source which takes a string and is per-instance. This is particularly bad with the current Cogl architecture because every pipeline created with a brand new state is stored in the program cache and never freed. This means that anything creating effects for actors that come and go will leak compiled programs. This eventually hits a g_warning in Cogl that the program cache is becoming large.
Created attachment 197802 [details] [review] clutter-shader-effect: Add a get_static_shader_source virtual This is used as an alternative to calling clutter_shader_effect_set_shader_source. A ClutterShaderEffect subclass is now expected to implement this method to return the source for the effect that will be used for all instances of this subclass. It is only called once regardless of the number of instances created. That way Clutter can avoid recompiling the shader source for every new instance of the effect.
Created attachment 197803 [details] [review] Add a conformance test for ClutterShaderEffect This adds a simple conformance test which sets up a few shader effects using both the old style with clutter_shader_effect_set_source and the new style by overriding get_static_shader_source. The effects are then verified to confirm that they drew the right pixel colour.
Review of attachment 197802 [details] [review]: looks good to me, apart from minor coding style/documentation issues; if you could fix them before committing it would be great. ::: clutter/clutter-shader-effect.c @@ +166,3 @@ +G_DEFINE_TYPE_WITH_CODE + (ClutterShaderEffect, please, don't break the line here. if you need a bigger screen I think we can arrange that. :-p @@ +344,3 @@ + CLUTTER_SHADER_EFFECT_GET_CLASS (self); + +clutter_shader_effect_try_static_source (ClutterShaderEffect *self) please, add "!= NULL": I prefer explicit comparisons for pointers. ::: clutter/clutter-shader-effect.h @@ +70,1 @@ * why did you delete the rest of the description? gtk-doc will actively complain.
Review of attachment 197803 [details] [review]: looks good to me.
Review of attachment 197802 [details] [review]: ::: clutter/clutter-shader-effect.h @@ +70,1 @@ * The ClutterShaderEffectClass structure no longer contains only private data so it wouldn't make any sense to leave it in. gtk-doc didn't seem to complain when I tried compiling it.
(In reply to comment #5) > Review of attachment 197802 [details] [review]: > > ::: clutter/clutter-shader-effect.h > @@ +70,1 @@ > * > > The ClutterShaderEffectClass structure no longer contains only private data so > it wouldn't make any sense to leave it in. gtk-doc didn't seem to complain when > I tried compiling it. no, it contains only private data. all class structures are defined as such; vfuncs that are public are marked as public, and thus appear in the documentation - everything else inside it is considered private.
> no, it contains only private data. all class structures are defined as such; > vfuncs that are public are marked as public, and thus appear in the > documentation - everything else inside it is considered private. Well, I don't really understand your reasoning to be honest ('it only contains private data except for the things that aren't private') but I'll leave it in anyway. I've pushed the patches as 8b995a9 and 3372a02. I even did the overly long line change even though it made me feel ill ;)