On Mon, Feb 11, 2019 at 1:01 AM Michael Niedermayer <michae...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
