> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> Ronan Waide
> Sent: Sonntag, 30. März 2025 12:18
> To: FFmpeg development discussions and patches <ffmpeg-
> de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v5] libavcodec/dvbsubenc.c: add a
> disable_2bpp option to work around some decoders.
>
>
>
> > On 8 Mar 2025, at 08:02, Ronan Waide <wai...@waider.ie> wrote:
> >
> >
> >
> >> On 2 Mar 2025, at 20:38, Soft Works <softworkz-at-
> hotmail....@ffmpeg.org> wrote:
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> Ronan
> >>> Waide
> >>> Sent: Sonntag, 2. März 2025 18:24
> >>> To: ffmpeg-devel@ffmpeg.org
> >>> Cc: Ronan Waide <wai...@waider.ie>
> >>> Subject: [FFmpeg-devel] [PATCH v5] 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. Per suggestion from sw, disable_2pp defaults to true.
> >>>
> >>> Signed-off-by: Ronan Waide <wai...@waider.ie>
> >>> ---
> >>> doc/encoders.texi | 27 +++++++++++++++++
> >>> libavcodec/dvbsubenc.c | 69 +++++++++++++++++++++++++++++++-------
> ----
> >>> 2 files changed, 78 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/doc/encoders.texi b/doc/encoders.texi
> >>> index f3fcc1aa60..6ee0065c7d 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.
> >>> +
> >>> +@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.
> (default)
> >>> +@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..4db2e1ddda 100644
> >>> --- a/libavcodec/dvbsubenc.c
> >>> +++ b/libavcodec/dvbsubenc.c
> >>> @@ -22,9 +22,12 @@
> >>> #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;
> >>> } DVBSubtitleContext;
> >>>
> >>> #define PUTBITS2(val)\
> >>> @@ -274,13 +277,15 @@ static int dvbsub_encode(AVCodecContext
> *avctx,
> >>> uint8_t *outbuf, int buf_size,
> >>> {
> >>> DVBSubtitleContext *s = avctx->priv_data;
> >>> uint8_t *q, *pseg_len;
> >>> - int page_id, region_id, clut_id, object_id, i, bpp_index,
> >>> page_state;
> >>> + int page_id, region_id, clut_id, object_id, i, bpp_index,
> >>> page_state, min_colors;
> >>>
> >>>
> >>> q = outbuf;
> >>>
> >>> page_id = 1;
> >>>
> >>> + min_colors = s->disable_2bpp ? 16 : 0;
> >>> +
> >>> if (h->num_rects && !h->rects)
> >>> return AVERROR(EINVAL);
> >>>
> >>> @@ -326,18 +331,20 @@ 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 = FFMAX(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 +361,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 +388,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 +396,15 @@ 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 = FFMAX(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 +440,19 @@ 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 = FFMAX(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 +524,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 = 1}, 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 +545,5 @@ const FFCodec ff_dvbsub_encoder = {
> >>> .p.id = AV_CODEC_ID_DVB_SUBTITLE,
> >>> .priv_data_size = sizeof(DVBSubtitleContext),
> >>> FF_CODEC_ENCODE_SUB_CB(dvbsub_encode),
> >>> + .p.priv_class = &dvbsubenc_class,
> >>> };
> >>> --
> >>
> >> Tested & LGTM
> >>
> >> Thanks for the patch!
> >
> > Hello,
> >
> > nudging on this - does it need any more work, or will it be merged?
> >
> > Cheers,
> > Waider.
> > --
> > Ronan Waide
> > wai...@waider.ie
> >
> >
> >
> >
>
> Hello again,
>
> this still applies cleanly to the current HEAD, i.e. no conflicts on
> rebase. Would it be possible to merge it, or is there additional work
> required?
>
> cheers,
> Waider.
> --
> Ronan Waide
> wai...@waider.ie
Hello Ronan,
I'll try to pick this up next or 2nd-next week. Patch looked good
but I want to test it locally before making a last call and applying
it - assuming all goes well.
Thanks
sw
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
