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

Wed Nov 29 18:49:18 UTC 2017

Hi,

this patch is Reviewed-by: Arnaud Vrac <avrac at freebox.fr>

Nicolas made some changes to the waylandsink element in gstreamer [1] to
better support pitfalls of the SHM protocol. With this patch and the one I
posted earlier today, gstreamer git now works perfectly with weston when
using SHM, even when using YUV formats. It really is too bad that it's not
possible to specify the stride of each plane in the SHM protocol, but it's
too late to change this.

[1]
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/log/?id=9c866da3369be003dc5f3bea3b488c86b825c5e2

Thanks,
-Arnaud

On Mon, Oct 9, 2017 at 10:34 AM Vincent ABRIOU <vincent.abriou at st.com>
wrote:

> I resent it (issue in the mailing list address)
>
> Hi Daniel,
>
> Your patch is good to me.
>
>
> On 10/09/2017 09:15 AM, Pekka Paalanen wrote:
> > 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. :)
>
> My only explanation is that my tests where going into the
> !gr->has_unpack_subimage statement but I honestly don't remember.
>
> It just make sense to have pitch affected by the hsub parameter has it
> was originally done in the patch fdeefe42418 for pitch in glTextImage2D.
>
> glPixelStorei when to the trap door and was not tested.
>
> BR
> 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
> >
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171129/fdfa143d/attachment-0001.html>