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

[Gerd Hoffmann <kraxel@redhat.com>]

  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