[PATCH weston v10 02/61] libweston: Add pixel-format helpers

[Pekka Paalanen]

On Tue,  4 Apr 2017 17:54:20 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Rather than duplicating knowledge of pixel formats across several
> components, create a custom central repository.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  Makefile.am               |   2 +
>  libweston/pixel-formats.c | 430 ++++++++++++++++++++++++++++++++++++++++++++++
>  libweston/pixel-formats.h | 194 +++++++++++++++++++++
>  3 files changed, 626 insertions(+)
>  create mode 100644 libweston/pixel-formats.c
>  create mode 100644 libweston/pixel-formats.h
> 
> v10: Add more docs. Build without EGL. Fix format transposition errors.
>      Insert omitted [hv]sub.
> 
> diff --git a/Makefile.am b/Makefile.am
> index 519d9115..7b24d40c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -105,6 +105,8 @@ libweston_ at LIBWESTON_MAJOR@_la_SOURCES =			\
>  	libweston/timeline-object.h			\
>  	libweston/linux-dmabuf.c			\
>  	libweston/linux-dmabuf.h			\
> +	libweston/pixel-formats.c			\
> +	libweston/pixel-formats.h			\
>  	shared/helpers.h				\
>  	shared/matrix.c					\
>  	shared/matrix.h					\

Hi,

all the actual content is good now, there are a couple of typoes in the
comments.

The remaining issue is the dependency on libdrm headers, more below.

> diff --git a/libweston/pixel-formats.c b/libweston/pixel-formats.c
> new file mode 100644
> index 00000000..2c58443c
> --- /dev/null
> +++ b/libweston/pixel-formats.c
> @@ -0,0 +1,430 @@
> +/*
> + * Copyright © 2016 Collabora, Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Author: Daniel Stone <daniels at collabora.com>
> + */
> +
> +#include "config.h"
> +
> +#include <endian.h>
> +#include <inttypes.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <libdrm/drm_fourcc.h>

Shouldn't this be just <drm_fourcc.h>? We have pkg-config providing us
with -I/usr/include/libdrm.

The problem with this is that this makes libweston unconditionally
depend on libdrm headers at build time (not the library, and nothing
new at runtime).

I think that is ok by now. Do people agree?

An alternative would be to #ifdef the whole array as empty if libdrm
headers are not available, or move this into drm-backend.so if possible.

Anyway we need libdrm cflags for compiling this file when libdrm
headers are needed.

Btw. gl-renderer.c is similar, it needs drm_fourcc.h but not libdrm.so
itself.

I presume the plan is to start using these in gl-renderer and
linux-dmabuf.c in the future, to reduce format definition duplication
and add more sanity checks for dmabufs.

Therefore I propose that we make libdrm a hard build dependency (not
runtime dependency) for libweston.so, i.e. libweston core.

I can write the patch to add that requirement, and then the changes
needed to this pixel-format patch will be minor.

How's that?

> +
> +#include "helpers.h"
> +#include "wayland-util.h"
> +#include "pixel-formats.h"
> +
> +#if ENABLE_EGL
> +#include <EGL/egl.h>
> +#include <EGL/eglext.h>
> +#include <GLES2/gl2.h>
> +#include <GLES2/gl2ext.h>
> +#define GL_FORMAT(fmt) .gl_format = (fmt)
> +#define GL_TYPE(type) .gl_type = (type)
> +#define SAMPLER_TYPE(type) .sampler_type = (type)
> +#else
> +#define GL_FORMAT(fmt) .gl_format = 0
> +#define GL_TYPE(type) .gl_type = 0
> +#define SAMPLER_TYPE(type) .sampler_type = 0
> +#endif
> +
> +#include "weston-egl-ext.h"
> +


> +
> +/**
> + * Return the effective sampling height for a given plane
> + *
> + * When vertical subsampling is in effect, a sampler bound to a secondary
> + * plane must bind the sampler with a smaller effective height. This function
> + * returns the effective width to use for the sampler, i.e. dividing by vsub.

s/width/height/

> + *
> + * If vertical subsampling is not in effect, this will be equal to the height.
> + *
> + * @param format Pixel format info structure
> + * @param plane Zero-indexed plane number
> + * @param width Height of the buffer

s/width/height/

> + * @returns Effective width for sampling
> + */
> +unsigned int
> +pixel_format_height_for_plane(const struct pixel_format_info *format,
> +			      unsigned int plane,
> +			      unsigned int height);


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/20170405/e7fcd1b7/attachment-0001.sig>