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

[Vittorio Giovara]

On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial at gmail.com> wrote:

> On 2/20/2020 5:02 PM, Vittorio Giovara 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
>
> This seems excessive only to use a bitstream reader to read a bunch of
> fields in a buffer.
>
> get_bits.h is included in lavf, so i don't see why it couldn't be used
> in lavu.


bacuase lavf is a library which depends on lavc, not vice versa, hierarchy
is respected

As you said it's inlined.
>

I still consider it a poor design choice: either make bitstream reader
public in a separate library (not easy, i am aware) or do the bitstream
reading where the bistream actually is, IMO

Vittorio


> >
> >
> >>  #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.
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".



-- 
Vittorio