[FFmpeg-devel,v2,1/3] libavutil: introduce AVFilmGrainParams side data

This patch introduces a new frame side data type AVFilmGrainParams for use 
with video codecs which are able to use it.

It is generalized rather than being AV1 specific as AV2 is expected to carry
the same data, as well as the fact there already exist rarely-used specifications
for both H.264 and HEVC.

Patch attached.
Subject: [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data

This patch introduces a new frame side data type AVFilmGrainParams for use
with video codecs which are able to use it.

It is generalized rather than being AV1 specific as AV2 is expected to carry
the same data, as well as the fact there already exist rarely-used specifications
for both H.264 and HEVC.
---
 doc/APIchanges                |   4 +
 libavutil/Makefile            |   2 +
 libavutil/film_grain_params.c |  42 +++++++++
 libavutil/film_grain_params.h | 159 ++++++++++++++++++++++++++++++++++
 libavutil/frame.c             |   1 +
 libavutil/frame.h             |   6 ++
 libavutil/version.h           |   2 +-
 7 files changed, 215 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/film_grain_params.c
 create mode 100644 libavutil/film_grain_params.h

On 11/12/2020 2:42 PM, Lynne wrote:
> This patch introduces a new frame side data type AVFilmGrainParams for use
> with video codecs which are able to use it.
> 
> It is generalized rather than being AV1 specific as AV2 is expected to carry
> the same data, as well as the fact there already exist rarely-used specifications
> for both H.264 and HEVC.
> 
> Patch attached.

> From 522e3a80f44129c98f0c564d44815dbe6a740568 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Thu, 12 Nov 2020 12:44:30 +0100
> Subject: [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data
> 
> This patch introduces a new frame side data type AVFilmGrainParams for use
> with video codecs which are able to use it.
> 
> It is generalized rather than being AV1 specific as AV2 is expected to carry
> the same data, as well as the fact there already exist rarely-used specifications
> for both H.264 and HEVC.
> ---
>  doc/APIchanges                |   4 +
>  libavutil/Makefile            |   2 +
>  libavutil/film_grain_params.c |  42 +++++++++
>  libavutil/film_grain_params.h | 159 ++++++++++++++++++++++++++++++++++
>  libavutil/frame.c             |   1 +
>  libavutil/frame.h             |   6 ++
>  libavutil/version.h           |   2 +-
>  7 files changed, 215 insertions(+), 1 deletion(-)
>  create mode 100644 libavutil/film_grain_params.c
>  create mode 100644 libavutil/film_grain_params.h
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index b70c78a483..41248724d9 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-xx-xx - xxxxxxxxxx - lavu 56.61.100 - film_grain_params.h
> +  Adds a new API for extracting codec film grain parameters as side data.
> +  Adds a new AVFrameSideDataType entry AV_FRAME_DATA_FILM_GRAIN_PARAMS for it.
> +
>  2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h
>    Add AVSTREAM_EVENT_FLAG_NEW_PACKETS.
>  
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 9b08372eb2..27bafe9e12 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -84,6 +84,7 @@ HEADERS = adler32.h                                                     \
>            xtea.h                                                        \
>            tea.h                                                         \
>            tx.h                                                          \
> +          film_grain_params.h                                           \
>  
>  HEADERS-$(CONFIG_LZO)                   += lzo.h
>  
> @@ -170,6 +171,7 @@ OBJS = adler32.o                                                        \
>         tx_double.o                                                      \
>         tx_int32.o                                                       \
>         video_enc_params.o                                               \
> +       film_grain_params.o                                              \
>  
>  
>  OBJS-$(CONFIG_CUDA)                     += hwcontext_cuda.o
> diff --git a/libavutil/film_grain_params.c b/libavutil/film_grain_params.c
> new file mode 100644
> index 0000000000..930d23c7fe
> --- /dev/null
> +++ b/libavutil/film_grain_params.c
> @@ -0,0 +1,42 @@
> +/**
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "film_grain_params.h"
> +
> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size)
> +{
> +    AVFilmGrainParams *params = av_mallocz(sizeof(AVFilmGrainParams));
> +
> +    if (size)
> +        *size = sizeof(*params);
> +
> +    return params;
> +}
> +
> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame)
> +{
> +    AVFrameSideData *side_data = av_frame_new_side_data(frame,
> +                                                        AV_FRAME_DATA_FILM_GRAIN_PARAMS,
> +                                                        sizeof(AVFilmGrainParams));
> +    if (!side_data)
> +        return NULL;
> +
> +    memset(side_data->data, 0, sizeof(AVFilmGrainParams));
> +
> +    return (AVFilmGrainParams *)side_data->data;
> +}
> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h
> new file mode 100644
> index 0000000000..ba20fe7fa6
> --- /dev/null
> +++ b/libavutil/film_grain_params.h
> @@ -0,0 +1,159 @@
> +/*
> + * Copyright (c) 2016 Neil Birkbeck <neil.birkbeck@gmail.com>

Copy paste mistake?

> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVUTIL_FILM_GRAIN_PARAMS_H
> +#define AVUTIL_FILM_GRAIN_PARAMS_H
> +
> +#include "frame.h"
> +
> +enum AVFilmGrainParamsType {
> +    AV_FILM_GRAM_PARAMS_NONE = 0,
> +
> +    /**
> +     * The union is valid when interpreted as AVFilmGrainAOMParams (codec.aom)
> +     */
> +    AV_FILM_GRAM_PARAMS_AV1,
> +};
> +
> +typedef struct AVFilmGrainAOMParams {

If you believe AV2 will have a compatible implementation, then this name 
is fine.

> +    /**
> +     * Number of points for the piecewise linear scaling function of the
> +     * luma plane.
> +     */
> +    int num_y_points;
> +
> +    /**
> +     * Piecewise scaling function values and scale for each point.
> +     */
> +    uint8_t y_points[14][2 /* value, scaling */];
> +
> +    /**
> +     * Signals whether to derive the chroma scaling function from the luma.
> +     * Not equivalent to copying the luma values and scales.
> +     */
> +    int chroma_scaling_from_luma;
> +
> +    /**
> +     * If chroma_scaling_from_luma is set to 0, signals the chroma scaling
> +     * function parameters.
> +     */
> +    int num_uv_points[2];
> +    uint8_t uv_points[2][10][2 /* value, scaling */];
> +
> +    /**
> +     * Specifies the shift applied to the chroma components. For AV1, its within
> +     * [8; 11] and determines the range and quantization of the film grain.
> +     */
> +    int scaling_shift;
> +
> +    /**
> +     * Specifies the auto-regression coefficients for both luma and chroma.
> +     */
> +    int ar_coeff_lag;
> +
> +    /**
> +     * Luma auto-regression coefficients.
> +     */
> +    int8_t ar_coeffs_y[24];
> +
> +    /**
> +     * Chroma auto-regression coefficients. Padded to help with SIMD, last
> +     * 3 values of each chroma coeff are not used.
> +     */
> +    int8_t ar_coeffs_uv[2][25 + 3 /* padding for alignment purposes */];

Shouldn't the compiler take care of alignment?

> +
> +    /**
> +     * Specifies the range of the auto-regressive coefficients. Values of 6,
> +     * 7, 8 and so on represent a range of [-2, 2), [-1, 1), [-0.5, 0.5) and
> +     * so on. For AV1 must be between 6 and 9.
> +     */
> +    uint64_t ar_coeff_shift;

Why uint64_t? Sounds like it may be excessive for this.

> +
> +    /**
> +     * Signals the down shift applied to the generated gaussian numbers during
> +     * synthesis.
> +     */
> +    int grain_scale_shift;
> +
> +    /**
> +     * Specifies the luma/chroma multipliers for the index to the component
> +     * scaling function.
> +     */
> +    int uv_mult[2];
> +    int uv_mult_luma[2];

int8_t, like ar_coeffs_y. Or could AV2 increase the range considerably?

> +
> +    /**
> +     * Offset used for component scaling function. For AV1 its a 9-bit value
> +     * with a range [-256, 255] */
> +    int uv_offset[2];

int16_t.

> +
> +    /**
> +     * Signals whether to overlap film grain blocks.
> +     */
> +    int overlap_flag;
> +} AVFilmGrainAOMParams;
> +
> +typedef struct AVFilmGrainParams {
> +    /**
> +     * Specifies the codec for which this structure is valid.
> +     */
> +    enum AVFilmGrainParamsType type;
> +
> +    /**
> +     * Seed to use for the synthesis process, if the codec allows for it.
> +     */
> +    uint32_t seed;

Maybe uint64_t, just in case. This is meant to apply to all implementations.

Looks good otherwise, but probably wait for comments from other devs.

> +
> +    /**
> +     * Signals to clip to limited color levels after film grain application
> +     * for AV1.
> +     */
> +    int limit_output_range;
> +
> +    /**
> +     * Additional fields may be added both here and in any structure included.
> +     * If a codec's film grain structure differs slightly over another
> +     * codec's, fields within may change meaning depending on the type.
> +     */
> +    union {
> +        AVFilmGrainAOMParams aom;
> +    } codec;
> +} AVFilmGrainParams;
> +
> +/**
> + * Allocate an AVFilmGrainParams structure and set its fields to
> + * default values. The resulting struct can be freed using av_freep().
> + * If size is not NULL it will be set to the number of bytes allocated.
> + *
> + * @return An AVFilmGrainParams filled with default values or NULL
> + *         on failure.
> + */
> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size);
> +
> +/**
> + * Allocate a complete AVFilmGrainParams and add it to the frame.
> + *
> + * @param frame The frame which side data is added to.
> + *
> + * @return The AVFilmGrainParams structure to be filled by caller.
> + */
> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame);
> +
> +#endif /* AVUTIL_FILM_GRAIN_PARAMS_H */
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 3ab1aa3242..668a0e9e3c 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -864,6 +864,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>      case AV_FRAME_DATA_REGIONS_OF_INTEREST: return "Regions Of Interest";
>      case AV_FRAME_DATA_VIDEO_ENC_PARAMS:            return "Video encoding parameters";
>      case AV_FRAME_DATA_SEI_UNREGISTERED:            return "H.26[45] User Data Unregistered SEI message";
> +    case AV_FRAME_DATA_FILM_GRAIN_PARAMS:           return "Film grain parameters";
>      }
>      return NULL;
>  }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3bd240fc97..392315f40f 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -192,6 +192,12 @@ enum AVFrameSideDataType {
>       * uuid_iso_iec_11578 followed by AVFrameSideData.size - 16 bytes of user_data_payload_byte.
>       */
>      AV_FRAME_DATA_SEI_UNREGISTERED,
> +
> +    /**
> +     * Film grain parameters for a frame, described by AVFilmGrainParameters.
> +     * Must be present for every frame which should have film grain applied.
> +     */
> +    AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>  };
>  
>  enum AVActiveFormatDescription {
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 1aca823650..55a59d6c58 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  56
> -#define LIBAVUTIL_VERSION_MINOR  60
> +#define LIBAVUTIL_VERSION_MINOR  61
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> -- 
> 2.29.2
>

Nov 12, 2020, 19:46 by jamrial@gmail.com:

> On 11/12/2020 2:42 PM, Lynne wrote:
>
>> This patch introduces a new frame side data type AVFilmGrainParams for use
>> with video codecs which are able to use it.
>>
>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>> the same data, as well as the fact there already exist rarely-used specifications
>> for both H.264 and HEVC.
>>
>> Patch attached.
>>
>> From 522e3a80f44129c98f0c564d44815dbe6a740568 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Thu, 12 Nov 2020 12:44:30 +0100
>> Subject: [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data
>>
>> This patch introduces a new frame side data type AVFilmGrainParams for use
>> with video codecs which are able to use it.
>>
>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>> the same data, as well as the fact there already exist rarely-used specifications
>> for both H.264 and HEVC.
>> ---
>>  doc/APIchanges                |   4 +
>>  libavutil/Makefile            |   2 +
>>  libavutil/film_grain_params.c |  42 +++++++++
>>  libavutil/film_grain_params.h | 159 ++++++++++++++++++++++++++++++++++
>>  libavutil/frame.c             |   1 +
>>  libavutil/frame.h             |   6 ++
>>  libavutil/version.h           |   2 +-
>>  7 files changed, 215 insertions(+), 1 deletion(-)
>>  create mode 100644 libavutil/film_grain_params.c
>>  create mode 100644 libavutil/film_grain_params.h
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index b70c78a483..41248724d9 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>  API changes, most recent first:
>>  +2020-xx-xx - xxxxxxxxxx - lavu 56.61.100 - film_grain_params.h
>> +  Adds a new API for extracting codec film grain parameters as side data.
>> +  Adds a new AVFrameSideDataType entry AV_FRAME_DATA_FILM_GRAIN_PARAMS for it.
>> +
>>  2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h
>>  Add AVSTREAM_EVENT_FLAG_NEW_PACKETS.
>>  diff --git a/libavutil/Makefile b/libavutil/Makefile
>> index 9b08372eb2..27bafe9e12 100644
>> --- a/libavutil/Makefile
>> +++ b/libavutil/Makefile
>> @@ -84,6 +84,7 @@ HEADERS = adler32.h                                                     \
>>  xtea.h                                                        \
>>  tea.h                                                         \
>>  tx.h                                                          \
>> +          film_grain_params.h                                           \
>>  HEADERS-$(CONFIG_LZO)                   += lzo.h
>>  @@ -170,6 +171,7 @@ OBJS = adler32.o                                                        \
>>  tx_double.o                                                      \
>>  tx_int32.o                                                       \
>>  video_enc_params.o                                               \
>> +       film_grain_params.o                                              \
>>  OBJS-$(CONFIG_CUDA)                     += hwcontext_cuda.o
>> diff --git a/libavutil/film_grain_params.c b/libavutil/film_grain_params.c
>> new file mode 100644
>> index 0000000000..930d23c7fe
>> --- /dev/null
>> +++ b/libavutil/film_grain_params.c
>> @@ -0,0 +1,42 @@
>> +/**
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#include "film_grain_params.h"
>> +
>> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size)
>> +{
>> +    AVFilmGrainParams *params = av_mallocz(sizeof(AVFilmGrainParams));
>> +
>> +    if (size)
>> +        *size = sizeof(*params);
>> +
>> +    return params;
>> +}
>> +
>> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame)
>> +{
>> +    AVFrameSideData *side_data = av_frame_new_side_data(frame,
>> +                                                        AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>> +                                                        sizeof(AVFilmGrainParams));
>> +    if (!side_data)
>> +        return NULL;
>> +
>> +    memset(side_data->data, 0, sizeof(AVFilmGrainParams));
>> +
>> +    return (AVFilmGrainParams *)side_data->data;
>> +}
>> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h
>> new file mode 100644
>> index 0000000000..ba20fe7fa6
>> --- /dev/null
>> +++ b/libavutil/film_grain_params.h
>> @@ -0,0 +1,159 @@
>> +/*
>> + * Copyright (c) 2016 Neil Birkbeck <neil.birkbeck@gmail.com>
>>
>
> Copy paste mistake?
>

Yup.



>> +typedef struct AVFilmGrainAOMParams {
>>
>
> If you believe AV2 will have a compatible implementation, then this name is fine.
>

I don't think they're going to change it at all. They haven't even proven all this
system is capable of in order to replace it with something better, so they might
just do some minor tweaks. After all, its already somewhat ossified thanks to
hardware.



>> +    int8_t ar_coeffs_uv[2][25 + 3 /* padding for alignment purposes */];
>>
> Shouldn't the compiler take care of alignment?
>

This simplified dav1d's SIMD, hence why its there, so I decided to leave it as-is.
The comment above says so too, but this small comment draws too much attention,
so I removed it.



>> +
>> +    /**
>> +     * Specifies the range of the auto-regressive coefficients. Values of 6,
>> +     * 7, 8 and so on represent a range of [-2, 2), [-1, 1), [-0.5, 0.5) and
>> +     * so on. For AV1 must be between 6 and 9.
>> +     */
>> +    uint64_t ar_coeff_shift;
>>
>
> Why uint64_t? Sounds like it may be excessive for this.
>

Same as above, simplifies SIMD. Directly used as a psrad memory arg.



>> +
>> +    /**
>> +     * Signals the down shift applied to the generated gaussian numbers during
>> +     * synthesis.
>> +     */
>> +    int grain_scale_shift;
>> +
>> +    /**
>> +     * Specifies the luma/chroma multipliers for the index to the component
>> +     * scaling function.
>> +     */
>> +    int uv_mult[2];
>> +    int uv_mult_luma[2];
>>
>
> int8_t, like ar_coeffs_y. Or could AV2 increase the range considerably?
>

Same as above, SIMD.



>> +
>> +    /**
>> +     * Offset used for component scaling function. For AV1 its a 9-bit value
>> +     * with a range [-256, 255] */
>> +    int uv_offset[2];
>>
>
> int16_t.
>

Could be changed, but there's a single movd in dav1d's SSSE3 SIMD.
Not sure if it worth it.



>> +
>> +    /**
>> +     * Signals whether to overlap film grain blocks.
>> +     */
>> +    int overlap_flag;
>> +} AVFilmGrainAOMParams;
>> +
>> +typedef struct AVFilmGrainParams {
>> +    /**
>> +     * Specifies the codec for which this structure is valid.
>> +     */
>> +    enum AVFilmGrainParamsType type;
>> +
>> +    /**
>> +     * Seed to use for the synthesis process, if the codec allows for it.
>> +     */
>> +    uint32_t seed;
>>
>
> Maybe uint64_t, just in case. This is meant to apply to all implementations.
>

Done.

Both changes applied locally.

Nov 12, 2020, 18:42 by dev@lynne.ee:

> This patch introduces a new frame side data type AVFilmGrainParams for use 
> with video codecs which are able to use it.
>
> It is generalized rather than being AV1 specific as AV2 is expected to carry
> the same data, as well as the fact there already exist rarely-used specifications
> for both H.264 and HEVC.
>
> Patch attached.
>

Ping on the patchset.

On 11/12/2020 4:28 PM, Lynne wrote:
> Nov 12, 2020, 19:46 by jamrial@gmail.com:
> 
>> On 11/12/2020 2:42 PM, Lynne wrote:
>>
>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>> with video codecs which are able to use it.
>>>
>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>> the same data, as well as the fact there already exist rarely-used specifications
>>> for both H.264 and HEVC.
>>>
>>> Patch attached.
>>>
>>>  From 522e3a80f44129c98f0c564d44815dbe6a740568 Mon Sep 17 00:00:00 2001
>>> From: Lynne <dev@lynne.ee>
>>> Date: Thu, 12 Nov 2020 12:44:30 +0100
>>> Subject: [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data
>>>
>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>> with video codecs which are able to use it.
>>>
>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>> the same data, as well as the fact there already exist rarely-used specifications
>>> for both H.264 and HEVC.
>>> ---
>>>   doc/APIchanges                |   4 +
>>>   libavutil/Makefile            |   2 +
>>>   libavutil/film_grain_params.c |  42 +++++++++
>>>   libavutil/film_grain_params.h | 159 ++++++++++++++++++++++++++++++++++
>>>   libavutil/frame.c             |   1 +
>>>   libavutil/frame.h             |   6 ++
>>>   libavutil/version.h           |   2 +-
>>>   7 files changed, 215 insertions(+), 1 deletion(-)
>>>   create mode 100644 libavutil/film_grain_params.c
>>>   create mode 100644 libavutil/film_grain_params.h
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index b70c78a483..41248724d9 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>>   API changes, most recent first:
>>>   +2020-xx-xx - xxxxxxxxxx - lavu 56.61.100 - film_grain_params.h
>>> +  Adds a new API for extracting codec film grain parameters as side data.
>>> +  Adds a new AVFrameSideDataType entry AV_FRAME_DATA_FILM_GRAIN_PARAMS for it.
>>> +
>>>   2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h
>>>   Add AVSTREAM_EVENT_FLAG_NEW_PACKETS.
>>>   diff --git a/libavutil/Makefile b/libavutil/Makefile
>>> index 9b08372eb2..27bafe9e12 100644
>>> --- a/libavutil/Makefile
>>> +++ b/libavutil/Makefile
>>> @@ -84,6 +84,7 @@ HEADERS = adler32.h                                                     \
>>>   xtea.h                                                        \
>>>   tea.h                                                         \
>>>   tx.h                                                          \
>>> +          film_grain_params.h                                           \
>>>   HEADERS-$(CONFIG_LZO)                   += lzo.h
>>>   @@ -170,6 +171,7 @@ OBJS = adler32.o                                                        \
>>>   tx_double.o                                                      \
>>>   tx_int32.o                                                       \
>>>   video_enc_params.o                                               \
>>> +       film_grain_params.o                                              \
>>>   OBJS-$(CONFIG_CUDA)                     += hwcontext_cuda.o
>>> diff --git a/libavutil/film_grain_params.c b/libavutil/film_grain_params.c
>>> new file mode 100644
>>> index 0000000000..930d23c7fe
>>> --- /dev/null
>>> +++ b/libavutil/film_grain_params.c
>>> @@ -0,0 +1,42 @@
>>> +/**
>>> + * This file is part of FFmpeg.
>>> + *
>>> + * FFmpeg is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * FFmpeg is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with FFmpeg; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>> + */
>>> +
>>> +#include "film_grain_params.h"
>>> +
>>> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size)
>>> +{
>>> +    AVFilmGrainParams *params = av_mallocz(sizeof(AVFilmGrainParams));
>>> +
>>> +    if (size)
>>> +        *size = sizeof(*params);
>>> +
>>> +    return params;
>>> +}
>>> +
>>> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame)
>>> +{
>>> +    AVFrameSideData *side_data = av_frame_new_side_data(frame,
>>> +                                                        AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>>> +                                                        sizeof(AVFilmGrainParams));
>>> +    if (!side_data)
>>> +        return NULL;
>>> +
>>> +    memset(side_data->data, 0, sizeof(AVFilmGrainParams));
>>> +
>>> +    return (AVFilmGrainParams *)side_data->data;
>>> +}
>>> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h
>>> new file mode 100644
>>> index 0000000000..ba20fe7fa6
>>> --- /dev/null
>>> +++ b/libavutil/film_grain_params.h
>>> @@ -0,0 +1,159 @@
>>> +/*
>>> + * Copyright (c) 2016 Neil Birkbeck <neil.birkbeck@gmail.com>
>>>
>>
>> Copy paste mistake?
>>
> 
> Yup.
> 
> 
> 
>>> +typedef struct AVFilmGrainAOMParams {
>>>
>>
>> If you believe AV2 will have a compatible implementation, then this name is fine.
>>
> 
> I don't think they're going to change it at all. They haven't even proven all this
> system is capable of in order to replace it with something better, so they might
> just do some minor tweaks. After all, its already somewhat ossified thanks to
> hardware.
> 
> 
> 
>>> +    int8_t ar_coeffs_uv[2][25 + 3 /* padding for alignment purposes */];
>>>
>> Shouldn't the compiler take care of alignment?
>>
> 
> This simplified dav1d's SIMD, hence why its there, so I decided to leave it as-is.
> The comment above says so too, but this small comment draws too much attention,
> so I removed it.

