From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 04/11] avutil/frame: av_frame_side_data_new_struct()
Date: Thu, 6 Mar 2025 15:50:27 +0100
Message-ID: <AS8P250MB0744DB9E6E40DE316F5D107F8FCA2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20250220172454.2952-4-jamrial@gmail.com>
James Almer:
> As well as the AV_SIDE_DATA_PROP_STRUCT prop, to define types that describe
> a fixed-size C struct and not a flat byte array.
> This excludes types like VIDEO_ENC_PARAMS as the struct it describes may have
> extra bytes allocated past the end of the struct.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> libavutil/ambient_viewing_environment.c | 9 ++-
> libavutil/frame.h | 17 ++++++
> libavutil/mastering_display_metadata.c | 9 ++-
> libavutil/side_data.c | 77 +++++++++++++++++++++----
> libavutil/side_data.h | 4 ++
> libavutil/spherical.c | 10 +++-
> 6 files changed, 107 insertions(+), 19 deletions(-)
>
> diff --git a/libavutil/ambient_viewing_environment.c b/libavutil/ambient_viewing_environment.c
> index e359727776..ee2e9427cd 100644
> --- a/libavutil/ambient_viewing_environment.c
> +++ b/libavutil/ambient_viewing_environment.c
> @@ -20,9 +20,12 @@
>
> #include "ambient_viewing_environment.h"
> #include "mem.h"
> +#include "side_data.h"
>
> -static void get_defaults(AVAmbientViewingEnvironment *env)
> +void ff_ave_get_defaults(void *obj)
> {
> + AVAmbientViewingEnvironment *env = obj;
> +
> env->ambient_illuminance =
> env->ambient_light_x =
> env->ambient_light_y = (AVRational) { 0, 1 };
> @@ -35,7 +38,7 @@ AVAmbientViewingEnvironment *av_ambient_viewing_environment_alloc(size_t *size)
> if (!env)
> return NULL;
>
> - get_defaults(env);
> + ff_ave_get_defaults(env);
>
> if (size)
> *size = sizeof(*env);
> @@ -53,7 +56,7 @@ AVAmbientViewingEnvironment *av_ambient_viewing_environment_create_side_data(AVF
> return NULL;
>
> memset(side_data->data, 0, side_data->size);
> - get_defaults((AVAmbientViewingEnvironment *)side_data->data);
> + ff_ave_get_defaults(side_data->data);
>
> return (AVAmbientViewingEnvironment *)side_data->data;
> }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index a707b35087..cd6b5194d1 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -304,6 +304,13 @@ enum AVSideDataProps {
> * or adjusted to the new layout.
> */
> AV_SIDE_DATA_PROP_CHANNEL_DEPENDENT = (1 << 4),
> +
> + /**
> + * Side data stores a fixed-size C struct and not a flat byte array.
> + * Entries of this type should be allocated with
> + * @ref av_frame_side_data_new_struct().
> + */
> + AV_SIDE_DATA_PROP_STRUCT = (1 << 5),
> };
>
> /**
> @@ -1121,6 +1128,16 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
> enum AVFrameSideDataType type,
> size_t size, unsigned int flags);
>
> +/**
> + * Variant of @ref av_frame_side_data_new() to add side data entires of a type
s/entires/entries/
> + * with the AV_SIDE_DATA_PROP_STRUCT prop.
Using AV_SIDE_DATA_PROP_STRUCT has a problem: It excludes trivial cases
like AV_FRAME_DATA_AFD, AV_FRAME_DATA_SKIP_SAMPLES and
AV_FRAME_DATA_VIEW_ID as well as not so trivial cases like
AV_FRAME_DATA_DOVI_METADATA. Maybe simply use a flag that says "can be
used with av_frame_side_data_new_struct()" instead.
> + *
> + * @return newly added side data on success, NULL on error.
> + */
> +AVFrameSideData *av_frame_side_data_new_struct(AVFrameSideData ***sd, int *nb_sd,
> + enum AVFrameSideDataType type,
> + unsigned int flags);
Can't you add a new parameter nb_elems to also handle all the side data
that only has a single array? This would then also cover
AV_FRAME_DATA_REGIONS_OF_INTEREST, AV_FRAME_DATA_VIDEO_ENC_PARAMS,
AV_FRAME_DATA_DETECTION_BBOXES, AV_FRAME_DATA_VIDEO_HINT.
AV_FRAME_DATA_A53_CC, AV_FRAME_DATA_ICC_PROFILE,
AV_FRAME_DATA_SEI_UNREGISTERED, AV_FRAME_DATA_DOVI_RPU_BUFFER and
AV_FRAME_DATA_LCEVC.
If you want to, I can write the patch for this.
> +
> /**
> * Add a new side data entry to an array from an existing AVBufferRef.
> *
> diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c
> index dd37ed7d0e..4948f30523 100644
> --- a/libavutil/mastering_display_metadata.c
> +++ b/libavutil/mastering_display_metadata.c
> @@ -24,9 +24,12 @@
>
> #include "mastering_display_metadata.h"
> #include "mem.h"
> +#include "side_data.h"
>
> -static void get_defaults(AVMasteringDisplayMetadata *mastering)
> +void ff_mdm_get_defaults(void *obj)
> {
> + AVMasteringDisplayMetadata *mastering = obj;
> +
> for (int i = 0; i < 3; i++)
> for (int j = 0; j < 2; j++)
> mastering->display_primaries[i][j] = (AVRational) { 0, 1 };
> @@ -47,7 +50,7 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *siz
> if (!mastering)
> return NULL;
>
> - get_defaults(mastering);
> + ff_mdm_get_defaults(mastering);
>
> if (size)
> *size = sizeof(*mastering);
> @@ -64,7 +67,7 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_create_side_data(AVFra
> return NULL;
>
> memset(side_data->data, 0, sizeof(AVMasteringDisplayMetadata));
> - get_defaults((AVMasteringDisplayMetadata *)side_data->data);
> + ff_mdm_get_defaults(side_data->data);
>
> return (AVMasteringDisplayMetadata *)side_data->data;
> }
> diff --git a/libavutil/side_data.c b/libavutil/side_data.c
> index beb8ea3212..49197e321d 100644
> --- a/libavutil/side_data.c
> +++ b/libavutil/side_data.c
> @@ -24,43 +24,78 @@
> #include "mem.h"
> #include "side_data.h"
>
> +// headers for struct sizes
> +#include "libavcodec/defs.h"
> +#include "ambient_viewing_environment.h"
> +#include "downmix_info.h"
> +#include "hdr_dynamic_metadata.h"
> +#include "hdr_dynamic_vivid_metadata.h"
> +#include "mastering_display_metadata.h"
> +#include "motion_vector.h"
> +#include "replaygain.h"
> +#include "spherical.h"
> +#include "stereo3d.h"
> +
> typedef struct FFSideDataDescriptor {
> AVSideDataDescriptor p;
> +
> + void (*init)(void *obj);
Can't you use a switch instead to avoid the relocations?
> +
> + size_t size;
> } FFSideDataDescriptor;
>
> static const FFSideDataDescriptor sd_props[] = {
> - [AV_FRAME_DATA_PANSCAN] = { .p = { "AVPanScan", AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> + [AV_FRAME_DATA_PANSCAN] = { .p = { "AVPanScan", AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
> + .size = sizeof(AVPanScan) },
> [AV_FRAME_DATA_A53_CC] = { .p = { "ATSC A53 Part 4 Closed Captions" } },
> [AV_FRAME_DATA_MATRIXENCODING] = { .p = { "AVMatrixEncoding", AV_SIDE_DATA_PROP_CHANNEL_DEPENDENT } },
> - [AV_FRAME_DATA_DOWNMIX_INFO] = { .p = { "Metadata relevant to a downmix procedure", AV_SIDE_DATA_PROP_CHANNEL_DEPENDENT } },
> + [AV_FRAME_DATA_DOWNMIX_INFO] = { .p = { "Metadata relevant to a downmix procedure", AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_CHANNEL_DEPENDENT },
> + .size = sizeof(AVDownmixInfo) },
> [AV_FRAME_DATA_AFD] = { .p = { "Active format description" } },
> - [AV_FRAME_DATA_MOTION_VECTORS] = { .p = { "Motion vectors", AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> + [AV_FRAME_DATA_MOTION_VECTORS] = { .p = { "Motion vectors", AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
> + .size = sizeof(AVMotionVector) },
> [AV_FRAME_DATA_SKIP_SAMPLES] = { .p = { "Skip samples" } },
> [AV_FRAME_DATA_GOP_TIMECODE] = { .p = { "GOP timecode" } },
> [AV_FRAME_DATA_S12M_TIMECODE] = { .p = { "SMPTE 12-1 timecode" } },
> - [AV_FRAME_DATA_DYNAMIC_HDR_PLUS] = { .p = { "HDR Dynamic Metadata SMPTE2094-40 (HDR10+)", AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> - [AV_FRAME_DATA_DYNAMIC_HDR_VIVID] = { .p = { "HDR Dynamic Metadata CUVA 005.1 2021 (Vivid)", AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> + [AV_FRAME_DATA_DYNAMIC_HDR_PLUS] = { .p = { "HDR Dynamic Metadata SMPTE2094-40 (HDR10+)", AV_SIDE_DATA_PROP_STRUCT| AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
> + .size = sizeof(AVDynamicHDRPlus) },
> + [AV_FRAME_DATA_DYNAMIC_HDR_VIVID] = { .p = { "HDR Dynamic Metadata CUVA 005.1 2021 (Vivid)", AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
> + .size = sizeof(AVDynamicHDRVivid) },
> [AV_FRAME_DATA_REGIONS_OF_INTEREST] = { .p = { "Regions Of Interest", AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> [AV_FRAME_DATA_VIDEO_ENC_PARAMS] = { .p = { "Video encoding parameters" } },
> - [AV_FRAME_DATA_FILM_GRAIN_PARAMS] = { .p = { "Film grain parameters" } },
> + [AV_FRAME_DATA_FILM_GRAIN_PARAMS] = { .p = { "Film grain parameters", AV_SIDE_DATA_PROP_STRUCT } },
> [AV_FRAME_DATA_DETECTION_BBOXES] = { .p = { "Bounding boxes for object detection and classification", AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> [AV_FRAME_DATA_DOVI_RPU_BUFFER] = { .p = { "Dolby Vision RPU Data", AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> [AV_FRAME_DATA_DOVI_METADATA] = { .p = { "Dolby Vision Metadata", AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> [AV_FRAME_DATA_LCEVC] = { .p = { "LCEVC NAL data", AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> [AV_FRAME_DATA_VIEW_ID] = { .p = { "View ID" } },
> - [AV_FRAME_DATA_STEREO3D] = { .p = { "Stereo 3D", AV_SIDE_DATA_PROP_GLOBAL } },
> - [AV_FRAME_DATA_REPLAYGAIN] = { .p = { "AVReplayGain", AV_SIDE_DATA_PROP_GLOBAL } },
> + [AV_FRAME_DATA_STEREO3D] = { .p = { "Stereo 3D", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT },
> + .size = sizeof(AVStereo3D) },
> + [AV_FRAME_DATA_REPLAYGAIN] = { .p = { "AVReplayGain", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT },
> + .size = sizeof(AVReplayGain) },
> [AV_FRAME_DATA_DISPLAYMATRIX] = { .p = { "3x3 displaymatrix", AV_SIDE_DATA_PROP_GLOBAL } },
> [AV_FRAME_DATA_AUDIO_SERVICE_TYPE] = { .p = { "Audio service type", AV_SIDE_DATA_PROP_GLOBAL } },
> - [AV_FRAME_DATA_MASTERING_DISPLAY_METADATA] = { .p = { "Mastering display metadata", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> - [AV_FRAME_DATA_CONTENT_LIGHT_LEVEL] = { .p = { "Content light level metadata", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> - [AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT] = { .p = { "Ambient viewing environment", AV_SIDE_DATA_PROP_GLOBAL } },
> - [AV_FRAME_DATA_SPHERICAL] = { .p = { "Spherical Mapping", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> + [AV_FRAME_DATA_MASTERING_DISPLAY_METADATA] = { .p = { "Mastering display metadata", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
> + .init = ff_mdm_get_defaults,
> + .size = sizeof(AVMasteringDisplayMetadata) },
> + [AV_FRAME_DATA_CONTENT_LIGHT_LEVEL] = { .p = { "Content light level metadata", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
> + .size = sizeof(AVContentLightMetadata) },
> + [AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT] = { .p = { "Ambient viewing environment", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT },
> + .init = ff_ave_get_defaults,
> + .size = sizeof(AVAmbientViewingEnvironment) },
> + [AV_FRAME_DATA_SPHERICAL] = { .p = { "Spherical Mapping", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
> + .init = ff_spherical_get_defaults,
> + .size = sizeof(AVSphericalMapping) },
> [AV_FRAME_DATA_ICC_PROFILE] = { .p = { "ICC profile", AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
> [AV_FRAME_DATA_SEI_UNREGISTERED] = { .p = { "H.26[45] User Data Unregistered SEI message", AV_SIDE_DATA_PROP_MULTI } },
> [AV_FRAME_DATA_VIDEO_HINT] = { .p = { "Encoding video hint", AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
> };
>
> +static const FFSideDataDescriptor *dp_from_desc(const AVSideDataDescriptor *desc)
> +{
> + return (const FFSideDataDescriptor *)desc;
> +}
> +
> const AVSideDataDescriptor *av_frame_side_data_desc(enum AVFrameSideDataType type)
> {
> unsigned t = type;
> @@ -247,6 +282,24 @@ AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
> return sd_dst;
> }
>
> +AVFrameSideData *av_frame_side_data_new_struct(AVFrameSideData ***sd, int *nb_sd,
> + enum AVFrameSideDataType type,
> + unsigned int flags)
> +{
> + const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
> + const FFSideDataDescriptor *dp = dp_from_desc(desc);
> + AVFrameSideData *ret;
> +
> + if (!desc || !(desc->props & AV_SIDE_DATA_PROP_STRUCT))
> + return NULL;
> +
> + av_assert0(dp->size);
> + ret = av_frame_side_data_new(sd, nb_sd, type, dp->size, flags);
> + if (ret && dp->init)
> + dp->init(ret->data);
> + return ret;
> +}
> +
> int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
> const AVFrameSideData *src, unsigned int flags)
> {
> diff --git a/libavutil/side_data.h b/libavutil/side_data.h
> index 8275aa35a5..5d25833882 100644
> --- a/libavutil/side_data.h
> +++ b/libavutil/side_data.h
> @@ -27,4 +27,8 @@ AVFrameSideData *ff_frame_side_data_add_from_buf(AVFrameSideData ***sd,
> enum AVFrameSideDataType type,
> AVBufferRef *buf);
>
> +void ff_mdm_get_defaults(void *obj);
> +void ff_ave_get_defaults(void *obj);
> +void ff_spherical_get_defaults(void *obj);
> +
> #endif // AVUTIL_SIDE_DATA_H
> diff --git a/libavutil/spherical.c b/libavutil/spherical.c
> index 64ade1d0ec..4ef2c225c2 100644
> --- a/libavutil/spherical.c
> +++ b/libavutil/spherical.c
> @@ -21,15 +21,23 @@
> #include "avstring.h"
> #include "macros.h"
> #include "mem.h"
> +#include "side_data.h"
> #include "spherical.h"
>
> +void ff_spherical_get_defaults(void *obj)
> +{
> + AVSphericalMapping *spherical = obj;
> +
> + spherical->projection = AV_SPHERICAL_RECTILINEAR;
> +}
> +
Can't this be inlined?
> AVSphericalMapping *av_spherical_alloc(size_t *size)
> {
> AVSphericalMapping *spherical = av_mallocz(sizeof(AVSphericalMapping));
> if (!spherical)
> return NULL;
>
> - spherical->projection = AV_SPHERICAL_RECTILINEAR;
> + ff_spherical_get_defaults(spherical);
>
> if (size)
> *size = sizeof(*spherical);
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
next prev parent reply other threads:[~2025-03-06 14:50 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-20 17:24 [FFmpeg-devel] [PATCH 01/11] avutil/frame: move side data helpers to a new file James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 02/11] avutil/side_data: allow the addition of internal fields to AVSideDataDescriptor James Almer
2025-03-06 14:56 ` Andreas Rheinhardt
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 03/11] avutil/frame: add functions to check and ensure a side data entry is writable James Almer
2025-02-21 11:44 ` Andreas Rheinhardt
2025-02-21 12:25 ` James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 04/11] avutil/frame: av_frame_side_data_new_struct() James Almer
2025-03-06 14:50 ` Andreas Rheinhardt [this message]
2025-03-06 15:39 ` James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 05/11] avutil/ambient_viewing_environment: deprecate av_ambient_viewing_environment_create_side_data() James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 06/11] avutil/downmix_info: deprecate av_downmix_info_update_side_data() James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 07/11] avutil/hdr_dynamic_metadata: deprecate av_dynamic_hdr_plus_create_side_data() James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 08/11] avutil/hdr_dynamic_vivid_metadata: deprecate av_dynamic_hdr_vivid_create_side_data() James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 09/11] avutil/mastering_display_metadata: deprecate av_{content_light, mastering_display_metadata}_create_side_data() James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 10/11] avutil/mastering_display_metadata: deprecate av_mastering_display_metadata_alloc() James Almer
2025-02-20 17:24 ` [FFmpeg-devel] [PATCH 11/11] avutil/stereo3d: deprecate av_stereo3d_create_side_data() James Almer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS8P250MB0744DB9E6E40DE316F5D107F8FCA2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM \
--to=andreas.rheinhardt@outlook.com \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git