[FFmpeg-devel] [vf_ass.c] 2 lines of code that need your attention

[Stefano Sabatini]

On date Thursday 2012-03-22 21:06:21 +0100, Nicolas George encoded:
> Le duodi 2 germinal, an CCXX, Stefano Sabatini a écrit :
> > And now I see that both VLC and MPlayer use a more complex logic...
> 
> As far as I can see, the logic for the font size in ASS is, in deductive
> order (v for virtual, p for pixels):
> 
> em_h_v = Fontsize * SclaleY/100
> em_h_p = Fontsize * SclaleY/100 * video_h / PlayResY
> em_w_p = Fontsize * SclaleX/100 * video_h / PlayResY
> em_w_v = Fontsize * SclaleX/100 * video_h / PlayResY * PlayResX / video_w
> 
> This works when video_w and video_h are the pixels dimensions of the video
> for which the ASS file has been conceived. Note that it completely
> disregards any kind of aspect ratio.
> 
> So, assuming the ASS file has been designed for the original unscaled video
> and we only scaled (no crop, no pad), we need an extra scaling on the X
> direction with ratio current_sar / original_sar = original_iar / current_iar
> (where iar is the aspect ratio in pixels, with dar = sar * iar).

OK so this is assuming that original_dar == current_dar.

> Now, reading the sources for libass, I find that ass_set_aspect_ratio takes
> two arguments: aspect and storage_aspect (and sar does not mean _sample_
> aspect!), which seems to be display aspects, and computes the extra X
> scaling as storage_aspect / aspect.
> 
> Therefore, the correct call would probably be:
> 
> ass_set_aspect_ratio(ass, current_iar, original_iar);
> 
> although the following should work too:
> 
> ass_set_aspect_ratio(ass, original_sar, current_sar);
> 
> It seems to be somewhat like what mplayer does.
> 

> Attached is a patch that does just that. I believe the dar option has not
> been around for enough time to make its removal a problem, especially since
> it did not work.

Sure, just remember to bump micro so we know where to point out when
users complain ;-).

> Regards,
> 
> -- 
>   Nicolas George

> From d8eedbcb25b7c12b7257e9d144caf7290fcabd50 Mon Sep 17 00:00:00 2001
> From: Nicolas George <nicolas.george at normalesup.org>
> Date: Thu, 22 Mar 2012 20:59:36 +0100
> Subject: [PATCH] ass: fix aspect ratio computation.
> 
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/filters.texi     |    7 ++++---
>  libavfilter/vf_ass.c |   23 +++++++++++------------
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index ac3e841..16b07e1 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -761,9 +761,10 @@ separated by ":".
>  A description of the accepted options follows.
>  
>  @table @option
> - at item dar
> -Specifies the display aspect ratio adopted for rendering the
> -subtitles. Default value is "1.0".
> + at item original_size
> +Specifies the size of the original video, the video for which the ASS file
> +was composed. Due to a misdesign in ASS aspect ratio arithmetic, this is
> +necessary to correctly scale the fonts if the aspect ratio has been changed.
>  @end table
>  
>  For example, to render the file @file{sub.ass} on top of the input
> diff --git a/libavfilter/vf_ass.c b/libavfilter/vf_ass.c
> index bf13287..1cf886b 100644
> --- a/libavfilter/vf_ass.c
> +++ b/libavfilter/vf_ass.c
> @@ -45,14 +45,14 @@ typedef struct {
>      char *filename;
>      uint8_t rgba_map[4];
>      int     pix_step[4];       ///< steps per pixel for each plane of the main output
> -    char *dar_str;
> -    AVRational dar;
> +    char *original_size_str;
> +    int original_w, original_h;
>  } AssContext;
>  
>  #define OFFSET(x) offsetof(AssContext, x)
>  
>  static const AVOption ass_options[] = {
> -    {"dar",  "set subtitles display aspect ratio", OFFSET(dar_str), AV_OPT_TYPE_STRING, {.str = "1.0"},  CHAR_MIN, CHAR_MAX },
> +    {"original_size",  "set the size of the original video (used to scale fonts)", OFFSET(original_size_str), AV_OPT_TYPE_STRING, {.str = NULL},  CHAR_MIN, CHAR_MAX },
>      {NULL},
>  };
>  
> @@ -107,10 +107,11 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
>          return ret;
>      }
>  
> -    if (av_parse_ratio(&ass->dar, ass->dar_str, 100, 0, ctx) < 0 ||
> -        ass->dar.num < 0 || ass->dar.den <= 0) {
> +    if (ass->original_size_str &&
> +        av_parse_video_size(&ass->original_w, &ass->original_h,
> +                            ass->original_size_str) < 0) {
>          av_log(ctx, AV_LOG_ERROR,
> -               "Invalid string '%s' or value for display aspect ratio.\n", ass->dar_str);
> +               "Invalid original size '%s'.\n", ass->original_size_str);
>          return AVERROR(EINVAL);
>      }
>  
> @@ -136,8 +137,6 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
>      }
>  
>      ass_set_fonts(ass->renderer, NULL, NULL, 1, NULL, 1);
> -
> -    av_log(ctx, AV_LOG_INFO, "dar:%f\n", av_q2d(ass->dar));
>      return 0;
>  }
>  
> @@ -146,7 +145,7 @@ static av_cold void uninit(AVFilterContext *ctx)
>      AssContext *ass = ctx->priv;
>  
>      av_freep(&ass->filename);
> -    av_freep(&ass->dar_str);
> +    av_freep(&ass->original_size_str);
>      if (ass->track)
>          ass_free_track(ass->track);
>      if (ass->renderer)
> @@ -173,8 +172,6 @@ static int config_input(AVFilterLink *inlink)
>  {
>      AssContext *ass = inlink->dst->priv;
>      const AVPixFmtDescriptor *pix_desc = &av_pix_fmt_descriptors[inlink->format];
> -    double sar = inlink->sample_aspect_ratio.num ?
> -        av_q2d(inlink->sample_aspect_ratio) : 1;
>  
>      av_image_fill_max_pixsteps(ass->pix_step, NULL, pix_desc);
>      ff_fill_rgba_map(ass->rgba_map, inlink->format);
> @@ -183,7 +180,9 @@ static int config_input(AVFilterLink *inlink)
>      ass->vsub = pix_desc->log2_chroma_h;
>  
>      ass_set_frame_size  (ass->renderer, inlink->w, inlink->h);
> -    ass_set_aspect_ratio(ass->renderer, av_q2d(ass->dar), sar);

> +    if (ass->original_w && ass->original_h)
> +        ass_set_aspect_ratio(ass->renderer, (double)inlink->w / inlink->h,
> +                             (double)ass->original_w / ass->original_h);
>  
>      return 0;
>  }

Looks good, and many thanks for investigating on the issue.
-- 
FFmpeg = Formidable and Fiendish Meaningful Plastic Exciting Gadget