[FFmpeg-devel] [PATCH] Update HDR10+ metadata structure.

[Vittorio Giovara]

On Thu, Feb 20, 2020 at 3:16 PM Mohammad Izadi <moh.izadi at gmail.com> wrote:

> On Thu, Feb 20, 2020 at 12:10 PM Vittorio Giovara <
> vittorio.giovara at gmail.com> wrote:
>
> > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi at gmail.com>
> > wrote:
> >
> > > From: Mohammad Izadi <izadi at google.com>
> > >
> > > Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side
> > > data in the follow-up CLs.
> > > ---
> > >  libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++
> > >  libavutil/hdr_dynamic_metadata.h |  13 ++-
> > >  libavutil/version.h              |   2 +-
> > >  3 files changed, 178 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavutil/hdr_dynamic_metadata.c
> > > b/libavutil/hdr_dynamic_metadata.c
> > > index 0fa1ee82de..f24bcb40f5 100644
> > > --- a/libavutil/hdr_dynamic_metadata.c
> > > +++ b/libavutil/hdr_dynamic_metadata.c
> > > @@ -19,8 +19,17 @@
> > >   */
> > >
> > >  #include "hdr_dynamic_metadata.h"
> > > +#include "libavcodec/get_bits.h"
> > >
> >
> > wait is it ok to use libavcodec headers in libavutil? while it's fine at
> > compilation time since it is an inlined header, I don't think it's a good
> > idea to freely use such functions in this way: what I would do is rather
> > implement the parsing in libavcodec, store the fields in a structure
> > defined in libavutil and then use this new structure in here
> >
> > What if I move it all to livavcodec?
>
>
that works too, but then you'd need to figure out a way to export this
information to the callers I suppose
See how it is done for the various SEI handling functions, that parse
fields in h264*.c or hevc*.c, call a lavu function, and then export such
information with a frame side data.

(note that you're replying with some formatting on, mixing your reply with
the previous one)


> >
> > >  #include "mem.h"
> > >
> > > +static const int32_t luminance_den = 1;
> > > +static const int32_t peak_luminance_den = 15;
> > > +static const int32_t rgb_den = 100000;
> > > +static const int32_t fraction_pixel_den = 1000;
> > > +static const int32_t knee_point_den = 4095;
> > > +static const int32_t bezier_anchor_den = 1023;
> > > +static const int32_t saturation_weight_den = 8;
> > >
> >
> > These fields could also be stored in the structure i mentioned, rather
> then
> > having them declared as static here.
> >
> You mean to create a new struct just for constants?
>

no i guess i don't understand why those constants are needed there: if they
are just used as constant you could just use #define instead of static int,
if they are used once, you could use them where needed, and avoid declaring
them in the first place

-- 
Vittorio