diff mbox series

[FFmpeg-devel,3/4] avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

Message ID 20200524170308.9994-3-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/4] avfilter/af_biquads: implement 1st order allpass
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Paul B Mahol May 24, 2020, 5:03 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
 libavfilter/filters.h  | 17 ++++++++++++
 2 files changed, 72 insertions(+), 6 deletions(-)

Comments

Paul B Mahol May 28, 2020, 9:07 a.m. UTC | #1
Will apply soon.

On 5/24/20, Paul B Mahol <onemda@gmail.com> wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
>  libavfilter/filters.h  | 17 ++++++++++++
>  2 files changed, 72 insertions(+), 6 deletions(-)
>
Nicolas George May 28, 2020, 10:57 a.m. UTC | #2
Paul B Mahol (12020-05-28):
> Will apply soon.

I will hurry looking at it.
Nicolas George May 28, 2020, 12:24 p.m. UTC | #3
Paul B Mahol (12020-05-24):
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
>  libavfilter/filters.h  | 17 ++++++++++++
>  2 files changed, 72 insertions(+), 6 deletions(-)

I am against this.

It adds quite a lot of complexity in common code.

This complexity is not needed: in patch 4/4, you always peek
s->window_size samples and skip s->hop_size, constants: just allocate a
permanent buffer with size s->window_size and roll by s->hop_size in it.
It will be more efficient (less copying around, less allocations) and
simpler.

Regards,
Paul B Mahol May 28, 2020, 3:50 p.m. UTC | #4
On 5/28/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-05-24):
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
>>  libavfilter/filters.h  | 17 ++++++++++++
>>  2 files changed, 72 insertions(+), 6 deletions(-)
>
> I am against this.
>
> It adds quite a lot of complexity in common code.
>
> This complexity is not needed: in patch 4/4, you always peek
> s->window_size samples and skip s->hop_size, constants: just allocate a
> permanent buffer with size s->window_size and roll by s->hop_size in it.
> It will be more efficient (less copying around, less allocations) and
> simpler.
>

Your arrogance means nothing to me.

Will apply in 5 minutes.
Nicolas George May 28, 2020, 4:51 p.m. UTC | #5
Paul B Mahol (12020-05-28):
> Will apply in 5 minutes.

No. This code is unnecessary and complicates things.
Paul B Mahol May 28, 2020, 5:02 p.m. UTC | #6
On 5/28/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-05-28):
>> Will apply in 5 minutes.
>
> No. This code is unnecessary and complicates things.

You can say whatever you want. I simply do not care.

You could block it months ago instead of requesting me to lose all my
precious time on your requested changes.
Nicolas George May 28, 2020, 5:07 p.m. UTC | #7
Paul B Mahol (12020-05-28):
> You can say whatever you want. I simply do not care.
> 
> You could block it months ago instead of requesting me to lose all my
> precious time on your requested changes.

This is not about you, it is about the code. I did not notice the
problem then, I notice it now. Better than after it is pushed

And since you are being rude, I am allowed to say what I have on my
mind:

I would not have to reject your code if it was good in the first place.
Spend fifteen minutes more thinking about your code before writing it,
and we would not have to waste both hours arguing over it.
Paul B Mahol May 28, 2020, 6 p.m. UTC | #8
On 5/28/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-05-28):
>> You can say whatever you want. I simply do not care.
>>
>> You could block it months ago instead of requesting me to lose all my
>> precious time on your requested changes.
>
> This is not about you, it is about the code. I did not notice the
> problem then, I notice it now. Better than after it is pushed
>
> And since you are being rude, I am allowed to say what I have on my
> mind:
>
> I would not have to reject your code if it was good in the first place.
> Spend fifteen minutes more thinking about your code before writing it,
> and we would not have to waste both hours arguing over it.
>

You are one that are extremely unpleasant and rude to me and to anyone else.

Your technical explanation is invalid, and your proposed solution
extremely convoluted
and needs more code for each filter needing this functionality.

Watch me applying this set.
diff mbox series

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index 394811916d..85010c88fe 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1131,6 +1131,7 @@  static int samples_ready(AVFilterLink *link, unsigned min)
 }
 
 static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
