Re: [Qemu-devel] [RfC PATCH 2/3] sdl2: add support for display rendering using opengl.

[Max Reitz <mreitz@redhat.com>]

On 2014-12-12 at 12:04, Gerd Hoffmann wrote:
>    Hi,
>
>>> +static void sdl2_gl_render_surface(struct sdl2_console *scon)
>>> +{
>>> +    int gw, gh, ww, wh, stripe;
>>> +    float sw, sh;
>>> +    GLuint tex;
>>> +
>>> +    gw = surface_width(scon->surface);
>>> +    gh = surface_height(scon->surface);
>>> +    SDL_GetWindowSize(scon->real_window, &ww, &wh);
>>> +    SDL_GL_MakeCurrent(scon->real_window, scon->winctx);
>>> +
>>> +    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();
>> It's been a surprisingly long time since I last saw the OpenGL builtin
>> matrix stack. :-)
> --verbose please.

Well, I'm mostly used to OpenGL 3/4 Core now where that builtin matrix 
stack does not exist anymore.

>>> +
>>> +    glMatrixMode(GL_MODELVIEW);
>>> +    glLoadIdentity();
>>> +
>>> +    glClearColor(0.0, 0.0, 0.0, 0);
>> The alpha value is a float, too. So I'd either write 0 for everything
>> and let the compiler handle the implicit conversion or (better, in my
>> opinion) use 0.f (or 0.0f). Which brings me to that I'd rather not use
>> doubles when the function takes floats...
> Ok.
>
>>> +    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
>> But you don't need glClearColor() at all, and you don't need this
>> glClear() either. The depth test is disabled and the quad fills the
>> whole screen, thus you can just leave the depth and color buffer as they
>> are.
> The quad fills the whole screen only in case host window and guest
> screen have the same aspect ratio, otherwise there is padding top/bottom
> or left/right so we don't change the guests screen aspect ratio.

Right; additionally, I thought glClear() will be limited by the 
viewport. It doesn't, it is indeed necessary, yes. (Well, you could drop 
the GL_DEPTH_BUFFER_BIT, but if you're already clearing the color buffer 
it shouldn't really matter)

>>> +
>>> +    glGenTextures(1, &tex);
>>> +    glBindTexture(GL_TEXTURE_2D, tex);
>>> +    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, gw, gh,
>>> +                 0, GL_BGRA_EXT,GL_UNSIGNED_BYTE,
>> I feared this extensions might not be widespread enough, but EXT_bgra is
>> from 1997 so it should be fine. :-)
> Note to self:  This also needs to be extended to handle other surface
> formats.
>
>>> +                 surface_data(scon->surface));
>>> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
>>> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
>>> +
>>> +    glEnable(GL_TEXTURE_2D);
>> Shouldn't you call this before doing the first operation on that target?
>> (that is, before the glBindTexture())
> Probably ...
>
>>> +    glBegin(GL_QUADS);
>>> +    glTexCoord2f(0, 1);  glVertex3f(-1, -1, 0);
>>> +    glTexCoord2f(0, 0);  glVertex3f(-1, 1, 0);
>>> +    glTexCoord2f(1, 0);  glVertex3f(1, 1, 0);
>>> +    glTexCoord2f(1, 1);  glVertex3f(1, -1, 0);
>>> +    glEnd();
>> I've been trained to hate direct mode, but it should be fine for just
>> this quad.
> --verbose please.  Guess for longer sequences it would be much more
> efficient to compile this into a shader program?

Well, again, I'm used to OpenGL 3/4 Core now which doesn't have the 
immediate mode any more. Now, there are vertex arrays which are just 
arrays of the vertex data which are transferred to the GPU and then stay 
there.

For OpenGL 2 at least, there is something which is called vertex arrays 
as well which works nearly the same way. The main difference is that 
here there are not only vertex arrays, but also texture coordinate 
arrays, normal arrays and color arrays (whereas in 3/4 Core there is 
only opaque vertex data, which OpenGL does not care about whether that's 
a position or a texture coordinate or whatever).

So, I'm not sure whether these exist for OpenGL 1 as well... There are 
display lists, but they were just a dead end. So, immediate mode is dead 
and buried today, but it shouldn't be too bad here and maybe for some 
reason there are people which want to use qemu with OpenGL acceleration 
on a pre OpenGL 2 machine.

>> First, you may consider using glVertex2f().
>>
>> Second, as hinted above, I don't like giving ints where floats are
>> expected. So I'd like this to be "glTexCoord2f(0.f, 1.f)" etc., or
>> (maybe even better because it prevents a discussion about whether to use
>> 0.f or 0 :-)) just glTexCoord2i() and glVertex2i().
> Using gl*i makes sense indeed.
>
>>> +
>>> +    SDL_GL_SwapWindow(scon->real_window);
>>> +
>>> +    glDisable(GL_TEXTURE_2D);
>>> +    glDeleteTextures(1, &tex);
>>> +}
>> Also, it hurts to always enable textures, generate one, load it, disable
>> textures and delete them just to render a single quad with a texture
>> loaded from main memory... (not to mention glViewport(), the matrix
>> operations and SDL_GL_MakeCurrent())
>>
>> Would it be possible to just enable GL_TEXTURE_2D once, store the GL ID
>> of the texture in the sdl2_console object and either use glTexImage2D()
>> or, technically better, glTexSubImage2D() here?
> Probably.  I don't want tie this into sdl too much though.  My
> longer-term plan is to have some generic gl helper functions in the
> console code and have all uis with gl support use these.

Well, then you could create an opaque gl_state object or something which 
must be passed by all UI implementations which want to use OpenGL.

Max

>> Using glTexSubImage2D() would give us the advantage of being able to
>> perform partial updates on the texture; but it seems to fit pretty bad
>> into the existing code. To make it fit, I'd call glTexSubImage2D()
>> directly in sdl2_gl_update() and just draw the quad here.
> Yes, that should work.
>
> cheers,
>    Gerd