On Thu, 5 Oct 2017 15:36:08 +0100 Daniel Stone <[email protected]> wrote:
> Multi-plane sub-sampled textures have partial width/height, e.g. > YUV420/I420 has a full-size Y plane, followed by a half-width/height U > plane, and a half-width/height V plane. > > zwp_linux_dmabuf_v1 allows clients to pass an explicit pitch for each > plane, but for wl_shm this must be inferred. gl-renderer was correctly > accounting for the width and height when subsampling, but the pitch was > being taken as the pitch for the first plane. > > This does not match the requirements for GStreamer's waylandsink, in > particular, as well as other clients. Fix the SHM upload path to > correctly set the pitch for each plane, according to subsampling. Hi, I feel the commit message needs more rationale on why this is the correct fix before I can give a R-b. How confident are we that waylandsink and the others (which?) are correct and did not have an unintended code change since fdeefe42418 was developed? Surely the original patch fdeefe42418 was tested to work fine when it was merged, so what changed and where to break things? 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? 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.) 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? > Tested with: > $ gst-launch-1.0 videotestsrc ! waylandsink > > Signed-off-by: Daniel Stone <[email protected]> > 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? > Reported-by: Fabien Lahoudere <[email protected]> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103063 > Cc: Vincent Abriou <[email protected]> > --- > libweston/gl-renderer.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c > index 244ce309..40bf0bb6 100644 > --- a/libweston/gl-renderer.c > +++ b/libweston/gl-renderer.c > @@ -1445,14 +1445,13 @@ gl_renderer_flush_damage(struct weston_surface > *surface) > goto done; > } > > - glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT, gs->pitch); > - > if (gs->needs_full_upload) { > glPixelStorei(GL_UNPACK_SKIP_PIXELS_EXT, 0); > glPixelStorei(GL_UNPACK_SKIP_ROWS_EXT, 0); > wl_shm_buffer_begin_access(buffer->shm_buffer); > for (j = 0; j < gs->num_textures; j++) { > glBindTexture(GL_TEXTURE_2D, gs->textures[j]); > + glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT, gs->pitch / > gs->hsub[j]); > glTexImage2D(GL_TEXTURE_2D, 0, > gs->gl_format[j], > gs->pitch / gs->hsub[j], > @@ -1477,6 +1476,7 @@ gl_renderer_flush_damage(struct weston_surface *surface) > glPixelStorei(GL_UNPACK_SKIP_ROWS_EXT, r.y1); > for (j = 0; j < gs->num_textures; j++) { > glBindTexture(GL_TEXTURE_2D, gs->textures[j]); > + glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT, gs->pitch / > gs->hsub[j]); > glTexSubImage2D(GL_TEXTURE_2D, 0, > r.x1 / gs->hsub[j], > r.y1 / gs->vsub[j], Technically the patch looks correct to me. Thanks, pq
pgpaRwsAf_hpc.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
