PR #25822 opened by romainhedouin URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25822 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25822.patch
shrink_palette() removes repeated colors from a frame's palette. It only saved the new index for the first copy of each color. A pixel on a later copy got the color of the first palette entry, so the picture had wrong colors. This only happens with a pal8 frame that has the same color twice and is written with its own palette (for example with `-global_palette 0`). It has been wrong since 5a343853c0. The second commit adds an API test that fails without the fix (225 of 256 pixels wrong) and passes with it. Here is an example: original / master / this PR  Noticed during a review sweep of ffmpeg using an LLM. <!-- If this PR requires new FATE test samples, attach them to the PR and list their target paths below (relative to the fate-suite root). Attached filenames must match the sample's filename: ```fate-samples # e.g. vorbis/new-sample.ogg ``` --> >From 2561f48cc01443a11c62d23b1aa94fc46260242d Mon Sep 17 00:00:00 2001 From: romainhedouin <[email protected]> Date: Fri, 6 Nov 2026 19:21:52 +0100 Subject: [PATCH 1/2] avcodec/gif: Map duplicate palette entries to their first occurrence shrink_palette() removes duplicate colors from a frame palette. It only stored the new index for the first copy of each color, so the index of a later duplicate stayed 0 and its pixels were written with the color of the first palette entry. Map each duplicate to the entry that was kept. This has been wrong since 5a343853c0. Noticed during a review sweep of ffmpeg using an LLM. Signed-off-by: romainhedouin <[email protected]> --- libavcodec/gif.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libavcodec/gif.c b/libavcodec/gif.c index e5a94426ab1..f6019269418 100644 --- a/libavcodec/gif.c +++ b/libavcodec/gif.c @@ -73,6 +73,7 @@ static void shrink_palette(const uint32_t *src, uint8_t *map, int seen = 0; for (size_t c = 0; c < colors_seen; c++) { if (src[i] == dst[c]) { + map[i] = c; seen = 1; break; } -- 2.54.0 >From adc385882b9c9cf5c63bb759196d4b27eb13e85d Mon Sep 17 00:00:00 2001 From: romainhedouin <[email protected]> Date: Fri, 6 Nov 2026 19:43:26 +0100 Subject: [PATCH 2/2] tests/api: add a test for GIF palettes with duplicate colors Encode a PAL8 frame with a local palette in which each of 16 colors is repeated 16 times, decode it and compare every pixel with the color of the palette entry it indexes. The encoder removes the duplicate palette entries and has to remap the pixels that used them. This fails without "avcodec/gif: Map duplicate palette entries to their first occurrence". Noticed during a review sweep of ffmpeg using an LLM. Signed-off-by: romainhedouin <[email protected]> --- tests/api/Makefile | 1 + tests/api/api-gif-test.c | 97 ++++++++++++++++++++++++++++++++++++++++ tests/fate/api.mak | 5 +++ 3 files changed, 103 insertions(+) create mode 100644 tests/api/api-gif-test.c diff --git a/tests/api/Makefile b/tests/api/Makefile index c411e82e5f1..7f2290dbb2a 100644 --- a/tests/api/Makefile +++ b/tests/api/Makefile @@ -1,6 +1,7 @@ APITESTPROGS-$(call ALLYES, AC3_ENCODER AC3_DECODER EAC3_DECODER AC3_FIXED_DECODER) += api-ac3-flush APITESTPROGS-$(call ALLYES, LIBASTCENC_ENCODER LIBASTCENC_DECODER) += api-astc-profile APITESTPROGS-$(call ENCDEC, FLAC, FLAC) += api-flac +APITESTPROGS-$(call ALLYES, GIF_ENCODER GIF_DECODER) += api-gif APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264 APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264-slice APITESTPROGS-$(CONFIG_MOV_MUXER) += api-movenc diff --git a/tests/api/api-gif-test.c b/tests/api/api-gif-test.c new file mode 100644 index 00000000000..3b4cd25da78 --- /dev/null +++ b/tests/api/api-gif-test.c @@ -0,0 +1,97 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** + * GIF encoder test. + * Encodes a PAL8 frame whose palette contains duplicate colors with a local + * palette, decodes it back and checks that every pixel has the color of the + * palette entry it indexes. + */ + +#include "libavcodec/avcodec.h" +#include "libavutil/intreadwrite.h" +#include "libavutil/opt.h" + +#define W 16 +#define H 16 + +int main(void) +{ + const AVCodec *enc = avcodec_find_encoder(AV_CODEC_ID_GIF); + const AVCodec *dec = avcodec_find_decoder(AV_CODEC_ID_GIF); + AVCodecContext *enc_ctx = avcodec_alloc_context3(enc); + AVCodecContext *dec_ctx = avcodec_alloc_context3(dec); + AVFrame *in = av_frame_alloc(); + AVFrame *out = av_frame_alloc(); + AVPacket *pkt = av_packet_alloc(); + int ret = 1, mismatches = 0; + + if (!enc || !dec || !enc_ctx || !dec_ctx || !in || !out || !pkt) + goto end; + + enc_ctx->pix_fmt = AV_PIX_FMT_PAL8; + enc_ctx->width = W; + enc_ctx->height = H; + enc_ctx->time_base = (AVRational){ 1, 25 }; + + in->format = AV_PIX_FMT_PAL8; + in->width = W; + in->height = H; + + if (av_opt_set_int(enc_ctx->priv_data, "global_palette", 0, 0) < 0 || + avcodec_open2(enc_ctx, enc, NULL) < 0 || + avcodec_open2(dec_ctx, dec, NULL) < 0 || + av_frame_get_buffer(in, 0) < 0) + goto end; + + /* 16 opaque colors, each of them repeated 16 times in the palette, + * and one pixel for each of the 256 palette indexes. */ + for (int i = 0; i < 256; i++) { + AV_WN32(in->data[1] + 4 * i, 0xFF000000 | (i % 16) * 0x010101); + in->data[0][i / W * in->linesize[0] + i % W] = i; + } + + if (avcodec_send_frame(enc_ctx, in) < 0 || + avcodec_receive_packet(enc_ctx, pkt) < 0 || + avcodec_send_packet(dec_ctx, pkt) < 0 || + avcodec_receive_frame(dec_ctx, out) < 0 || + out->format != AV_PIX_FMT_RGB32 || out->width != W || out->height != H) { + av_log(NULL, AV_LOG_ERROR, "Encoding or decoding failed\n"); + goto end; + } + + for (int y = 0; y < H; y++) { + for (int x = 0; x < W; x++) { + uint32_t expected = AV_RN32(in->data[1] + 4 * in->data[0][y * in->linesize[0] + x]); + uint32_t decoded = AV_RN32(out->data[0] + y * out->linesize[0] + 4 * x); + + if (expected != decoded && !mismatches++) + av_log(NULL, AV_LOG_ERROR, "Pixel (%d, %d) is 0x%08"PRIX32", expected 0x%08"PRIX32"\n", + x, y, decoded, expected); + } + } + ret = !!mismatches; + +end: + avcodec_free_context(&enc_ctx); + avcodec_free_context(&dec_ctx); + av_frame_free(&in); + av_frame_free(&out); + av_packet_free(&pkt); + return ret; +} diff --git a/tests/fate/api.mak b/tests/fate/api.mak index 1659c7979c0..5d103a76309 100644 --- a/tests/fate/api.mak +++ b/tests/fate/api.mak @@ -12,6 +12,11 @@ fate-api-flac: $(APITESTSDIR)/api-flac-test$(EXESUF) fate-api-flac: CMD = run $(APITESTSDIR)/api-flac-test$(EXESUF) fate-api-flac: CMP = null +FATE_API_LIBAVCODEC-$(call ALLYES, GIF_ENCODER GIF_DECODER) += fate-api-gif +fate-api-gif: $(APITESTSDIR)/api-gif-test$(EXESUF) +fate-api-gif: CMD = run $(APITESTSDIR)/api-gif-test$(EXESUF) +fate-api-gif: CMP = null + FATE_API_LIBAVCODEC-$(CONFIG_PCM_S16LE_DECODER) += fate-api-skip-samples fate-api-skip-samples: $(APITESTSDIR)/api-skip-samples-test$(EXESUF) fate-api-skip-samples: CMD = run $(APITESTSDIR)/api-skip-samples-test$(EXESUF) -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
