Re: [FFmpeg-devel] [PATCH 2/4] lavfi/buffersrc: add add av_buffersrc_close().

2014-08-02 Thread Michael Niedermayer
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 geo...@nsup.org
 ---
  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().

2014-07-30 Thread Nicolas George
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().

2014-07-30 Thread Nicolas George
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().

2014-07-30 Thread Derek Buitenhuis
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().

2014-07-29 Thread wm4
On Thu, 24 Jul 2014 15:49:33 +0200
Nicolas George geo...@nsup.org 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().

2014-07-29 Thread wm4
On Wed, 30 Jul 2014 00:13:38 +0200
Nicolas George geo...@nsup.org 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().

2014-07-24 Thread wm4
On Thu, 24 Jul 2014 15:39:43 +0200
Nicolas George geo...@nsup.org 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 geo...@nsup.org
 ---
  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