[FFmpeg-devel] [PATCH] libavutil: add mastering display metadata sidedata

[wm4]

On Tue, 19 Jan 2016 13:02:25 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Tue, Jan 19, 2016 at 10:18:16AM +0100, wm4 wrote:
> > On Tue, 19 Jan 2016 00:32:43 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Sat, Jan 16, 2016 at 04:19:38PM -0800, Neil Birkbeck wrote:  
> > > > Adding mastering display metadata struct to avutil. The mastering display metadata contains information 
> > > > about the mastering display color volume (SMPTE 2086:2014). 
> > > > 
> > > > This info comes from HEVC in the SEI_TYPE_MASTERING_DISPLAY_INFO and is soon to be included in MKV:
> > > > https://mailarchive.ietf.org/arch/search/?email_list=cellar&gbt=1&index=sZyfPTM-QY69P-0omfOIiTN622o
> > > > so it is similar to SEI FPA / stereo_mode in MKV and as such this patch follows how AVStereo3D is implemented.
> > > > 
> > > > I'll add support to HEVC in a follow-up (and MKV when spec is approved).
> > > > 
> > > > Signed-off-by: Neil Birkbeck <neil.birkbeck at gmail.com>
> > > > ---
> > > >  libavutil/Makefile                     |  2 +
> > > >  libavutil/frame.h                      |  7 ++-
> > > >  libavutil/mastering_display_metadata.c | 43 +++++++++++++++++
> > > >  libavutil/mastering_display_metadata.h | 87 ++++++++++++++++++++++++++++++++++
> > > >  libavutil/version.h                    |  2 +-
> > > >  5 files changed, 139 insertions(+), 2 deletions(-)
> > > >  create mode 100644 libavutil/mastering_display_metadata.c
> > > >  create mode 100644 libavutil/mastering_display_metadata.h
> > > > 
> > > > diff --git a/libavutil/Makefile b/libavutil/Makefile
> > > > index bf8c713..65b2d25 100644
> > > > --- a/libavutil/Makefile
> > > > +++ b/libavutil/Makefile
> > > > @@ -38,6 +38,7 @@ HEADERS = adler32.h                                                     \
> > > >            log.h                                                         \
> > > >            macros.h                                                      \
> > > >            mathematics.h                                                 \
> > > > +          mastering_display_metadata.h                                  \
> > > >            md5.h                                                         \
> > > >            mem.h                                                         \
> > > >            motion_vector.h                                               \
> > > > @@ -115,6 +116,7 @@ OBJS = adler32.o                                                        \
> > > >         log.o                                                            \
> > > >         log2_tab.o                                                       \
> > > >         mathematics.o                                                    \
> > > > +       mastering_display_metadata.o                                     \
> > > >         md5.o                                                            \
> > > >         mem.o                                                            \
> > > >         murmur3.o                                                        \
> > > > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > > > index 9c6061a..308355b 100644
> > > > --- a/libavutil/frame.h
> > > > +++ b/libavutil/frame.h
> > > > @@ -106,12 +106,17 @@ enum AVFrameSideDataType {
> > > >       * @endcode
> > > >       */
> > > >      AV_FRAME_DATA_SKIP_SAMPLES,
> > > > -
> > > >      /**
> > > >       * This side data must be associated with an audio frame and corresponds to
> > > >       * enum AVAudioServiceType defined in avcodec.h.
> > > >       */
> > > >      AV_FRAME_DATA_AUDIO_SERVICE_TYPE,    
> > >   
> > > > +    /**
> > > > +     * Mastering display metadata associated with a video frame. The payload is
> > > > +     * an AVMasteringDisplayMetadata type and contains information about the
> > > > +     * mastering display color volume.
> > > > +     */
> > > > +    AV_FRAME_DATA_MASTERING_DISPLAY_METADATA    
> > > 
> > > will this always stay the same or could it require to be extended in
> > > the future ?
> > > if future extensions may occur then it should be documented how that
> > > would work. Would fields be added to the end of the struct ?
> > > If so how would this be detected ?
> > > 
> > > C does not gurantee the sizeof to change for the addition of fields
> > > for example these 2 are both 8bytes on x86-64
> > > typedef struct X { int a; char b; }X;
> > > typedef struct Y { int a; char b; char c; }Y;
> > > 
> > > defining the meaning of the sidedata byte per byte avoids the problem
> > > of detectability it also makes the function endian and ABI independant
> > > and would allow passing it bytewise between computers, no idea how
> > > much thats a real world use case though
> > > 
> > > [...]  
> > 
> > I thought we decided side data can be memcpy'd struct? (I.e. can be
> > ABI-dependent.)  
> 
> i dont remember

Well, we have e.g. AVStereo3D in AV_FRAME_DATA_STEREO3D.

> but more importantly, how would such struct be extended, if we
> want to add a field ?

Like we always do, append a field at the end.

> searching for AV_FRAME_DATA* finds hits in libavcodec, libavdevice,
> libavfilter, ffprobe as well as doc/examples
> 
> adding a field is not guranteed to change the sizeof() of the struct
> so using the sidedatas size to detect the version would be undefined
> behavior also the exact size could with structs differ between
> platforms making compatibility code potentially a mess.

True, but this is also true for all other structs we use everywhere.

> 
> We could hope to never need to extend the struct or bump all major
> versions when we need to, or add a version field
> a version field would need some documentation what version
> means which fields are available.
> 
> either way iam fine with any solution that works
> 
> [...]