From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33984)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1YC35p-0007hd-2D
	for qemu-devel@nongnu.org; Fri, 16 Jan 2015 04:21:25 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1YC35m-0005ZB-C0
	for qemu-devel@nongnu.org; Fri, 16 Jan 2015 04:21:24 -0500
Received: from mx1.redhat.com ([209.132.183.28]:49439)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1YC35m-0005Z5-3M
	for qemu-devel@nongnu.org; Fri, 16 Jan 2015 04:21:22 -0500
Message-ID: <1421400077.21318.31.camel@nilsson.home.kraxel.org>
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 16 Jan 2015 10:21:17 +0100
In-Reply-To: <54B8270A.8050007@redhat.com>
References: <1421066126-25737-1-git-send-email-kraxel@redhat.com>
	<1421066126-25737-3-git-send-email-kraxel@redhat.com>
	<54B8270A.8050007@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 2/4] console: add opengl rendering helper
 functions
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Max Reitz <mreitz@redhat.com>
Cc: qemu-devel@nongnu.org, Anthony Liguori <aliguori@amazon.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