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

* Outputs ASS lines with basic coloring and font scaling for each
  given region.
* Sets the default style to the resolution of the subtitle plane
  (for example, 960x540 / 36pt font for profile A).
* Has options to:
  * Disable ruby text (which is coded as regions which have
    half-height text in libaribb24).
    Enabled by default as without positioning ruby text only
    confuses as it is usually coded in the beginning of the decoded
    subtitle line.
  * Set the working directory, in which libaribb24 will read
    configuration as well as into which it may save broadcast extra
    symbols as PNG.
    Unset by default.

The unconventional library check can be explained by the library's
current master branch being licensed as LGPLv3, but at the time of
writing the latest official release is still licensed under GPLv3.

Thus, one either has to wait for the following release, or enable
GPLv3.
---

Version 3 changes:
  - Pre-converted subtitle region colors to BGR in local variables
    to make checks work even in cases where the default colors are not
    all 0xFF or 0x00-filled, and to use BGR values consistently.
  - Added extra parenthesis around the active parts of the RGB_TO_BGR
    macro to separate its content from the position it is used in.

Version 2 changes:
  - Corrected colors by adding a macro that converts from RGB to BGR,
    I had forgotten that ASS is not RGB.
    Thanks to a nice person from the .jp DTV community to have
    noticed this.

 Changelog               |   1 +
 configure               |   7 +
 doc/decoders.texi       |  25 +++
 libavcodec/Makefile     |   1 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/avcodec.h    |   4 +
 libavcodec/codec_desc.c |   9 +-
 libavcodec/libaribb24.c | 384 ++++++++++++++++++++++++++++++++++++++++
 libavcodec/profiles.c   |   6 +
 libavcodec/profiles.h   |   1 +
 libavcodec/version.h    |   2 +-
 11 files changed, 439 insertions(+), 2 deletions(-)
 create mode 100644 libavcodec/libaribb24.c

