[Intel-gfx] [PATCH] i915 XvMC: fixup colors

[Daniel Vetter]

On Mon, Mar 08, 2010 at 10:49:25AM -0800, Carl Worth wrote:
> On Mon,  8 Mar 2010 10:27:47 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > I think we can safely assume that I'm colorblind ;)
> > My cleanup accidently created a inconsistency in the YUV plane ordering.
> 
> Hi Daniel,
> 
> I'm quite video-clueless, so can you explain the change to me in a bit
> more detail?
> 
> From the description I expected to find that your earlier cleanup
> changed the code here unintentionally and that this current change would
> revert it. But when I looked at the code, I see the following:
> 
> Before b11623f20e303ae1d90d4a6bf0d5d73970b4e9bf:
> 
> #define YOFFSET(surface)        (surface->srf.offset)
> #define UOFFSET(surface)        (surface->srf.offset + \
>                                  SIZE_Y420(surface->width, surface->height) + \
>                                  SIZE_UV420(surface->width, surface->height))
> #define VOFFSET(surface)        (surface->srf.offset + \
>                                  SIZE_Y420(surface->width, surface->height))
> 
> After  b11623f20e303ae1d90d4a6bf0d5d73970b4e9bf:
> 
> #define UOFFSET(surface)        (SIZE_Y420(surface->width, surface->height) + \
>                                  SIZE_UV420(surface->width, surface->height))
> #define VOFFSET(surface)        (SIZE_Y420(surface->width, surface->height))
> 
> After the proposed patch:
> 
> #define UOFFSET(surface)        (SIZE_Y420(surface->width, surface->height))
> #define VOFFSET(surface)        (SIZE_Y420(surface->width, surface->height) + \
>                                  SIZE_UV420(surface->width, surface->height))
> 
> Which is to say, the current patch is the one change changes the
> calculations of UOFFSET and VOFFSET. So is there some other side effect
> not visible in this code that justifies that change? If so, could you
> explain that in the commit message?
> 
> -Carl (who is perhaps missing something obvious here...)

Nope, you're not missing something obvious. Here's the explanation:

For Xv planar formats, the three planes are stored consecutively in
memory, ordered Y U V. Now for some totally odd reason (= none at all),
i915 xvmc stored it in Y V U order. Right after the release of 2.10, with
commit "Xv: consolidate xmvc passthrough handling" I've inadvertently
broken xvmc support (which started this whole odyssey into xvmc). When
fixing stuff up, I neglected this special plane ordering and simply
assumed it to be the same as Xv and dropped that special case for i915 in
src/i830_video.c. This patch completes the change to standard YUV plane
ordering by making the corresponding change in src/xvmc/i915_xvmc.c.

btw, if you wonder how I could miss this in all the testing (and assume
I'm not totatlly color-blind): I've used a random, rather greyish
war-scene movie snippet to test and always blamed the greenish look on
night-vision. Turns out it's a daylight shot ...

Hope that helps, Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48