[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RfC PATCH 2/3] sdl2: add support for display rendering
From: |
Gerd Hoffmann |
Subject: |
Re: [Qemu-devel] [RfC PATCH 2/3] sdl2: add support for display rendering using opengl. |
Date: |
Fri, 12 Dec 2014 12:04:57 +0100 |
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
Re: [Qemu-devel] [RfC PATCH 0/3] sdl2: add opengl rendering support, Paolo Bonzini, 2014/12/11