Re: [FFmpeg-devel] [PATCH v4] libavcodec/dvbsubenc.c: add a disable_2bpp option to work around some decoders.

[Ronan Waide <waider@waider.ie>]

> On 2 Mar 2025, at 15:00, Soft Works <softworkz@hotmail.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Ronan
>> Waide
>> Sent: Samstag, 1. März 2025 16:32
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: Ronan Waide <waider@waider.ie>
>> Subject: [FFmpeg-devel] [PATCH v4] libavcodec/dvbsubenc.c: add a
>> disable_2bpp option to work around some decoders.
>> 
>> As noted in the code in several places, some DVB subtitle decoders
>> don't handle 2bpp color. This patch adds a disable_2bpp option which
>> disables the 2bpp format; subtitles which would use 2bpp will be bumped
>> up to 4bpp.
>> 
>> Signed-off-by: Ronan Waide <waider@waider.ie>
>> ---
>> doc/encoders.texi      | 27 ++++++++++++++
>> libavcodec/dvbsubenc.c | 85 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 95 insertions(+), 17 deletions(-)
>> 
>> diff --git a/doc/encoders.texi b/doc/encoders.texi
>> index f3fcc1aa60..07984b6b54 100644
>> --- a/doc/encoders.texi
>> +++ b/doc/encoders.texi
>> @@ -4484,6 +4484,33 @@ Reduces detail but attempts to preserve color at
>> extremely low bitrates.
>> @chapter Subtitles Encoders
>> @c man begin SUBTITLES ENCODERS
>> 
>> +@section dvbsub
>> +
>> +This codec encodes the bitmap subtitle format that is used in DVB
>> +broadcasts and recordings. The bitmaps are typically embedded in a
>> +container such as MPEG-TS as a separate stream.
>> +
>> +@subsection Options
>> +
>> +@table @option
>> +@item disable_2bpp @var{boolean}
>> +Disable the 2 bits-per-pixel encoding format.
>> +
>> +DVB supports 2, 4, and 8 bits-per-pixel color lookup tables. However,
>> +not all players support or properly support 2 bits-per-pixel,
>> +resulting in unusable subtitles.
>> +@table @option
>> +@item 0
>> +The 2 bits-per-pixel encoding format will be used for subtitles with 4
>> +colors or less. (default)
>> +
>> +@item 1
>> +The 2 bits-per-pixel encoding format will be disabled, and subtitles
>> +with 4 colors or less will use a 4 bits-per-pixel format.
>> +@end table
>> +
>> +@end table
>> +
>> @section dvdsub
>> 
>> This codec encodes the bitmap subtitle format that is used in DVDs.
>> diff --git a/libavcodec/dvbsubenc.c b/libavcodec/dvbsubenc.c
>> index 822e3a5309..727a75e901 100644
>> --- a/libavcodec/dvbsubenc.c
>> +++ b/libavcodec/dvbsubenc.c
>> @@ -22,9 +22,13 @@
>> #include "bytestream.h"
>> #include "codec_internal.h"
>> #include "libavutil/colorspace.h"
>> +#include "libavutil/opt.h"
>> 
>> typedef struct DVBSubtitleContext {
>> +    AVClass * class;
>>     int object_version;
>> +    int disable_2bpp;
>> +    int min_colors;
>> } DVBSubtitleContext;
>> 
>> #define PUTBITS2(val)\
>> @@ -38,6 +42,18 @@ typedef struct DVBSubtitleContext {
>>     }\
>> }
>> 
>> +static int dvbsub_init(AVCodecContext *avctx)
>> +{
>> +    DVBSubtitleContext *s = avctx->priv_data;
>> +    if (s->disable_2bpp) {
>> +        av_log(avctx, AV_LOG_DEBUG, "Disabling 2bpp subtitles.\n");
>> +        s->min_colors = 16;
>> +    } else {
>> +        s->min_colors = 0;
>> +    }
>> +    return 0;
>> +}
>> +
>> static int dvb_encode_rle2(uint8_t **pq, int buf_size,
>>                            const uint8_t *bitmap, int linesize,
>>                            int w, int h)
>> @@ -326,18 +342,22 @@ static int dvbsub_encode(AVCodecContext *avctx,
>> uint8_t *outbuf, int buf_size,
>> 
>>     if (h->num_rects) {
>>         for (clut_id = 0; clut_id < h->num_rects; clut_id++) {
>> -            if (buf_size < 6 + h->rects[clut_id]->nb_colors * 6)
>> +            int nb_colors = s->min_colors > h->rects[clut_id]-
>>> nb_colors
>> +                            ? s->min_colors
>> +                            : h->rects[clut_id]->nb_colors;
>> +
>> +            if (buf_size < 6 + nb_colors * 6)
>>                 return AVERROR_BUFFER_TOO_SMALL;
>> 
>>             /* CLUT segment */
>> 
>> -            if (h->rects[clut_id]->nb_colors <= 4) {
>> +            if (nb_colors <= 4) {
>>                 /* 2 bpp, some decoders do not support it correctly */
>>                 bpp_index = 0;
>> -            } else if (h->rects[clut_id]->nb_colors <= 16) {
>> +            } else if (nb_colors <= 16) {
>>                 /* 4 bpp, standard encoding */
>>                 bpp_index = 1;
>> -            } else if (h->rects[clut_id]->nb_colors <= 256) {
>> +            } else if (nb_colors <= 256) {
>>                 /* 8 bpp, standard encoding */
>>                 bpp_index = 2;
>>             } else {
>> @@ -354,16 +374,24 @@ static int dvbsub_encode(AVCodecContext *avctx,
>> uint8_t *outbuf, int buf_size,
>>             *q++ = clut_id;
>>             *q++ = (0 << 4) | 0xf; /* version = 0 */
>> 
>> -            for(i = 0; i < h->rects[clut_id]->nb_colors; i++) {
>> +            for(i = 0; i < nb_colors; i++) {
>>                 *q++ = i; /* clut_entry_id */
>>                 *q++ = (1 << (7 - bpp_index)) | (0xf << 1) | 1; /* 2
>> bits/pixel full range */
>>                 {
>>                     int a, r, g, b;
>> -                    uint32_t x= ((uint32_t*)h->rects[clut_id]-
>>> data[1])[i];
>> -                    a = (x >> 24) & 0xff;
>> -                    r = (x >> 16) & 0xff;
>> -                    g = (x >>  8) & 0xff;
>> -                    b = (x >>  0) & 0xff;
>> +                    if (i < h->rects[clut_id]->nb_colors) {
>> +                        uint32_t x= ((uint32_t*)h->rects[clut_id]-
>>> data[1])[i];
>> +                        a = (x >> 24) & 0xff;
>> +                        r = (x >> 16) & 0xff;
>> +                        g = (x >>  8) & 0xff;
>> +                        b = (x >>  0) & 0xff;
>> +                    } else {
>> +                        /* pad out the CLUT */
>> +                        a = 0;
>> +                        r = 0;
>> +                        g = 0;
>> +                        b = 0;
>> +                    }
>> 
>>                     *q++ = RGB_TO_Y_CCIR(r, g, b);
>>                     *q++ = RGB_TO_V_CCIR(r, g, b, 0);
>> @@ -373,7 +401,7 @@ static int dvbsub_encode(AVCodecContext *avctx,
>> uint8_t *outbuf, int buf_size,
>>             }
>> 
>>             bytestream_put_be16(&pseg_len, q - pseg_len - 2);
>> -            buf_size -= 6 + h->rects[clut_id]->nb_colors * 6;
>> +            buf_size -= 6 + nb_colors * 6;
>>         }
>> 
>>         if (buf_size < h->num_rects * 22)
>> @@ -381,14 +409,17 @@ static int dvbsub_encode(AVCodecContext *avctx,
>> uint8_t *outbuf, int buf_size,
>>         for (region_id = 0; region_id < h->num_rects; region_id++) {
>> 
>>             /* region composition segment */
>> +            int nb_colors = s->min_colors > h->rects[region_id]-
>>> nb_colors
>> +                            ? s->min_colors
>> +                            : h->rects[region_id]->nb_colors;
>> 
>> -            if (h->rects[region_id]->nb_colors <= 4) {
>> +            if (nb_colors <= 4) {
>>                 /* 2 bpp, some decoders do not support it correctly */
>>                 bpp_index = 0;
>> -            } else if (h->rects[region_id]->nb_colors <= 16) {
>> +            } else if (nb_colors <= 16) {
>>                 /* 4 bpp, standard encoding */
>>                 bpp_index = 1;
>> -            } else if (h->rects[region_id]->nb_colors <= 256) {
>> +            } else if (nb_colors <= 256) {
>>                 /* 8 bpp, standard encoding */
>>                 bpp_index = 2;
>>             } else {
>> @@ -424,17 +455,21 @@ static int dvbsub_encode(AVCodecContext *avctx,
>> uint8_t *outbuf, int buf_size,
>>                                   const uint8_t *bitmap, int linesize,
>>                                   int w, int h);
>> 
>> +            int nb_colors = s->min_colors > h->rects[object_id]-
>>> nb_colors
>> +                                ? s->min_colors
>> +                                : h->rects[object_id]->nb_colors;
>> +
>>             if (buf_size < 13)
>>                 return AVERROR_BUFFER_TOO_SMALL;
>> 
>>             /* bpp_index maths */
>> -            if (h->rects[object_id]->nb_colors <= 4) {
>> +            if (nb_colors <= 4) {
>>                 /* 2 bpp, some decoders do not support it correctly */
>>                 dvb_encode_rle = dvb_encode_rle2;
>> -            } else if (h->rects[object_id]->nb_colors <= 16) {
>> +            } else if (nb_colors <= 16) {
>>                 /* 4 bpp, standard encoding */
>>                 dvb_encode_rle = dvb_encode_rle4;
>> -            } else if (h->rects[object_id]->nb_colors <= 256) {
>> +            } else if (nb_colors <= 256) {
>>                 /* 8 bpp, standard encoding */
>>                 dvb_encode_rle = dvb_encode_rle8;
>>             } else {
>> @@ -506,6 +541,20 @@ static int dvbsub_encode(AVCodecContext *avctx,
>> uint8_t *outbuf, int buf_size,
>>     return q - outbuf;
>> }
>> 
>> +#define OFFSET(x) offsetof(DVBSubtitleContext, x)
>> +#define SE AV_OPT_FLAG_SUBTITLE_PARAM | AV_OPT_FLAG_ENCODING_PARAM
>> +static const AVOption options[] = {
>> +    {"disable_2bpp", "disable the 2bpp subtitle encoder",
>> OFFSET(disable_2bpp), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, SE},
>> +    { NULL },
>> +};
>> +
>> +static const AVClass dvbsubenc_class = {
>> +    .class_name = "DVBSUB subtitle encoder",
>> +    .item_name  = av_default_item_name,
>> +    .option     = options,
>> +    .version    = LIBAVUTIL_VERSION_INT,
>> +};
>> +
>> const FFCodec ff_dvbsub_encoder = {
>>     .p.name         = "dvbsub",
>>     CODEC_LONG_NAME("DVB subtitles"),
>> @@ -513,4 +562,6 @@ const FFCodec ff_dvbsub_encoder = {
>>     .p.id           = AV_CODEC_ID_DVB_SUBTITLE,
>>     .priv_data_size = sizeof(DVBSubtitleContext),
>>     FF_CODEC_ENCODE_SUB_CB(dvbsub_encode),
>> +    .init           = dvbsub_init,
>> +    .p.priv_class   = &dvbsubenc_class,
>> };
>> --
> 
> 
> Hi Ronan,
> 
> thanks for your patch, it LGTM.
> 
> This was ringing some bell, I didn't immediately know what it was, but now I remember that we had encountered the 2bpp incompatibility with certain decoders when implementing the text2graphicsub filter (demo here: https://github.com/softworkz/SubtitleFilteringDemos/tree/master/Demo1), and so we chose to use a default of 16 for the output colors of the text2graphicsub filter to avoid that situation:
> 
> https://github.com/softworkz/FFmpeg/blob/bb1179897474e20966d5cb7724877ca651fe13d8/libavfilter/sf_text2graphicsub.c#L587-L588
> 
> 
> In the light of these experiences, I would propose to go even one step further and make 4bpp the default minimum, either by setting the default value of disable_2bpp to 1 - or by renaming it to enable_2bpp with a default of 0 and a description like "2bpp is not supported by all decoders. By default, 4bpp is used as the minimum. Set this to enable 2pbb encoding."
> 
> Did you try running FATE while disable_2bpp is hardcoded to 1? If it causes any test failures, then the FATE reference results would need to be adjusted accordingly.
> 
> sw
> 
> 

thanks for the feedback, Andreas & sw. V5 submitted with your suggestions included. I've run it against FATE and compared the output with previous versions and it all looks good.

Cheers,
Waider.
-- 
Ronan Waide
waider@waider.ie




_______________________________________________
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".