[Mesa-dev] [PATCH 01/21] mesa/format_unpack: Fix YCBCR unpack

[Ian Romanick]

On 06/11/2012 07:41 AM, Brian Paul wrote:
> On 06/11/2012 12:59 AM, Pauli Nieminen wrote:
>> The YCBCR unpack was reading one RGBA value from 4 bytes of YCBCR
>> texture. But single pixel is only 2 bytes in the format. After
>> noticing that error Ville told me that second chroma sample has to
>> be average to match video requirements.
>>
>> Looking at places where unpack to rgba is called it is possible to get
>> odd X coordinate and odd number of bytes. That makes unpack operation
>> complex problem. But I assume that source allocation is 4 byte aligned
>> always. That should hold for most of allocations but may fail.
>>
>> Better solution would be threating YCBCR as compressed format
>> internally. But I suspect that I would break a lot of assumptions
>> everywhere with that changes.
>>
>> But same time as I was checking call sites I started suspecting that we
>> can't ever hit these unpack functions for YCBCR. But I need to study the
>> code paths a lot more to be sure if that is true.
>>
>> Changes to fix unpacking:
>> * Always start reading from the start of chroma pair.
>> * Always read to the end of chroma pair
>> * Unpack two RGB values in single iteration
>> * Read next chroma pair (if available) to calculate average for second
>> sample.
>> * Refactor shared color space conversion code to separate function.
>>
>> Signed-off-by: Pauli Nieminen<pauli.nieminen at linux.intel.com>
>
> This unpack code isn't used during texture sampling by swrast. I think
> the only way it would be hit is if one did a glReadPixels from ycbcr
> texture attached to a FBO. But we should probably disallow that
> (attaching ycbcr textures to FBOs) in the first place.

Either that, or glGetTexImage(GL_RGB) of a GL_YCBCR_MESA texture.  I 
don't think we're permitted to disallow that.

I think we need a test case for this.

> Have you found another path that hits this code?
>
> The changes look OK in any case.
>
> Reviewed-by: Brian Paul <brianp at vmware.com>
>
> -Brian
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>