+                        unsigned peek_samples,
                         AVFrame **rframe)
 {
     AVFrame *frame0, *frame, *buf;
@@ -1142,7 +1143,10 @@  static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
     av_assert1(samples_ready(link, link->min_samples));
     frame0 = frame = ff_framequeue_peek(&link->fifo, 0);
     if (!link->fifo.samples_skipped && frame->nb_samples >= min && frame->nb_samples <= max) {
-        *rframe = ff_framequeue_take(&link->fifo);
+        if (peek_samples)
+            *rframe = av_frame_clone(frame);
+        else
+            *rframe = ff_framequeue_take(&link->fifo);
         return 0;
     }
     nb_frames = 0;
@@ -1172,18 +1176,20 @@  static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
 
     p = 0;
     for (i = 0; i < nb_frames; i++) {
-        frame = ff_framequeue_take(&link->fifo);
+        frame = peek_samples ? ff_framequeue_peek(&link->fifo, i) : ff_framequeue_take(&link->fifo);
         av_samples_copy(buf->extended_data, frame->extended_data, p, 0,
                         frame->nb_samples, link->channels, link->format);
         p += frame->nb_samples;
-        av_frame_free(&frame);
+        if (!peek_samples)
+            av_frame_free(&frame);
     }
     if (p < nb_samples) {
         unsigned n = nb_samples - p;
-        frame = ff_framequeue_peek(&link->fifo, 0);
+        frame = ff_framequeue_peek(&link->fifo, peek_samples ? i : 0);
         av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n,
                         link->channels, link->format);
-        ff_framequeue_skip_samples(&link->fifo, n, link->time_base);
+        if (!peek_samples)
+            ff_framequeue_skip_samples(&link->fifo, n, link->time_base);
     }
 
     *rframe = buf;
@@ -1504,7 +1510,7 @@  int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
         return 0;
     if (link->status_in)
         min = FFMIN(min, ff_framequeue_queued_samples(&link->fifo));
-    ret = take_samples(link, min, max, &frame);
+    ret = take_samples(link, min, max, 0, &frame);
     if (ret < 0)
         return ret;
     consume_update(link, frame);
@@ -1512,6 +1518,49 @@  int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
     return 1;
 }
 
+int ff_inlink_peek_samples(AVFilterLink *link, unsigned nb_samples,
+                           AVFrame **rframe)
+{
+    AVFrame *frame;
+    int ret;
+
+    av_assert1(nb_samples);
+    *rframe = NULL;
+    if (!ff_inlink_check_available_samples(link, nb_samples))
+        return 0;
+    if (link->status_in)
+        nb_samples = FFMIN(nb_samples, ff_framequeue_queued_samples(&link->fifo));
+    ret = take_samples(link, nb_samples, nb_samples, 1, &frame);
+    if (ret < 0)
+        return ret;
+    *rframe = frame;
+    return !!frame;
+}
+
+void ff_inlink_skip_samples(AVFilterLink *link, unsigned skip_samples)
+{
+    skip_samples = FFMIN(skip_samples, ff_framequeue_queued_samples(&link->fifo));
+
+    while (skip_samples > 0) {
+        AVFrame *frame = ff_inlink_peek_frame(link, 0);
+        if (skip_samples >= frame->nb_samples) {
+            frame = ff_framequeue_take(&link->fifo);
+            skip_samples -= frame->nb_samples;
+            av_frame_free(&frame);
+        } else {
+            break;
+        }
+    }
+
+    if (skip_samples)
+        ff_framequeue_skip_samples(&link->fifo, skip_samples, link->time_base);
+
+    if (ff_inlink_queued_frames(link)) {
+        AVFrame *frame = ff_inlink_peek_frame(link, 0);
+        consume_update(link, frame);
+    }
+}
+
 AVFrame *ff_inlink_peek_frame(AVFilterLink *link, size_t idx)
 {
     return ff_framequeue_peek(&link->fifo, idx);
diff --git a/libavfilter/filters.h b/libavfilter/filters.h
index 1157755403..7dc0b35981 100644
--- a/libavfilter/filters.h
+++ b/libavfilter/filters.h
@@ -115,6 +115,23 @@  int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe);
 int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
                             AVFrame **rframe);
 
+/**
+ * Peek samples from the link's FIFO.
+ *
+ * @return  >0 if a samples are available,
+ *          0 and set rframe to NULL if no samples are available,
+ *          or AVERROR code
+ */
+int ff_inlink_peek_samples(AVFilterLink *link, unsigned nb_samples,
+                           AVFrame **rframe);
+
+/**
+ * Skip samples from the link's FIFO.
+ *
+ * @note  May trigger process_command() and/or update is_disabled.
+ */
+void ff_inlink_skip_samples(AVFilterLink *link, unsigned skip);
+
 /**
  * Access a frame in the link fifo without consuming it.
  * The first frame is numbered 0; the designated frame must exist.