diff mbox series

[FFmpeg-devel,v3] flac: add GIF image support

Message ID 20210129161226.50087-1-leo@60228.dev
State New
Headers show
Series [FFmpeg-devel,v3] flac: add GIF image support | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

leo60228 Jan. 29, 2021, 4:12 p.m. UTC 
The FLAC specification requires GIF images to contain their number of
colors. While I can't find a specific reference to that effect, I'm
assuming that's why GIF images were previously unsupported. This was
implemented by just writing AVPALETTE_COUNT for paletted images.
---
This version no longer includes a changelog entry or accidentally adds a
newline to existing code in flac_write_picture. This is identical to v2
except for fixing a mistake while submitting, sorry about that.

 libavformat/flacenc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Derek Buitenhuis Jan. 31, 2021, 8:06 p.m. UTC | #1 
On 29/01/2021 16:12, leo60228 wrote:
> +    if (st->codecpar->format == AV_PIX_FMT_PAL8)
> +        avio_wb32(pb, AVPALETTE_COUNT);
> +    else
> +        avio_wb32(pb, 0);

Is this correct, though?

GIFs encoded by things that are not libavcodec may have less than AVPALETTE_COUNT entries/colors.


- Derek
leo60228 Jan. 31, 2021, 8:41 p.m. UTC | #2 
That's true, but as far as I can tell FFmpeg assumes that all paletted
images have 256 colors, and thus there isn't a way to get the exact number
of colors in a GIF.
I also doubt that this will cause problems in practice, it's somewhat
strange that this field is even there.
If I'm wrong on any of this, please let me know.

On Sun, Jan 31, 2021 at 3:07 PM Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> On 29/01/2021 16:12, leo60228 wrote:
> > +    if (st->codecpar->format == AV_PIX_FMT_PAL8)
> > +        avio_wb32(pb, AVPALETTE_COUNT);
> > +    else
> > +        avio_wb32(pb, 0);
>
> Is this correct, though?
>
> GIFs encoded by things that are not libavcodec may have less than
> AVPALETTE_COUNT entries/colors.
>
>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
leo60228 Feb. 1, 2021, 12:57 a.m. UTC | #3 
After some further thought and discussion on the FFmpeg and Xiph.org IRC
channels, the current behavior seems reasonable. To my knowledge, PAL8 is
guaranteed to have exactly 256 colors. This is why, somewhat surprisingly,
GIFs aren't decoded as PAL8, they're decoded as BGRA. This means that they
get a 0 written. In addition, it was suggested that 0 could mean that the
number of colors is unknown, which would be exactly the case in this
scenario.
These fields don't seem to be considered especially important, either. For
example, the reference FLAC encoder always writes 24-bit color for GIF,
despite having a comment acknowledging that this may be incorrect.

On Sun, Jan 31, 2021 at 3:41 PM leo60228 <leo@60228.dev> wrote:

> That's true, but as far as I can tell FFmpeg assumes that all paletted
> images have 256 colors, and thus there isn't a way to get the exact number
> of colors in a GIF.
> I also doubt that this will cause problems in practice, it's somewhat
> strange that this field is even there.
> If I'm wrong on any of this, please let me know.
>
> On Sun, Jan 31, 2021 at 3:07 PM Derek Buitenhuis <
> derek.buitenhuis@gmail.com> wrote:
>
>> On 29/01/2021 16:12, leo60228 wrote:
>> > +    if (st->codecpar->format == AV_PIX_FMT_PAL8)
>> > +        avio_wb32(pb, AVPALETTE_COUNT);
>> > +    else
>> > +        avio_wb32(pb, 0);
>>
>> Is this correct, though?
>>
>> GIFs encoded by things that are not libavcodec may have less than
>> AVPALETTE_COUNT entries/colors.
>>
>>
>> - Derek
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
diff mbox series

Patch

diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index 1c983486aa..c9834b7d93 100644
--- a/libavformat/flacenc.c
+++ b/libavformat/flacenc.c
@@ -155,7 +155,10 @@  static int flac_write_picture(struct AVFormatContext *s, AVPacket *pkt)
         avio_wb32(pb, av_get_bits_per_pixel(pixdesc));
     else
         avio_wb32(pb, 0);
-    avio_wb32(pb, 0);
+    if (st->codecpar->format == AV_PIX_FMT_PAL8)
+        avio_wb32(pb, AVPALETTE_COUNT);
+    else
+        avio_wb32(pb, 0);
 
     avio_wb32(pb, pkt->size);
     avio_write(pb, pkt->data, pkt->size);
@@ -218,9 +221,6 @@  static int flac_init(struct AVFormatContext *s)
             if (!(st->disposition & AV_DISPOSITION_ATTACHED_PIC)) {
                 av_log(s, AV_LOG_WARNING, "Video stream #%d is not an attached picture. Ignoring\n", i);
                 continue;
-            } else if (st->codecpar->codec_id == AV_CODEC_ID_GIF) {
-                av_log(s, AV_LOG_ERROR, "GIF image support is not implemented.\n");
-                return AVERROR_PATCHWELCOME;
             } else if (!c->write_header) {
                 av_log(s, AV_LOG_ERROR, "Can't write attached pictures without a header.\n");
                 return AVERROR(EINVAL);