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

[Hendrik Leppkes]

On Fri, Feb 21, 2020 at 5:59 AM Vittorio Giovara
<vittorio.giovara at gmail.com> wrote:
>
> 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
>

I agree that the parsing should just move to avcodec. We parse every
other SEI straight in the codec it comes from, why does this one have
parsing in avutil? It seems weird.

- Hendrik