Re: [Buildroot] [PATCH] package/zimg: add zimg package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Michael,

On Tue,  6 Jan 2026 19:57:26 +0100
Michael Cullen <michael@michaelcullen.name> wrote:

> This commit adds an image transformation library called zimg.
> 
> There are a number of patches applied. One is from upstream and supports
> building on GCC-15, the others are autotools patches to allow installing
> various components useful for verifying the library is working on the
> target, which are not usually installed by the upstream project.
> 
> Note that while this version of zimg claims to support C++11 upwards,
> upstream is moving towards only supporting C++17 upwards in the master
> branch.
> 
> This has been tested on a Raspberry Pi 4 and a Raspberry Pi 5, and on
> x86_64 (run in a chroot).
> 
> Signed-off-by: Michael Cullen <michael@michaelcullen.name>

Thanks for this patch! It overall looks good, and I only have a few
comments see below.


> diff --git a/package/zimg/0002-Install-test-app.patch b/package/zimg/0002-Install-test-app.patch
> new file mode 100644
> index 0000000000..cbafd0bd51
> --- /dev/null
> +++ b/package/zimg/0002-Install-test-app.patch
> @@ -0,0 +1,55 @@
> +From d4da533bec77c7f11aef08bed4888a82e1af8cba Mon Sep 17 00:00:00 2001
> +From: Michael Cullen <michael@michaelcullen.name>
> +Date: Tue, 6 Jan 2026 10:35:15 +0100
> +Subject: [PATCH] Install test app
> +
> +To make sure zimg is working, it would be useful to be able to run the
> +test app on the target. Currently, however, it is not installed.
> +
> +This patch renames it and installs it.
> +
> +Upstream: N/A - running test app on the target only applies to cross-compilation situations

For this one, and the other 2 patches that you marked as not
upstreamable, could you take the same approach as your patch
0004-Add-option-to-install-tests.patch, which is to add a ./configure
option to enable/disable installation of the test application, of the
examples. And then it makes that upstreamable. You keep the default as
disabled, so nothing changes, but you allow people like you/Buildroot
who need those features to actually enable them. There's nothing "not
upstreamable" in those changes IMO, and it would anyway not be nice to
integrate a brand new package in Buildroot that has so many
non-upstream patches.

> diff --git a/package/zimg/Config.in b/package/zimg/Config.in
> new file mode 100644
> index 0000000000..3fbac545dc
> --- /dev/null
> +++ b/package/zimg/Config.in
> @@ -0,0 +1,27 @@
> +config BR2_PACKAGE_ZIMG
> +	bool "zimg"
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 # C++11
> +	help
> +	  Scaling, colorspace conversion, and dithering library
> +
> +	  https://github.com/sekrit-twc/zimg
> +
> +if BR2_PACKAGE_ZIMG
> +
> +config BR2_PACKAGE_ZIMG_TESTAPP
> +	bool "zimg test app"
> +
> +config BR2_PACKAGE_ZIMG_EXAMPLES
> +	bool "zimg examples"
> +
> +config BR2_PACKAGE_ZIMG_UNIT_TESTS
> +	bool "zimg unit tests"
> +
> +config BR2_PACKAGE_ZIMG_DISABLE_SIMD
> +	bool "Disable SIMD in zimg"
> +
> +endif
> +
> +comment "zimg needs a toolchain w/ C++11 support"

This should be:

comment "zime needs a toolchain w/ C++, gcc >= 4.9"

> +	depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_GCC_AT_LEAST_4_9
> diff --git a/package/zimg/zimg.mk b/package/zimg/zimg.mk
> new file mode 100644
> index 0000000000..0dc314c765
> --- /dev/null
> +++ b/package/zimg/zimg.mk
> @@ -0,0 +1,33 @@
> +################################################################################
> +#
> +# zimg
> +#
> +################################################################################
> +
> +ZIMG_VERSION = release-3.0.6
> +# This project uses submodules so the tarballs aren't really useful
> +ZIMG_SITE_METHOD = git
> +ZIMG_GIT_SUBMODULES = yes
> +ZIMG_SITE = https://github.com/sekrit-twc/zimg.git
> +ZIMG_LICENSE = WTFPL
> +ZIMG_LICENSE_FILES = COPYING
> +ZIMG_INSTALL_STAGING = YES
> +ZIMG_AUTORECONF = YES
> +
> +ifeq ($(BR2_PACKAGE_ZIMG_TESTAPP),y)
> +ZIMG_CONF_OPTS += --enable-testapp
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ZIMG_EXAMPLES),y)
> +ZIMG_CONF_OPTS += --enable-example
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ZIMG_UNIT_TESTS),y)
> +ZIMG_CONF_OPTS += --enable-unit-test --enable-install-tests
> +endif

Please add explicit disable in "else" clauses:

ifeq (...)
FOO_CONF_OPTS += --enable-bar
else
FOO_CONF_OPTS += --disable-bar
endif

> +
> +ifeq ($(BR2_PACKAGE_ZIMG_DISABLE_SIMD),y)

Meh, can we instead rely on existing architecture options, like MMX,
SSE, AVX, NEON, etc. to decide whether to enable SIMD or not?

> +ZIMG_CONF_OPTS += --disable-simd
> +endif
> +
> +$(eval $(autotools-package))

Aside from those comments, looks good. Could you rework a bit, and
submit a v2?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot