[2/4] console: add opengl rendering helper functions

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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

On 12/01/2015 13:35, Gerd Hoffmann wrote:
> +/*
> + * QEMU graphical console -- opengl helper bits
> + *
> + * Copyright (c) 2004 Fabrice Bellard
> + *

Copyright Red Hat / Author you?

Paolo

On Mo, 2015-01-12 at 14:04 +0100, Paolo Bonzini wrote:
> 
> On 12/01/2015 13:35, Gerd Hoffmann wrote:
> > +/*
> > + * QEMU graphical console -- opengl helper bits
> > + *
> > + * Copyright (c) 2004 Fabrice Bellard
> > + *
> 
> Copyright Red Hat / Author you?

Ahem, yes.  Just carelessly moved over the comment from console.c, which
doesn't make much sense given this is all new code ...

cheers,
  Gerd

On 2015-01-12 at 07:35, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   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

Hi,

> > +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?

Inflight change, there is a patch series on the list adding a
check_format callback to DisplayChangeListenerOps, for format
negotiation.

> > +
> > +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.

endianness indeed.  pixman formats are native endian, so this actually
is bgra ordering in memory.  While looking at it:  I guess GL_BGRA is
fixed byte ordering?  So this probably needs fixing to work correctly on
bigendian machines ...

> Discarding the alpha channel is intentional, I suppose?

Yes.

> 
> > +                 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)?

Usually this is the case, but there can be exceptions.  Hmm, can I
explicitly pass the stride?

thanks,
  Gerd

On 2015-01-16 at 04:21, Gerd Hoffmann wrote:
>    Hi,
>
>>> +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?
> Inflight change, there is a patch series on the list adding a
> check_format callback to DisplayChangeListenerOps, for format
> negotiation.

OK.

>>> +
>>> +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.
> endianness indeed.  pixman formats are native endian, so this actually
> is bgra ordering in memory.  While looking at it:  I guess GL_BGRA is
> fixed byte ordering?

Yes, I think so.

> So this probably needs fixing to work correctly on
> bigendian machines ...

Probably, right.

>> Discarding the alpha channel is intentional, I suppose?
> Yes.
>
>>> +                 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)?
> Usually this is the case, but there can be exceptions.  Hmm, can I
> explicitly pass the stride?

The internet™ tells me about glPixelStorei(GL_UNPACK_ROW_LENGTH, 
$pixels_per_row) and glPixelStorei(GL_UNPACK_ALIGNMENT, 
$row_alignment_in_bytes) (which need to be called before using 
glTexImage2D() or glTexSubImage2D()). You may want to use it for 
surface_gl_update_texture(), too (if that isn't too much work).

Max