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

[Pekka Paalanen]

Hi Nicolas and Daniel,

excellent comments, I think we just need a summary of them in the
commit message and we could land the original patch. If you want to
squash in the glTexSubImage2D fix, that would be fine as well.

On Fri, 06 Oct 2017 15:06:55 -0400
Nicolas Dufresne <nicolas.dufresne at collabora.com> wrote:

> 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

So it is actually legal to have a non-divisible width/height for
formats with some sub-sampled channels? I wasn't aware of even that,
very good to know. Shows that we really need all rounding rules you
refer to.

I suppose there is no written specification anywhere to refer to,
everything just grew up as code in various places like Xv
implementations?

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

Ok. Then I have no objections to this patch.

> >   
> > > 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 a convincing precedent, yes.

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

I wonder if it all actually fits in wayland.xml or do we need to refer
to a chapter in the Wayland docbook.

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

Agreed.

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

To test the code path in suspect, the client (GStreamer) would need to
post a frame with WL_SHM_FORMAT_YUV420 format, but with the damage
rectangle/region being only a subset of the image, e.g. 25%
sub-rectangle. The old and new images should be different and with a
pattern that makes it easy to see that the image is not broken,
rotated, mirrored or shifted.

I would image we could later replicate that test in Weston's test suite.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171009/cb0c9ae7/attachment.sig>