Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Wed, Jul 30, 2014 at 11:44:47PM +0200, Nicolas George wrote:
> Also deprecate adding a NULL frame to mark EOF.
>
> TODO APIchanges entry, version bump.
>
> Signed-off-by: Nicolas George
> ---
> libavfilter/buffersrc.c | 40 ++--
> libavfilter/buffersrc.h | 15 +++
> 2 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> index 27d3db0..6d71587 100644
> --- a/libavfilter/buffersrc.c
> +++ b/libavfilter/buffersrc.c
> @@ -63,6 +63,7 @@ typedef struct BufferSourceContext {
> char*channel_layout_str;
>
> int eof;
> +int64_t eof_pts;
> } BufferSourceContext;
>
> #define CHECK_VIDEO_PARAM_CHANGE(s, c, width, height, format)\
> @@ -77,6 +78,27 @@ typedef struct BufferSourceContext {
> return AVERROR(EINVAL);\
> }
>
> +static int push_if_flag(AVFilterContext *ctx, int flags)
> +{
> +return (flags & AV_BUFFERSRC_FLAG_PUSH) ?
> +ctx->output_pads[0].request_frame(ctx->outputs[0]) : 0;
> +}
> +
> +int av_buffersrc_close(AVFilterContext *ctx, int64_t pts, int flags)
> +{
> +BufferSourceContext *s = ctx->priv;
> +
> +if (pts == AV_NOPTS_VALUE) {
> +av_log(ctx, AV_LOG_WARNING, "No EOF timestamp\n");
> +/* FIXME use duration for audio */
> +pts = av_rescale_q(ctx->outputs[0]->current_pts,
> + AV_TIME_BASE_Q, ctx->outputs[0]->time_base) + 1;
> +}
> +s->eof_pts = pts;
> +s->eof = 1;
> +return push_if_flag(ctx, flags);
> +}
> +
> int attribute_align_arg av_buffersrc_write_frame(AVFilterContext *ctx, const
> AVFrame *frame)
> {
> return av_buffersrc_add_frame_flags(ctx, (AVFrame *)frame,
> @@ -125,8 +147,7 @@ static int
> av_buffersrc_add_frame_internal(AVFilterContext *ctx,
> s->nb_failed_requests = 0;
>
> if (!frame) {
> -s->eof = 1;
> -return 0;
> +return av_buffersrc_close(ctx, AV_NOPTS_VALUE, flags);
> } else if (s->eof)
> return AVERROR(EINVAL);
>
> @@ -177,11 +198,7 @@ static int
> av_buffersrc_add_frame_internal(AVFilterContext *ctx,
> return ret;
> }
>
> -if ((flags & AV_BUFFERSRC_FLAG_PUSH))
> -if ((ret = ctx->output_pads[0].request_frame(ctx->outputs[0])) < 0)
> -return ret;
> -
> -return 0;
> +return push_if_flag(ctx, flags);
> }
some of this patch seems to be factorizing code,
other add fuctionality like ff_filter_link_close()
all LGTM i think but maybe should be split
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
Le duodi 12 thermidor, an CCXXII, Derek Buitenhuis a écrit : > Yeah, why should you have to explain the reason for a change in > a review of that change? How silly. I should just divine the reason > why. I guess we don't count as "everyone". To review a change, the minimum is to read it. The answers to the first questions were in the proposed commit messages. I hope you'll excuse me if I take offence when someone nags me with questions while they obviously did not care enough to read them. If you have read them and would like details or find something unclear, OTOH, I would gladly explain what needs explaining. New patch series sent, with the constructive comments hopefully taken into account. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On 7/30/2014 6:10 PM, Nicolas George wrote: > This is tiring. Everyone knows it already and it has been explained numerous > times. Yeah, why should you have to explain the reason for a change in a review of that change? How silly. I should just divine the reason why. I guess we don't count as "everyone". - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
Le duodi 12 thermidor, an CCXXII, wm4 a écrit : > Demuxers and decoding don't return such an end PTS, so why should > filters? Demuxers apparently use the frame duration to time the last > frame, so why not use that in lavfi too? (Demuxers also report the > total length, but that is probably not useful at all for timing the > last frame.) Packets have duration, frames do not. Using the duration causes consistency problems. This has been discussed numerous times, I do not intend to spend more time repeating the obvious. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Wed, 30 Jul 2014 19:10:04 +0200 Nicolas George wrote: > Le duodi 12 thermidor, an CCXXII, wm4 a écrit : > > You never explained what's wrong with it. > > This is tiring. Everyone knows it already and it has been explained numerous > times. > > THERE IS NO TIMESTAMP FOR THE END OF STREAM. You could just have said that right in the beginning. Even in the commit message you could have made explicit that the purpose of the change is adding an end PTS, instead of that being a positive side-effect. > Is it enough, or do I have to waste more time explaining how that makes the > duration of the last frame wrong, and how that is a problem when accurate > timing is necessary, for concatenating or looping for example? > Demuxers and decoding don't return such an end PTS, so why should filters? Demuxers apparently use the frame duration to time the last frame, so why not use that in lavfi too? (Demuxers also report the total length, but that is probably not useful at all for timing the last frame.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
Le duodi 12 thermidor, an CCXXII, wm4 a écrit : > You never explained what's wrong with it. This is tiring. Everyone knows it already and it has been explained numerous times. THERE IS NO TIMESTAMP FOR THE END OF STREAM. Is it enough, or do I have to waste more time explaining how that makes the duration of the last frame wrong, and how that is a problem when accurate timing is necessary, for concatenating or looping for example? -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Wed, 30 Jul 2014 10:37:12 +0200 Nicolas George wrote: > Le duodi 12 thermidor, an CCXXII, wm4 a écrit : > > The libavfilter API > > for EOF is ok, > > No, it is not. You never explained what's wrong with it. > > So if you're not actually fixing anything > > I am. Implementation, not API AFAIK. But you didn't bother explaining anything anyway. > > What wrong result? > > See my first answer when you started this bikeshedding. What's the name for patches that change API around without any sense of design? If bikeshedding is needed to avoid making the API even more confusing, so be it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
Le duodi 12 thermidor, an CCXXII, wm4 a écrit : >The libavfilter API > for EOF is ok, No, it is not. > So if you're not actually fixing anything I am. > What wrong result? See my first answer when you started this bikeshedding. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Wed, 30 Jul 2014 00:13:38 +0200 Nicolas George wrote: > Le duodi 12 thermidor, an CCXXII, wm4 a écrit : > > I asked for the reason of the API change. What's the purpose of the API > > change, if the "deprecated" method is supported forever anyway? > > What's the purpose of deprecating gets() in favour of fgets() if the > deprecated function is supported forever anyway? gets() has fundamental, unfixable security issues. The libavfilter API for EOF is ok, and not much different from e.g. libavcodec codec flushing. So if you're not actually fixing anything (in the API, not filter implementations), it's more important not to change the API for no reason. > The reason is called "compatibility": old programs will still work, with the > same problems as before. Fortunately, unless gets(), adding a NULL frame > does not open a security risk, it just gives a slightly wrong result. What wrong result? Isn't it an implementation problem? You could fix it without changing the API. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
Le duodi 12 thermidor, an CCXXII, wm4 a écrit : > I asked for the reason of the API change. What's the purpose of the API > change, if the "deprecated" method is supported forever anyway? What's the purpose of deprecating gets() in favour of fgets() if the deprecated function is supported forever anyway? The reason is called "compatibility": old programs will still work, with the same problems as before. Fortunately, unless gets(), adding a NULL frame does not open a security risk, it just gives a slightly wrong result. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Thu, 24 Jul 2014 15:49:33 +0200 Nicolas George wrote: > Le sextidi 6 thermidor, an CCXXII, wm4 a écrit : > > No, you can't do this. > > Yes, I can. > > > It breaks compatibility with Libav; > > No, it does not. Removing it would break compatibility. Deprecating it does > nothing. > > > also what's even the purpose of this (API) change? > > Fixing a long-standing issue, see the rest of the series. I asked for the reason of the API change. What's the purpose of the API change, if the "deprecated" method is supported forever anyway? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Thu, Jul 24, 2014 at 03:39:43PM +0200, Nicolas George wrote: > Also deprecate adding a NULL frame to mark EOF. > > TODO APIchanges entry, version bump. > [...] > diff --git a/libavfilter/buffersrc.h b/libavfilter/buffersrc.h > index ea34c04..28ca545 100644 > --- a/libavfilter/buffersrc.h > +++ b/libavfilter/buffersrc.h > @@ -145,6 +145,8 @@ int av_buffersrc_add_frame(AVFilterContext *ctx, AVFrame > *frame); > * > * @param buffer_src pointer to a buffer source context > * @param frame a frame, or NULL to mark EOF > + *(Using NULL to mark EOF is deprecated, use > + *av_buffersrc_close() instead.) > * @param flags a combination of AV_BUFFERSRC_FLAG_* > * @return>= 0 in case of success, a negative AVERROR code > *in case of failure > @@ -154,6 +156,19 @@ int av_buffersrc_add_frame_flags(AVFilterContext > *buffer_src, > > > /** > + * Close a buffer source. > + * > + * This cause EOF to be propagated along the filter graph. > + * > + * @param buffer_src pointer to a buffer source context > + * @param pts the timestamp of the end of stream > + * @param flags a combination of AV_BUFFERSRC_FLAG_* > + * @return>= 0 in case of success, a negative AVERROR code > + *in case of failure > + */ > +int av_buffersrc_close(AVFilterContext *buffer_src, int64_t pts, int flags); API LGTM [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
Le sextidi 6 thermidor, an CCXXII, wm4 a écrit : > No, you can't do this. Yes, I can. > It breaks compatibility with Libav; No, it does not. Removing it would break compatibility. Deprecating it does nothing. > also what's even the purpose of this (API) change? Fixing a long-standing issue, see the rest of the series. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().
On Thu, 24 Jul 2014 15:39:43 +0200
Nicolas George wrote:
> Also deprecate adding a NULL frame to mark EOF.
No, you can't do this. It breaks compatibility with Libav; also what's
even the purpose of this (API) change?
> TODO APIchanges entry, version bump.
>
> Signed-off-by: Nicolas George
> ---
> libavfilter/buffersrc.c | 40 ++--
> libavfilter/buffersrc.h | 15 +++
> 2 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> index 27d3db0..6d71587 100644
> --- a/libavfilter/buffersrc.c
> +++ b/libavfilter/buffersrc.c
> @@ -63,6 +63,7 @@ typedef struct BufferSourceContext {
> char*channel_layout_str;
>
> int eof;
> +int64_t eof_pts;
> } BufferSourceContext;
>
> #define CHECK_VIDEO_PARAM_CHANGE(s, c, width, height, format)\
> @@ -77,6 +78,27 @@ typedef struct BufferSourceContext {
> return AVERROR(EINVAL);\
> }
>
> +static int push_if_flag(AVFilterContext *ctx, int flags)
> +{
> +return (flags & AV_BUFFERSRC_FLAG_PUSH) ?
> +ctx->output_pads[0].request_frame(ctx->outputs[0]) : 0;
> +}
> +
> +int av_buffersrc_close(AVFilterContext *ctx, int64_t pts, int flags)
> +{
> +BufferSourceContext *s = ctx->priv;
> +
> +if (pts == AV_NOPTS_VALUE) {
> +av_log(ctx, AV_LOG_WARNING, "No EOF timestamp\n");
> +/* FIXME use duration for audio */
> +pts = av_rescale_q(ctx->outputs[0]->current_pts,
> + AV_TIME_BASE_Q, ctx->outputs[0]->time_base) + 1;
> +}
> +s->eof_pts = pts;
> +s->eof = 1;
> +return push_if_flag(ctx, flags);
> +}
> +
> int attribute_align_arg av_buffersrc_write_frame(AVFilterContext *ctx, const
> AVFrame *frame)
> {
> return av_buffersrc_add_frame_flags(ctx, (AVFrame *)frame,
> @@ -125,8 +147,7 @@ static int
> av_buffersrc_add_frame_internal(AVFilterContext *ctx,
> s->nb_failed_requests = 0;
>
> if (!frame) {
> -s->eof = 1;
> -return 0;
> +return av_buffersrc_close(ctx, AV_NOPTS_VALUE, flags);
> } else if (s->eof)
> return AVERROR(EINVAL);
>
> @@ -177,11 +198,7 @@ static int
> av_buffersrc_add_frame_internal(AVFilterContext *ctx,
> return ret;
> }
>
> -if ((flags & AV_BUFFERSRC_FLAG_PUSH))
> -if ((ret = ctx->output_pads[0].request_frame(ctx->outputs[0])) < 0)
> -return ret;
> -
> -return 0;
> +return push_if_flag(ctx, flags);
> }
>
> #if FF_API_AVFILTERBUFFER
> @@ -211,8 +228,7 @@ int av_buffersrc_add_ref(AVFilterContext *ctx,
> AVFilterBufferRef *buf,
> int ret = 0, planes, i;
>
> if (!buf) {
> -s->eof = 1;
> -return 0;
> +return av_buffersrc_close(ctx, AV_NOPTS_VALUE, flags);
> } else if (s->eof)
> return AVERROR(EINVAL);
>
> @@ -487,10 +503,14 @@ static int request_frame(AVFilterLink *link)
> {
> BufferSourceContext *c = link->src->priv;
> AVFrame *frame;
> +int ret;
>
> if (!av_fifo_size(c->fifo)) {
> -if (c->eof)
> +if (c->eof) {
> +if ((ret = ff_filter_link_close(link, c->eof_pts)) < 0)
> +return ret;
> return AVERROR_EOF;
> +}
> c->nb_failed_requests++;
> return AVERROR(EAGAIN);
> }
> diff --git a/libavfilter/buffersrc.h b/libavfilter/buffersrc.h
> index ea34c04..28ca545 100644
> --- a/libavfilter/buffersrc.h
> +++ b/libavfilter/buffersrc.h
> @@ -145,6 +145,8 @@ int av_buffersrc_add_frame(AVFilterContext *ctx, AVFrame
> *frame);
> *
> * @param buffer_src pointer to a buffer source context
> * @param frame a frame, or NULL to mark EOF
> + *(Using NULL to mark EOF is deprecated, use
> + *av_buffersrc_close() instead.)
> * @param flags a combination of AV_BUFFERSRC_FLAG_*
> * @return>= 0 in case of success, a negative AVERROR code
> *in case of failure
> @@ -154,6 +156,19 @@ int av_buffersrc_add_frame_flags(AVFilterContext
> *buffer_src,
>
>
> /**
> + * Close a buffer source.
> + *
> + * This cause EOF to be propagated along the filter graph.
> + *
> + * @param buffer_src pointer to a buffer source context
> + * @param pts the timestamp of the end of stream
> + * @param flags a combination of AV_BUFFERSRC_FLAG_*
> + * @return>= 0 in case of success, a negative AVERROR code
> + *in case of failure
> + */
> +int av_buffersrc_close(AVFilterContext *buffer_src, int64_t pts, int flags);
> +
> +/**
> * @}
> */
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