On Sun, Feb 03, 2019 at 09:38:40PM +0200, Jan Ekström wrote:
> * Outputs ASS lines with basic coloring and font scaling for each
>   given region.
> * Sets the default style to the resolution of the subtitle plane
>   (for example, 960x540 / 36pt font for profile A).
> * Has options to:
>   * Disable ruby text (which is coded as regions which have
>     half-height text in libaribb24).
>     Enabled by default as without positioning ruby text only
>     confuses as it is usually coded in the beginning of the decoded
>     subtitle line.
>   * Set the working directory, in which libaribb24 will read
>     configuration as well as into which it may save broadcast extra
>     symbols as PNG.
>     Unset by default.
> 
> The unconventional library check can be explained by the library's
> current master branch being licensed as LGPLv3, but at the time of
> writing the latest official release is still licensed under GPLv3.
> 
> Thus, one either has to wait for the following release, or enable
> GPLv3.
> ---
> 
> Version 3 changes:
>   - Pre-converted subtitle region colors to BGR in local variables
>     to make checks work even in cases where the default colors are not
>     all 0xFF or 0x00-filled, and to use BGR values consistently.
>   - Added extra parenthesis around the active parts of the RGB_TO_BGR
>     macro to separate its content from the position it is used in.
> 
> Version 2 changes:
>   - Corrected colors by adding a macro that converts from RGB to BGR,
>     I had forgotten that ASS is not RGB.
>     Thanks to a nice person from the .jp DTV community to have
>     noticed this.
> 
>  Changelog               |   1 +
>  configure               |   7 +
>  doc/decoders.texi       |  25 +++
>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h    |   4 +
>  libavcodec/codec_desc.c |   9 +-
>  libavcodec/libaribb24.c | 384 ++++++++++++++++++++++++++++++++++++++++
>  libavcodec/profiles.c   |   6 +
>  libavcodec/profiles.h   |   1 +
>  libavcodec/version.h    |   2 +-
>  11 files changed, 439 insertions(+), 2 deletions(-)
>  create mode 100644 libavcodec/libaribb24.c
> 
> diff --git a/Changelog b/Changelog
> index 06cf6bf080..d5515c4911 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -17,6 +17,7 @@ version <next>:
>  - maskfun filter
>  - hcom demuxer and decoder
>  - ARBC decoder
> +- libaribb24 based ARIB STD-B24 caption support (profiles A and C)
>  
>  
>  version 4.1:
> diff --git a/configure b/configure
> index e1412352fa..cb852c6289 100755
> --- a/configure
> +++ b/configure
> @@ -218,6 +218,7 @@ External library support:
>    --enable-jni             enable JNI support [no]
>    --enable-ladspa          enable LADSPA audio filtering [no]
>    --enable-libaom          enable AV1 video encoding/decoding via libaom [no]
> +  --enable-libaribb24      enable ARIB text and caption decoding via libaribb24 [no]
>    --enable-libass          enable libass subtitles rendering,
>                             needed for subtitles and ass filter [no]
>    --enable-libbluray       enable BluRay reading using libbluray [no]
> @@ -1684,6 +1685,7 @@ EXTERNAL_LIBRARY_NONFREE_LIST="
>  EXTERNAL_LIBRARY_VERSION3_LIST="
>      gmp
>      liblensfun
> +    libaribb24
>      libopencore_amrnb
>      libopencore_amrwb
>      libvmaf
> @@ -1707,6 +1709,7 @@ EXTERNAL_LIBRARY_LIST="
>      jni
>      ladspa
>      libaom
> +    libaribb24
>      libass
>      libbluray
>      libbs2b
> @@ -3094,6 +3097,7 @@ hevc_videotoolbox_encoder_select="videotoolbox_encoder"
>  libaom_av1_decoder_deps="libaom"
>  libaom_av1_encoder_deps="libaom"
>  libaom_av1_encoder_select="extract_extradata_bsf"
> +libaribb24_decoder_deps="libaribb24"
>  libcelt_decoder_deps="libcelt"
>  libcodec2_decoder_deps="libcodec2"
>  libcodec2_encoder_deps="libcodec2"
> @@ -6080,6 +6084,9 @@ enabled gnutls            && require_pkg_config gnutls gnutls gnutls/gnutls.h gn
>  enabled jni               && { [ $target_os = "android" ] && check_headers jni.h && enabled pthreads || die "ERROR: jni not found"; }
>  enabled ladspa            && require_headers "ladspa.h dlfcn.h"
>  enabled libaom            && require_pkg_config libaom "aom >= 1.0.0" aom/aom_codec.h aom_codec_version
> +enabled libaribb24        && { check_pkg_config libaribb24 "aribb24 > 1.0.3" "aribb24/aribb24.h" arib_instance_new ||
> +                               { enabled gpl && require_pkg_config libaribb24 aribb24 "aribb24/aribb24.h" arib_instance_new; } ||
> +                               die "ERROR: libaribb24 requires version higher than 1.0.3 or --enable-gpl."; }
>  enabled lv2               && require_pkg_config lv2 lilv-0 "lilv/lilv.h" lilv_world_new
>  enabled libiec61883       && require libiec61883 libiec61883/iec61883.h iec61883_cmp_connect -lraw1394 -lavc1394 -lrom1394 -liec61883
>  enabled libass            && require_pkg_config libass libass ass/ass.h ass_library_init
> diff --git a/doc/decoders.texi b/doc/decoders.texi
> index 25187e30f1..704bd60b9f 100644
> --- a/doc/decoders.texi
> +++ b/doc/decoders.texi
> @@ -194,6 +194,31 @@ without this library.
>  @chapter Subtitles Decoders
>  @c man begin SUBTILES DECODERS
>  
> +@section libaribb24
> +
> +ARIB STD-B24 caption decoder.
> +
> +Implements profiles A and C of the ARIB STD-B24 standard.
> +
> +@subsection libaribb24 Decoder Options
> +
> +@table @option
> +
> +@item -aribb24-base-path @var{path}
> +Sets the base path for the libaribb24 library. This is utilized for reading of
> +configuration files (for custom unicode conversions), and for dumping of
> +non-text symbols as images under that location.
> +
> +Unset by default.
> +
> +@item -aribb24-skip-ruby-text @var{boolean}
> +Tells the decoder wrapper to skip text blocks that contain half-height ruby
> +text.
> +
> +Enabled by default.
> +
> +@end table
> +
>  @section dvbsub
>  
>  @subsection Options
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 8e240aecf0..21c39281e4 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -957,6 +957,7 @@ OBJS-$(CONFIG_PCM_ALAW_AT_ENCODER)        += audiotoolboxenc.o
>  OBJS-$(CONFIG_PCM_MULAW_AT_ENCODER)       += audiotoolboxenc.o
>  OBJS-$(CONFIG_LIBAOM_AV1_DECODER)         += libaomdec.o
>  OBJS-$(CONFIG_LIBAOM_AV1_ENCODER)         += libaomenc.o
> +OBJS-$(CONFIG_LIBARIBB24_DECODER)         += libaribb24.o ass.o
>  OBJS-$(CONFIG_LIBCELT_DECODER)            += libcelt_dec.o
>  OBJS-$(CONFIG_LIBCODEC2_DECODER)          += libcodec2.o codec2utils.o
>  OBJS-$(CONFIG_LIBCODEC2_ENCODER)          += libcodec2.o codec2utils.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 5cbb09a5a4..b26aeca239 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -677,6 +677,7 @@ extern AVCodec ff_qdmc_at_decoder;
>  extern AVCodec ff_qdm2_at_decoder;
>  extern AVCodec ff_libaom_av1_decoder;
>  extern AVCodec ff_libaom_av1_encoder;
> +extern AVCodec ff_libaribb24_decoder;
>  extern AVCodec ff_libcelt_decoder;
>  extern AVCodec ff_libcodec2_encoder;
>  extern AVCodec ff_libcodec2_decoder;
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index f554c53f0e..0ce22ec4fa 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -677,6 +677,7 @@ enum AVCodecID {
>      AV_CODEC_ID_ASS,
>      AV_CODEC_ID_HDMV_TEXT_SUBTITLE,
>      AV_CODEC_ID_TTML,
> +    AV_CODEC_ID_ARIB_CAPTION,
>  
>      /* other specific kind of codecs (generally used for attachments) */
>      AV_CODEC_ID_FIRST_UNKNOWN = 0x18000,           ///< A dummy ID pointing at the start of various fake codecs.
> @@ -2993,6 +2994,9 @@ typedef struct AVCodecContext {
>  #define FF_PROFILE_PRORES_4444      4
>  #define FF_PROFILE_PRORES_XQ        5
>  
> +#define FF_PROFILE_ARIB_PROFILE_A 0
> +#define FF_PROFILE_ARIB_PROFILE_C 1
> +
>      /**
>       * level
>       * - encoding: Set by user.
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 7b254c1d45..a3de8e1c2b 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -3133,7 +3133,14 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .long_name = NULL_IF_CONFIG_SMALL("Timed Text Markup Language"),
>          .props     = AV_CODEC_PROP_TEXT_SUB,
>      },
> -
> +    {
> +        .id        = AV_CODEC_ID_ARIB_CAPTION,
> +        .type      = AVMEDIA_TYPE_SUBTITLE,
> +        .name      = "arib_caption",
> +        .long_name = NULL_IF_CONFIG_SMALL("ARIB STD-B24 caption"),
> +        .props     = AV_CODEC_PROP_TEXT_SUB,
> +        .profiles  = NULL_IF_CONFIG_SMALL(ff_arib_caption_profiles),
> +    },
>  
>      /* other kind of codecs and pseudo-codecs */
>      {
> diff --git a/libavcodec/libaribb24.c b/libavcodec/libaribb24.c
> new file mode 100644
> index 0000000000..bc0255286c
> --- /dev/null
> +++ b/libavcodec/libaribb24.c
> @@ -0,0 +1,384 @@
> +/*
> + * ARIB STD-B24 caption decoder using the libaribb24 library
> + * Copyright (c) 2019 Jan Ekström
> + *
> + * 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 "avcodec.h"
> +#include "libavcodec/ass.h"
> +#include "libavutil/log.h"
> +#include "libavutil/opt.h"
> +
> +#include <aribb24/aribb24.h>
> +#include <aribb24/parser.h>
> +#include <aribb24/decoder.h>
> +
> +typedef struct Libaribb24Context {
> +    AVClass *class;
> +
> +    arib_instance_t *lib_instance;
> +    arib_parser_t *parser;
> +    arib_decoder_t *decoder;
> +
> +    int read_order;
> +
> +    char        *aribb24_base_path;
> +    unsigned int aribb24_skip_ruby;
> +} Libaribb24Context;
> +
> +static unsigned int get_profile_font_size(int profile)
> +{
> +    switch (profile) {
> +    case FF_PROFILE_ARIB_PROFILE_A:
> +        return 36;
> +    case FF_PROFILE_ARIB_PROFILE_C:
> +        return 18;
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static void libaribb24_log(void *p, const char *msg)
> +{
> +    av_log((AVCodecContext *)p, AV_LOG_INFO, "%s\n", msg);
> +}
> +
> +static int libaribb24_generate_ass_header(AVCodecContext *avctx)
> +{
> +    unsigned int plane_width = 0;
> +    unsigned int plane_height = 0;
> +    unsigned int font_size = 0;
> +
> +    switch (avctx->profile) {
> +    case FF_PROFILE_ARIB_PROFILE_A:
> +        plane_width = 960;
> +        plane_height = 540;
> +        font_size = get_profile_font_size(avctx->profile);
> +        break;
> +    case FF_PROFILE_ARIB_PROFILE_C:
> +        plane_width = 320;
> +        plane_height = 180;
> +        font_size = get_profile_font_size(avctx->profile);
> +        break;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "Unknown or unsupported profile set!\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    avctx->subtitle_header = av_asprintf(
> +             "[Script Info]\r\n"
> +             "; Script generated by FFmpeg/Lavc%s\r\n"
> +             "ScriptType: v4.00+\r\n"
> +             "PlayResX: %d\r\n"
> +             "PlayResY: %d\r\n"
> +             "\r\n"
> +             "[V4+ Styles]\r\n"
> +
> +             /* ASSv4 header */
> +             "Format: Name, "
> +             "Fontname, Fontsize, "
> +             "PrimaryColour, SecondaryColour, OutlineColour, BackColour, "
> +             "Bold, Italic, Underline, StrikeOut, "
> +             "ScaleX, ScaleY, "
> +             "Spacing, Angle, "
> +             "BorderStyle, Outline, Shadow, "
> +             "Alignment, MarginL, MarginR, MarginV, "
> +             "Encoding\r\n"
> +
> +             "Style: "
> +             "Default,"             /* Name */
> +             "%s,%d,"               /* Font{name,size} */
> +             "&H%x,&H%x,&H%x,&H%x," /* {Primary,Secondary,Outline,Back}Colour */
> +             "%d,%d,%d,0,"          /* Bold, Italic, Underline, StrikeOut */
> +             "100,100,"             /* Scale{X,Y} */
> +             "0,0,"                 /* Spacing, Angle */
> +             "%d,1,0,"              /* BorderStyle, Outline, Shadow */
> +             "%d,10,10,10,"         /* Alignment, Margin[LRV] */
> +             "0\r\n"                /* Encoding */
> +
> +             "\r\n"
> +             "[Events]\r\n"
> +             "Format: Layer, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text\r\n",
> +             !(avctx->flags & AV_CODEC_FLAG_BITEXACT) ? AV_STRINGIFY(LIBAVCODEC_VERSION) : "",
> +             plane_width, plane_height,
> +             ASS_DEFAULT_FONT, font_size, ASS_DEFAULT_COLOR,
> +             ASS_DEFAULT_COLOR, ASS_DEFAULT_BACK_COLOR, ASS_DEFAULT_BACK_COLOR,
> +             -ASS_DEFAULT_BOLD, -ASS_DEFAULT_ITALIC, -ASS_DEFAULT_UNDERLINE,
> +             ASS_DEFAULT_BORDERSTYLE, ASS_DEFAULT_ALIGNMENT);
> +
> +    if (!avctx->subtitle_header)
> +        return AVERROR(ENOMEM);
> +
> +    avctx->subtitle_header_size = strlen(avctx->subtitle_header);
> +
> +    return 0;
> +}
> +
> +static int libaribb24_init(AVCodecContext *avctx)
> +{
> +    Libaribb24Context *b24 = avctx->priv_data;
> +    void(* arib_dec_init)(arib_decoder_t* decoder) = NULL;
> +    int ret_code = AVERROR_EXTERNAL;
> +
> +    if (!(b24->lib_instance = arib_instance_new(avctx))) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to initialize libaribb24!\n");
> +        goto init_fail;
> +    }
> +
> +    if (b24->aribb24_base_path) {
> +        av_log(avctx, AV_LOG_INFO, "Setting the libaribb24 base path to '%s'\n",
> +               b24->aribb24_base_path);
> +        arib_set_base_path(b24->lib_instance, b24->aribb24_base_path);
> +    }
> +
> +    arib_register_messages_callback(b24->lib_instance, libaribb24_log);
> +
> +    if (!(b24->parser = arib_get_parser(b24->lib_instance))) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to initialize libaribb24 PES parser!\n");
> +        goto init_fail;
> +    }
> +    if (!(b24->decoder = arib_get_decoder(b24->lib_instance))) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to initialize libaribb24 decoder!\n");
> +        goto init_fail;
> +    }
> +
> +    switch (avctx->profile) {
> +    case FF_PROFILE_ARIB_PROFILE_A:
> +        arib_dec_init = arib_initialize_decoder_a_profile;
> +        break;
> +    case FF_PROFILE_ARIB_PROFILE_C:
> +        arib_dec_init = arib_initialize_decoder_c_profile;
> +        break;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "Unknown or unsupported profile set!\n");
> +        ret_code = AVERROR(EINVAL);
> +        goto init_fail;
> +    }
> +
> +    arib_dec_init(b24->decoder);
> +
> +    if (libaribb24_generate_ass_header(avctx) < 0) {
> +        ret_code = AVERROR(ENOMEM);
> +        goto init_fail;
> +    }
> +
> +    return 0;
> +
> +init_fail:
> +    if (b24->decoder)
> +        arib_finalize_decoder(b24->decoder);
> +
> +    if (b24->lib_instance)
> +        arib_instance_destroy(b24->lib_instance);
> +
> +    return ret_code;
> +}
> +
> +static int libaribb24_close(AVCodecContext *avctx)
> +{
> +    Libaribb24Context *b24 = avctx->priv_data;
> +
> +    if (b24->decoder)
> +        arib_finalize_decoder(b24->decoder);
> +
> +    if (b24->lib_instance)
> +        arib_instance_destroy(b24->lib_instance);
> +
> +    return 0;
> +}
> +

> +#define RGB_TO_BGR(c) ((c & 0xff) << 16 | (c & 0xff00) | ((c >> 16) & 0xff))

c should be protected by a ()


> +
> +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



> +        }
> +
> +        // 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 ?


> +    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

[...]

On Mon, Feb 11, 2019 at 1:01 AM Michael Niedermayer <michaelni@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