| Message ID | 20240923150146.31693-2-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/7] lavfi/buffersink: set AVFrame.time_base | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/configure_x86 | warning | Failed to apply patch |
Anton Khirnov (12024-09-23): > This way, av_buffersink_get_frame_flags() can replace almost all > av_buffersink_get_*(), including some future parameters we will want to > export. > --- > doc/APIchanges | 1 + > libavfilter/buffersink.c | 47 ++++++++++++++++++++++++++++++++++++++++ > libavfilter/buffersink.h | 16 ++++++++++++-- > 3 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index b392c756d7..5ddd7189f8 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -4,6 +4,7 @@ API changes, most recent first: > > 2024-09-xx - xxxxxxxxxx - lavfi 10.4.100 > Buffersink now sets AVFrame.time_base on the frames it outputs. > + Add AV_BUFFERSINK_FLAG_PARAMS. > > 2024-09-23 - xxxxxxxxxx - lavc 61.18.100 - avcodec.h > Add a new flag AV_CODEC_EXPORT_DATA_ENHANCEMENTS for export_side_data. > diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c > index 575075ff47..d9cd074f17 100644 > --- a/libavfilter/buffersink.c > +++ b/libavfilter/buffersink.c > @@ -128,8 +128,55 @@ static int get_frame_internal(AVFilterContext *ctx, AVFrame *frame, int flags, i > } > } > > +static int get_frame_params(AVFilterContext *ctx, AVFrame *frame) > +{ > + FilterLink *l = ff_filter_link(ctx->inputs[0]); > + int ret = 0; > + > + frame->time_base = l->pub.time_base; > + frame->format = l->pub.format; > + > + switch (l->pub.type) { > + case AVMEDIA_TYPE_AUDIO: > + ret = av_channel_layout_copy(&frame->ch_layout, &l->pub.ch_layout); > + if (ret < 0) > + goto fail; > + > + frame->sample_rate = l->pub.sample_rate; > + break; > + case AVMEDIA_TYPE_VIDEO: > + frame->width = l->pub.w; > + frame->height = l->pub.h; > + frame->sample_aspect_ratio = l->pub.sample_aspect_ratio; > + frame->colorspace = l->pub.colorspace; > + frame->color_range = l->pub.color_range; > + break; > + default: av_assert0(0); > + } > + > + if (l->hw_frames_ctx) { > + frame->hw_frames_ctx = av_buffer_ref(l->hw_frames_ctx); > + if (!frame->hw_frames_ctx) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > + } > + > + return 0; > +fail: > + av_frame_unref(frame); > + return ret; > +} > + > int attribute_align_arg av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flags) > { > + if (flags & AV_BUFFERSINK_FLAG_PARAMS) { > + if (flags & ~AV_BUFFERSINK_FLAG_PARAMS) > + return AVERROR(EINVAL); > + > + return get_frame_params(ctx, frame); > + } > + > return get_frame_internal(ctx, frame, flags, > ff_filter_link(ctx->inputs[0])->min_samples); > } > diff --git a/libavfilter/buffersink.h b/libavfilter/buffersink.h > index 361d603679..9c4468af5b 100644 > --- a/libavfilter/buffersink.h > +++ b/libavfilter/buffersink.h > @@ -87,14 +87,26 @@ int av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flag > * reference, but not remove it from the buffer. This is useful if you > * need only to read a video/samples buffer, without to fetch it. > */ > -#define AV_BUFFERSINK_FLAG_PEEK 1 > +#define AV_BUFFERSINK_FLAG_PEEK (1 << 0) > > /** > * Tell av_buffersink_get_buffer_ref() not to request a frame from its input. > * If a frame is already buffered, it is read (and removed from the buffer), > * but if no frame is present, return AVERROR(EAGAIN). > */ > -#define AV_BUFFERSINK_FLAG_NO_REQUEST 2 > +#define AV_BUFFERSINK_FLAG_NO_REQUEST (1 << 1) > + > +/** > + * Retrieve stream parameters rather than frame data. > + * > + * When this flag is set, av_buffersink_get_frame_flags() fills the non-data > + * fields of the supplied frame without causing any filtergraph activity. > + * > + * @note While frame data will be NULL, certain other allocated fields may be > + * filled (e.g. ch_layout, hw_frames_ctx), so the frame should still be > + * unreffed before reuse. > + */ > +#define AV_BUFFERSINK_FLAG_PARAMS (1 << 2) NAK, this is one of the worst API design I can think of for this task. > > /** > * Set the frame size for an audio buffer sink.
On Mon, 23 Sep 2024, Anton Khirnov wrote: > This way, av_buffersink_get_frame_flags() can replace almost all > av_buffersink_get_*(), including some future parameters we will want to > export. Why don't you simply expose the AVFilterLink which contains all the parameters of the sink? That seems a lot cleaner. Thanks, Marton > --- > doc/APIchanges | 1 + > libavfilter/buffersink.c | 47 ++++++++++++++++++++++++++++++++++++++++ > libavfilter/buffersink.h | 16 ++++++++++++-- > 3 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index b392c756d7..5ddd7189f8 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -4,6 +4,7 @@ API changes, most recent first: > > 2024-09-xx - xxxxxxxxxx - lavfi 10.4.100 > Buffersink now sets AVFrame.time_base on the frames it outputs. > + Add AV_BUFFERSINK_FLAG_PARAMS. > > 2024-09-23 - xxxxxxxxxx - lavc 61.18.100 - avcodec.h > Add a new flag AV_CODEC_EXPORT_DATA_ENHANCEMENTS for export_side_data. > diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c > index 575075ff47..d9cd074f17 100644 > --- a/libavfilter/buffersink.c > +++ b/libavfilter/buffersink.c > @@ -128,8 +128,55 @@ static int get_frame_internal(AVFilterContext *ctx, AVFrame *frame, int flags, i > } > } > > +static int get_frame_params(AVFilterContext *ctx, AVFrame *frame) > +{ > + FilterLink *l = ff_filter_link(ctx->inputs[0]); > + int ret = 0; > + > + frame->time_base = l->pub.time_base; > + frame->format = l->pub.format; > + > + switch (l->pub.type) { > + case AVMEDIA_TYPE_AUDIO: > + ret = av_channel_layout_copy(&frame->ch_layout, &l->pub.ch_layout); > + if (ret < 0) > + goto fail; > + > + frame->sample_rate = l->pub.sample_rate; > + break; > + case AVMEDIA_TYPE_VIDEO: > + frame->width = l->pub.w; > + frame->height = l->pub.h; > + frame->sample_aspect_ratio = l->pub.sample_aspect_ratio; > + frame->colorspace = l->pub.colorspace; > + frame->color_range = l->pub.color_range; > + break; > + default: av_assert0(0); > + } > + > + if (l->hw_frames_ctx) { > + frame->hw_frames_ctx = av_buffer_ref(l->hw_frames_ctx); > + if (!frame->hw_frames_ctx) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > + } > + > + return 0; > +fail: > + av_frame_unref(frame); > + return ret; > +} > + > int attribute_align_arg av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flags) > { > + if (flags & AV_BUFFERSINK_FLAG_PARAMS) { > + if (flags & ~AV_BUFFERSINK_FLAG_PARAMS) > + return AVERROR(EINVAL); > + > + return get_frame_params(ctx, frame); > + } > + > return get_frame_internal(ctx, frame, flags, > ff_filter_link(ctx->inputs[0])->min_samples); > } > diff --git a/libavfilter/buffersink.h b/libavfilter/buffersink.h > index 361d603679..9c4468af5b 100644 > --- a/libavfilter/buffersink.h > +++ b/libavfilter/buffersink.h > @@ -87,14 +87,26 @@ int av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flag > * reference, but not remove it from the buffer. This is useful if you > * need only to read a video/samples buffer, without to fetch it. > */ > -#define AV_BUFFERSINK_FLAG_PEEK 1 > +#define AV_BUFFERSINK_FLAG_PEEK (1 << 0) > > /** > * Tell av_buffersink_get_buffer_ref() not to request a frame from its input. > * If a frame is already buffered, it is read (and removed from the buffer), > * but if no frame is present, return AVERROR(EAGAIN). > */ > -#define AV_BUFFERSINK_FLAG_NO_REQUEST 2 > +#define AV_BUFFERSINK_FLAG_NO_REQUEST (1 << 1) > + > +/** > + * Retrieve stream parameters rather than frame data. > + * > + * When this flag is set, av_buffersink_get_frame_flags() fills the non-data > + * fields of the supplied frame without causing any filtergraph activity. > + * > + * @note While frame data will be NULL, certain other allocated fields may be > + * filled (e.g. ch_layout, hw_frames_ctx), so the frame should still be > + * unreffed before reuse. > + */ > +#define AV_BUFFERSINK_FLAG_PARAMS (1 << 2) > > /** > * Set the frame size for an audio buffer sink. > -- > 2.43.0 > > _______________________________________________ > 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". >
Quoting Marton Balint (2024-09-25 10:25:44) > > > On Mon, 23 Sep 2024, Anton Khirnov wrote: > > > This way, av_buffersink_get_frame_flags() can replace almost all > > av_buffersink_get_*(), including some future parameters we will want to > > export. > > Why don't you simply expose the AVFilterLink which contains all the > parameters of the sink? That seems a lot cleaner. Because AVFilterLink is internal state of the filtergraph and should not be exposed at all.
Anton Khirnov (12024-09-25): > Because AVFilterLink is internal state of the filtergraph and should not > be exposed at all. Anyway, if you cannot come up with an API that does not require dynamic allocations and all the boilerplate code it requires just to retrieve a few integers, then drop the series altogether.
On Wed, 25 Sep 2024, Nicolas George wrote: > Anton Khirnov (12024-09-25): >> Because AVFilterLink is internal state of the filtergraph and should not >> be exposed at all. > > Anyway, if you cannot come up with an API that does not require dynamic > allocations and all the boilerplate code it requires just to retrieve a > few integers, then drop the series altogether. I kind of agree. It is suboptimal that the buffersink stores all its parameters already in a public struct, but you can only access them if you get yourself an owned copy with all the overhead of copying / referencing, plus now you have to do extra allocation for the results and error checking as well. Also I don't like that you are misusing an AVFrame struct to pass parameters. How the user should know which parameters are set in AVFrame and which are not? A dedicated struct would be better, or just use AVFilterLink. Regards, Marton
On 9/25/2024 4:27 PM, Marton Balint wrote: > > > On Wed, 25 Sep 2024, Nicolas George wrote: > >> Anton Khirnov (12024-09-25): >>> Because AVFilterLink is internal state of the filtergraph and should not >>> be exposed at all. >> >> Anyway, if you cannot come up with an API that does not require dynamic >> allocations and all the boilerplate code it requires just to retrieve a >> few integers, then drop the series altogether. > > I kind of agree. It is suboptimal that the buffersink stores all its > parameters already in a public struct, but you can only access them if > you get yourself an owned copy with all the overhead of copying / > referencing, plus now you have to do extra allocation for the results > and error checking as well. > > Also I don't like that you are misusing an AVFrame struct to pass > parameters. How the user should know which parameters are set in AVFrame > and which are not? A dedicated struct would be better, or just use > AVFilterLink. There is AVBufferSrcParameters, which could be renamed and reused here. Exact same fields, but for the opposite purpose. It still requires allocating it before using it, but since a normal filterchain will have buffersrc -> [...] -> buffersink, the same allocated struct can be used for both. The main benefit i see from doing this is that, much like you don't create references/copies for the fields you put in AVBufferSrcParameters because av_buffersrc_parameters_set() is what internally will do that, buffersink wouldn't need to create references/copies to store anything there either, leaving that to the caller (who will be informed the fields in the output struct are still owned by the library).
James Almer (12024-09-25): > It still requires allocating it before using it, but since a normal > filterchain will have buffersrc -> [...] -> buffersink, the same allocated > struct can be used for both. A filter chain can have buffersinks but no buffersrc, if it uses other sources. Also, a dynamic allocation does the same damage to the code readability and reliability whether it is in a speed-critical part of the code or not. We should be trying to REMOVE instances where a dynamic allocation is necessary, not add to them. This consideration takes precedence over cosmetic considerations like internal consistency or adherence to established practices (aka coding like Java developers who see C for the first time). In this case, that means we should find a solution without dynamic allocation for buffersink and then implement it for buffersrc also rather than using the solution with dynamic allocation on both. And we should do that especially because it is very easy: two solutions came to me in the first few minutes, more could come. - Let callers allocate their structure as they wish, on the stack if necessary, pass sizeof(params) as an extra argument and use memcpy() to fetch/return it. - Have an instance of the struct in the buffersrc/sink and make it available for that use. Regards,
diff --git a/doc/APIchanges b/doc/APIchanges index b392c756d7..5ddd7189f8 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -4,6 +4,7 @@ API changes, most recent first: 2024-09-xx - xxxxxxxxxx - lavfi 10.4.100 Buffersink now sets AVFrame.time_base on the frames it outputs. + Add AV_BUFFERSINK_FLAG_PARAMS. 2024-09-23 - xxxxxxxxxx - lavc 61.18.100 - avcodec.h Add a new flag AV_CODEC_EXPORT_DATA_ENHANCEMENTS for export_side_data. diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c index 575075ff47..d9cd074f17 100644 --- a/libavfilter/buffersink.c +++ b/libavfilter/buffersink.c @@ -128,8 +128,55 @@ static int get_frame_internal(AVFilterContext *ctx, AVFrame *frame, int flags, i } } +static int get_frame_params(AVFilterContext *ctx, AVFrame *frame) +{ + FilterLink *l = ff_filter_link(ctx->inputs[0]); + int ret = 0; + + frame->time_base = l->pub.time_base; + frame->format = l->pub.format; + + switch (l->pub.type) { + case AVMEDIA_TYPE_AUDIO: + ret = av_channel_layout_copy(&frame->ch_layout, &l->pub.ch_layout); + if (ret < 0) + goto fail; + + frame->sample_rate = l->pub.sample_rate; + break; + case AVMEDIA_TYPE_VIDEO: + frame->width = l->pub.w; + frame->height = l->pub.h; + frame->sample_aspect_ratio = l->pub.sample_aspect_ratio; + frame->colorspace = l->pub.colorspace; + frame->color_range = l->pub.color_range; + break; + default: av_assert0(0); + } + + if (l->hw_frames_ctx) { + frame->hw_frames_ctx = av_buffer_ref(l->hw_frames_ctx); + if (!frame->hw_frames_ctx) { + ret = AVERROR(ENOMEM); + goto fail; + } + } + + return 0; +fail: + av_frame_unref(frame); + return ret; +} + int attribute_align_arg av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flags) { + if (flags & AV_BUFFERSINK_FLAG_PARAMS) { + if (flags & ~AV_BUFFERSINK_FLAG_PARAMS) + return AVERROR(EINVAL); + + return get_frame_params(ctx, frame); + } + return get_frame_internal(ctx, frame, flags, ff_filter_link(ctx->inputs[0])->min_samples); } diff --git a/libavfilter/buffersink.h b/libavfilter/buffersink.h index 361d603679..9c4468af5b 100644 --- a/libavfilter/buffersink.h +++ b/libavfilter/buffersink.h @@ -87,14 +87,26 @@ int av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flag * reference, but not remove it from the buffer. This is useful if you * need only to read a video/samples buffer, without to fetch it. */ -#define AV_BUFFERSINK_FLAG_PEEK 1 +#define AV_BUFFERSINK_FLAG_PEEK (1 << 0) /** * Tell av_buffersink_get_buffer_ref() not to request a frame from its input. * If a frame is already buffered, it is read (and removed from the buffer), * but if no frame is present, return AVERROR(EAGAIN). */ -#define AV_BUFFERSINK_FLAG_NO_REQUEST 2 +#define AV_BUFFERSINK_FLAG_NO_REQUEST (1 << 1) + +/** + * Retrieve stream parameters rather than frame data. + * + * When this flag is set, av_buffersink_get_frame_flags() fills the non-data + * fields of the supplied frame without causing any filtergraph activity. + * + * @note While frame data will be NULL, certain other allocated fields may be + * filled (e.g. ch_layout, hw_frames_ctx), so the frame should still be + * unreffed before reuse. + */ +#define AV_BUFFERSINK_FLAG_PARAMS (1 << 2) /** * Set the frame size for an audio buffer sink.