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

[Chad Versace]

On 10/12/2012 07:56 AM, Brian Paul wrote:
> 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

Ok, I NAK this series and will submit a fix local to the Intel driver.

I tried to use a big hammer, and it seems that it was *too* big.

-Chad