add read/write support for FLIF #174

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@jonsneyers
Contributor

This should add support for FLIF (Free Lossless Image Format) to ImageMagick.

Build instructions for FLIF:

git clone https://github.com/FLIF-hub/FLIF.git
cd FLIF
make
sudo make install
sudo make install-dev

To compile ImageMagick with FLIF support, it's probably necessary to first run autoconf and automake before doing the usual ./configure; make.

@glennrp
Contributor
glennrp commented Apr 21, 2016

Should there be an entry in ImageMagick's NOTICE file regarding the copyright and licensing of the FLIF delegate?

@jonsneyers
Contributor

The code in coders/flif.c I just contribute to ImageMagick, with no special licensing or copyright clauses needed. It is mostly based on code from other coders anyway.
The code of libflif is another matter, but I don't think that is in the scope of ImageMagick's NOTICE file.

@glennrp
Contributor
glennrp commented Apr 21, 2016

The libflif license requires this, among other things:

a) Give prominent notice with each copy of the Combined Work that
the Library is used in it and that the Library and its use are
covered by this License.

In ImageMagick, the NOTICES file would be an appropriate place to satisfy
that requirement.

Also, The FLIF license appears to require mentioning the libflif (LGPLv3) license
and copyright in ImageMagick's "-version" output and perhaps other places.

@jonsneyers
Contributor

Oh, OK then, I suppose something can be added to NOTICES.

I don't think there has to be libflif license/copyright info in the ImageMagick -version output. Just having it in the NOTICES is more than good enough as far as I'm concerned (being the main author of FLIF/libflif), and I assume also as far as the LGPLv3 is concerned.

@jonsneyers
Contributor

Actually, I don't think even a mention in NOTICES is needed. After all, ImageMagick is not distributing FLIF (not in source form and not in binary form). So I think the situation is exactly like that of WebP or most other delegates, and they also don't have a mention in NOTICES or in the -version output.

The only issue is if you want to distribute actual FLIF code, in source or binary form, e.g. a statically linked version of ImageMagick that contains the FLIF code. In that case, the copyright/license information for FLIF would have to be added (just like it is done in the static binary releases distributed on http://www.imagemagick.org/script/binary-releases.php: they have a much larger NOTICES file than the one over here).

So I don't think it has to be added to this pull request, unless I'm missing something.

@dlemstra
Member
dlemstra commented Apr 22, 2016 edited

I almost got your patches working under the Windows build of ImageMagick ๐Ÿ‘ Thanks a lot for your contribution. I still have a couple of questions:

Can you clarify why we can always call flif_image_read_row_RGBA16? Shouldn't we call the RGBA8 version when the depth is 8?

In IM7 we don't need the whole RGBA image when flif_image_get_nb_channels only returns 3 channels so we could optimize that. Can we make the following assumptions? 1 channel = R, 2 = RA, 3 = RGB, 4 = RGBA?

Is the call to flif_abort_decoder correct in ReadFLIFImage or should we call flif_destroy_decoder instead because there was a call to flif_decoder_decode_memory?

We would prefer to make changes to the flif coder ourselves because we already modified some of your code.

@jonsneyers
Contributor

You could call the RGBA8 version to optimize the case when IM quantum depth
is 8. It shouldn't make a difference.

flif_image_get_nb_channels returns 1 for grayscale, 3 for RGB, 4 for RGBA.
Grayscale + alpha is encoded as RGBA, it never returns 2 (at the moment).
On Apr 23, 2016 01:57, "Dirk Lemstra" notifications@github.com wrote:

I almost got your patches working under the Windows build of ImageMagick [image:
๐Ÿ‘] Thanks a lot for your contribution. I still have a couple of
questions:

Can you clarify why we can always call flif_image_read_row_RGBA16?
Shouldn't we call the RGBA8 version when the depth is 8?

In IM7 we don't need the whole RGBA image when flif_image_get_nb_channels
only returns 3 channels so we could optimize that. Can we make the
following assumptions? 1 channel = R, 2 = RA, 3 = RGB, 4 = RGBA?

We would prefer to make changes to the flif coder ourselves because we
already modified some of your code.

โ€”
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#174 (comment)

@dlemstra
Member

Thanks for your response. I do think we need to call flif_destroy_decoder instead of flif_abort_decoder because it is possible that the error occurs for the second image.

@jonsneyers
Contributor

Oh yes, it should be flif_destroy_decoder to clean up after a decode, my mistake, sorry about that. The function flif_abort_decoder is not implemented very well currently, and it is meant to handle a different thing: to abort a progressive decode that is happening in some other thread (it is used in viewflif, for example). It should not be needed in IM.

@dlemstra
Member

Do we need to free the buffer returned from flif_encoder_encode_memory or is that done by flif_destroy_encoder? The type is a void* and not a const void* so that suggest that we should free it?

@jonsneyers
Contributor

Good question, I hadn't even considered that. Yes, you should free that buffer. Another thing I forgot to do. ImageMagick is basically the first real application to use libflif, so there might be some more rough edges like that... sorry about that.

@dlemstra
Member
dlemstra commented Apr 25, 2016 edited

No need to apologize, we are really happy that you send us this pull request. I hope I found most of the rough edges. I will try to commit my changes very soon.

I have another question. I see the writer has a check to make sure the frames all have the same dimensions. I just added a check for this in the reader also. Is that a correct change?

@jonsneyers
Contributor

The writer checks this because that's a limitation/assumption of the FLIF format/encoder.

It's not needed to check this in the reader, because it will always be the case that all frames have the same dimensions -- the format only allows to specify one width and height, which are the dimensions for all frames.

Some more things I forgot to check (in the writer) :

  • The maximum number of frames the format can currently represent is 0xFFFE. Though maybe this doesn't need to be checked, if someone finds a use for more frames (knowing that they all have to fit in memory at the same time!), it's straightforward to modify the library to allow that.
  • The delay between frames is stored in milliseconds, with a maximum of 60000 (one minute). Undefined behavior will be caused by using higher frame delays.
  • For animations, to the outside, the FLIF encoder has no notion of frame disposal: it simply assumes that each frame completely replaces the previous frame. The FLIF encoder always takes a "film strip" series of images as input and the decoder produces such a series as output. So probably I'm not doing the right thing when converting GIF files: effectively what the encoder assumes is that -coalesce is already done.
@dlemstra
Member

We did a 'manual merge' of your commit. The next version of ImageMagick will include the flif coder. Thanks for your contribution ๐Ÿ‘

@dlemstra dlemstra closed this Apr 26, 2016
@jonsneyers
Contributor

Great! Thanks!

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