Re: [FFmpeg-devel] [PoC][PATCH 08/14] avutil/frame: add a param change side data type

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> Similar in purpose as the packet side data of the same name, but for encoders.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> An example of RefCount used for side data, including a copy and uninit callback
> to handle allocations within the entry.
> 
>  libavutil/frame.h     | 14 ++++++++++++++
>  libavutil/side_data.c | 23 +++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 1feb11506a..c677407781 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -243,8 +243,22 @@ enum AVFrameSideDataType {
>       * The data is an int storing the view ID.
>       */
>      AV_FRAME_DATA_VIEW_ID,
> +
> +    /**
> +     * A side data used to signal an encoder that certain parameters changed.
> +     * The payload is an AVParamChange struct.
> +     */
> +    AV_FRAME_DATA_PARAM_CHANGE,
>  };
>  
> +typedef struct AVParamChange {
> +    /**
> +     * A dictionary filled with options to be passed to an already initialized
> +     * encoder. May contain global and encoder private options.
> +     */
> +    AVDictionary *opts;
> +} AVParamChange;
> +
>  enum AVActiveFormatDescription {
>      AV_AFD_SAME         = 8,
>      AV_AFD_4_3          = 9,
> diff --git a/libavutil/side_data.c b/libavutil/side_data.c
> index 597228baf1..c156d2bef8 100644
> --- a/libavutil/side_data.c
> +++ b/libavutil/side_data.c
> @@ -63,6 +63,24 @@ typedef struct FFSideDataDescriptor {
>      size_t size;
>  } FFSideDataDescriptor;
>  
> +/* Type specific refstruct callbacks */
> +
> +static int copy_param_change(void *_dst, const void *_src)
> +{
> +    const AVParamChange *src = _src;
> +    AVParamChange *dst = _dst;
> +    int ret = av_dict_copy(&dst->opts, src->opts, 0);
> +    if (ret < 0)
> +        av_dict_free(&dst->opts);
> +    return ret;
> +}
> +
> +static void uninit_param_change(AVRefStructOpaque opaque, void *obj)
> +{
> +    AVParamChange *param = obj;
> +    av_dict_free(&param->opts);
> +}
> +
>  static const FFSideDataDescriptor sd_props[] = {
>      [AV_FRAME_DATA_PANSCAN]                     = { .p = { "AVPanScan",                                    AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
>                                                      .size = sizeof(AVPanScan) },
> @@ -88,6 +106,11 @@ static const FFSideDataDescriptor sd_props[] = {
>      [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_PARAM_CHANGE]                = { .p = { "Param Change",                                 AV_SIDE_DATA_PROP_STRUCT },
> +                                                    .props = FF_SIDE_DATA_PROP_REFSTRUCT,
> +                                                    .copy = copy_param_change,
> +                                                    .uninit = uninit_param_change,
> +                                                    .size = sizeof(AVParamChange) },
>      [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 },

I dislike the whole approach: Frame side data is supposed to contain
side-data that applies to the frame; yet a lot of encoder options which
will ultimately be added to this side data (if it gets committed) are
not frame properties at all.

Furthermore, this approach necessitates checking every frame (even of
encoders that do not support parameter changes) for the existence of
said side-data, although this side data will not exist in the
overwhelming majority of cases. Why not add a new function for this
task? (This does not imply that I am in favor of the reconfiguration
approach at all. I'm still leaning towards "close+reopen".)

- Andreas

_______________________________________________
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".