Hi Leo On Thu, Mar 26, 2026 at 01:13:39PM -0400, Leo Izen via ffmpeg-devel wrote: > On 3/25/26 13:45, Michael Niedermayer via ffmpeg-devel wrote: > > Hi all > > > > I think the EXIF code would beneift from a review > > > > I would do this on https://code.ffmpeg.org/ but > > https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20041 > > does not show the actual diff of the patches so i cannot > > do it that way at least > > > > This is review of patch #2 of 20 from 20041 > > ill probably simply reply to cvslog for the others > > but this was still from the old log script which threw everything > > in one mail making taht a pain too > > > index fa2a2c2a13b..8df117478ae 100644 > > > --- a/libavutil/side_data.c > > > +++ b/libavutil/side_data.c > > > @@ -53,6 +53,7 @@ static const AVSideDataDescriptor sd_props[] = { > > > [AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT] = { "Ambient viewing > > > environment", AV_SIDE_DATA_PROP_GLOBAL }, > > > [AV_FRAME_DATA_SPHERICAL] = { "Spherical > > > Mapping", AV_SIDE_DATA_PROP_GLOBAL | > > > AV_SIDE_DATA_PROP_SIZE_DEPENDENT }, > > > [AV_FRAME_DATA_ICC_PROFILE] = { "ICC profile", > > > AV_SIDE_DATA_PROP_GLOBAL | > > > AV_SIDE_DATA_PROP_COLOR_DEPENDENT }, > > > + [AV_FRAME_DATA_EXIF] = { "EXIF metadata", > > > AV_SIDE_DATA_PROP_GLOBAL }, > > IIUC EXIF can contain orientation, dimensions and colorspace-related > > fields, so > > this single monolithic struct with just AV_SIDE_DATA_PROP_GLOBAL may be bad > > If this is your criticism of the patch then you haven't really looked at any > of the functionality in any of the code. There's already existing tools in > FFmpeg for orientation (notably AV_SIDE_DATA_DISPLAYMATRIX) which we pop off > of the EXIF metadata struct and attach as its own separate side data on > read, and which we read and re-attach on write. Dimensions are also > sanitized to match the dimensions of the AVFrame it's attached to. There's > quite a bit of code to reconcile these sorts of issues.
My point is that AV_FRAME_DATA_EXIF is a single side data type on AVFrame, but
the EXIF payload can contain fields with different semantics, including fields
that are size-dependent and fields that are color-related.
The current code does appear to special-case some of this already
But that does not make the underlying classification concern disappear, it
rather confirms that EXIF is not purely "global" in the same sense as metadata
that is independent of frame geometry or color representation.
also there appears to be tags that are not reconciled / sanitized / handled
Looking at the EXIF tags, maybe some of the folllowing:
{"BitsPerSample", 0x102},
{"PhotometricInterpretation", 0x106},
{"SamplesPerPixel", 0x115},
{"PlanarConfiguration", 0x11C},
{"YCbCrSubSampling", 0x212},
{"YCbCrPositioning", 0x213},
{"XResolution", 0x11A},
{"YResolution", 0x11B},
{"ResolutionUnit", 0x128},
{"StripOffsets", 0x111},
{"RowsPerStrip", 0x116},
{"StripByteCounts", 0x117},
{"JPEGInterchangeFormat", 0x201},
{"JPEGInterchangeFormatLength",0x202},
{"YCbCrCoefficients", 0x211},
{"ColorSpace", 0xA001},
{"ComponentsConfiguration", 0x9101},
So I think the concern about representing all of this as one monolithic
AV_FRAME_DATA_EXIF with only AV_SIDE_DATA_PROP_GLOBAL is valid.
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
