[Mesa-dev] [PATCH] osmesa: fix renderbuffer format selection

[Brian Paul]

On 01/13/2012 09:11 AM, Jose Fonseca wrote:
> ----- Original Message -----
>> The gl_renderbuffer::Format field wasn't always set properly.  This
>> didn't matter much in the past but with the recent
>> swrast/renderbuffer
>> mapping changes, core Mesa will be directly touching OSMesa
>> colorbuffers
>> so using the right MESA_FORMAT_x value is important.
>>
>> Unfortunately, there aren't MESA_FORMATs for all the possible OSmesa
>> format/type combinations, such as GL_FLOAT / OSMESA_ARGB.  If anyone
>> runs into these we can add new Mesa formats.
>>
>> NOTE: This is a candidate for the 8.0 branch.
>> ---
>>   src/mesa/drivers/osmesa/osmesa.c |   80
>>   ++++++++++++++++----------------------
>>   1 files changed, 34 insertions(+), 46 deletions(-)
>>
>> diff --git a/src/mesa/drivers/osmesa/osmesa.c
>> b/src/mesa/drivers/osmesa/osmesa.c
>> index 67d329f..57b7e88 100644
>> --- a/src/mesa/drivers/osmesa/osmesa.c
>> +++ b/src/mesa/drivers/osmesa/osmesa.c
>> @@ -723,9 +723,8 @@ osmesa_choose_line( struct gl_context *ctx )
>>   static void
>>   compute_row_addresses( OSMesaContext osmesa )
>>   {
>> -   GLint bytesPerPixel, bytesPerRow, i;
>> +   GLint bytesPerRow, i;
>>      GLubyte *origin = (GLubyte *) osmesa->rb->Data;
>> -   GLint bpc; /* bytes per channel */
>>      GLint rowlength; /* in pixels */
>>      GLint height = osmesa->rb->Height;
>>
>> @@ -734,32 +733,7 @@ compute_row_addresses( OSMesaContext osmesa )
>>      else
>>         rowlength = osmesa->rb->Width;
>>
>> -   if (osmesa->rb->DataType == GL_UNSIGNED_BYTE)
>> -      bpc = 1;
>> -   else if (osmesa->rb->DataType == GL_UNSIGNED_SHORT)
>> -      bpc = 2;
>> -   else if (osmesa->rb->DataType == GL_FLOAT)
>> -      bpc = 4;
>> -   else {
>> -      _mesa_problem(&osmesa->mesa,
>> -                    "Unexpected datatype in
>> osmesa::compute_row_addresses");
>> -      return;
>> -   }
>> -
>> -   if ((osmesa->format == OSMESA_RGB) || (osmesa->format ==
>> OSMESA_BGR)) {
>> -      /* RGB mode */
>> -      bytesPerPixel = 3 * bpc;
>> -   }
>> -   else if (osmesa->format == OSMESA_RGB_565) {
>> -      /* 5/6/5 RGB pixel in 16 bits */
>> -      bytesPerPixel = 2;
>> -   }
>> -   else {
>> -      /* RGBA mode */
>> -      bytesPerPixel = 4 * bpc;
>> -   }
>> -
>> -   bytesPerRow = rowlength * bytesPerPixel;
>> +   bytesPerRow = rowlength *
>> _mesa_get_format_bytes(osmesa->rb->Format);
>>
>>      if (osmesa->yup) {
>>         /* Y=0 is bottom line of window */
>> @@ -802,20 +776,33 @@ osmesa_renderbuffer_storage(struct gl_context
>> *ctx, struct gl_renderbuffer *rb,
>>      /* Note: we can ignoring internalFormat for "window-system"
>>      renderbuffers */
>>      (void) internalFormat;
>>
>> +   /* Given the user-provided format and type, figure out which
>> MESA_FORMAT_x
>> +    * to use.
>> +    * XXX There aren't Mesa formats for all the possible
>> combinations here!
>> +    * XXX Specifically, there's only RGBA-order 16-bit/channel and
>> float
>> +    * XXX formats.
>> +    * XXX The 8-bit/channel formats should all be OK.
>> +    */
>>      if (osmesa->format == OSMESA_RGBA) {
>>         if (rb->DataType == GL_UNSIGNED_BYTE) {
>> +         if (_mesa_little_endian())
>> +            rb->Format = MESA_FORMAT_RGBA8888_REV;
>> +         else
>> +            rb->Format = MESA_FORMAT_RGBA8888;
>>            rb->GetRow = get_row_RGBA8;
>>            rb->GetValues = get_values_RGBA8;
>>            rb->PutRow = put_row_RGBA8;
>>            rb->PutValues = put_values_RGBA8;
>>         }
>>         else if (rb->DataType == GL_UNSIGNED_SHORT) {
>> +         rb->Format = MESA_FORMAT_RGBA_16;
>>            rb->GetRow = get_row_RGBA16;
>>            rb->GetValues = get_values_RGBA16;
>>            rb->PutRow = put_row_RGBA16;
>>            rb->PutValues = put_values_RGBA16;
>>         }
>>         else {
>> +         rb->Format = MESA_FORMAT_RGBA_FLOAT32;
>>            rb->GetRow = get_row_RGBA32;
>>            rb->GetValues = get_values_RGBA32;
>>            rb->PutRow = put_row_RGBA32;
>> @@ -824,18 +811,24 @@ osmesa_renderbuffer_storage(struct gl_context
>> *ctx, struct gl_renderbuffer *rb,
>>      }
>>      else if (osmesa->format == OSMESA_BGRA) {
>>         if (rb->DataType == GL_UNSIGNED_BYTE) {
>> +         if (_mesa_little_endian())
>> +            rb->Format = MESA_FORMAT_ARGB8888;
>> +         else
>> +            rb->Format = MESA_FORMAT_ARGB8888_REV;
>>            rb->GetRow = get_row_BGRA8;
>>            rb->GetValues = get_values_BGRA8;
>>            rb->PutRow = put_row_BGRA8;
>>            rb->PutValues = put_values_BGRA8;
>>         }
>>         else if (rb->DataType == GL_UNSIGNED_SHORT) {
>> +         rb->Format = MESA_FORMAT_RGBA_16; /* not exactly right */
>
> Instead of continuing silently it might be better fail, assert, or write a message here, and save time when diagnosing such limitation, for those osmesa users who don't follow the list.

Yeah, that would save some time.


>
>>            rb->GetRow = get_row_BGRA16;
>>            rb->GetValues = get_values_BGRA16;
>>            rb->PutRow = put_row_BGRA16;
>>            rb->PutValues = put_values_BGRA16;
>>         }
>>         else {
>> +         rb->Format = MESA_FORMAT_RGBA_FLOAT32; /* not exactly right
>> */
>>            rb->GetRow = get_row_BGRA32;
>>            rb->GetValues = get_values_BGRA32;
>>            rb->PutRow = put_row_BGRA32;
>> @@ -844,18 +837,24 @@ osmesa_renderbuffer_storage(struct gl_context
>> *ctx, struct gl_renderbuffer *rb,
>>      }
>>      else if (osmesa->format == OSMESA_ARGB) {
>>         if (rb->DataType == GL_UNSIGNED_BYTE) {
>> +         if (_mesa_little_endian())
>> +            rb->Format = MESA_FORMAT_ARGB8888;
>> +         else
>> +            rb->Format = MESA_FORMAT_ARGB8888_REV;
>
> Shouldn't this one be the other way around? It's exactly like the OSMESA_BGRA case above.

Yes, you're right.


 >
 > Otherwise looks ok to me.
 >
 > Jose

Thanks.

-Brian