[FFmpeg-devel] [PATCH 1/2 v3] add libaribb24 ARIB STD-B24 caption decoder

[Jan Ekström]

On Mon, Feb 11, 2019 at 1:01 AM Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Sun, Feb 03, 2019 at 09:38:40PM +0200, Jan Ekström wrote:
> > +#define RGB_TO_BGR(c) ((c & 0xff) << 16 | (c & 0xff00) | ((c >> 16) & 0xff))
>
> c should be protected by a ()
>

First of all, thank you for the technical review.

But yes, it seems like I was too focused on single values it seems (in
which case there is no ambiguity regarding order). Will do.

>
> > +
> > +static void libaribb24_handle_regions(AVCodecContext *avctx, AVSubtitle *sub)
> > +{
> > +    Libaribb24Context *b24 = avctx->priv_data;
> > +    const arib_buf_region_t *region = arib_decoder_get_regions(b24->decoder);
> > +    unsigned int profile_font_size = get_profile_font_size(avctx->profile);
> > +    AVBPrint buf = { 0 };
> > +
> > +    av_bprint_init(&buf, 0, AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +    while (region) {
> > +        ptrdiff_t region_length = region->p_end - region->p_start;
> > +        unsigned int ruby_region =
> > +            region->i_fontheight == (profile_font_size / 2);
> > +
> > +        // ASS requires us to make the colors BGR, so we convert here
> > +        int foreground_bgr_color = RGB_TO_BGR(region->i_foreground_color);
> > +        int background_bgr_color = RGB_TO_BGR(region->i_background_color);
> > +
> > +        if (region_length < 0) {
> > +            av_log(avctx, AV_LOG_ERROR, "Invalid negative region length!\n");
> > +            break;
> > +        }
> > +
> > +        if (region_length == 0 || (ruby_region && b24->aribb24_skip_ruby)) {
> > +            goto next_region;
> > +        }
> > +
> > +        // color and alpha
> > +        if (foreground_bgr_color != ASS_DEFAULT_COLOR)
> > +            av_bprintf(&buf, "{\\1c&H%06x&}", foreground_bgr_color);
> > +
> > +        if (region->i_foreground_alpha != 0)
> > +            av_bprintf(&buf, "{\\1a&H%02x&}", region->i_foreground_alpha);
> > +
> > +        if (background_bgr_color != ASS_DEFAbufULT_BACK_COLOR)
> > +            av_bprintf(&buf, "{\\3c&H%06x&}", background_bgr_color);
> > +
> > +        if (region->i_background_alpha != 0)
> > +            av_bprintf(&buf, "{\\3a&H%02x&}", region->i_background_alpha);
> > +
> > +        // font size
> > +        if (region->i_fontwidth  != profile_font_size ||
> > +            region->i_fontheight != profile_font_size) {
>
> > +            av_bprintf(&buf, "{\\fscx%d\\fscy%d}",
> > +                       (int)round(((double)region->i_fontwidth /
> > +                                   (double)profile_font_size) * 100),
> > +                       (int)round(((double)region->i_fontheight /
> > +                                   (double)profile_font_size) * 100));
>
> Please use integers ((u)int64_t) not floating point. This ensures that the results
> are reliably reproducable.
> av_rescale*() may be usefull for this
>

Yes, I was planning to work on this if someone brought it up, but so
far nobody had commented on it.

Will try to post a recalc patch ASAP.

>
>
> > +        }
> > +
> > +        // TODO: positioning
> > +
> > +        av_bprint_append_data(&buf, region->p_start, region_length);
> > +
> > +        av_bprintf(&buf, "{\\r}");
> > +
> > +next_region:
> > +        region = region->p_next;
> > +    }
> > +
> > +    av_log(avctx, AV_LOG_DEBUG, "Styled ASS line: %s\n",
> > +           buf.str);
>
> is this function missing malloc failure checks for bprintf* ?
> or is there something that avoids this ?
>

I looked at other subtitle decoders (such as ccaption_dec and libzvbi)
and i didn't notice any checks when adding things to the buffer or
when initializing the bprint context.

Now that you have brought this up, it seems like I was missing
av_bprint_finalize, is this what is meant by this? That seems to be
the only thing where memory allocation is checked with these APIs in
many places. If that is it, I will post a fixup patch ASAP. It did
feel kind of weird not seeing memory allocation checks at each step.

>
> > +    ff_ass_add_rect(sub, buf.str, b24->read_order++,
> > +                    0, NULL, NULL);
> > +
> > +    av_bprint_finalize(&buf, NULL);
> > +}
> > +
> > +static int libaribb24_decode(AVCodecContext *avctx, void *data, int *got_sub_ptr, AVPacket *pkt)
> > +{
> > +    Libaribb24Context *b24 = avctx->priv_data;
> > +    AVSubtitle *sub = data;
> > +    size_t parsed_data_size = 0;
> > +    size_t decoded_subtitle_size = 0;
> > +    const unsigned char *parsed_data = NULL;
> > +    char *decoded_subtitle = NULL;
> > +    time_t subtitle_duration = 0;
> > +
> > +    if (pkt->size <= 0)
> > +        return pkt->size;
> > +
> > +    arib_parse_pes(b24->parser, pkt->data, pkt->size);
> > +
> > +    parsed_data = arib_parser_get_data(b24->parser,
> > +                                       &parsed_data_size);
> > +    if (!parsed_data || !parsed_data_size) {
> > +        av_log(avctx, AV_LOG_DEBUG, "No decode'able data was received from "
> > +                                    "packet (dts: %"PRId64", pts: %"PRId64").\n",
> > +               pkt->dts, pkt->pts);
> > +        return pkt->size;
> > +    }
> > +
> > +    decoded_subtitle_size = parsed_data_size * 4;
> > +    if (!(decoded_subtitle = av_mallocz(decoded_subtitle_size + 1))) {
> > +        av_log(avctx, AV_LOG_ERROR,
> > +               "Failed to allocate buffer for decoded subtitle!\n");
> > +        return AVERROR(ENOMEM);
> > +    }
> > +
> > +    decoded_subtitle_size = arib_decode_buffer(b24->decoder,
> > +                                               parsed_data,
> > +                                               parsed_data_size,
> > +                                               decoded_subtitle,
> > +                                               decoded_subtitle_size);
> > +
> > +    subtitle_duration = arib_decoder_get_time(b24->decoder);
> > +
> > +    if (avctx->pkt_timebase.num && pkt->pts != AV_NOPTS_VALUE)
> > +        sub->pts = av_rescale_q(pkt->pts,
> > +                                avctx->pkt_timebase, AV_TIME_BASE_Q);
> > +
> > +    sub->end_display_time = subtitle_duration ?
> > +                            av_rescale_q(subtitle_duration,
> > +                                         AV_TIME_BASE_Q,
> > +                                         (AVRational){1, 1000}) :
> > +                            UINT32_MAX;
> > +
> > +    av_log(avctx, AV_LOG_DEBUG,
> > +           "Result: '%s' (size: %zu, pkt_pts: %"PRId64", sub_pts: %"PRId64" "
> > +           "duration: %"PRIu32", pkt_timebase: %d/%d, time_base: %d/%d')\n",
> > +           decoded_subtitle ? decoded_subtitle : "<no subtitle>",
> > +           decoded_subtitle_size,
> > +           pkt->pts, sub->pts,
> > +           sub->end_display_time,
> > +           avctx->pkt_timebase.num, avctx->pkt_timebase.den,
> > +           avctx->time_base.num, avctx->time_base.den);
> > +
> > +    if (decoded_subtitle)
> > +        libaribb24_handle_regions(avctx, sub);
> > +
> > +    *got_sub_ptr = sub->num_rects > 0;
> > +
> > +    av_free(decoded_subtitle);
> > +
> > +    // flush the region buffers, otherwise the linked list keeps getting
> > +    // longer and longer...
> > +    arib_finalize_decoder(b24->decoder);
> > +
> > +    return pkt->size;
> > +}
> > +
> > +static void libaribb24_flush(AVCodecContext *avctx)
> > +{
> > +    Libaribb24Context *b24 = avctx->priv_data;
> > +    if (!(avctx->flags2 & AV_CODEC_FLAG2_RO_FLUSH_NOOP))
> > +        b24->read_order = 0;
> > +}
> > +
> > +#define OFFSET(x) offsetof(Libaribb24Context, x)
> > +#define SD AV_OPT_FLAG_SUBTITLE_PARAM | AV_OPT_FLAG_DECODING_PARAM
> > +static const AVOption options[] = {
> > +    { "aribb24-base-path", "set the base path for the libaribb24 library",
> > +      OFFSET(aribb24_base_path), AV_OPT_TYPE_STRING, { 0 }, 0, 0, SD },
> > +    { "aribb24-skip-ruby-text", "skip ruby text blocks during decoding",
> > +      OFFSET(aribb24_skip_ruby), AV_OPT_TYPE_BOOL, { 1 }, 0, 1, SD },
>                                                        ^^^
> This should state which type is set here , same for AV_OPT_TYPE_STRING
>

Yes, sorry. No idea how I missed that one for the default value. Will
post a fixup patch set in a moment.

Jan