Message ID | 20171024164022.64121-1-bjorn@giphy.com |
---|---|
State | New |
Headers | show |
2017-10-24 18:40 GMT+02:00 Bjorn Roche <bjorn@giphy.com>: > This patch assumes paletteuse has already been patched to support > transparency. (e.g. lavfi/paletteuse: fix to support transparency) Which should - imo - be unrelated to this patch and indeed, transcoding a pal8 apng (produced with your paletteuse patch) produces a good gif with this patch. I don't know if this is the best approach though. Thank you, Carl Eugen
2017-10-24 18:40 GMT+02:00 Bjorn Roche <bjorn@giphy.com>: [...] The patch adds two warnings here on compilation of gif.c, they should be fixed at some point: libavcodec/gif.c:164:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] uint8_t *frame_disposal = av_packet_new_side_data(pkt, AV_PKT_DATA_GIF_FRAME_DISPOSAL, 1); ^~~~~~~ libavcodec/gif.c: In function ‘gif_image_write_translucent’: libavcodec/gif.c:333:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] uint8_t *frame_disposal = av_packet_new_side_data(pkt, AV_PKT_DATA_GIF_FRAME_DISPOSAL, 1); ^~~~~~~ Carl Eugen
On Tue, Oct 24, 2017 at 4:24 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-10-24 18:40 GMT+02:00 Bjorn Roche <bjorn@giphy.com>: > > > This patch assumes paletteuse has already been patched to support > > transparency. (e.g. lavfi/paletteuse: fix to support transparency) > > Which should - imo - be unrelated to this patch and indeed, transcoding > a pal8 apng (produced with your paletteuse patch) produces a good gif > with this patch. > I don't know if this is the best approach though. > Ah, okay, then my comment is wrong -- I just didn't see a way to test it without paletteuse being fixed, but a pal8 apng makes sense. bjorn
On Tue, Oct 24, 2017 at 12:40:22PM -0400, Bjorn Roche wrote: > Support for transparencies in animated gifs requires modifying both > libavcodec/gif.c and libavformat/gif.c because both the graphics > control extension (handled by libavformat/gif.c) and the raw frame data > (handled by libavcodec/gif.c) must be changed. This is because > transparencies in GIF can be used both to create a transparent image, > and to provide optimization. > > How transparencies are interpreted in a given frame is controlled by > the “disposal method”, which must be set appropriately in the graphics > control extension. > > The “in place” disposal method is used when transparency indicates > optimization, and the “background” disposal method is used when > transparency is intended to be preserved. In order to support both > disposal methods, libavcodec/gif.c must signal to libavformat/gif.c > which disposal method is required for every frame. This is done with a > new side data type: AV_PKT_DATA_GIF_FRAME_DISPOSAL. This requires a > change to avcodec.h > > Unfortunately, the addition of a new side data type causes some of the > FATE tests to fail. This is not addressed here. > > This patch assumes paletteuse has already been patched to support > transparency. (e.g. lavfi/paletteuse: fix to support transparency) > > Feedback I definitely need: > - I’ve done a fair bit of testing, but I’m sure I missed some important > cases. > - I don’t know if/how to update the FATE tests. > --- > libavcodec/avcodec.h | 6 ++ > libavcodec/gif.c | 196 +++++++++++++++++++++++++++++++++++++++++++++++++-- > libavformat/gif.c | 16 ++++- > 3 files changed, 212 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 52cc5b0ca0..82a5328ce1 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -1599,6 +1599,12 @@ enum AVPacketSideDataType { > */ > AV_PKT_DATA_CONTENT_LIGHT_LEVEL, > > + /** > + * The disposal method that should be used with the frame. If missing, > + * the frame will not be disposed. This contains exactly one byte. > + */ > + AV_PKT_DATA_GIF_FRAME_DISPOSAL, > + > /** > * ATSC A53 Part 4 Closed Captions. This metadata should be associated with > * a video stream. A53 CC bitstream is stored as uint8_t in AVPacketSideData.data. you cannot add values in the middle of a public enum, that would change the ABI [...]
On Tue, Oct 24, 2017 at 8:56 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Tue, Oct 24, 2017 at 12:40:22PM -0400, Bjorn Roche wrote: > > Support for transparencies in animated gifs requires modifying both > > libavcodec/gif.c and libavformat/gif.c because both the graphics > > control extension (handled by libavformat/gif.c) and the raw frame data > > (handled by libavcodec/gif.c) must be changed. This is because > > transparencies in GIF can be used both to create a transparent image, > > and to provide optimization. > > > > How transparencies are interpreted in a given frame is controlled by > > the “disposal method”, which must be set appropriately in the graphics > > control extension. > > > > The “in place” disposal method is used when transparency indicates > > optimization, and the “background” disposal method is used when > > transparency is intended to be preserved. In order to support both > > disposal methods, libavcodec/gif.c must signal to libavformat/gif.c > > which disposal method is required for every frame. This is done with a > > new side data type: AV_PKT_DATA_GIF_FRAME_DISPOSAL. This requires a > > change to avcodec.h > > > > Unfortunately, the addition of a new side data type causes some of the > > FATE tests to fail. This is not addressed here. > > > > This patch assumes paletteuse has already been patched to support > > transparency. (e.g. lavfi/paletteuse: fix to support transparency) > > > > Feedback I definitely need: > > - I’ve done a fair bit of testing, but I’m sure I missed some important > > cases. > > - I don’t know if/how to update the FATE tests. > > --- > > libavcodec/avcodec.h | 6 ++ > > libavcodec/gif.c | 196 ++++++++++++++++++++++++++++++ > +++++++++++++++++++-- > > libavformat/gif.c | 16 ++++- > > 3 files changed, 212 insertions(+), 6 deletions(-) > > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > > index 52cc5b0ca0..82a5328ce1 100644 > > --- a/libavcodec/avcodec.h > > +++ b/libavcodec/avcodec.h > > @@ -1599,6 +1599,12 @@ enum AVPacketSideDataType { > > */ > > AV_PKT_DATA_CONTENT_LIGHT_LEVEL, > > > > + /** > > + * The disposal method that should be used with the frame. If > missing, > > + * the frame will not be disposed. This contains exactly one byte. > > + */ > > + AV_PKT_DATA_GIF_FRAME_DISPOSAL, > > + > > /** > > * ATSC A53 Part 4 Closed Captions. This metadata should be > associated with > > * a video stream. A53 CC bitstream is stored as uint8_t in > AVPacketSideData.data. > > you cannot add values in the middle of a public enum, that would > change the ABI > Ah, thanks -- I thought that was internal only. Is it safe to add to the end? bjorn
On Wed, Oct 25, 2017 at 09:51:39AM -0400, Bjorn Roche wrote: > On Tue, Oct 24, 2017 at 8:56 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > On Tue, Oct 24, 2017 at 12:40:22PM -0400, Bjorn Roche wrote: > > > Support for transparencies in animated gifs requires modifying both > > > libavcodec/gif.c and libavformat/gif.c because both the graphics > > > control extension (handled by libavformat/gif.c) and the raw frame data > > > (handled by libavcodec/gif.c) must be changed. This is because > > > transparencies in GIF can be used both to create a transparent image, > > > and to provide optimization. > > > > > > How transparencies are interpreted in a given frame is controlled by > > > the “disposal method”, which must be set appropriately in the graphics > > > control extension. > > > > > > The “in place” disposal method is used when transparency indicates > > > optimization, and the “background” disposal method is used when > > > transparency is intended to be preserved. In order to support both > > > disposal methods, libavcodec/gif.c must signal to libavformat/gif.c > > > which disposal method is required for every frame. This is done with a > > > new side data type: AV_PKT_DATA_GIF_FRAME_DISPOSAL. This requires a > > > change to avcodec.h > > > > > > Unfortunately, the addition of a new side data type causes some of the > > > FATE tests to fail. This is not addressed here. > > > > > > This patch assumes paletteuse has already been patched to support > > > transparency. (e.g. lavfi/paletteuse: fix to support transparency) > > > > > > Feedback I definitely need: > > > - I’ve done a fair bit of testing, but I’m sure I missed some important > > > cases. > > > - I don’t know if/how to update the FATE tests. > > > --- > > > libavcodec/avcodec.h | 6 ++ > > > libavcodec/gif.c | 196 ++++++++++++++++++++++++++++++ > > +++++++++++++++++++-- > > > libavformat/gif.c | 16 ++++- > > > 3 files changed, 212 insertions(+), 6 deletions(-) > > > > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > > > index 52cc5b0ca0..82a5328ce1 100644 > > > --- a/libavcodec/avcodec.h > > > +++ b/libavcodec/avcodec.h > > > @@ -1599,6 +1599,12 @@ enum AVPacketSideDataType { > > > */ > > > AV_PKT_DATA_CONTENT_LIGHT_LEVEL, > > > > > > + /** > > > + * The disposal method that should be used with the frame. If > > missing, > > > + * the frame will not be disposed. This contains exactly one byte. > > > + */ > > > + AV_PKT_DATA_GIF_FRAME_DISPOSAL, > > > + > > > /** > > > * ATSC A53 Part 4 Closed Captions. This metadata should be > > associated with > > > * a video stream. A53 CC bitstream is stored as uint8_t in > > AVPacketSideData.data. > > > > you cannot add values in the middle of a public enum, that would > > change the ABI > > > > Ah, thanks -- I thought that was internal only. Is it safe to add to the > end? additions should be immedeatly before AV_PKT_DATA_NB if this is insufficiently documented (seems so) then a patch improving the documentation is welcome too thx [...]
> - I don’t know if/how to update the FATE tests.
Can anyone comment on this? Do I update the tests in the same patch or
separate that?
On Thu, Nov 02, 2017 at 11:45:20 -0400, Bjorn Roche wrote: > > - I don’t know if/how to update the FATE tests. > > Can anyone comment on this? Do I update the tests in the same patch or > separate that? Your patch is not allowed to break fate. If the fate results are correct references, then fate is catching an error in your patch which you need to correct. If the change in fate is expected (implied by the patch), then you need to change the fate results or adapt the fate test within the same patch. New additional fate tests are welcome (or preferred? not sure) as separate patches. How to update fate: There's a web (or wiki) page about that, and James wrote this excellent stuff recently (perhaps not related to your quest here): http://ffmpeg.org/pipermail/ffmpeg-devel/2017-October/217473.html Moritz
2017-10-24 18:40 GMT+02:00 Bjorn Roche <bjorn@giphy.com>:
> - I don’t know if/how to update the FATE tests.
A quick way (that needs some double-checking) is:
$ make GEN=1 SAMPLES=fate-suite fate
(This overwrites all values with the new output.)
Is the new side-data unavoidable?
(Would the only alternative be to merge the muxer into the
encoder?)
Carl Eugen
Thanks Moritz and Carl for your help with fate. This email was stuck in my drafts :( Comments below: On Fri, Nov 3, 2017 at 8:11 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-10-24 18:40 GMT+02:00 Bjorn Roche <bjorn@giphy.com>: > > > - I don’t know if/how to update the FATE tests. > > A quick way (that needs some double-checking) is: > $ make GEN=1 SAMPLES=fate-suite fate > > (This overwrites all values with the new output.) > Thanks. I ended up doing it manually to be sure, but this is useful! > Is the new side-data unavoidable? > (Would the only alternative be to merge the muxer into the > encoder?) > The only way I can see is to move some functionality between the mixer and the encoder. It's not a clear line between the codec and the container in GIF, so I wouldn't object to an approach different from the one I took. bjorn
2017-11-27 19:30 GMT+01:00 Bjorn Roche <bjorn@giphy.com>: > > On Fri, Nov 3, 2017 at 8:11 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> 2017-10-24 18:40 GMT+02:00 Bjorn Roche <bjorn@giphy.com>: >> >> > - I don’t know if/how to update the FATE tests. >> >> A quick way (that needs some double-checking) is: >> $ make GEN=1 SAMPLES=fate-suite fate >> >> (This overwrites all values with the new output.) > > Thanks. I ended up doing it manually to be sure, but this is useful! > >> Is the new side-data unavoidable? >> (Would the only alternative be to merge the muxer into the >> encoder?) > > The only way I can see is to move some functionality between the mixer and > the encoder. It's not a clear line between the codec and the container in > GIF, so I wouldn't object to an approach different from the one I took. (Just a question!) Does remuxing (animated) gif to gif work now? (I believe it does.) Does it work with your patch? Thank you, Carl Eugen
On Mon, Nov 27, 2017 at 5:35 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-11-27 19:30 GMT+01:00 Bjorn Roche <bjorn@giphy.com>: > > > > > The only way I can see is to move some functionality between the mixer > and > > the encoder. It's not a clear line between the codec and the container in > > GIF, so I wouldn't object to an approach different from the one I took. > > (Just a question!) > > Does remuxing (animated) gif to gif work now? (I believe it does.) > Does it work with your patch? I think this would require changes to the reader. AFAICT, it only outputs BGRA. Something I can look into if that seems right to you.
2017-11-28 1:12 GMT+01:00 Bjorn Roche <bjorn@giphy.com>: > On Mon, Nov 27, 2017 at 5:35 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> 2017-11-27 19:30 GMT+01:00 Bjorn Roche <bjorn@giphy.com>: >> > >> >> > The only way I can see is to move some functionality between the mixer >> and >> > the encoder. It's not a clear line between the codec and the container in >> > GIF, so I wouldn't object to an approach different from the one I took. >> >> (Just a question!) >> >> Does remuxing (animated) gif to gif work now? (I believe it does.) >> Does it work with your patch? > > I think this would require changes to the reader. AFAICT, it only outputs > BGRA. > Something I can look into if that seems right to you. I have no idea. I just wanted to point out some tests that may or may not show if the whole approach makes sense. Carl Eugen
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 52cc5b0ca0..82a5328ce1 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -1599,6 +1599,12 @@ enum AVPacketSideDataType { */ AV_PKT_DATA_CONTENT_LIGHT_LEVEL, + /** + * The disposal method that should be used with the frame. If missing, + * the frame will not be disposed. This contains exactly one byte. + */ + AV_PKT_DATA_GIF_FRAME_DISPOSAL, + /** * ATSC A53 Part 4 Closed Captions. This metadata should be associated with * a video stream. A53 CC bitstream is stored as uint8_t in AVPacketSideData.data. diff --git a/libavcodec/gif.c b/libavcodec/gif.c index d9c99d52cf..6c839d99d2 100644 --- a/libavcodec/gif.c +++ b/libavcodec/gif.c @@ -74,11 +74,35 @@ static int pick_palette_entry(const uint8_t *buf, int linesize, int w, int h) return -1; } -static int gif_image_write_image(AVCodecContext *avctx, - uint8_t **bytestream, uint8_t *end, - const uint32_t *palette, - const uint8_t *buf, const int linesize, - AVPacket *pkt) +// returns true if any of the pixels are transparent +static int is_image_translucent(AVCodecContext *avctx, + const uint32_t *palette, + const uint8_t *buf, const int linesize) +{ + GIFContext *s = avctx->priv_data; + int trans = s->transparent_index; + int p; + const int m = avctx->width * avctx->height ; + + if (trans < 0) { + return 0; + } + + for (p=0; p<m; ++p) { + if (buf[p] == trans) { + return 1; + } + } + return 0; +} + +// writes an opaque image. ie an image with no transparency. +// it also works, and should be used, for a first image. +static int gif_image_write_opaque(AVCodecContext *avctx, + uint8_t **bytestream, uint8_t *end, + const uint32_t *palette, + const uint8_t *buf, const int linesize, + AVPacket *pkt) { GIFContext *s = avctx->priv_data; int len = 0, height = avctx->height, width = avctx->width, x, y; @@ -137,6 +161,11 @@ static int gif_image_write_image(AVCodecContext *avctx, width, height, x_start, y_start, avctx->width, avctx->height); } + uint8_t *frame_disposal = av_packet_new_side_data(pkt, AV_PKT_DATA_GIF_FRAME_DISPOSAL, 1); + if (!frame_disposal) + return AVERROR(ENOMEM); + *frame_disposal = GCE_DISPOSAL_INPLACE; + /* image block */ bytestream_put_byte(bytestream, GIF_IMAGE_SEPARATOR); bytestream_put_le16(bytestream, x_start); @@ -214,6 +243,163 @@ static int gif_image_write_image(AVCodecContext *avctx, return 0; } +// wrtites an image that may contain transparency +// this should work for opaque images as well, but will be less optimized. +static int gif_image_write_translucent(AVCodecContext *avctx, + uint8_t **bytestream, uint8_t *end, + const uint32_t *palette, + const uint8_t *buf, const int linesize, + AVPacket *pkt) +{ + GIFContext *s = avctx->priv_data; + int len = 0, height = avctx->height, width = avctx->width, y; + int x_start = 0, y_start = 0, trans = s->transparent_index; + const uint8_t *ptr; + + /* Crop Image */ + if ((s->flags & GF_OFFSETTING) && trans >=0) { + const int w = avctx->width; + const int h = avctx->height; + int x_end = w - 1, + y_end = h - 1; + + // crop top + while (y_start < y_end) { + int i; + int is_trans = 1; + for (i=0; i<w; ++i) { + if( buf[w*y_start+i] != trans ) { + is_trans = 0; + break; + } + } + if (!is_trans) + break; + y_start++; + } + + // crop bottom + while (y_end < h) { + int i; + int is_trans = 1; + for (i=0; i<w; ++i) { + if (buf[w*y_end+i] != trans) { + is_trans = 0; + break; + } + } + if (!is_trans) + break; + y_end--; + } + + // crop left + while (x_start < x_end) { + int i; + int is_trans = 1; + for (i=y_start; i<y_end; ++i) { + if (buf[w*i+x_start] != trans) { + is_trans = 0; + break; + } + } + if (!is_trans) + break; + x_start++; + } + + // crop right + while (x_end < w) { + int i; + int is_trans = 1; + for (i=y_start; i<y_end; ++i) { + if (buf[w*i+x_end] != trans) { + is_trans = 0; + break; + } + } + if (!is_trans) + break; + x_end--; + } + + height = y_end + 1 - y_start; + width = x_end + 1 - x_start; + av_log(avctx, AV_LOG_DEBUG,"%dx%d image at pos (%d;%d) [area:%dx%d]\n", + width, height, x_start, y_start, avctx->width, avctx->height); + } + + + uint8_t *frame_disposal = av_packet_new_side_data(pkt, AV_PKT_DATA_GIF_FRAME_DISPOSAL, 1); + if (!frame_disposal) + return AVERROR(ENOMEM); + *frame_disposal = GCE_DISPOSAL_BACKGROUND; + + /* image block */ + bytestream_put_byte(bytestream, GIF_IMAGE_SEPARATOR); + bytestream_put_le16(bytestream, x_start); + bytestream_put_le16(bytestream, y_start); + bytestream_put_le16(bytestream, width); + bytestream_put_le16(bytestream, height); + + if (!palette) { + bytestream_put_byte(bytestream, 0x00); /* flags */ + } else { + unsigned i; + bytestream_put_byte(bytestream, 1<<7 | 0x7); /* flags */ + for (i = 0; i < AVPALETTE_COUNT; i++) { + const uint32_t v = palette[i]; + bytestream_put_be24(bytestream, v); + } + } + + bytestream_put_byte(bytestream, 0x08); + + ff_lzw_encode_init(s->lzw, s->buf, s->buf_size, + 12, FF_LZW_GIF, put_bits); + + ptr = buf + y_start*linesize + x_start; + + for (y = 0; y < height; y++) { + len += ff_lzw_encode(s->lzw, ptr, width); + ptr += linesize; + } + + len += ff_lzw_encode_flush(s->lzw, flush_put_bits); + + ptr = s->buf; + while (len > 0) { + int size = FFMIN(255, len); + bytestream_put_byte(bytestream, size); + if (end - *bytestream < size) + return -1; + bytestream_put_buffer(bytestream, ptr, size); + ptr += size; + len -= size; + } + bytestream_put_byte(bytestream, 0x00); /* end of image block */ + + return 0; +} + +static int gif_image_write_image(AVCodecContext *avctx, + uint8_t **bytestream, uint8_t *end, + const uint32_t *palette, + const uint8_t *buf, const int linesize, + AVPacket *pkt) +{ + GIFContext *s = avctx->priv_data; + if (!s->last_frame) { + return gif_image_write_opaque(avctx, bytestream, end, palette, buf, linesize, pkt); + } + + if (is_image_translucent(avctx, palette, buf, linesize)) { + return gif_image_write_translucent(avctx, bytestream, end, palette, buf, linesize, pkt); + } else { + return gif_image_write_opaque(avctx, bytestream, end, palette, buf, linesize, pkt); + } +} + static av_cold int gif_encode_init(AVCodecContext *avctx) { GIFContext *s = avctx->priv_data; diff --git a/libavformat/gif.c b/libavformat/gif.c index 01d98a27b0..b4a8b6aa94 100644 --- a/libavformat/gif.c +++ b/libavformat/gif.c @@ -144,6 +144,8 @@ static int flush_packet(AVFormatContext *s, AVPacket *new) AVIOContext *pb = s->pb; const uint32_t *palette; AVPacket *pkt = gif->prev_pkt; + uint8_t *disposal; + uint8_t packed; if (!pkt) return 0; @@ -157,16 +159,28 @@ static int flush_packet(AVFormatContext *s, AVPacket *new) } bcid = get_palette_transparency_index(palette); + disposal = av_packet_get_side_data(pkt, AV_PKT_DATA_GIF_FRAME_DISPOSAL, &size); + if (disposal && size != 1) { + av_log(s, AV_LOG_ERROR, "Invalid gif frame disposal extradata\n"); + return AVERROR_INVALIDDATA; + } + if (new && new->pts != AV_NOPTS_VALUE) gif->duration = av_clip_uint16(new->pts - gif->prev_pkt->pts); else if (!new && gif->last_delay >= 0) gif->duration = gif->last_delay; /* graphic control extension block */ + if (disposal) { + packed = (0xff & (*disposal)<<2) | (bcid >= 0); + } else { + packed = 1<<2 | (bcid >= 0); + } + avio_w8(pb, 0x21); avio_w8(pb, 0xf9); avio_w8(pb, 0x04); /* block size */ - avio_w8(pb, 1<<2 | (bcid >= 0)); + avio_w8(pb, packed); avio_wl16(pb, gif->duration); avio_w8(pb, bcid < 0 ? DEFAULT_TRANSPARENCY_INDEX : bcid); avio_w8(pb, 0x00);