[FFmpeg-devel] [PATCH] libavutil: add mastering display metadata sidedata
Neil Birkbeck
neil.birkbeck at gmail.com
Thu Jan 21 06:40:29 CET 2016
Thanks for the detailed responses and discussions, and apologies for
delayed response (I was considering to change the functions to a
serialize/deserialize to some wire format and keeping the struct, but wire
format would've essentially be a memcpy to the struct on little endian and
would only benefit if there was a place that was generically writing the
SideData's data to file--wasn't sure if that was happening anywhere).
On Tue, Jan 19, 2016 at 7:39 AM, Michael Niedermayer <michael at niedermayer.cc
> wrote:
> On Tue, Jan 19, 2016 at 01:34:12PM +0100, wm4 wrote:
> > 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.
>
> true, patch applied
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Its not that you shouldnt use gotos but rather that you should write
> readable code and code with gotos often but not always is less readable
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
More information about the ffmpeg-devel
mailing list