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

> commit bb90b262d6d23f1bca3587a48abc15b951cbbf05
> Author: Leo Izen <[email protected]>
> Date:   Wed Mar 5 09:20:20 2025 -0500
>
>     avutil/side_data: add EXIF side data type
>
>     This commit adds support for the additional side data type
>     AV_FRAME_DATA_EXIF, which contains a buffer of an EXIF metadata
>     payload.
>
>     Signed-off-by: Leo Izen <[email protected]>
>
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index d440cfba2e3..d7d98e67e4b 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -254,6 +254,12 @@ enum AVFrameSideDataType {
>       * libavutil/tdrdi.h.
>       */
>      AV_FRAME_DATA_3D_REFERENCE_DISPLAYS,
> +
> +    /**
> +     * Extensible image file format metadata. The payload is a buffer 
> containing

EXIF is Exchangeable not Extensible


> +     * EXIF metadata, starting with either 49 49 2a 00, or 4d 4d 00 2a.

This could be a bit more verbose and mention what these bytes are not just 
their value

> +     */
> +     AV_FRAME_DATA_EXIF,

The indention is off by 1 relative to teh previous


> 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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If people can choose the ruler, we call it a democracy-even if they can’t
influence a single law or decision, and the leader can break every promise.
If people can’t choose the ruler, we call it a dictatorship-even if public
preferences are followed every time. -- chatgpt 5.2

Attachment: signature.asc
Description: PGP signature

_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]

Reply via email to