[FFmpeg-devel] [PATCH] avfilter: take_samples: do not directly return frame when samples are skipped

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Muhammad Faiz a écrit :
> Should fix Ticket6349.
> Modifying data pointer may make it unaligned.
> 
> Also change frame->nb_samples < max to frame->nb_samples <= max.
> This improves performance. Benchmark:
> ./ffmpeg -filter_complex "aevalsrc=0:n=1166,firequalizer=fixed=on" -f null null
> old:
>   25767 decicycles in take_samples,    1023 runs,      1 skips
>   25422 decicycles in take_samples,    2047 runs,      1 skips
>   25181 decicycles in take_samples,    4095 runs,      1 skips
>   24904 decicycles in take_samples,    8191 runs,      1 skips
> 
> new:
>     550 decicycles in take_samples,    1024 runs,      0 skips
>     548 decicycles in take_samples,    2048 runs,      0 skips
>     545 decicycles in take_samples,    4096 runs,      0 skips
>     544 decicycles in take_samples,    8192 runs,      0 skips
> 
> Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
> ---
>  libavfilter/avfilter.c   | 3 ++-
>  libavfilter/framequeue.c | 2 ++
>  libavfilter/framequeue.h | 5 +++++
>  3 files changed, 9 insertions(+), 1 deletion(-)

This is an interesting idea, but...

> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 08b86b0..1b6c432 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -1191,7 +1191,7 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
>         called with enough samples. */
>      av_assert1(samples_ready(link, link->min_samples));
>      frame0 = frame = ff_framequeue_peek(&link->fifo, 0);
> -    if (frame->nb_samples >= min && frame->nb_samples < max) {
> +    if (!link->fifo.samples_skipped && frame->nb_samples >= min && frame->nb_samples <= max) {
>          *rframe = ff_framequeue_take(&link->fifo);
>          return 0;
>      }
> @@ -1522,6 +1522,7 @@ int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe)
>      *rframe = NULL;
>      if (!ff_inlink_check_available_frame(link))
>          return 0;

> +    av_assert1(!link->fifo.samples_skipped);

... I am pretty sure that this assert can fail. Not with the current
code, but with future filters that use the ff_inlink API directly.

>      frame = ff_framequeue_take(&link->fifo);
>      consume_update(link, frame);
>      *rframe = frame;
> diff --git a/libavfilter/framequeue.c b/libavfilter/framequeue.c
> index 26bfa49..fed1118 100644
> --- a/libavfilter/framequeue.c
> +++ b/libavfilter/framequeue.c
> @@ -107,6 +107,7 @@ AVFrame *ff_framequeue_take(FFFrameQueue *fq)
>      fq->tail &= fq->allocated - 1;
>      fq->total_frames_tail++;
>      fq->total_samples_tail += b->frame->nb_samples;
> +    fq->samples_skipped = 0;
>      check_consistency(fq);
>      return b->frame;
>  }
> @@ -146,5 +147,6 @@ void ff_framequeue_skip_samples(FFFrameQueue *fq, size_t samples, AVRational tim
>      for (i = 0; i < planes && i < AV_NUM_DATA_POINTERS; i++)
>          b->frame->data[i] = b->frame->extended_data[i];
>      fq->total_samples_tail += samples;
> +    fq->samples_skipped = 1;
>      ff_framequeue_update_peeked(fq, 0);
>  }
> diff --git a/libavfilter/framequeue.h b/libavfilter/framequeue.h
> index 5aa2c72..c49d872 100644
> --- a/libavfilter/framequeue.h
> +++ b/libavfilter/framequeue.h
> @@ -100,6 +100,11 @@ typedef struct FFFrameQueue {
>       */
>      uint64_t total_samples_tail;
>  
> +    /**
> +     * Indicate that samples are skipped
> +     */
> +    int samples_skipped;
> +
>  } FFFrameQueue;
>  
>  /**

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170518/2134ea26/attachment.sig>