[RFC weston 0/2] Adding a more flexible shader generation

[John Kåre Alsaker]

On Thu, Nov 15, 2012 at 11:01 AM, Ander Conselvan de Oliveira
<conselvan2 at 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
>>
>