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

[Jan Ekström]

On Tue, Dec 18, 2018 at 12:41 AM Vittorio Giovara
<vittorio.giovara at gmail.com> wrote:
>
> 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
>

I don't think SMPTE ST.2094-XX utilized the same fields?

I think SMPTE ST.2094-40 is Samsung's dynamic metadata which is effect
usually called "HDR10+" ? Not just calling it AVDynamicHDR could come
up useful if we plan on adding support for SMPTE ST.2094-10 which is
one part of what's called "Dolby Vision" (even though it is just
dynamic metadata - because of course you have to stick everything
under a single marketing name).

Just my 20 cents.

Jan