Hi Michael,

2016-09-15 9:56 GMT+02:00 Michael Olbrich <m.olbr...@pengutronix.de>:
> 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.

In the submitted .make file there is a comment right below explaining
this along with other decisions (see "Notes:" below).

>
>> >
>> >>> +
>> >>> +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'.

Will do.

>
>> >
>> >>> +       --$(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.

Ok.

>
>> >
>> >>> +
>> >>> +       @$(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.

Yes, AC_PATH_PROG is used. Re. environment vars in _CONF_ENV: I don't
have much experience with autoconf, can you point to an example .make
file in ptxdist where this is done ?

Thank you,

Guillermo

>
> 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



-- 
Guillermo Rodriguez Garcia
guille.rodrig...@gmail.com

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de

Reply via email to