Re: [Qemu-devel] [PATCH v2 5/7] console-gl: externalize shader programs

[Max Reitz]

On 2015-01-19 at 08:36, Gerd Hoffmann wrote:
> ---

S-o-b is missing.

>   Makefile                    | 17 +++++++++++++++++
>   scripts/shaderinclude.pl    | 16 ++++++++++++++++
>   ui/console-gl.c             | 28 ++--------------------------
>   ui/shader/texture-blit.frag | 10 ++++++++++
>   ui/shader/texture-blit.vert | 11 +++++++++++
>   5 files changed, 56 insertions(+), 26 deletions(-)
>   create mode 100644 scripts/shaderinclude.pl
>   create mode 100644 ui/shader/texture-blit.frag
>   create mode 100644 ui/shader/texture-blit.vert
>
> diff --git a/Makefile b/Makefile
> index 6817c6f..6d77782 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -292,6 +292,7 @@ clean:
>         rm -f fsdev/*.pod
>         rm -rf .libs */.libs
>         rm -f qemu-img-cmds.h
> +       rm -f ui/shader/*-vert.h ui/shader/*-frag.h
>         @# May not be present in GENERATED_HEADERS
>         rm -f trace/generated-tracers-dtrace.dtrace*
>         rm -f trace/generated-tracers-dtrace.h*
> @@ -437,6 +438,22 @@ cscope:
>         find "$(SRC_PATH)" -name "*.[chsS]" -print | sed 's,^\./,,' > 
> ./cscope.files
>         cscope -b
>   
> +# opengl shader programs
> +ui/shader/%-vert.h: $(SRC_PATH)/ui/shader/%.vert 
> $(SRC_PATH)/scripts/shaderinclude.pl
> +       @mkdir -p $(dir $@)
> +       $(call quiet-command,\
> +               perl $(SRC_PATH)/scripts/shaderinclude.pl $< > $@,\
> +               "  VERT  $@")
> +
> +ui/shader/%-frag.h: $(SRC_PATH)/ui/shader/%.frag 
> $(SRC_PATH)/scripts/shaderinclude.pl
> +       @mkdir -p $(dir $@)
> +       $(call quiet-command,\
> +               perl $(SRC_PATH)/scripts/shaderinclude.pl $< > $@,\
> +               "  FRAG  $@")
> +
> +ui/console-gl.o: $(SRC_PATH)/ui/console-gl.c \
> +       ui/shader/texture-blit-vert.h ui/shader/texture-blit-frag.h
> +
>   # documentation
>   MAKEINFO=makeinfo
>   MAKEINFOFLAGS=--no-headers --no-split --number-sections
> diff --git a/scripts/shaderinclude.pl b/scripts/shaderinclude.pl
> new file mode 100644
> index 0000000..81b5146
> --- /dev/null
> +++ b/scripts/shaderinclude.pl
> @@ -0,0 +1,16 @@
> +#!/usr/bin/perl
> +use strict;
> +use warnings;
> +
> +my $file = shift;
> +open FILE, "<", $file or die "open $file: $!";
> +my $name = $file;
> +$name =~ s|.*/||;
> +$name =~ s/[-.]/_/g;
> +print "static GLchar ${name}_src[] =\n";
> +while (<FILE>) {
> +    chomp;
> +    printf "    \"%s\\n\"\n", $_;
> +}
> +print "    \"\\n\";\n";
> +close FILE;


> diff --git a/ui/console-gl.c b/ui/console-gl.c
> index 589c682..2c9412d 100644
> --- a/ui/console-gl.c
> +++ b/ui/console-gl.c
> @@ -33,32 +33,8 @@ struct ConsoleGLState {
>   
>   /* ---------------------------------------------------------------------- */
>   
> -static GLchar texture_blit_vert_src[] =
> -    "\n"
> -    "#version 300 es\n"
> -    "\n"
> -    "in vec2  in_position;\n"
> -    "in vec2  in_tex_coord;\n"
> -    "out vec2 ex_tex_coord;\n"
> -    "\n"
> -    "void main(void) {\n"
> -    "    gl_Position = vec4(in_position.x, in_position.y, 0.0, 1.0);\n"
> -    "    ex_tex_coord = in_tex_coord;\n"
> -    "}\n"
> -    "\n";
> -
> -static GLchar texture_blit_frag_src[] =
> -    "\n"
> -    "#version 300 es\n"
> -    "\n"
> -    "uniform sampler2D image;\n"
> -    "in  highp vec2 ex_tex_coord;\n"
> -    "out highp vec4 out_frag_color;\n"
> -    "\n"
> -    "void main(void) {\n"
> -    "     out_frag_color = texture(image, ex_tex_coord);\n"
> -    "}\n"
> -    "\n";
> +#include "shader/texture-blit-vert.h"
> +#include "shader/texture-blit-frag.h"
>   
>   static void gl_run_texture_blit(ConsoleGLState *gls)
>   {
> diff --git a/ui/shader/texture-blit.frag b/ui/shader/texture-blit.frag
> new file mode 100644
> index 0000000..148b1aa
> --- /dev/null
> +++ b/ui/shader/texture-blit.frag
> @@ -0,0 +1,10 @@
> +
> +#version 300 es
> +
> +uniform sampler2D image;
> +in  highp vec2 ex_tex_coord;
> +out highp vec4 out_frag_color;

Apart from me not knowing what highp really does because I don't know 
GLES, I can imagine what it's probably supposed to do (high-precision); 
do we really need it for these values?

(I know, I should've said that in the previous patch already…)

> +
> +void main(void) {
> +     out_frag_color = texture(image, ex_tex_coord);
> +}
> diff --git a/ui/shader/texture-blit.vert b/ui/shader/texture-blit.vert
> new file mode 100644
> index 0000000..4ffb5d1
> --- /dev/null
> +++ b/ui/shader/texture-blit.vert
> @@ -0,0 +1,11 @@
> +
> +#version 300 es
> +
> +in vec2  in_position;
> +in vec2  in_tex_coord;
> +out vec2 ex_tex_coord;
> +
> +void main(void) {
> +    gl_Position = vec4(in_position.x, in_position.y, 0.0, 1.0);
> +    ex_tex_coord = in_tex_coord;
> +}

gl_Position = vec4(in_position, 0.0, 1.0);
ex_tex_coord = vec2(1.0 + in_position.x, 1.0 - in_position.y) * 0.5;

*duck*

Anyway:

Reviewed-by: Max Reitz <address@hidden>