On Thu, Nov 15, 2012 at 11:01 AM, Ander Conselvan de Oliveira <conselv...@gmail.com> wrote: > On 11/14/2012 05:41 PM, John Kåre Alsaker wrote: >> >> These two patches adds a flexible shader generation which is based on >> concatenating strings instead of duplicating them into a lot of literals. >> This becomes more useful as I add shader variants for converting to/from >> sRGB gamma and it also allows me to easily get rid of the alpha uniform used >> for fading surfaces. >> >> I have not tested the YUV shaders since that doesn't appear to be too easy >> without an Intel GPU. I also have a more vectorized variant of the YUV->RGB >> conversion I'd like to test. >> >> I've split this into two patches to make the introduction of this a bit >> clearer. I seemed to rewrite all of gl-shaders.c anyway with only 2 >> functions being recognisable. So I was wondering if I should just merge >> these patches. >> >> The first commit which really utilizes this is the one which adds gamma >> correct rendering. Should I wait until cleaning up that before submitting >> this? > > > I think it it would be a good idea to send it if that would show why do you > want to make this change. > > A few thoughts: > > - Are the shader constructor functions and the shader_builder really > necessary? It seems the only thing the constructors do is call > shader_build_add(). If the you could use a struct with a few string fields > (one for global, one for main, etc) maybe the shader source wouldn't be > burried so deep in the constructor calls. The constructor functions deal only with generating the input and would have to be replaced with a large switch in create_shader_permutation.
I don't see how using string fields instead of a string array would clear things up much. You'd still need a shader_builder_add utility function to concatenate them (or the pointers to them if you like). > > Besides, if you only have a list of string you can avoid the memcpy in > shader_build_add() and let glShaderSource do the concatenation. That would just be a insignificant performance improvement. > > Anyway, I just think this makes the shader sources much less readable. That is a side-effect of splitting them up. > > - What about performance implications? > > The whole create all permutations is scary. If the number of shaders > start growing it would be good to compile only the shaders that are being > used. We could change it to compile on demand if it grows too high, but that might lead to stuttering. > > Also, before the shader would be selected for the surface on attach, but now > you call gl_use_shader() on draw surface. Is this really necessary? I ask > because this function is a for loop on the number of attributes and this > makes me think of how scalable this is. I think you mean gl_select_shader? This is because only the input shader attribute (and later the conversion attribute) is known at attach-time. The loop in permutation_from_attributes should be unrolled be the compiler. I see the lookup into the array of shader pointers as the biggest performance problem there. > > Cheers, > Ander > > > > >> John Kåre Alsaker (2): >> gl-renderer: Move shader functions into it's own file. >> gl-renderer: Add flexible shader generation. >> >> src/Makefile.am | 2 + >> src/gl-internal.h | 171 ++++++++++++++++ >> src/gl-renderer.c | 428 ++++---------------------------------- >> src/gl-shaders.c | 602 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 819 insertions(+), 384 deletions(-) >> create mode 100644 src/gl-internal.h >> create mode 100644 src/gl-shaders.c >> > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel