Hi,

On Thu, Sep 15, 2016 at 08:34:45AM +0200, Guillermo Rodriguez Garcia wrote:
> Thank you for reviewing. Some comments below:
> 
> 2016-09-14 23:32 GMT+02:00 Roland Hieber <roh...@rohieb.name>:
> > Hey hey,
> >
> > On 14.09.2016 08:31, Guillermo Rodriguez Garcia wrote:
> >> Hello, any feedback on this one?
> >>
> >> Guillermo
> >>
> >> 2016-09-02 12:14 GMT+02:00 Guillermo Rodriguez 
> >> <guille.rodrig...@gmail.com>:
> >>> This adds a new package for ImageMagick 7. Some configuration
> >>> options are set to sensible defaults for embedded targets (quantum
> >>> depth set to 8 pixels, HDRI disabled). See notes in imagemagick.make.
> >>>
> >>> Signed-off-by: Guillermo Rodriguez <guille.rodrig...@gmail.com>
> >>>
> >>> ---
> >>>  rules/imagemagick.in   |   41 ++++++++++++++
> >>>  rules/imagemagick.make |  142 
> >>> ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 183 insertions(+)
> >>>  create mode 100644 rules/imagemagick.in
> >>>  create mode 100644 rules/imagemagick.make
> >>>
> >>> diff --git a/rules/imagemagick.in b/rules/imagemagick.in
> >>> new file mode 100644
> >>> index 0000000..90a0eb6
> >>> --- /dev/null
> >>> +++ b/rules/imagemagick.in
> >>> @@ -0,0 +1,41 @@
> >>> +## SECTION=multimedia_tools
> >>> +
> >>> +menuconfig IMAGEMAGICK
> >>> +       tristate
> >>> +       select ZLIB if IMAGEMAGICK_USE_ZLIB
> >>> +       select LIBPNG if IMAGEMAGICK_USE_LIBPNG
> >>> +       select LIBJPEG if IMAGEMAGICK_USE_LIBJPEG
> >
> > I guess these should be tab-aligned before "if", at least I got that as
> > feedback for my patch :)
> 
> This one is easy :)
> 
> >
> >>> +       prompt "imagemagick                   "
> >>> +       help
> >>> +         ImageMagick® is a software suite to create, edit, compose, or 
> >>> convert
> >>> +         bitmap images. It can read and write images in a variety of 
> >>> formats
> >>> +         (over 200) including PNG, JPEG, JPEG-2000, GIF, TIFF, DPX, EXR, 
> >>> WebP,
> >>> +         Postscript, PDF, and SVG. Use ImageMagick to resize, flip, 
> >>> mirror,
> >>> +         rotate, distort, shear and transform images, adjust image 
> >>> colors, apply
> >>> +         various special effects, or draw text, lines, polygons, 
> >>> ellipses and
> >>> +         B├ęzier curves.
> >>> +
> >>> +if IMAGEMAGICK
> >>> +
> >>> +config IMAGEMAGICK_USE_ZLIB
> >>> +       bool
> >>> +       default y
> >>> +       prompt "Enable ZLIB"
> >>> +       help
> >>> +         Enable ZLIB support
> >>> +
> >>> +config IMAGEMAGICK_USE_LIBPNG
> >>> +       bool
> >>> +       default y
> >>> +       prompt "Enable PNG"
> >>> +       help
> >>> +         Enable PNG support
> >>> +
> >>> +config IMAGEMAGICK_USE_LIBJPEG
> >>> +       bool
> >>> +       default y
> >>> +       prompt "Enable JPEG"
> >>> +       help
> >>> +         Enable JPEG support
> >>> +
> >>> +endif
> >>> diff --git a/rules/imagemagick.make b/rules/imagemagick.make
> >>> new file mode 100644
> >>> index 0000000..f07bab7
> >>> --- /dev/null
> >>> +++ b/rules/imagemagick.make
> >>> @@ -0,0 +1,142 @@
> >>> +# -*-makefile-*-
> >>> +#
> >>> +# Copyright (C) 2016 by Guillermo Rodriguez <guille.rodrig...@gmail.com>
> >>> +#
> >>> +# See CREDITS for details about who has contributed to this project.
> >>> +#
> >>> +# For further information about the PTXdist project and license 
> >>> conditions
> >>> +# see the README file.
> >>> +#
> >>> +
> >>> +#
> >>> +# We provide this package
> >>> +#
> >>> +PACKAGES-$(PTXCONF_IMAGEMAGICK) += imagemagick
> >>> +
> >>> +#
> >>> +# Paths and names
> >>> +#
> >>> +IMAGEMAGICK_VERSION    := 7.0.2-10
> >>> +IMAGEMAGICK_MD5                := e1cb23d9c10a8eff228ef30ee281711a
> >>> +IMAGEMAGICK            := ImageMagick-$(IMAGEMAGICK_VERSION)
> >>> +IMAGEMAGICK_SUFFIX     := tar.xz
> >>> +IMAGEMAGICK_URL                := 
> >>> ftp://ftp.nluug.nl/pub/ImageMagick/$(IMAGEMAGICK).$(IMAGEMAGICK_SUFFIX)
> >>> +IMAGEMAGICK_SOURCE     := $(SRCDIR)/$(IMAGEMAGICK).$(IMAGEMAGICK_SUFFIX)
> >>> +IMAGEMAGICK_DIR                := $(BUILDDIR)/$(IMAGEMAGICK)
> >>> +IMAGEMAGICK_LICENSE    := Apache-2.0
> >>> +
> >>> +# 
> >>> ----------------------------------------------------------------------------
> >>> +# Prepare
> >>> +# 
> >>> ----------------------------------------------------------------------------
> >>> +
> >>> +IMAGEMAGICK_QUANTUM_DEPTH := 8
> >
> > If this is an option which is changed often, make it a text field in the
> > .in file.
> 
> As far as I can tell there is no point in changing this currently.
> Allowed values are 8, 16, and 32. Most image formats don't support
> more than 8 bits per pixel quantum. Since I only enabled jpeg and png
> for now, changing the quantum depth would have no actual benefit, and
> would certainly have an impact in performance and RAM consumption.
> According to the ImageMagick site: "For example, using sixteen-bit
> pixel quantums can cause ImageMagick to run 15% to 50% slower (and
> take twice as much memory) than when it is built to support eight-bit
> pixel quantums".
> 
> That's why I currently hardcode this to 8, instead of making it
> configurable. If/when more image formats are enabled, it may make
> sense to have this as an option in the .in file, but right now there
> is no point in that.

Add a short comment about this above the definition. No kconfig option for
now is ok.

> >
> >>> +
> >>> +IMAGEMAGICK_PATH       := PATH=$(CROSS_PATH)
> >>> +IMAGEMAGICK_ENV        := $(CROSS_ENV)
> >>> +
> >>> +#
> >>> +# See: http://www.imagemagick.org/script/advanced-unix-installation.php
> >>> +#
> >>> +# Notes:
> >>> +# - Threading is disabled as it brings in a dependency with libgomp.so
> >>> +#   (OpenMP) which fails at runtime; disabling openmp itself doesn't seem
> >>> +#   to be enough.
> >>> +# - Quantum depth is set to 8. Most display adapters and image formats
> >>> +#   don't support more than 8 bits per pixel quantum (i.e. per each of 
> >>> the
> >>> +#   R, G, B, and alpha components), and higher values have an impact in
> >>> +#   runtime performance.
> >>> +# - HDRI is disabled. It is not supported by most image formats, and has
> >>> +#   a severe impact in runtime performance.
> >>> +# - The configure script will try to detect external "helper" programs
> >>> +#   available in the host and store their paths in delegates.xml. These
> >>> +#   are obviously not applicable on the target. Just ignore the generated
> >>> +#   delegates.xml file.
> >>> +#
> >>> +IMAGEMAGICK_AUTOCONF := \
> >>> +       $(CROSS_AUTOCONF_USR) \
> >>> +       --disable-docs \
> >>> +       --enable-shared \
> >>> +       --disable-static \
> >>> +       --disable-openmp \
> >>> +       --without-threads \
> >>> +       --without-modules \
> >>> +       --with-quantum-depth=$(IMAGEMAGICK_QUANTUM_DEPTH) \
> >>> +       --disable-hdri \
> >>> +       --without-autotrace \
> >>> +       --without-bzlib \
> >>> +       --without-djvu \
> >>> +       --without-dps \
> >>> +       --without-fftw \
> >>> +       --without-flif \
> >>> +       --without-fpx \
> >>> +       --without-fontconfig \
> >>> +       --without-freetype \
> >>> +       --without-gslib \
> >>> +       --without-gvc \
> >>> +       --without-jbig \
> >>> +       --without-lcms \
> >>> +       --without-lqr \
> >>> +       --without-lzma \
> >>> +       --without-magick-plus-plus \
> >>> +       --without-openexr \
> >>> +       --without-openjp2 \
> >>> +       --without-pango \
> >>> +       --without-perl \
> >>> +       --without-raqm \
> >>> +       --without-rsvg \
> >>> +       --without-tiff \
> >>> +       --without-webp \
> >>> +       --without-wmf \
> >>> +       --without-x \
> >>> +       --without-xml \
> >
> > These options mostly look like they should be additional flags in the
> > .in file, and pull in the respective packages.
> 
> Sure. But I didn't want to just enable a heap of options without
> proper testing. I only enabled the options that I use myself and thus
> I can guarantee that they work. I though it would be better to submit
> this patch as a starting point, then other users can submit patches as
> they need other features, after they have tested them.

