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

On Thu, Nov 1, 2018 at 1:55 PM Valery Kot <valery.kot@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@4cinsights.com>
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(+)

On 01/11/18 13:24, Valery Kot wrote:
> On Thu, Nov 1, 2018 at 1:55 PM Valery Kot <valery.kot@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@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

On Mon, Nov 5, 2018 at 4:50 PM Mark Thompson <sw@jkqxz.net> wrote:
>
> LGTM, tested and applied.
>
> - Mark

Thanks!

Is it also possible to apply this patch on release/4.1 branch, so that
it will end up in the next release?

Valery

On Tue, Nov 6, 2018 at 9:02 AM Valery Kot <valery.kot@gmail.com> wrote:
>
> On Mon, Nov 5, 2018 at 4:50 PM Mark Thompson <sw@jkqxz.net> wrote:
> >
> > LGTM, tested and applied.
> >
> > - Mark
>
> Thanks!
>
> Is it also possible to apply this patch on release/4.1 branch, so that
> it will end up in the next release?
>

Unfortunately, its a tad bit late for that now. This is basically a
feature, and we don't backport features into stable branches once a
release has been made from them.

- Hendrik