Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added HEIF writer and replaced HEIF reader with a more complete HEIF implementation #1099

Merged
merged 12 commits into from
Apr 27, 2018

Conversation

farindk
Copy link
Contributor

@farindk farindk commented Apr 17, 2018

Added code for writing HEIF files based on libheif.
https://github.com/strukturag/libheif
There is a Debian package for libheif, but you will need the latest git version for the encoder (will be released soon).

I also replaced the HEIF reader with the implementation from libheif as the old implementation was limited to only reading HEIF files generated by iPhones. The new implementation is a full implementation of the HEIF standard (still image part) including all image compositing operations.

Alpha channel support is in libheif, but not yet implemented here, since I could not figure out yet how to implement this in ImageMagick.

@farindk farindk mentioned this pull request Apr 17, 2018
@dlemstra
Copy link
Member

Thanks for the PR 👍 Happy to see we get write support. I was wonder if it would be possible to do also add something for when we only want to ping (get basic image information) the image. Now the whole file is loaded into memory and I was wondering if we could load only part of the image when we want to ping it.

@farindk
Copy link
Contributor Author

farindk commented Apr 17, 2018 via email

@farindk
Copy link
Contributor Author

farindk commented Apr 17, 2018

Here is support for the 'ping' flag. Hope I did it right.

@dbtsai
Copy link

dbtsai commented Apr 17, 2018

Thanks for this PR. This is a great feature supporting HEIF write.

@dlemstra
Copy link
Member

When will the encoder be released? I think it will be wise to wait till that is available before we merge this. I would prefer it if our users would not need to compile the latest git checkout of libheif to get heif support.

  • whether I am doing things right for "lists of images"

LGTM

  • how to handle alpha channels

You can use SetPixelAlpha to set the alpha channel.

  • what I should do when the HEIF contains several images (libheif supports that)

You can take a look at an other decoder that also reads multiple frames and look how AcquireNextImage is used.

  • the iPhone also saves "depth images" into the HEIF as an auxiliary image. How could I pass them to ImageMagick?

What are depth images and how could we use them?

  • could I pass more options to the HEIF encoder, like whether it should generate thumbnails, or whether to switch to lossless mode?

You will need to create options that start with heic: for example heic:lossless and check them in the encoder or decoder where you need those options. You can take a look at the other coders to see how we do that.

Prob best to put these extra changes in a separate PR after we merge this one.

coders/heic.c Outdated
goto error_cleanup;
}


if (image->quality != UndefinedCompressionQuality) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use image_info->quality instead.

ilovezfs pushed a commit to Homebrew/homebrew-core that referenced this pull request Apr 23, 2018
Provides optional HEIF support.

Note the upstream PR here, which may change things in the future in
terms of how this option needs handling:
ImageMagick/ImageMagick#1099

Fixes #26444.

Closes #26959.

Signed-off-by: ilovezfs <ilovezfs@icloud.com>
@farindk
Copy link
Contributor Author

farindk commented Apr 25, 2018

The libheif package with encoder (v1.1.0) is now in debian testing: https://tracker.debian.org/pkg/libheif
It is also in Ubuntu 18.04.

@dlemstra
Copy link
Member

@farindk Thats great news!!! 🙌 👍 Could you resolve the conflicts in this branch so I can merge this Friday? We will run autoconf ourselves so maybe you could exclude some of your changes.

@dlemstra
Copy link
Member

dlemstra commented Apr 26, 2018

@farindk It looks like we need to use x265 to get encoding support? That won't be an option for us since that is licensed under the GPL v2 license. We could still use the libheif library if that means we would get better support for reading files. Can you remove the writer for now?

EDIT: I can also merge as is and remove the writer myself. Whats your preference?

@farindk
Copy link
Contributor Author

farindk commented Apr 26, 2018

I was not aware that the ImageMagick license does not allow any GPL v2 libraries, as the ImageMagick license page says that it is compatible to GPL v3. It is because of v2/v3 or because of linking against GPL in general?

How about this solution: I could change the encoding code in libheif in such a way that it does not link to libx265 at all, but uses the separate "x265" program for encoding, passing the data through a pipe. That way, libheif would be LGPL, if I am correct.
In the long term, my plan is to use libde265 itself for encoding instead of x265, but the encoding API is not ready yet.

EDIT: isn't libfftw also GPL?

@dlemstra
Copy link
Member

dlemstra commented Apr 26, 2018

We try to avoid linking against GPL libraries. If we also want to get this building under Windows that would mean we would also need to build the x265 library. But I don't want to invest any time in getting that building on Windows due to the license that is being used. The MAGICKCORE_HEIC_DELEGATE define is for both the reader and the writer.

While writing the message I just got another idea. I could disable writing with the MAGICKCORE_WINDOWS_SUPPORT define under Windows and still enable support for writing under other platforms. I am still that your long term plan is getting rid of the x265 dependency. But aren't you forced to use GPL instead of LGPL for the encoder because you are using x265?

@farindk
Copy link
Contributor Author

farindk commented Apr 26, 2018

Disabling it only for Windows sounds like a good solution to me. Then we can easily enable it later when the dependency is removed.

libheif does not require linking to x265. One can register any number of encoding plugins even during runtime. x265 is only one option that is provided by default. But one can also add encoders for AV1, h264, or JPEG. If libx265 is not detected during configuration, that plugin is switched off. In that sense libheif itself is LGPL, but when used together with x265, the combination is GPL.

@dlemstra dlemstra merged commit 4179457 into ImageMagick:master Apr 27, 2018
@dbtsai
Copy link

dbtsai commented Aug 16, 2018

@farindk

I'm using

# convert --version
Version: ImageMagick 7.0.8-10 Q16 x86_64 2018-08-15 https://www.imagemagick.org
Copyright: © 1999-2018 ImageMagick Studio LLC
License: https://www.imagemagick.org/script/license.php
Features: Cipher DPC HDRI Modules 
Delegates (built-in): bzlib freetype heic jng jpeg ltdl lzma png tiff xml zlib

with heic support.

When I converted TIFF files with AdobeRGB profile to HEIC files, the color will be changed. convert DSC01108.tif DSC01108.heic

Seems the color profile is not properly copied to the new HEIC files. Also, the input file has Colorspace RGB while the output HEIC one is YCbCr. Is it possible to keep as it? How do I specify the compression rate and using 12bit color depth?

Thanks.

@farindk
Copy link
Contributor Author

farindk commented Aug 17, 2018

Support for color profiles was just added to libheif (strukturag/libheif#58), but it still needs to be integrated to ImageMagick.

RGB colorspace and HDR is on my to-do list. See also strukturag/libheif#62.

@dbtsai
Copy link

dbtsai commented Aug 17, 2018

Thanks, @farindk ! I also ran into lossless mode issue.

FYI, I found compare will give huge error due to comparing image in YCbCr and RGB directly without converting YCbCr to RGB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants