From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 48D7849325 for ; Wed, 7 Feb 2024 18:33:18 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8496168D14A; Wed, 7 Feb 2024 20:33:16 +0200 (EET) Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com [209.85.215.180]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0E5D468CCD5 for ; Wed, 7 Feb 2024 20:33:09 +0200 (EET) Received: by mail-pg1-f180.google.com with SMTP id 41be03b00d2f7-58962bf3f89so222586a12.0 for ; Wed, 07 Feb 2024 10:33:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707330788; x=1707935588; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=hTlzYTCuQo3uoUE8qzfAYRbnd2Q9mm4OQ1NAZDsgGgg=; b=AWYkEAxkhUHBXZ7yUH+u2YWFfw0coaGnRXSOB+5wN6tP0koDW8U2CWyNe41IaZ/2RZ YwlmXDRcW3xfGRr3fpIkcmbWjrnbZW9sYhjDxGaQYiCEOcq9edb30VaxlJBpby7Zus/Y K4DpQbbb3QY68RcOyYbSWwi2IVvRLKUEPBfBfI3jEvegrT44p4vNwjO4jA1aeBoECUEf 5DoFSwYhy44jbcjCivuSkTjt6mB/3nQ9zp8w7ByYayUXdyYS1GpDxQwbS7kIVnEKACkz xfLEwlACI6KI2KTzh3xcDTrDSmy8/jXOMOZulQWS88rQjOa1dmHNCPpy/xNlXja2TTaz 36kQ== X-Gm-Message-State: AOJu0YwonW0pnq503ngmkrYUIsB1qNk9HEhkzl0dv2jZF1zUF8hJp/zG gXGnhd1CaIGTlEUUEBvwRL6M4sVClQK0X/zBUI3L5jMCkXd6gx6e6XE7NaMrmGxIld0ynRlGUPM HXZUWPVOO0Rp1bMWv4hmgH1LHCxsQQeX/QXsI9kGjHQSlsH68 X-Google-Smtp-Source: AGHT+IEIuMgpmgMSnGr1fbxl34pg5KQP2qkQWjwqHYy7PJsCeiSxPPq8FNykAk97gqQIKoMVkBNahcaP2wSFi7r20xM= X-Received: by 2002:a05:6a20:5499:b0:19e:ac60:1268 with SMTP id i25-20020a056a20549900b0019eac601268mr564401pzk.0.1707330787535; Wed, 07 Feb 2024 10:33:07 -0800 (PST) MIME-Version: 1.0 References: <20240205000242.959501-1-marth64@proxyid.net> <20240205054826.1179277-1-marth64@proxyid.net> <20240205054826.1179277-2-marth64@proxyid.net> In-Reply-To: From: Marth64 Date: Wed, 7 Feb 2024 12:32:55 -0600 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH v7 2/2] libavformat/dvdvideo: add DVD CLUT utilities and enable palette support X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Thank you, Andreas. This was very helpful. I will clean it up this evening. On Wed, Feb 7, 2024 at 12:07 Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Marth64: > > DVD subtitle palettes, which are natively YUV, are currently carried as > > a hex string in their respective subtitle streams and have > > no concept of colorspace tagging. In fact, the convention is to convert > > them to RGB prior to storage. Common players will only render > > the palettes properly if they are stored as RGB. Even ffmpeg itself > > expects this, and already does -in libavformat- the YUV-RGB conversions, > > specifically in mov.c and movenc.c. > > > > The point of this patch is to provide a consolidation of the code > > that deals with creating the extradata as well as the RGB conversion. > > That can then (1) enable usable palette support for DVD demuxer if it is > merged > > and (2) start the process of consolidating the related conversions in > > MOV muxer/demuxer and eventually find a way to properly tag > > the colorspace. > > > > > > Signed-off-by: Marth64 > > --- > > doc/demuxers.texi | 5 ++ > > libavformat/Makefile | 2 +- > > libavformat/dvdclut.c | 104 ++++++++++++++++++++++++++++++++++++++ > > libavformat/dvdclut.h | 37 ++++++++++++++ > > libavformat/dvdvideodec.c | 14 +++++ > > 5 files changed, 161 insertions(+), 1 deletion(-) > > create mode 100644 libavformat/dvdclut.c > > create mode 100644 libavformat/dvdclut.h > > > > diff --git a/doc/demuxers.texi b/doc/demuxers.texi > > index ad0906f6ec..9c666a29c1 100644 > > --- a/doc/demuxers.texi > > +++ b/doc/demuxers.texi > > @@ -390,6 +390,11 @@ often with junk data intended for controlling a > real DVD player's > > buffering speed and with no other material data value. > > Default is 1, true. > > > > +@item clut_rgb > > +Output subtitle palettes (CLUTs) as RGB, required for Matroska and MP4. > > +Disable to output the palette in its original YUV colorspace. > > +Default is 1, true. > > + > > @end table > > > > @subsection Examples > > diff --git a/libavformat/Makefile b/libavformat/Makefile > > index df69734877..8f17e43e49 100644 > > --- a/libavformat/Makefile > > +++ b/libavformat/Makefile > > @@ -192,7 +192,7 @@ OBJS-$(CONFIG_DTS_MUXER) += rawenc.o > > OBJS-$(CONFIG_DV_MUXER) += dvenc.o > > OBJS-$(CONFIG_DVBSUB_DEMUXER) += dvbsub.o rawdec.o > > OBJS-$(CONFIG_DVBTXT_DEMUXER) += dvbtxt.o rawdec.o > > -OBJS-$(CONFIG_DVDVIDEO_DEMUXER) += dvdvideodec.o > > +OBJS-$(CONFIG_DVDVIDEO_DEMUXER) += dvdvideodec.o dvdclut.o > > OBJS-$(CONFIG_DXA_DEMUXER) += dxa.o > > OBJS-$(CONFIG_EA_CDATA_DEMUXER) += eacdata.o > > OBJS-$(CONFIG_EA_DEMUXER) += electronicarts.o > > diff --git a/libavformat/dvdclut.c b/libavformat/dvdclut.c > > new file mode 100644 > > index 0000000000..71fdda7925 > > --- /dev/null > > +++ b/libavformat/dvdclut.c > > @@ -0,0 +1,104 @@ > > +/* > > + * DVD-Video subpicture CLUT (Color Lookup Table) utilities > > + * > > + * 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 > > + */ > > + > > +#include "libavutil/bprint.h" > > +#include "libavutil/colorspace.h" > > +#include "libavutil/intreadwrite.h" > > Seems unneeded, just like avformat.h. > > > + > > +#include "avformat.h" > > +#include "dvdclut.h" > > +#include "internal.h" > > + > > +/* crop table for YUV to RGB subpicture palette conversion */ > > +#define FF_DVDCLUT_YUV_NEG_CROP_MAX 1024 > > An FF-prefix for an internal define is not needed. > > > +#define times4(x) x, x, x, x > > +#define times256(x) > times4(times4(times4(times4(times4(x))))) > > This is actually a times1024 (which matches the size of the LUT). > > > + > > +const uint8_t ff_dvdclut_yuv_crop_tab[256 + 2 * > FF_DVDCLUT_YUV_NEG_CROP_MAX] = { > > +times256(0x00), > > > +0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0A,0x0B,0x0C,0x0D,0x0E,0x0F, > > > +0x10,0x11,0x12,0x13,0x14,0x15,0x16,0x17,0x18,0x19,0x1A,0x1B,0x1C,0x1D,0x1E,0x1F, > > > +0x20,0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,0x29,0x2A,0x2B,0x2C,0x2D,0x2E,0x2F, > > > +0x30,0x31,0x32,0x33,0x34,0x35,0x36,0x37,0x38,0x39,0x3A,0x3B,0x3C,0x3D,0x3E,0x3F, > > > +0x40,0x41,0x42,0x43,0x44,0x45,0x46,0x47,0x48,0x49,0x4A,0x4B,0x4C,0x4D,0x4E,0x4F, > > > +0x50,0x51,0x52,0x53,0x54,0x55,0x56,0x57,0x58,0x59,0x5A,0x5B,0x5C,0x5D,0x5E,0x5F, > > > +0x60,0x61,0x62,0x63,0x64,0x65,0x66,0x67,0x68,0x69,0x6A,0x6B,0x6C,0x6D,0x6E,0x6F, > > > +0x70,0x71,0x72,0x73,0x74,0x75,0x76,0x77,0x78,0x79,0x7A,0x7B,0x7C,0x7D,0x7E,0x7F, > > > +0x80,0x81,0x82,0x83,0x84,0x85,0x86,0x87,0x88,0x89,0x8A,0x8B,0x8C,0x8D,0x8E,0x8F, > > > +0x90,0x91,0x92,0x93,0x94,0x95,0x96,0x97,0x98,0x99,0x9A,0x9B,0x9C,0x9D,0x9E,0x9F, > > > +0xA0,0xA1,0xA2,0xA3,0xA4,0xA5,0xA6,0xA7,0xA8,0xA9,0xAA,0xAB,0xAC,0xAD,0xAE,0xAF, > > > +0xB0,0xB1,0xB2,0xB3,0xB4,0xB5,0xB6,0xB7,0xB8,0xB9,0xBA,0xBB,0xBC,0xBD,0xBE,0xBF, > > > +0xC0,0xC1,0xC2,0xC3,0xC4,0xC5,0xC6,0xC7,0xC8,0xC9,0xCA,0xCB,0xCC,0xCD,0xCE,0xCF, > > > +0xD0,0xD1,0xD2,0xD3,0xD4,0xD5,0xD6,0xD7,0xD8,0xD9,0xDA,0xDB,0xDC,0xDD,0xDE,0xDF, > > > +0xE0,0xE1,0xE2,0xE3,0xE4,0xE5,0xE6,0xE7,0xE8,0xE9,0xEA,0xEB,0xEC,0xED,0xEE,0xEF, > > > +0xF0,0xF1,0xF2,0xF3,0xF4,0xF5,0xF6,0xF7,0xF8,0xF9,0xFA,0xFB,0xFC,0xFD,0xFE,0xFF, > > +times256(0xFF) > > +}; > > 1. This LUT is not used outside this module, so should be static if > used. And therefore should not use an ff_ prefix at all. > 2. Given that this code here is absolutely not speed relevant, the LUT > can be avoided by using av_clip_uint8(value - 1024). > > > + > > +int ff_dvdclut_palette_extradata_cat(const uint32_t *clut, > > + const size_t clut_size, > > + AVCodecParameters *par) > > +{ > > + int ret = 0; > > + AVBPrint bp; > > + > > + if (clut_size != FF_DVDCLUT_CLUT_SIZE) > > + return AVERROR(EINVAL); > > + > > + av_bprint_init(&bp, 0, FF_DVDCLUT_EXTRADATA_SIZE); > > + > > + av_bprintf(&bp, "palette: "); > > + > > + for (int i = 0; i < FF_DVDCLUT_CLUT_LEN; i++) > > + av_bprintf(&bp, "%06"PRIx32"%s", clut[i], i != 15 ? ", " : ""); > > Don't hardcode 15 for the last iteration. > > > + > > + av_bprintf(&bp, "\n"); > > + > > + ret = ff_bprint_to_codecpar_extradata(par, &bp); > > + > > + av_bprint_finalize(&bp, NULL); > > Unnecessary, as ff_bprint_to_codecpar_extradata() has already finalized > the bprint. > > > + > > + return ret; > > +} > > + > > +int ff_dvdclut_yuv_to_rgb(uint32_t *clut, const size_t clut_size) > > +{ > > + const uint8_t *cm = ff_dvdclut_yuv_crop_tab + > FF_DVDCLUT_YUV_NEG_CROP_MAX; > > + > > + int i, y, cb, cr; > > + uint8_t r, g, b; > > + int r_add, g_add, b_add; > > i should have loop scope and the other variables loop body scope. > > > + > > + if (clut_size != FF_DVDCLUT_CLUT_SIZE) > > + return AVERROR(EINVAL); > > + > > + for (i = 0; i < FF_DVDCLUT_CLUT_LEN; i++) { > > + y = (clut[i] >> 16) & 0xFF; > > + cr = (clut[i] >> 8) & 0xFF; > > + cb = clut[i] & 0xFF; > > + > > + YUV_TO_RGB1_CCIR(cb, cr); > > + YUV_TO_RGB2_CCIR(r, g, b, y); > > + > > + clut[i] = (r << 16) | (g << 8) | b; > > + } > > + > > + return 0; > > +} > > diff --git a/libavformat/dvdclut.h b/libavformat/dvdclut.h > > new file mode 100644 > > index 0000000000..40eff6de34 > > --- /dev/null > > +++ b/libavformat/dvdclut.h > > @@ -0,0 +1,37 @@ > > +/* > > + * DVD-Video subpicture CLUT (Color Lookup Table) utilities > > + * > > + * 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 > > + */ > > + > > +#ifndef AVFORMAT_DVDCLUT_H > > +#define AVFORMAT_DVDCLUT_H > > + > > +#include "avformat.h" > > lavc/codec_par.h is enough > > > + > > +/* ("palette: ") + ("rrggbb, "*15) + ("rrggbb") + \n + \0 */ > > +#define FF_DVDCLUT_EXTRADATA_SIZE (9 + (8 * 15) + 6 + 1 + 1) > > +#define FF_DVDCLUT_CLUT_LEN 16 > > +#define FF_DVDCLUT_CLUT_SIZE FF_DVDCLUT_CLUT_LEN * > sizeof(uint32_t) > > + > > +int ff_dvdclut_palette_extradata_cat(const uint32_t *clut, > > + const size_t clut_size, > > + AVCodecParameters *par); > > + > > +int ff_dvdclut_yuv_to_rgb(uint32_t *clut, const size_t clut_size); > > + > > +#endif /* AVFORMAT_DVDCLUT_H */ > > diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c > > index 2e363a4a22..657825ba3e 100644 > > --- a/libavformat/dvdvideodec.c > > +++ b/libavformat/dvdvideodec.c > > @@ -50,6 +50,7 @@ > > #include "avio_internal.h" > > #include "avlanguage.h" > > #include "demux.h" > > +#include "dvdclut.h" > > #include "internal.h" > > #include "url.h" > > > > @@ -91,6 +92,7 @@ typedef struct DVDVideoPGCAudioStreamEntry { > > > > typedef struct DVDVideoPGCSubtitleStreamEntry { > > int startcode; > > + uint32_t *clut; > > int disposition; > > char *lang_iso; > > enum DVDVideoSubpictureViewport viewport; > > @@ -132,6 +134,7 @@ typedef struct DVDVideoDemuxContext { > > int opt_region; /* the > user-provided region digit */ > > int opt_preindex; /* pre-indexing > mode (2-pass read) */ > > int opt_trim; /* trim padding > cells at beginning and end */ > > + int opt_clut_rgb; /* output subtitle > palette (CLUT) as RGB */ > > > > /* subdemux */ > > const AVInputFormat *mpeg_fmt; /* inner MPEG-PS > (VOB) demuxer */ > > @@ -1034,6 +1037,11 @@ static int > dvdvideo_subp_stream_analyze(AVFormatContext *s, uint32_t offset, sub > > if (subp_attr.lang_extension == 9) > > entry->disposition |= AV_DISPOSITION_FORCED; > > > > + memcpy(entry->clut, c->play_state.pgc->palette, > FF_DVDCLUT_CLUT_SIZE); > > + > > + if (c->opt_clut_rgb) > > + ff_dvdclut_yuv_to_rgb(entry->clut, FF_DVDCLUT_CLUT_SIZE); > > + > > AV_WB16(lang_dvd, subp_attr.lang_code); > > entry->lang_iso = (char *) ff_convert_lang_to(lang_dvd, > AV_LANG_ISO639_2_BIBL); > > > > @@ -1055,6 +1063,9 @@ static int > dvdvideo_subp_stream_add(AVFormatContext *s, DVDVideoPGCSubtitleStrea > > st->codecpar->codec_type = AVMEDIA_TYPE_SUBTITLE; > > st->codecpar->codec_id = AV_CODEC_ID_DVD_SUBTITLE; > > > > + if ((ret = ff_dvdclut_palette_extradata_cat(entry->clut, > FF_DVDCLUT_CLUT_SIZE, st->codecpar)) < 0) > > + return ret; > > + > > if (entry->lang_iso) > > av_dict_set(&st->metadata, "language", entry->lang_iso, 0); > > > > @@ -1082,6 +1093,7 @@ static int > dvdvideo_subp_stream_add_internal(AVFormatContext *s, uint32_t offset > > int ret = 0; > > > > entry = av_mallocz(sizeof(DVDVideoPGCSubtitleStreamEntry)); > > + entry->clut = av_mallocz(FF_DVDCLUT_CLUT_SIZE); > > entry->viewport = viewport; > > > > if ((ret = dvdvideo_subp_stream_analyze(s, offset, subp_attr, > entry)) < 0) > > @@ -1102,6 +1114,7 @@ end_free_error: > > av_log(s, AV_LOG_ERROR, "Unable to allocate subtitle stream\n"); > > > > end_free: > > + av_freep(&entry->clut); > > av_freep(&entry); > > > > return ret; > > @@ -1381,6 +1394,7 @@ static const AVOption dvdvideo_options[] = { > > {"angle", "playback angle number", > OFFSET(opt_angle), AV_OPT_TYPE_INT, { .i64=1 }, > 1, 9, AV_OPT_FLAG_DECODING_PARAM }, > > {"chapter_end", "exit chapter (PTT) number (0=end)", > OFFSET(opt_chapter_end), AV_OPT_TYPE_INT, { .i64=0 }, > 0, 99, AV_OPT_FLAG_DECODING_PARAM }, > > {"chapter_start", "entry chapter (PTT) number", > OFFSET(opt_chapter_start), AV_OPT_TYPE_INT, { .i64=1 }, > 1, 99, AV_OPT_FLAG_DECODING_PARAM }, > > + {"clut_rgb", "output subtitle palette (CLUT) as RGB", > OFFSET(opt_clut_rgb), AV_OPT_TYPE_BOOL, { .i64=1 }, > 0, 1, AV_OPT_FLAG_DECODING_PARAM }, > > {"pg", "entry PG number (0=auto)", > OFFSET(opt_pg), AV_OPT_TYPE_INT, { .i64=0 }, > 0, 255, AV_OPT_FLAG_DECODING_PARAM }, > > {"pgc", "entry PGC number (0=auto)", > OFFSET(opt_pgc), AV_OPT_TYPE_INT, { .i64=0 }, > 0, 999, AV_OPT_FLAG_DECODING_PARAM }, > > {"preindex", "enable for accurate chapter markers, slow > (2-pass read)", OFFSET(opt_preindex), AV_OPT_TYPE_BOOL, { .i64=0 > }, 0, 1, AV_OPT_FLAG_DECODING_PARAM }, > > _______________________________________________ > 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". > _______________________________________________ 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".