Add support for APNG files (fix #101) #102
Conversation
|
Wow, this is pretty cool!
I think this is fine. Discord's project that uses lilliput can allow or forbid certain source transformations. I would imagine others do as well. I will review some of the specific ergonomics around that to be certain though.
Appreciated. Obviously shipping binaries is not ideal but we do it to make lilliput a bit easier to plug and play. We have a trusted build process that generates these. Hopefully building there makes everyone feel a bit more comfortable about the contents of them. About the PR specifically, Discord has made some product decisions about APNGs which, for better or worse, treats them as differently than GIFs. From that perspective my request would probably just be to make the logic selectable, so that we can keep using the old path for now. Unfortunately the freezing of PNGs has become rather load-bearing. |
| @@ -46,6 +46,8 @@ make install | |||
|
|
|||
| mkdir -p $BASEDIR/libpng | |||
| tar -xzf $SRCDIR/libpng-1.6.29.tar.gz -C $BASEDIR/libpng --strip-components 1 | |||
| cd $BASEDIR/libpng | |||
| patch -Np1 -i $SRCDIR/libpng-1.6.29-apng.patch | |||
There was a problem hiding this comment.
Is this already part of the libpng-1.6.29.tar.gz source in https://github.com/discord/lilliput-dep-source ?
There was a problem hiding this comment.
The distribution of libpng-apng is a bit weird - the APNG spec hasn't been officially ratified by whatever committee is in charge of PNG, so support for it can't be included in libpng. Instead, libpng-apng is distributed as a patch applied to the base libpng library. The version of libpng currently being used doesn't have this patch included, so it needs to be applied separately.
There was a problem hiding this comment.
Ah ok, so we would need to package the patch into the lilliput-dep-source repo?
| return apng_decoder_have_next_frame; | ||
| } | ||
|
|
||
| void BlendOver(unsigned char * dst, unsigned int dst_width, unsigned char ** rows_src, unsigned int x, unsigned int y, unsigned int w, unsigned int h) |
There was a problem hiding this comment.
Out of curiosity, I wonder if opencv has a vectorized function for this operation?
There was a problem hiding this comment.
I'm not too familiar with OpenCV, but when I checked the documentation I didn't see anything that does this. The closest thing I could find was addWeighted, but that uses a single alpha value for the entire image, rather than using the alpha value for each pixel. It's entirely possible that such a function exists and I didn't notice it, though.
|
APNG support would be a great addition. Anything I can do to help get this merged? |
| width = png_get_image_width(d->png_ptr, d->info_ptr); | ||
| height = png_get_image_height(d->png_ptr, d->info_ptr); | ||
|
|
||
| d->prev_frame = (uint8_t *) calloc(4, width * height); |
There was a problem hiding this comment.
One thing I'm wondering on this - in ops.go we already toggle between two pre-allocated frame buffers. What would you think about just taking a previous frame argument for the decoding below? Then the caller would be responsible for passing previous frame, and we could avoid the allocations?
| png_write_image(e->png_ptr, row_pointers); | ||
| png_write_frame_tail(e->png_ptr, e->info_ptr); | ||
|
|
||
| mat->copyTo(*e->prev_frame); |
There was a problem hiding this comment.
Is there a reason we couldn't use mat's buffer instead of creating buf above?
|
We need this more than ever now that the most recent patch to PNG (addressing the aCropalypse issue) has broken APNGs using the *.png and *.apng extensions all together. |
|
Any word on when this will get merged? |
Currently, lilliput removes all of the APNG-specific data when resizing an animated PNG file, causing it to only output the first frame of the animation. This pull request adds a new encoder and decoder type for animated PNG files that properly handles subsequent frames.
GIF-to-APNG conversion currently does not work, since the number of frames in a GIF is not known until all frames have been read, while the total number of frames must be known before encoding the first frame of an animated PNG.
This change also requires an update to the dependencies to include the patch adding APNG support to libpng. Since I'm not sure what the process for updating them is, I haven't touched the dependencies beyond updating build-deps-linux.sh.