I don't think that's something we should consider in our public API. We 
should prioritize small portable structs, and not trying to optimize 
fields for one potential use in one potential target out of many.

This applies to every other case where you gave SIMD as reason.

> 
> 
> 
>>> +
>>> +    /**
>>> +     * Specifies the range of the auto-regressive coefficients. Values of 6,
>>> +     * 7, 8 and so on represent a range of [-2, 2), [-1, 1), [-0.5, 0.5) and
>>> +     * so on. For AV1 must be between 6 and 9.
>>> +     */
>>> +    uint64_t ar_coeff_shift;
>>>
>>
>> Why uint64_t? Sounds like it may be excessive for this.
>>
> 
> Same as above, simplifies SIMD. Directly used as a psrad memory arg.
> 
> 
> 
>>> +
>>> +    /**
>>> +     * Signals the down shift applied to the generated gaussian numbers during
>>> +     * synthesis.
>>> +     */
>>> +    int grain_scale_shift;
>>> +
>>> +    /**
>>> +     * Specifies the luma/chroma multipliers for the index to the component
>>> +     * scaling function.
>>> +     */
>>> +    int uv_mult[2];
>>> +    int uv_mult_luma[2];
>>>
>>
>> int8_t, like ar_coeffs_y. Or could AV2 increase the range considerably?
>>
> 
> Same as above, SIMD.
> 
> 
> 
>>> +
>>> +    /**
>>> +     * Offset used for component scaling function. For AV1 its a 9-bit value
>>> +     * with a range [-256, 255] */
>>> +    int uv_offset[2];
>>>
>>
>> int16_t.
>>
> 
> Could be changed, but there's a single movd in dav1d's SSSE3 SIMD.
> Not sure if it worth it.
> 
> 
> 
>>> +
>>> +    /**
>>> +     * Signals whether to overlap film grain blocks.
>>> +     */
>>> +    int overlap_flag;
>>> +} AVFilmGrainAOMParams;
>>> +
>>> +typedef struct AVFilmGrainParams {
>>> +    /**
>>> +     * Specifies the codec for which this structure is valid.
>>> +     */
>>> +    enum AVFilmGrainParamsType type;
>>> +
>>> +    /**
>>> +     * Seed to use for the synthesis process, if the codec allows for it.
>>> +     */
>>> +    uint32_t seed;
>>>
>>
>> Maybe uint64_t, just in case. This is meant to apply to all implementations.
>>
> 
> Done.
> 
> Both changes applied locally.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

Nov 22, 2020, 21:28 by jamrial@gmail.com:

> On 11/12/2020 4:28 PM, Lynne wrote:
>
>> Nov 12, 2020, 19:46 by jamrial@gmail.com:
>>
>>> On 11/12/2020 2:42 PM, Lynne wrote:
>>>
>>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>>> with video codecs which are able to use it.
>>>>
>>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>>> the same data, as well as the fact there already exist rarely-used specifications
>>>> for both H.264 and HEVC.
>>>>
>>>> Patch attached.
>>>>
>>>>  From 522e3a80f44129c98f0c564d44815dbe6a740568 Mon Sep 17 00:00:00 2001
>>>> From: Lynne <dev@lynne.ee>
>>>> Date: Thu, 12 Nov 2020 12:44:30 +0100
>>>> Subject: [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data
>>>>
>>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>>> with video codecs which are able to use it.
>>>>
>>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>>> the same data, as well as the fact there already exist rarely-used specifications
>>>> for both H.264 and HEVC.
>>>> ---
>>>>  doc/APIchanges                |   4 +
>>>>  libavutil/Makefile            |   2 +
>>>>  libavutil/film_grain_params.c |  42 +++++++++
>>>>  libavutil/film_grain_params.h | 159 ++++++++++++++++++++++++++++++++++
>>>>  libavutil/frame.c             |   1 +
>>>>  libavutil/frame.h             |   6 ++
>>>>  libavutil/version.h           |   2 +-
>>>>  7 files changed, 215 insertions(+), 1 deletion(-)
>>>>  create mode 100644 libavutil/film_grain_params.c
>>>>  create mode 100644 libavutil/film_grain_params.h
>>>>
>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>> index b70c78a483..41248724d9 100644
>>>> --- a/doc/APIchanges
>>>> +++ b/doc/APIchanges
>>>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>>>  API changes, most recent first:
>>>>  +2020-xx-xx - xxxxxxxxxx - lavu 56.61.100 - film_grain_params.h
>>>> +  Adds a new API for extracting codec film grain parameters as side data.
>>>> +  Adds a new AVFrameSideDataType entry AV_FRAME_DATA_FILM_GRAIN_PARAMS for it.
>>>> +
>>>>  2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h
>>>>  Add AVSTREAM_EVENT_FLAG_NEW_PACKETS.
>>>>  diff --git a/libavutil/Makefile b/libavutil/Makefile
>>>> index 9b08372eb2..27bafe9e12 100644
>>>> --- a/libavutil/Makefile
>>>> +++ b/libavutil/Makefile
>>>> @@ -84,6 +84,7 @@ HEADERS = adler32.h                                                     \
>>>>  xtea.h                                                        \
>>>>  tea.h                                                         \
>>>>  tx.h                                                          \
>>>> +          film_grain_params.h                                           \
>>>>  HEADERS-$(CONFIG_LZO)                   += lzo.h
>>>>  @@ -170,6 +171,7 @@ OBJS = adler32.o                                                        \
>>>>  tx_double.o                                                      \
>>>>  tx_int32.o                                                       \
>>>>  video_enc_params.o                                               \
>>>> +       film_grain_params.o                                              \
>>>>  OBJS-$(CONFIG_CUDA)                     += hwcontext_cuda.o
>>>> diff --git a/libavutil/film_grain_params.c b/libavutil/film_grain_params.c
>>>> new file mode 100644
>>>> index 0000000000..930d23c7fe
>>>> --- /dev/null
>>>> +++ b/libavutil/film_grain_params.c
>>>> @@ -0,0 +1,42 @@
>>>> +/**
>>>> + * This file is part of FFmpeg.
>>>> + *
>>>> + * FFmpeg is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>> + *
>>>> + * FFmpeg is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with FFmpeg; if not, write to the Free Software
>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>>> + */
>>>> +
>>>> +#include "film_grain_params.h"
>>>> +
>>>> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size)
>>>> +{
>>>> +    AVFilmGrainParams *params = av_mallocz(sizeof(AVFilmGrainParams));
>>>> +
>>>> +    if (size)
>>>> +        *size = sizeof(*params);
>>>> +
>>>> +    return params;
>>>> +}
>>>> +
>>>> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame)
>>>> +{
>>>> +    AVFrameSideData *side_data = av_frame_new_side_data(frame,
>>>> +                                                        AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>>>> +                                                        sizeof(AVFilmGrainParams));
>>>> +    if (!side_data)
>>>> +        return NULL;
>>>> +
>>>> +    memset(side_data->data, 0, sizeof(AVFilmGrainParams));
>>>> +
>>>> +    return (AVFilmGrainParams *)side_data->data;
>>>> +}
>>>> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h
>>>> new file mode 100644
>>>> index 0000000000..ba20fe7fa6
>>>> --- /dev/null
>>>> +++ b/libavutil/film_grain_params.h
>>>> @@ -0,0 +1,159 @@
>>>> +/*
>>>> + * Copyright (c) 2016 Neil Birkbeck <neil.birkbeck@gmail.com>
>>>>
>>>
>>> Copy paste mistake?
>>>
>>
>> Yup.
>>
>>
>>
>>>> +typedef struct AVFilmGrainAOMParams {
>>>>
>>>
>>> If you believe AV2 will have a compatible implementation, then this name is fine.
>>>
>>
>> I don't think they're going to change it at all. They haven't even proven all this
>> system is capable of in order to replace it with something better, so they might
>> just do some minor tweaks. After all, its already somewhat ossified thanks to
>> hardware.
>>
>>
>>
>>>> +    int8_t ar_coeffs_uv[2][25 + 3 /* padding for alignment purposes */];
>>>>
>>> Shouldn't the compiler take care of alignment?
>>>
>>
>> This simplified dav1d's SIMD, hence why its there, so I decided to leave it as-is.
>> The comment above says so too, but this small comment draws too much attention,
>> so I removed it.
>>
>
> I don't think that's something we should consider in our public API. We should prioritize small portable structs, and not trying to optimize fields for one potential use in one potential target out of many.
>
> This applies to every other case where you gave SIMD as reason.
>

