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

[Gerd Hoffmann <kraxel@redhat.com>]

  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.

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

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

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

> 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