Re: [FFmpeg-devel] [PATCH v7 2/2] libavformat/dvdvideo: add DVD CLUT utilities and enable palette support

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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 <marth64@proxyid.net>
> ---
>  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".