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 <mich...@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 <mich...@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 <mich...@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.birkb...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel