[Mesa-dev] [PATCH 1/2] mesa: Add functions _mesa_[un]pack_is_default()

[Brian Paul]

On 10/12/2012 05:31 AM, Neil Roberts wrote:
> Chad Versace<chad.versace at linux.intel.com>  writes:
>
>> +bool
>> +_mesa_pack_is_default(struct gl_context *ctx)
>> +{
>> +   return memcmp(&ctx->Pack,&ctx->DefaultPacking,
>> +                 sizeof(struct gl_pixelstore_attrib)) == 0;
>> +}
>
> It seems like a shame that this wouldn't catch the cases where the row
> length and alignment are set to values that are equivalent to the
> default. For example, the API in Cogl just accepts a rowstride and we
> use that to calculate values to pass for the alignment and row length.
> That means the row length would never be zero and Cogl would never be
> able to take advantage of the optimisation. We can't be the only ones
> who do that! Maybe the function could also take the width and format and
> check that the alignment is a multiple of the cpp of the format and that
> the row length == the width.

Also note that SkipRows and SkipPixels wind up just being an offset to 
the start of the image data.  It shouldn't be too hard to incoporate 
that offset into any memcpy/transfer.

And note that SkipImages is to be ignored when transferring a 1D or 2D 
image.  But I doubt anyone would knowingly set SkipImages when working 
with 2D images.

Some of this is a bit obscure and wouldn't be hit much in practice but 
Niel's case seems pretty reasonable.


> The gl_pixelstore_attrib has a pointer at the bottom after 3 byte
> fields. That means the struct will have some padding to make it aligned
> and for the memcmp to pass it will also have have the same values for
> the unused padding bytes. I'm not sure if that's a real concern or not
> but it doesn't seem very robust.

I think it's OK (modulo my next paragraph).  Every driver that I'm 
aware of allocates its context objects with calloc().  So, the unused 
bytes in ctx->Pack, ctx->Unpack and ctx->DefaultPacking should be 
zero, and I don't see any way for them to become non-zero later.

HOWEVER, note that ctx->DefaultPacking.Alignment = 1, not 4, so this 
object doesn't really reflect the OpenGL spec.

The original use of ctx->DefaultPacking was for display lists where we 
need to create copies of texture images, glDrawPixels images, glBitmap 
images, etc.  In the later case in particular, we can store bitmap 
images (font glyphs) more compactly by using a 1-byte row alignment, 
rather than 4-byte alignment.

Chad, for the time being, I think it would be best if you implemented 
your own pack_is_default() function in the i965 driver so you can 
catch exactly the cases you're interested in.

-Brian