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

[Michael Niedermayer]

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
but more importantly, how would such struct be extended, if we
want to add a field ?
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.

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160119/3037d913/attachment.sig>