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

[Brian Paul]

On 06/11/2012 03:15 PM, Ian Romanick wrote:
> 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.

Looks like I never spec'd the behaviour for glGetTexImage().  Reading 
the code, it appears that getting ycbcr as RGB would work and Pauli's 
fix would be relevant for that.

-Brian