[FFmpeg-devel] [PATCH] Add HDR dynamic metadata struct (for SPMTE 2094-40) to libavutil.

[Vittorio Giovara]

On Mon, Dec 10, 2018 at 2:50 PM Mohammad Izadi <moh.izadi at gmail.com> wrote:

> From: Mohammad Izadi <izadi at google.com>
>
> The dynamic metadata contains data for color volume transform -
> application 4 of SPMTE 2094-40:2016 standard. The data comes from HEVC in
> the SEI_TYPE_USER_DATA_REGISTERED_ITU_T_T35.
>
> I'll add support to HEVC in a follow-up.
>
---
>  libavutil/Makefile               |   2 +
>  libavutil/frame.c                |   1 +
>  libavutil/frame.h                |   7 +
>  libavutil/hdr_dynamic_metadata.c |  40 ++++
>  libavutil/hdr_dynamic_metadata.h | 344 +++++++++++++++++++++++++++++++
>  libavutil/version.h              |   2 +-
>  6 files changed, 395 insertions(+), 1 deletion(-)
>  create mode 100644 libavutil/hdr_dynamic_metadata.c
>  create mode 100644 libavutil/hdr_dynamic_metadata.h
>
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index caddc7e155..7dcb92b06b 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -31,6 +31,7 @@ HEADERS = adler32.h
>                \
>            file.h                                                        \
>            frame.h                                                       \
>            hash.h                                                        \
> +          hdr_dynamic_metadata.h                                        \
>

There

>            hmac.h                                                        \
>            hwcontext.h                                                   \
>            hwcontext_cuda.h                                              \
> @@ -119,6 +120,7 @@ OBJS = adler32.o
>                   \
>         fixed_dsp.o                                                      \
>         frame.o                                                          \
>         hash.o                                                           \
> +       hdr_dynamic_metadata.o                                           \
>         hmac.o                                                           \
>         hwcontext.o                                                      \
>         imgutils.o                                                       \
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 9b3fb13e68..c5f30b6847 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum
> AVFrameSideDataType type)
>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table
> properties";
>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table
> data";
>  #endif
> +    case AV_FRAME_DATA_HDR_DYNAMIC_METADATA_SMPTE2094_40: return "HDR
> Dynamic Metadata SMPTE2094-40";
>

I like VeryLongJavaLikeNamingForFunctionsAndDataTypesAsWellAsEnumsOfCourse
as much as the next guy, but this is overly too long and the related
structure name (AVDynamicMetadataSMPTE2094_40) is inconsistent with the
public type naming present in this project.

If I may suggest, please use the following names:
- AV_FRAME_DATA_DYNAMIC_HDR for frame data type: it is obviously metadata,
and the fact that is based on SMPTE2094-40 should not be hardcoded in the
name
- dynamic_hdr.h as header name: again the fact that it's metadata does not
be carried everywhere
- AVDynamicHDR as structure name: short and to the point, and without spec
numbers or underscores


>      }
>      return NULL;
>  }
> diff --git a/libavutil/hdr_dynamic_metadata.c
> b/libavutil/hdr_dynamic_metadata.c
> new file mode 100644
> index 0000000000..59dfcc84e4
> --- /dev/null
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -0,0 +1,40 @@
> +
> +#include "hdr_dynamic_metadata.h"
> +#include "mem.h"
> +
> +AVDynamicMetadataSMPTE2094_40
> *av_dynamic_metadata_smpte2094_40_alloc(void)
>

you need to return the size of allocation back to the caller, see
spherical.h as example (especially since you document that "its size is not
a part of the public ABI.")

+/**
> + * Option for overlapping elliptical pixel selectors in an image.
> + */
> +enum AVOverlapProcessOption {
> +    AV_OVERLAP_PROCESS_WEIGHTED_AVERAGING = 0,
> +    AV_OVERLAP_PROCESS_LAYERING = 1,
> +};
>

I'm not a fan of these names, but i've bikeshed enough


> +
> +/**
> + * Percentile represents the percentile at a specific percentage in
> + * a distribution.
> + */
> +typedef struct AVPercentile {
> +    /**
> +     * The percentage value corresponding to a spesific percentile
> linearized
>

typo 'spesific'

+     * RGB value in the processing window in the scene. The value shall be
> in
> +     * the range of 0 to100, inclusive.
> +     */
> +    uint8_t percentage;
> +    /**
> +     * The linearized maxRGB value at a specific percentile in the
> processing
> +     * window in the scene. The value shall be in the range of 0 to 1,
> inclusive
> +     * and in multiples of 0.00001.
> +     */
> +    AVRational percentile;
> +} AVPercentile;
> +
>
-- 
Vittorio