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

[Nicolas Dufresne]

Hi Daniel,

Le vendredi 06 octobre 2017 à 19:03 +0100, Daniel Stone a écrit :
> 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.

The GStreamer waylandsink have not changed much in this regard, it is
blindly passing the first plane pitch though. Ideally it should
extrapolate the pitches bases on the first pitch so see if it matches
the actual pitches and if not copy in order to "normalize" the pitches.

Though, videotestsrc pitches are already normalized base on common
knowledge found in UVC, XV and many software decoders. The
extrapolation is currently implemented in V4L2 and KMS plugins, see:

https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/kms/gstkmsallocator.c#n141
https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.c#n3143

Where GST_VIDEO_FORMAT_INFO_SCALE_WIDTH is something you could read as
"stride / hsub[i]", but rounded up for the case we scale a width, which
could be odd number. You can also find a switch which calculate default
strides from width/height/format. It contains all the rounding rules
that are commonly used to align lines on 32bit (or more) and also the
one imposed by sub-sampling, so each Y don't get left without a UV
pair.

https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/video-info.c#n698

> 
> > 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. :)

YUV over SHM never worked for me in GStreamer, but it wasn't clear
who's fault it was back then. It's just like YUV over DUMB buffer in
KMS/DRM. There is un-specified bits, so we can make it work, but it's
not very robust (was the same with XV really).

> 
> > 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

Strides in bytes are the same for both planes in NV12, but half in
pixels due to UV pairing, just to clarify.

> 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.

With a bit more detail of what you want, I can provide some test code
that would do that. Thanks for looking into this issue, it is
appreciated.

regards,
Nicolas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171006/f671cad3/attachment.sig>