Re: [Qemu-devel] [PATCH 2/4] console: add opengl rendering helper functions

[Max Reitz]

On 2015-01-12 at 07:35, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>   include/ui/console.h |  23 ++++++++++
>   ui/Makefile.objs     |   5 ++
>   ui/console-gl.c      | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 155 insertions(+)
>   create mode 100644 ui/console-gl.c
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 22ef8ca..5cb169c 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -9,6 +9,11 @@
>   #include "qapi-types.h"
>   #include "qapi/error.h"
>   
> +#ifdef CONFIG_OPENGL
> +#include <GL/gl.h>
> +#include <GL/glext.h>
> +#endif
> +
>   /* keyboard/mouse support */
>   
>   #define MOUSE_EVENT_LBUTTON 0x01
> @@ -118,6 +123,11 @@ struct DisplaySurface {
>       pixman_format_code_t format;
>       pixman_image_t *image;
>       uint8_t flags;
> +#ifdef CONFIG_OPENGL
> +    GLenum glformat;
> +    GLenum gltype;
> +    GLuint texture;
> +#endif
>   };
>   
>   typedef struct QemuUIInfo {
> @@ -320,6 +330,19 @@ void qemu_console_copy(QemuConsole *con, int src_x, int 
> src_y,
>   DisplaySurface *qemu_console_surface(QemuConsole *con);
>   DisplayState *qemu_console_displaystate(QemuConsole *console);
>   
> +#ifdef CONFIG_OPENGL
> +/* console-gl.c */
> +bool console_gl_check_format(DisplayChangeListener *dcl,
> +                             pixman_format_code_t format);
> +void surface_gl_create_texture(DisplaySurface *surface);
> +void surface_gl_update_texture(DisplaySurface *surface,
> +                               int x, int y, int w, int h);
> +void surface_gl_render_texture(DisplaySurface *surface);
> +void surface_gl_destroy_texture(DisplaySurface *surface);
> +void surface_gl_setup_viewport(DisplaySurface *surface,
> +                               int ww, int wh);
> +#endif
> +
>   /* sdl.c */
>   void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
>   
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index 13b5cfb..3173778 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -24,4 +24,9 @@ sdl.mo-objs := sdl2.o sdl2-input.o sdl2-2d.o
>   endif
>   sdl.mo-cflags := $(SDL_CFLAGS)
>   
> +ifeq ($(CONFIG_OPENGL),y)
> +common-obj-y += console-gl.o
> +libs_softmmu += $(OPENGL_LIBS)
> +endif
> +
>   gtk.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS)
> diff --git a/ui/console-gl.c b/ui/console-gl.c
> new file mode 100644
> index 0000000..470dd61
> --- /dev/null
> +++ b/ui/console-gl.c
> @@ -0,0 +1,127 @@
> +/*
> + * QEMU graphical console -- opengl helper bits
> + *
> + * Copyright (c) 2004 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu-common.h"
> +#include "ui/console.h"
> +
> +bool console_gl_check_format(DisplayChangeListener *dcl,
> +                             pixman_format_code_t format)
> +{
> +    switch (format) {
> +    case PIXMAN_x8r8g8b8:
> +    case PIXMAN_a8r8g8b8:
> +    case PIXMAN_r5g6b5:
> +        return true;
> +    default:
> +        return false;
> +    }
> +}

What is this function supposed to be used for?

> +
> +void surface_gl_create_texture(DisplaySurface *surface)
> +{
> +    switch (surface->format) {
> +    case PIXMAN_x8r8g8b8:
> +    case PIXMAN_a8r8g8b8:
> +        surface->glformat = GL_BGRA;

Why does the format code seem to imply ARGB order but you're using the 
exact opposite? I could imagine something like endianness being the 
reason, but you're using RGB below where the format code says exactly that.

> +        surface->gltype = GL_UNSIGNED_BYTE;
> +        break;
> +    case PIXMAN_r5g6b5:
> +        surface->glformat = GL_RGB;
> +        surface->gltype = GL_UNSIGNED_SHORT_5_6_5;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    glGenTextures(1, &surface->texture);
> +    glEnable(GL_TEXTURE_2D);
> +    glBindTexture(GL_TEXTURE_2D, surface->texture);
> +    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,

Discarding the alpha channel is intentional, I suppose?

> +                 surface_width(surface),
> +                 surface_height(surface),
> +                 0, surface->glformat, surface->gltype,
> +                 surface_data(surface));

Is surface_stride(surface) specified to be surface_width(surface) * 
bytes_per_pixel (I don't know pixman so I don't know)? If it isn't then 
this may not work.

> +
> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> +}
> +
> +void surface_gl_update_texture(DisplaySurface *surface,
> +                               int x, int y, int w, int h)
> +{
> +    uint8_t *data = (void *)surface_data(surface);
> +
> +    glTexSubImage2D(GL_TEXTURE_2D, 0,
> +                    0, y,
> +                    surface_width(surface), h,
> +                    surface->glformat, surface->gltype,
> +                    data + surface_stride(surface) * y);
> +}
> +
> +void surface_gl_render_texture(DisplaySurface *surface)
> +{
> +    glClearColor(0.0, 0.0, 0.0, 0);

I still find this strange (the RGB values are doubles, the alpha channel 
is an integer; and actually all parameters are expected to be floats), 
but I can live with it.

> +    glClear(GL_COLOR_BUFFER_BIT);
> +
> +    glBegin(GL_QUADS);
> +    glTexCoord2i(0, 1);  glVertex3i(-1, -1, 0);
> +    glTexCoord2i(0, 0);  glVertex3i(-1, 1, 0);
> +    glTexCoord2i(1, 0);  glVertex3i(1, 1, 0);
> +    glTexCoord2i(1, 1);  glVertex3i(1, -1, 0);

I don't know whether I've said it before, but you can use glVertex2i() here.

> +    glEnd();
> +}
> +
> +void surface_gl_destroy_texture(DisplaySurface *surface)
> +{
> +    if (!surface || !surface->texture) {
> +        return;
> +    }
> +    glDeleteTextures(1, &surface->texture);
> +    surface->texture = 0;
> +}
> +
> +void surface_gl_setup_viewport(DisplaySurface *surface,
> +                               int ww, int wh)
> +{
> +    int gw, gh, stripe;
> +    float sw, sh;
> +
> +    gw = surface_width(surface);
> +    gh = surface_height(surface);
> +
> +    sw = (float)ww/gw;
> +    sh = (float)wh/gh;
> +    if (sw < sh) {
> +        stripe = wh - wh*sw/sh;
> +        glViewport(0, stripe / 2, ww, wh - stripe);
> +    } else {
> +        stripe = ww - ww*sh/sw;
> +        glViewport(stripe / 2, 0, ww - stripe, wh);
> +    }
> +
> +    glMatrixMode(GL_PROJECTION);
> +    glLoadIdentity();
> +
> +    glMatrixMode(GL_MODELVIEW);
> +    glLoadIdentity();
> +}

Looks fine to me in general.

Max