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

[Pekka Paalanen]

On Thu,  5 Oct 2017 15:36:08 +0100
Daniel Stone <daniels at collabora.com> 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 <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?

> Reported-by: Fabien Lahoudere <fabien.lahoudere at collabora.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103063
> Cc: Vincent Abriou <vincent.abriou at st.com>
> ---
>  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
-------------- 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/20171006/6f776021/attachment-0001.sig>