I think I'd disagree with that in this case, some lenience here could help,
but I don't have a strong opinion on it, so whatever.
Looked through the spec and mapped all options to the smallest type they'll
fit, so num_y_points, num_uv_points, scaling_shift, ar_coeff_lag, ar_coeff_shift
grain_scale_shift are now uint8_t, ar_coeffs_uv, uv_mult and uv_mult_luma are
int8_t and uv_offset is int16_t.

Patch attached.

On 11/23/2020 9:08 AM, Lynne wrote:


> From e11df30e25f1b27a4ec3efb7dc894b8cf59113d2 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Thu, 12 Nov 2020 12:44:30 +0100
> Subject: [PATCH v3 1/4] libavutil: introduce AVFilmGrainParams side data
> 
> This patch introduces a new frame side data type AVFilmGrainParams for use
> with video codecs which are able to use it.
> 
> It is generalized rather than being AV1 specific as AV2 is expected to carry
> the same data, as well as the fact there already exist rarely-used specifications
> for both H.264 and HEVC.

This part of the commit message is no longer true, so remove it.

> ---
>  doc/APIchanges                |   4 +
>  libavutil/Makefile            |   2 +
>  libavutil/film_grain_params.c |  42 +++++++++
>  libavutil/film_grain_params.h | 156 ++++++++++++++++++++++++++++++++++
>  libavutil/frame.c             |   1 +
>  libavutil/frame.h             |   6 ++
>  libavutil/version.h           |   2 +-
>  7 files changed, 212 insertions(+), 1 deletion(-)
>  create mode 100644 libavutil/film_grain_params.c
>  create mode 100644 libavutil/film_grain_params.h
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index b70c78a483..41248724d9 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-xx-xx - xxxxxxxxxx - lavu 56.61.100 - film_grain_params.h
> +  Adds a new API for extracting codec film grain parameters as side data.
> +  Adds a new AVFrameSideDataType entry AV_FRAME_DATA_FILM_GRAIN_PARAMS for it.
> +
>  2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h
>    Add AVSTREAM_EVENT_FLAG_NEW_PACKETS.
>  
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 9b08372eb2..27bafe9e12 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -84,6 +84,7 @@ HEADERS = adler32.h                                                     \
>            xtea.h                                                        \
>            tea.h                                                         \
>            tx.h                                                          \
> +          film_grain_params.h                                           \
>  
>  HEADERS-$(CONFIG_LZO)                   += lzo.h
>  
> @@ -170,6 +171,7 @@ OBJS = adler32.o                                                        \
>         tx_double.o                                                      \
>         tx_int32.o                                                       \
>         video_enc_params.o                                               \
> +       film_grain_params.o                                              \
>  
>  
>  OBJS-$(CONFIG_CUDA)                     += hwcontext_cuda.o
> diff --git a/libavutil/film_grain_params.c b/libavutil/film_grain_params.c
> new file mode 100644
> index 0000000000..930d23c7fe
> --- /dev/null
> +++ b/libavutil/film_grain_params.c
> @@ -0,0 +1,42 @@
> +/**
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "film_grain_params.h"
> +
> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size)
> +{
> +    AVFilmGrainParams *params = av_mallocz(sizeof(AVFilmGrainParams));
> +
> +    if (size)
> +        *size = sizeof(*params);
> +
> +    return params;
> +}
> +
> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame)
> +{
> +    AVFrameSideData *side_data = av_frame_new_side_data(frame,
> +                                                        AV_FRAME_DATA_FILM_GRAIN_PARAMS,
> +                                                        sizeof(AVFilmGrainParams));
> +    if (!side_data)
> +        return NULL;
> +
> +    memset(side_data->data, 0, sizeof(AVFilmGrainParams));
> +
> +    return (AVFilmGrainParams *)side_data->data;
> +}
> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h
> new file mode 100644
> index 0000000000..978fc289c9
> --- /dev/null
> +++ b/libavutil/film_grain_params.h
> @@ -0,0 +1,156 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVUTIL_FILM_GRAIN_PARAMS_H
> +#define AVUTIL_FILM_GRAIN_PARAMS_H
> +
> +#include "frame.h"
> +
> +enum AVFilmGrainParamsType {
> +    AV_FILM_GRAM_PARAMS_NONE = 0,
> +
> +    /**
> +     * The union is valid when interpreted as AVFilmGrainAOMParams (codec.aom)
> +     */
> +    AV_FILM_GRAM_PARAMS_AV1,
> +};
> +
> +typedef struct AVFilmGrainAOMParams {

Add a simple doxy description for the struct, and a @note mentioning 
this struct's size is not part of the ABI and 
av_film_grain_params_alloc() must be used to allocate it, like it was 
done in mastering_display_metadata.h and spherical.h

> +    /**
> +     * Number of points for the piecewise linear scaling function of the
> +     * luma plane.
> +     */
> +    uint8_t num_y_points;
> +
> +    /**
> +     * Piecewise scaling function values and scale for each point.
> +     */
> +    uint8_t y_points[14][2 /* value, scaling */];
> +
> +    /**
> +     * Signals whether to derive the chroma scaling function from the luma.
> +     * Not equivalent to copying the luma values and scales.
> +     */
> +    int chroma_scaling_from_luma;
> +
> +    /**
> +     * If chroma_scaling_from_luma is set to 0, signals the chroma scaling
> +     * function parameters.
> +     */
> +    uint8_t num_uv_points[2];
> +    uint8_t uv_points[2][10][2 /* value, scaling */];
> +
> +    /**
> +     * Specifies the shift applied to the chroma components. For AV1, its within
> +     * [8; 11] and determines the range and quantization of the film grain.
> +     */
> +    uint8_t scaling_shift;
> +
> +    /**
> +     * Specifies the auto-regression coefficients for both luma and chroma.
> +     */
> +    uint8_t ar_coeff_lag;
> +
> +    /**
> +     * Luma auto-regression coefficients.
> +     */
> +    int8_t ar_coeffs_y[24];
> +
> +    /**
> +     * Chroma auto-regression coefficients.
> +     */
> +    int8_t ar_coeffs_uv[2][25];
> +
> +    /**
> +     * Specifies the range of the auto-regressive coefficients. Values of 6,
> +     * 7, 8 and so on represent a range of [-2, 2), [-1, 1), [-0.5, 0.5) and
> +     * so on. For AV1 must be between 6 and 9.
> +     */
> +    uint8_t ar_coeff_shift;
> +
> +    /**
> +     * Signals the down shift applied to the generated gaussian numbers during
> +     * synthesis.
> +     */
> +    uint8_t grain_scale_shift;
> +
> +    /**
> +     * Specifies the luma/chroma multipliers for the index to the component
> +     * scaling function.
> +     */
> +    int8_t uv_mult[2];
> +    int8_t uv_mult_luma[2];
> +
> +    /**
> +     * Offset used for component scaling function. For AV1 its a 9-bit value
> +     * with a range [-256, 255] */
> +    int16_t uv_offset[2];
> +
> +    /**
> +     * Signals whether to overlap film grain blocks.
> +     */
> +    int overlap_flag;
> +} AVFilmGrainAOMParams;
> +
> +typedef struct AVFilmGrainParams {
> +    /**
> +     * Specifies the codec for which this structure is valid.
> +     */
> +    enum AVFilmGrainParamsType type;
> +
> +    /**
> +     * Seed to use for the synthesis process, if the codec allows for it.
> +     */
> +    uint64_t seed;
> +
> +    /**
> +     * Signals to clip to limited color levels after film grain application
> +     * for AV1.
> +     */
> +    int limit_output_range;

Looking at the HEVC spec, film_grain_full_range_flag does not have the 
same semantics as clip_to_restricted_range.

If this field is going to be outside AVFilmGrain###Params, then it needs 
to be generic enough for it. So maybe call it color_range and make it of 
type AVColorRange?
Alternatively, just put it in AVFilmGrainAOMParams and lets not try to 
predict what other FG implementations are going to do in the future.

> +
> +    /**
> +     * Additional fields may be added both here and in any structure included.
> +     * If a codec's film grain structure differs slightly over another
> +     * codec's, fields within may change meaning depending on the type.
> +     */
> +    union {
> +        AVFilmGrainAOMParams aom;
> +    } codec;
> +} AVFilmGrainParams;
> +
> +/**
> + * Allocate an AVFilmGrainParams structure and set its fields to
> + * default values. The resulting struct can be freed using av_freep().
> + * If size is not NULL it will be set to the number of bytes allocated.
> + *
> + * @return An AVFilmGrainParams filled with default values or NULL
> + *         on failure.
> + */
> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size);
> +
> +/**
> + * Allocate a complete AVFilmGrainParams and add it to the frame.
> + *
> + * @param frame The frame which side data is added to.
> + *
> + * @return The AVFilmGrainParams structure to be filled by caller.
> + */
> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame);
> +
> +#endif /* AVUTIL_FILM_GRAIN_PARAMS_H */
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index b019779b1a..eab51b6a32 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -851,6 +851,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>      case AV_FRAME_DATA_REGIONS_OF_INTEREST: return "Regions Of Interest";
>      case AV_FRAME_DATA_VIDEO_ENC_PARAMS:            return "Video encoding parameters";
>      case AV_FRAME_DATA_SEI_UNREGISTERED:            return "H.26[45] User Data Unregistered SEI message";
> +    case AV_FRAME_DATA_FILM_GRAIN_PARAMS:           return "Film grain parameters";
>      }
>      return NULL;
>  }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3bd240fc97..392315f40f 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -192,6 +192,12 @@ enum AVFrameSideDataType {
>       * uuid_iso_iec_11578 followed by AVFrameSideData.size - 16 bytes of user_data_payload_byte.
>       */
>      AV_FRAME_DATA_SEI_UNREGISTERED,
> +
> +    /**
> +     * Film grain parameters for a frame, described by AVFilmGrainParameters.
> +     * Must be present for every frame which should have film grain applied.
> +     */
> +    AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>  };
>  
>  enum AVActiveFormatDescription {
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 1aca823650..55a59d6c58 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  56
> -#define LIBAVUTIL_VERSION_MINOR  60
> +#define LIBAVUTIL_VERSION_MINOR  61
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> -- 
> 2.29.2.222.g5d2a92d10f8
>

Nov 23, 2020, 15:06 by jamrial@gmail.com:

> On 11/23/2020 9:08 AM, Lynne wrote:
>
>
>> From e11df30e25f1b27a4ec3efb7dc894b8cf59113d2 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Thu, 12 Nov 2020 12:44:30 +0100
>> Subject: [PATCH v3 1/4] libavutil: introduce AVFilmGrainParams side data
>>
>> This patch introduces a new frame side data type AVFilmGrainParams for use
>> with video codecs which are able to use it.
>>
>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>> the same data, as well as the fact there already exist rarely-used specifications
>> for both H.264 and HEVC.
>>
>
> This part of the commit message is no longer true, so remove it.
>

Done.


> Looking at the HEVC spec, film_grain_full_range_flag does not have the same semantics as clip_to_restricted_range.
>
> If this field is going to be outside AVFilmGrain###Params, then it needs to be generic enough for it. So maybe call it color_range and make it of type AVColorRange?
> Alternatively, just put it in AVFilmGrainAOMParams and lets not try to predict what other FG implementations are going to do in the future.
>

I've put it into AOM's struct. I think that's safer and I thought it somehow didn't
fit in into the main one.

I've added the comments you requested for both structs, fixed a few comments
that were wrong, and as discussed on IRC changed the data types to what is
smallest and matches best for the coefficients and generic ints for everything
else (#coeffs, shifts, offsets and multipliers).

Attached v4 and locally updated the libdav1d patch.

Nov 23, 2020, 17:02 by dev@lynne.ee:

> Nov 23, 2020, 15:06 by jamrial@gmail.com:
>
>> On 11/23/2020 9:08 AM, Lynne wrote:
>>
>>
>>> From e11df30e25f1b27a4ec3efb7dc894b8cf59113d2 Mon Sep 17 00:00:00 2001
>>> From: Lynne <dev@lynne.ee>
>>> Date: Thu, 12 Nov 2020 12:44:30 +0100
>>> Subject: [PATCH v3 1/4] libavutil: introduce AVFilmGrainParams side data
>>>
>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>> with video codecs which are able to use it.
>>>
>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>> the same data, as well as the fact there already exist rarely-used specifications
>>> for both H.264 and HEVC.
>>>
>>
>> This part of the commit message is no longer true, so remove it.
>>
>
> Done.
>
>
>> Looking at the HEVC spec, film_grain_full_range_flag does not have the same semantics as clip_to_restricted_range.
>>
>> If this field is going to be outside AVFilmGrain###Params, then it needs to be generic enough for it. So maybe call it color_range and make it of type AVColorRange?
>> Alternatively, just put it in AVFilmGrainAOMParams and lets not try to predict what other FG implementations are going to do in the future.
>>
>
> I've put it into AOM's struct. I think that's safer and I thought it somehow didn't
> fit in into the main one.
>
> I've added the comments you requested for both structs, fixed a few comments
> that were wrong, and as discussed on IRC changed the data types to what is
> smallest and matches best for the coefficients and generic ints for everything
> else (#coeffs, shifts, offsets and multipliers).
>
> Attached v4 and locally updated the libdav1d patch.
>

Pushed, thanks to jamrial and elenril for the reviews.