[PATCH weston-ivi-shell v2 01/10] This vfunc lets us read out a rectangle of pixels from the currently attached surface buffer.

[Pekka Paalanen]

On Wed, 12 Mar 2014 23:46:35 +0900
Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp> wrote:

> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> ---
> 
> Changes for v2:
>    - gl_renderer_read_shmbuffer_pixels to support different types of format of
>      shmbuffer.
> 
>  src/compositor.h  |   3 ++
>  src/gl-renderer.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/src/compositor.h b/src/compositor.h
> index 22a485f..ace75da 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -540,6 +540,9 @@ struct weston_renderer {
>  			       pixman_format_code_t format, void *pixels,
>  			       uint32_t x, uint32_t y,
>  			       uint32_t width, uint32_t height);
> +	int (*read_surface_pixels)(struct weston_surface *es,
> +				   pixman_format_code_t format, void *pixels,
> +				   int x, int y, int width, int height);
>  	void (*repaint_output)(struct weston_output *output,
>  			       pixman_region32_t *output_damage);
>  	void (*flush_damage)(struct weston_surface *surface);
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index 0e5afbe..7881fd8 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -106,6 +106,8 @@ struct gl_renderer {
>  	EGLContext egl_context;
>  	EGLConfig egl_config;
>  
> +	GLuint fbo;
> +
>  	struct wl_array vertices;
>  	struct wl_array vtxcnt;
>  
> @@ -585,6 +587,114 @@ out:
>  	pixman_region32_fini(&repaint);
>  }
>  
> +static int
> +gl_renderer_read_shmbuffer_pixels(struct wl_shm_buffer *shm_buffer,
> +				  void *pixels, int x, int y, int width, int height)

How is 'pixels' allocated, what should we assume?

Is the size of the 'pixels' buffer as width*height*4 bytes, with
stride=width*4 bytes?

What do we assume about the 'pixels' color format?

What if the shm_buffer has color format RGB565?

What if shm_buffer stride is more than width*4 bytes?

But, see below.

> +{
> +	int i;
> +	int pixel_size = 0;
> +	uint8_t *ptr = NULL;
> +	int32_t src_width, src_height, src_stride;
> +	int32_t dst_stride;
> +	int32_t offset;
> +
> +	/* Get some parameters of wl_shm_buffer. */
> +	src_width = wl_shm_buffer_get_width(shm_buffer);
> +	src_height = wl_shm_buffer_get_height(shm_buffer);
> +	src_stride = wl_shm_buffer_get_stride(shm_buffer);
> +
> +	assert((src_width > 0) && (src_height > 0));
> +	if ((src_width <= 0) || (src_height <= 0))
> +		return -1;
> +
> +	/* the start of reading position has to be changed. */
> +	ptr = wl_shm_buffer_get_data(shm_buffer);
> +	ptr += y * src_stride;
> +	if ((x == 0) && (width == src_width) && (height <= (src_height - y))) {
> +		/* If x is 0 and widths are the same,
> +		 * whole pixel can be copied in only one time.
> +		 */
> +		memcpy(pixels, ptr, src_stride * height);
> +	} else {
> +		/* If x is not 0,
> +		 * every line have to be copied one by one.
> +		 */
> +		pixel_size = src_stride / src_width;
> +		assert(pixel_size < 5);
> +		if (pixel_size > 4) return -1;
> +
> +		dst_stride = width * pixel_size;
> +		offset = x * pixel_size;
> +
> +		for (i = 0; i < height; i++, ptr += src_stride) {
> +			memcpy(pixels, ptr + offset, dst_stride);
> +			pixels = (uint8_t*)pixels + dst_stride;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +gl_renderer_read_glsurface_pixels(struct weston_surface *es,
> +				pixman_format_code_t format, void *pixels,
> +				int x, int y, int width, int height)
> +{
> +	struct weston_compositor *ec = es->compositor;
> +	struct gl_renderer *gr = get_renderer(ec);
> +	struct gl_surface_state *gs = get_surface_state(es);
> +	GLenum gl_format;
> +
> +	switch (format) {
> +	case PIXMAN_a8r8g8b8:
> +		gl_format = GL_BGRA_EXT;
> +		break;
> +	case PIXMAN_a8b8g8r8:
> +		gl_format = GL_RGBA;
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	if (gr->fbo == 0)
> +		glGenFramebuffers(1, &gr->fbo);
> +	glBindFramebuffer(GL_FRAMEBUFFER, gr->fbo);
> +	glFramebufferTexture2D(GL_FRAMEBUFFER,
> +			       GL_COLOR_ATTACHMENT0,
> +			       GL_TEXTURE_2D,
> +			       gs->textures[0], 0);
> +
> +	glReadPixels(x, y, width, height,
> +		     gl_format, GL_UNSIGNED_BYTE, pixels);
> +
> +	glBindFramebuffer(GL_FRAMEBUFFER, 0);
> +
> +	return 0;
> +}
> +
> +static int
> +gl_renderer_read_surface_pixels(struct weston_surface *es,
> +				pixman_format_code_t format, void *pixels,
> +				int x, int y, int width, int height)
> +{
> +	struct weston_buffer *buffer = es->buffer_ref.buffer;
> +	struct wl_shm_buffer *shm_buffer = NULL;
> +	int ret = 0;
> +
> +	if (buffer) {
> +		shm_buffer = wl_shm_buffer_get(buffer->resource);
> +	}
> +	if (shm_buffer) {
> +		ret = gl_renderer_read_shmbuffer_pixels(shm_buffer,
> +				   pixels, x, y, width, height);
> +	} else {
> +		ret = gl_renderer_read_glsurface_pixels(es,
> +				   format, pixels, x, y, width, height);
> +	}

I just realized that we are dealing with the GL-renderer only here.
That means that the surface will always have a GL texture, which you
can read back, regardless of if the original wl_buffer was wl_shm or
EGL based.

Therefore I think you could remove the whole shm path. I suppose
optimizing the shm read-back path is not too useful, right?

That is also the reason why you would probably never hit the shm case
to begin with. GL-renderer releases the wl_buffer as soon as it has
copied its content into a texture, so the buffer pointer is likely
always NULL.

Sorry for not realizing that earlier.

So just one more question: what about YUV color formats? That's why
gl_surface_state has an array of textures instead of just one. YUV data
can come in via EGL based buffers. You could at least return failure
for unhandled color formats, maybe?


Thanks,
pq

> +
> +	return ret;
> +}
> +
>  static void
>  repaint_views(struct weston_output *output, pixman_region32_t *damage)
>  {
> @@ -1602,6 +1712,9 @@ gl_renderer_destroy(struct weston_compositor *ec)
>  
>  	wl_signal_emit(&gr->destroy_signal, gr);
>  
> +	if (gr->fbo)
> +		glDeleteFramebuffers(1, &gr->fbo);
> +
>  	if (gr->has_bind_display)
>  		gr->unbind_display(gr->egl_display, ec->wl_display);
>  
> @@ -1699,6 +1812,7 @@ gl_renderer_create(struct weston_compositor *ec, EGLNativeDisplayType display,
>  		return -1;
>  
>  	gr->base.read_pixels = gl_renderer_read_pixels;
> +	gr->base.read_surface_pixels = gl_renderer_read_surface_pixels;
>  	gr->base.repaint_output = gl_renderer_repaint_output;
>  	gr->base.flush_damage = gl_renderer_flush_damage;
>  	gr->base.attach = gl_renderer_attach;