[PATCH weston] gl-renderer: Set pitch correctly for subsampled textures

[Daniel Stone]

Hi Pekka,

On 6 October 2017 at 08:41, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> I feel the commit message needs more rationale on why this is the
> correct fix before I can give a R-b.

Fair enough.

> How confident are we that waylandsink and the others (which?) are
> correct and did not have an unintended code change since fdeefe42418
> was developed?

I have no idea.

> Surely the original patch fdeefe42418 was tested to work fine when it
> was merged, so what changed and where to break things?

No idea; I'd like to hear more from Vincent. :)

> Does the format specify the U and V pitch, or is it left as
> implementation defined which means we cannot universally support
> sub-sampled planes with wl_shm at all?

The format is taken from KMS, which has explicit per-plane pitch
carried. wl_shm doesn't, so it's up to us. If by 'the format' you mean
the FourCC, then no, it doesn't define anything to do with strides.
Intel's Xv implementation halves the stride for the chroma planes of
I420 & NV12; Xv is probably the most widely-deployed user which
doesn't have an explict stride for the secondary plane(s). So I'd
suggest it's the right thing to do.

> That is, do we need to augment the Wayland wl_shm YUV420 format
> definition to specify the exact pitch requirements for a working
> implementation to be possible? (We already have expectations on the U
> and V plane offsets, obviously.)

Sure, we can do that. Ditto NV12 etc.

> After all, it is perfectly possible to construct a YUV420 buffer with
> either the old or the new pitch layouts both. If we only support one
> layout, what is the basis of deciding which one to support and which
> one to deem invalid?

According to a Google search, as well as a GitHub code search, the
only user of WL_SHM_FORMAT_YUV420 is ... GStreamer waylandsink. And
that doesn't seem to have changed in quite some time, though Nicolas
could probably confirm that. So this is probably just fixing it up
full stop?

>> Tested with:
>>   $ gst-launch-1.0 videotestsrc ! waylandsink
>>
>> Signed-off-by: Daniel Stone <daniels at collabora.com>
>> Fixes: fdeefe42418 ("gl-renderer: add support of WL_SHM_FORMAT_YUV420")
>
> When I read fdeefe42418, it seems to me that the glTexSubImage2D() path
> as well was broken in a different way originally. The pixel and row
> skips are not sub-sampled for U and V either. Should they be?

Hm. Probably, yes; I'd need to figure out how to make GStreamer export
a partial surface to test it, but I can't see why that wouldn't be
true. I've added that in locally.

Cheers,
Daniel