I agree. But make sure that all options are sorted the same ways as in the
output of './configure --help'.

> >
> >>> +       --$(call ptx/wwo, PTXCONF_IMAGEMAGICK_USE_ZLIB)-zlib \
> >>> +       --$(call ptx/wwo, PTXCONF_IMAGEMAGICK_USE_LIBPNG)-png \
> >>> +       --$(call ptx/wwo, PTXCONF_IMAGEMAGICK_USE_LIBJPEG)-jpeg
> >
> > ... like here :)
> >
> >>> +
> >>> +# 
> >>> ----------------------------------------------------------------------------
> >>> +# Target-Install
> >>> +# 
> >>> ----------------------------------------------------------------------------
> >>> +
> >>> +$(STATEDIR)/imagemagick.targetinstall:
> >>> +       @$(call targetinfo)
> >>> +
> >>> +       @$(call install_init, imagemagick)
> >>> +       @$(call install_fixup, imagemagick, PRIORITY, optional)
> >>> +       @$(call install_fixup, imagemagick, SECTION, base)
> >>> +       @$(call install_fixup, imagemagick, AUTHOR, "Guillermo Rodriguez 
> >>> <guille.rodrig...@gmail.com>")
> >>> +       @$(call install_fixup, imagemagick, DESCRIPTION, missing)
> >>> +
> >>> +       @$(call install_copy, imagemagick, 0, 0, 0755, -, /usr/bin/magick)
> >>> +       @$(call install_lib, imagemagick, 0, 0, 0644, 
> >>> libMagickCore-7.Q$(IMAGEMAGICK_QUANTUM_DEPTH))
> >>> +       @$(call install_lib, imagemagick, 0, 0, 0644, 
> >>> libMagickWand-7.Q$(IMAGEMAGICK_QUANTUM_DEPTH))
> >>> +
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/compare)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/composite)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/conjure)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/convert)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/identify)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/magick-script)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/mogrify)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/montage)
> >>> +       @$(call install_link, imagemagick, magick, /usr/bin/stream)
> >>> +# These require X11 support:
> >>> +#      @$(call install_link, imagemagick, magick, /usr/bin/animate)
> >>> +#      @$(call install_link, imagemagick, magick, /usr/bin/display)
> >>> +#      @$(call install_link, imagemagick, magick, /usr/bin/import)
> >
> > ... then you can also install those when the user selects --with-x
> 
> Yes, same as above :)

Just leave the stuff out. We can add it again when needed.

> >
> >>> +
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/coder.xml)
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/colors.xml)
> >>> +# This file contains paths to external programs detected in the host:
> >>> +#      @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/delegates.xml)

How are these detected? AC_PATH_PROG or something like that? In that case,
add correct environment variables to the _CONF_ENV should fix that.

Michael

> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/log.xml)
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/magic.xml)
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/mime.xml)
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/policy.xml)
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/quantization-table.xml)
> >>> +       @$(call install_alternative, imagemagick, 0, 0, 0644, 
> >>> /etc/ImageMagick-7/thresholds.xml)
> >>> +
> >>> +       @$(call install_finish, imagemagick)
> >>> +
> >>> +       @$(call touch)
> >>> +
> >>> +# vim: syntax=make

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de

Reply via email to