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

[Mark Thompson]

On 01/11/18 13:24, Valery Kot wrote:
> On Thu, Nov 1, 2018 at 1:55 PM Valery Kot <valery.kot at gmail.com> wrote:
>>
>>> 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>.
>>
>> Thanks for the suggestion! Here is an updated patch.
> 
> Was too hurry to send a patch, sorry! Here is the correct one.
> 
> 
> From 638277354338bf42020854e5bebec5fe61677135 Mon Sep 17 00:00:00 2001
> From: vkot <valery.kot at 4cinsights.com>

You might want to fix up this name - I corrected it manually.

> Date: Thu, 1 Nov 2018 14:15:11 +0100
> Subject: [PATCH] Handle sample_aspect_ratio in libopenh264-encoder
> 
> ---
>  libavcodec/libopenh264enc.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index 83c3f0ce20..b3ddb4609b 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -164,6 +164,47 @@ 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 && avctx->sample_aspect_ratio.den) {
> +        // Table E-1.
> +        static const AVRational sar_idc[] = {
> +            {   0,  0 }, // Unspecified (never written here).
> +            {   1,  1 }, {  12, 11 }, {  10, 11 }, {  16, 11 },
> +            {  40, 33 }, {  24, 11 }, {  20, 11 }, {  32, 11 },
> +            {  80, 33 }, {  18, 11 }, {  15, 11 }, {  64, 33 },
> +            { 160, 99 }, // Last 3 are unknown to openh264: {   4,  3 }, {   3,  2 }, {   2,  1 },
> +        };
> +        static const ESampleAspectRatio asp_idc[] = {
> +            ASP_UNSPECIFIED,
> +            ASP_1x1,      ASP_12x11,   ASP_10x11,   ASP_16x11,
> +            ASP_40x33,    ASP_24x11,   ASP_20x11,   ASP_32x11,
> +            ASP_80x33,    ASP_18x11,   ASP_15x11,   ASP_64x33,
> +            ASP_160x99,
> +        };
> +        int num, den, i;
> +    
> +        av_reduce(&num, &den, avctx->sample_aspect_ratio.num,
> +                  avctx->sample_aspect_ratio.den, 65535);
> +    
> +        for (i = 1; i < FF_ARRAY_ELEMS(sar_idc); i++) {
> +            if (num == sar_idc[i].num &&
> +                den == sar_idc[i].den)
> +                break;
> +        }
> +        if (i == FF_ARRAY_ELEMS(sar_idc)) {
> +            param.sSpatialLayers[0].eAspectRatio = ASP_EXT_SAR;
> +            param.sSpatialLayers[0].sAspectRatioExtWidth = num;
> +            param.sSpatialLayers[0].sAspectRatioExtHeight = den;
> +        } else {
> +            param.sSpatialLayers[0].eAspectRatio = asp_idc[i];
> +        }
> +        param.sSpatialLayers[0].bAspectRatioPresent = true;
> +    }
> +    else {
> +        param.sSpatialLayers[0].bAspectRatioPresent = false;
> +    }
> +#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
> 

LGTM, tested and applied.

(Seems libopenh264 accepts any value in this field, so you can actually just use the later values they don't provide in the enum.  Not sure whether that's actually an API violation which they might reject in some cases, though...)

Thanks!

- Mark