[FFmpeg-devel] [PATCH] avcodec/libopenh264enc.c: Handle sample_aspect_ratio in libopenh264 encoder

[Mark Thompson]

On 12/10/18 08:14, Valery Kot wrote:
> When using libx264 (GPL) encoder, sample aspect ratio gets stored on
> both container and frame levels. For libopenh264 (LGPL) encoder,
> aspect ratio on codec/frame level currently is ignored, which results
> in weird display aspect ratio for non-square pixels on some players.
> 
> Proposed patch fixes that, if FFmpeg being build against libopenh264
> 1.7 or newer.
> 
> From 2d22329e01b892576b856806c93d484c304f11d8 Mon Sep 17 00:00:00 2001
> From: vkot <valery.kot at 4cinsights.com>
> Date: Fri, 12 Oct 2018 09:02:59 +0200
> Subject: [PATCH] Handle sample_aspect_ratio in libopenh264 encoder
> 
> ---
>  libavcodec/libopenh264enc.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index 83c3f0ce20..c630dff33c 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -164,6 +164,32 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      param.sSpatialLayers[0].iSpatialBitrate     = param.iTargetBitrate;
>      param.sSpatialLayers[0].iMaxSpatialBitrate  = param.iMaxBitrate;
>  
> +#if OPENH264_VER_AT_LEAST(1, 7)
> +    if(avctx->sample_aspect_ratio.num == 0 || avctx->sample_aspect_ratio.den == 0)
> +        param.sSpatialLayers[0].bAspectRatioPresent = false;
> +    else {

Reduce the fraction to have numerator/denominator fitting in 16 bits before doing the following - if libopenh264 doesn't handle the aspect_ratio_idc values itself then I doubt it does the reduction needed for the extended values.

> +        param.sSpatialLayers[0].bAspectRatioPresent = true;
> +        if      (!av_cmp_q(av_make_q( 1,  1), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_1x1;
> +        else if (!av_cmp_q(av_make_q(12, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_12x11;
> +        else if (!av_cmp_q(av_make_q(10, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_10x11;
> +        else if (!av_cmp_q(av_make_q(16, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_16x11;
> +        else if (!av_cmp_q(av_make_q(40, 33), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_40x33;
> +        else if (!av_cmp_q(av_make_q(24, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_24x11;
> +        else if (!av_cmp_q(av_make_q(20, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_20x11;
> +        else if (!av_cmp_q(av_make_q(32, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_32x11;
> +        else if (!av_cmp_q(av_make_q(80, 33), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_80x33;
> +        else if (!av_cmp_q(av_make_q(18, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_18x11;
> +        else if (!av_cmp_q(av_make_q(15, 11), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_15x11;
> +        else if (!av_cmp_q(av_make_q(64, 33), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_64x33;
> +        else if (!av_cmp_q(av_make_q(160,99), avctx->sample_aspect_ratio))  param.sSpatialLayers[0].eAspectRatio = ASP_160x99;

I think this would look nicer (and generate less code) as a table + loop rather than an if-ladder making each fraction structure inline?

E.g. something like <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/h264_metadata_bsf.c;h=bf37528234c7ab8bac56f773ad99f5a67f79db29;hb=HEAD#l95>.

> +        else {
> +            param.sSpatialLayers[0].eAspectRatio = ASP_EXT_SAR;
> +            param.sSpatialLayers[0].sAspectRatioExtWidth = avctx->sample_aspect_ratio.num;
> +            param.sSpatialLayers[0].sAspectRatioExtHeight = avctx->sample_aspect_ratio.den;
> +        }
> +    }
> +#endif
> +
>      if ((avctx->slices > 1) && (s->max_nal_size)) {
>          av_log(avctx, AV_LOG_ERROR,
>                 "Invalid combination -slices %d and -max_nal_size %d.\n",
> -- 
> 2.15.1.windows.2
> 

Thanks,

- Mark