[FFmpeg-devel] [PATCH 1/2] avcodec/libaomenc: support setting chroma sample location

[Mark Thompson]

On 16/09/18 19:29, James Almer wrote:
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavcodec/libaomenc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index 6a79d9b873..55d50ded28 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -76,6 +76,7 @@ static const char *const ctlidstr[] = {
>      [AOME_SET_ENABLEAUTOALTREF] = "AOME_SET_ENABLEAUTOALTREF",
>      [AOME_SET_STATIC_THRESHOLD] = "AOME_SET_STATIC_THRESHOLD",
>      [AV1E_SET_COLOR_RANGE]      = "AV1E_SET_COLOR_RANGE",
> +    [AV1E_SET_CHROMA_SAMPLE_POSITION] = "AV1E_SET_CHROMA_SAMPLE_POSITION",
>      [AV1E_SET_COLOR_PRIMARIES]  = "AV1E_SET_COLOR_PRIMARIES",
>      [AV1E_SET_MATRIX_COEFFICIENTS] = "AV1E_SET_MATRIX_COEFFICIENTS",
>      [AV1E_SET_TRANSFER_CHARACTERISTICS] = "AV1E_SET_TRANSFER_CHARACTERISTICS",
> @@ -284,6 +285,22 @@ static void set_color_range(AVCodecContext *avctx)
>      codecctl_int(avctx, AV1E_SET_COLOR_RANGE, aom_cr);
>  }
>  
> +static void set_chroma_location(AVCodecContext *avctx)
> +{
> +    enum aom_chroma_sample_position aom_cps;
> +    switch (avctx->chroma_sample_location) {
> +    case AVCHROMA_LOC_UNSPECIFIED: aom_cps = AOM_CSP_UNKNOWN;   break;
> +    case AVCHROMA_LOC_LEFT:        aom_cps = AOM_CSP_VERTICAL;  break;
> +    case AVCHROMA_LOC_TOPLEFT:     aom_cps = AOM_CSP_COLOCATED; break;
> +    default:
> +        av_log(avctx, AV_LOG_WARNING, "Unsupported chroma sample location (%d)\n",
> +               avctx->chroma_sample_location);
> +        return;
> +    }
> +
> +    codecctl_int(avctx, AV1E_SET_CHROMA_SAMPLE_POSITION, aom_cps);
> +}

I think you should only set this if the input is 4:2:0, since the value is only used in that case.

> +
>  static av_cold int aom_init(AVCodecContext *avctx,
>                              const struct aom_codec_iface *iface)
>  {
> @@ -452,6 +469,7 @@ static av_cold int aom_init(AVCodecContext *avctx,
>      codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
>      codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
>      codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, avctx->color_trc);
> +    set_chroma_location(avctx);
>      set_color_range(avctx);
>  
>      // provide dummy value to initialize wrapper, values will be updated each _encode()
> 

Otherwise LGTM.

It's somewhat unfortunate that the obvious way of encoding a sequence of JPEGs will always hit the warning, but it is correct...

Thanks,

- Mark