diff mbox series

[FFmpeg-devel,2/7] avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

Message ID 20200611161227.5622-2-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/7] avfilter/af_biquads: switch to activate()
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 June 11, 2020, 4:12 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

Nicolas George June 11, 2020, 6:31 p.m. UTC | #1
Paul B Mahol (12020-06-11):
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
>  libavfilter/filters.h  | 17 ++++++++++++
>  2 files changed, 72 insertions(+), 6 deletions(-)

Still no. It makes a very important function much more complex, it would
be simpler implemented in the filters directly since they are not using
the full features of the queue.
Paul B Mahol June 11, 2020, 10:46 p.m. UTC | #2
On 6/11/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-11):
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
>>  libavfilter/filters.h  | 17 ++++++++++++
>>  2 files changed, 72 insertions(+), 6 deletions(-)
>
> Still no. It makes a very important function much more complex, it would
> be simpler implemented in the filters directly since they are not using
> the full features of the queue.

Can I get another objective opinion?

This is needed also by showspectrumpic filter and it needs random number of
samples to skip and peek.
Nicolas George June 11, 2020, 10:53 p.m. UTC | #3
Paul B Mahol (12020-06-12):
> This is needed also by showspectrumpic filter and it needs random number of
> samples to skip and peek.

I looked at the code for showspectrumpic before making my comment: the
second part of your statement does not seem accurate.
Paul B Mahol June 11, 2020, 10:55 p.m. UTC | #4
On 6/12/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-12):
>> This is needed also by showspectrumpic filter and it needs random number
>> of
>> samples to skip and peek.
>
> I looked at the code for showspectrumpic before making my comment: the
> second part of your statement does not seem accurate.

How so? Its purely random.

>
> --
>   Nicolas George
>
Paul B Mahol June 12, 2020, 2 p.m. UTC | #5
On 6/12/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 6/12/20, Nicolas George <george@nsup.org> wrote:
>> Paul B Mahol (12020-06-12):
>>> This is needed also by showspectrumpic filter and it needs random number
>>> of
>>> samples to skip and peek.
>>
>> I looked at the code for showspectrumpic before making my comment: the
>> second part of your statement does not seem accurate.
>
> How so? Its purely random.

Got no answer, will apply.

>
>>
>> --
>>   Nicolas George
>>
>
Nicolas George June 16, 2020, 10:53 a.m. UTC | #6
Paul B Mahol (12020-06-12):
> How so? Its purely random.

+        ret = ff_inlink_peek_samples(inlink, s->win_size, &fin);

+        ret = ff_inlink_peek_samples(inlink, s->window_size, &in);

They seem constant in both cases. Can you elaborate about this
randomness?
Paul B Mahol June 19, 2020, 9:03 p.m. UTC | #7
On 6/16/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-12):
>> How so? Its purely random.
>
> +        ret = ff_inlink_peek_samples(inlink, s->win_size, &fin);
>
> +        ret = ff_inlink_peek_samples(inlink, s->window_size, &in);
>
> They seem constant in both cases. Can you elaborate about this
> randomness?
>

For showspectrumpic window size is random in sense it depends
on input, it is not random for every call.

> --
>   Nicolas George
>
Paul B Mahol June 24, 2020, 9:30 a.m. UTC | #8
On 6/19/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 6/16/20, Nicolas George <george@nsup.org> wrote:
>> Paul B Mahol (12020-06-12):
>>> How so? Its purely random.
>>
>> +        ret = ff_inlink_peek_samples(inlink, s->win_size, &fin);
>>
>> +        ret = ff_inlink_peek_samples(inlink, s->window_size, &in);
>>
>> They seem constant in both cases. Can you elaborate about this
>> randomness?
>>
>
> For showspectrumpic window size is random in sense it depends
> on input, it is not random for every call.

Also memory and CPU overhead is very small if it exists at all.
Nicolas George June 24, 2020, 10:12 a.m. UTC | #9
Paul B Mahol (12020-06-24):
> Also memory and CPU overhead is very small if it exists at all.

They're not the problem, code complexity is.

Just use a fixed buffer in your code and be done with it.
Paul B Mahol June 24, 2020, 10:15 a.m. UTC | #10
On 6/24/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-24):
>> Also memory and CPU overhead is very small if it exists at all.
>
> They're not the problem, code complexity is.
>
> Just use a fixed buffer in your code and be done with it.
>

As already explained I can not use fixed buffer at all.
Nicolas George June 24, 2020, 10:16 a.m. UTC | #11
Paul B Mahol (12020-06-24):
> As already explained I can not use fixed buffer at all.

Not true.

Bring new, accurate arguments, or stop.
Paul B Mahol June 24, 2020, 7:37 p.m. UTC | #12
On 6/24/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-24):
>> As already explained I can not use fixed buffer at all.
>
> Not true.
>
> Bring new, accurate arguments, or stop.

You never provided argument to show how to accomplish anything with
fixed buffer.

You are being rude.
Paul B Mahol June 27, 2020, 12:55 p.m. UTC | #13
On 6/24/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 6/24/20, Nicolas George <george@nsup.org> wrote:
>> Paul B Mahol (12020-06-24):
>>> As already explained I can not use fixed buffer at all.
>>
>> Not true.
>>
>> Bring new, accurate arguments, or stop.
>
> You never provided argument to show how to accomplish anything with
> fixed buffer.
>
> You are being rude.

Going to apply this, as not valid technical arguments are provided to
act differently.
Nicolas George June 27, 2020, 12:57 p.m. UTC | #14
Paul B Mahol (12020-06-27):
> Going to apply this, as not valid technical arguments are provided to
> act differently.

This is not true.
Paul B Mahol June 27, 2020, 12:59 p.m. UTC | #15
On 6/27/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-27):
>> Going to apply this, as not valid technical arguments are provided to
>> act differently.
>
> This is not true.
>

You need to provide valid proof. Simply stating something is not valid.
Paul B Mahol July 2, 2020, 12:57 p.m. UTC | #16
On 6/27/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 6/27/20, Nicolas George <george@nsup.org> wrote:
>> Paul B Mahol (12020-06-27):
>>> Going to apply this, as not valid technical arguments are provided to
>>> act differently.
>>
>> This is not true.
>>
>
> You need to provide valid proof. Simply stating something is not valid.
>

I think anyone agreed that change is ok so i gonna apply this.
Nicolas George July 2, 2020, 1:03 p.m. UTC | #17
Paul B Mahol (12020-07-02):
> I think anyone agreed that change is ok so i gonna apply this.

Absolutely not, and your intimidation tactics are unacceptable.
Paul B Mahol July 2, 2020, 1:58 p.m. UTC | #18
On 7/2/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-07-02):
>> I think anyone agreed that change is ok so i gonna apply this.
>
> Absolutely not, and your intimidation tactics are unacceptable.

You are not absolute judge in this matter.
Paul B Mahol July 19, 2020, 5:37 p.m. UTC | #19
On 7/2/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 7/2/20, Nicolas George <george@nsup.org> wrote:
>> Paul B Mahol (12020-07-02):
>>> I think anyone agreed that change is ok so i gonna apply this.
>>
>> Absolutely not, and your intimidation tactics are unacceptable.
>
> You are not absolute judge in this matter.
>

I officially request technical committee to give final decision on this subject.
Paul B Mahol July 19, 2020, 6:04 p.m. UTC | #20
On 7/19/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 7/2/20, Paul B Mahol <onemda@gmail.com> wrote:
>> On 7/2/20, Nicolas George <george@nsup.org> wrote:
>>> Paul B Mahol (12020-07-02):
>>>> I think anyone agreed that change is ok so i gonna apply this.
>>>
>>> Absolutely not, and your intimidation tactics are unacceptable.
>>
>> You are not absolute judge in this matter.
>>
>
> I officially request technical committee to give final decision on this
> subject.
>

Can I get confirmation that this will be dealt with in near future?
Steinar H. Gunderson July 19, 2020, 6:07 p.m. UTC | #21
On Sun, Jul 19, 2020 at 07:37:15PM +0200, Paul B Mahol wrote:
> I officially request technical committee to give final decision on this subject.

I will reiterate that I do not intend to block this patch if you can get some
other developer to approve it.

/* Steinar */
Steinar H. Gunderson July 19, 2020, 6:09 p.m. UTC | #22
On Sun, Jul 19, 2020 at 08:07:27PM +0200, Steinar H. Gunderson wrote:
>> I officially request technical committee to give final decision on this subject.
> I will reiterate that I do not intend to block this patch if you can get some
> other developer to approve it.

I'm sorry, I replied to the wrong thread.

/* Steinar */
Nicolas George July 19, 2020, 6:10 p.m. UTC | #23
Steinar H. Gunderson (12020-07-19):
> I will reiterate that I do not intend to block this patch if you can get some
> other developer to approve it.

You are confusing two discussions. I am blocking this patch, because it
makes an important and shared code path significantly more complex, for
the sake of two filters that can be implemented in a simpler way without
it. I am happy to provide details to help the committee decide if
necessary.

Regards,
Paul B Mahol July 19, 2020, 6:25 p.m. UTC | #24
On 7/19/20, Nicolas George <george@nsup.org> wrote:
> Steinar H. Gunderson (12020-07-19):
>> I will reiterate that I do not intend to block this patch if you can get
>> some
>> other developer to approve it.
>
> You are confusing two discussions. I am blocking this patch, because it
> makes an important and shared code path significantly more complex, for
> the sake of two filters that can be implemented in a simpler way without
> it. I am happy to provide details to help the committee decide if
> necessary.

It is not just two filters, there are numerous other filters that
currently uses hacks.
And you never provided any solution to such simple way approach.

You think that you defacto know all about libavfilters but your
knowledge is extremely biased and limited.
Michael Niedermayer July 20, 2020, 8:49 p.m. UTC | #25
On Thu, Jun 11, 2020 at 08:31:57PM +0200, Nicolas George wrote:
> Paul B Mahol (12020-06-11):
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
> >  libavfilter/filters.h  | 17 ++++++++++++
> >  2 files changed, 72 insertions(+), 6 deletions(-)
> 
> Still no. It makes a very important function much more complex, it would
> be simpler implemented in the filters directly since they are not using
> the full features of the queue.

shouldnt code which is used by multiple filters be shared and available
to all filters ?
but maybe iam missing something

And it would be nice if you and paul could come up with some solution
which you both are happy with
because i dont think arbitration by someone is going to make both of
you happy. No matter what the result, likely one at least would be unhappy

Thanks

[...]
Nicolas George July 20, 2020, 8:52 p.m. UTC | #26
Michael Niedermayer (12020-07-20):
> shouldnt code which is used by multiple filters be shared and available
> to all filters ?

Yes indeed. Since a fixed-size window on samples is a recurring pattern
in filters, a helper API for it would be very welcome. But it should not
make the evolution of the code for all filters more complex, and
therefore it should not be mixed with the common code.

> And it would be nice if you and paul could come up with some solution
> which you both are happy with
> because i dont think arbitration by someone is going to make both of
> you happy. No matter what the result, likely one at least would be unhappy

There is no way I will work with Paul after all the abuse I receive from
him, and I resent you a little for suggesting it.

Regards,
Paul B Mahol July 20, 2020, 9 p.m. UTC | #27
On 7/20/20, Nicolas George <george@nsup.org> wrote:
> Michael Niedermayer (12020-07-20):
>> shouldnt code which is used by multiple filters be shared and available
>> to all filters ?
>
> Yes indeed. Since a fixed-size window on samples is a recurring pattern
> in filters, a helper API for it would be very welcome. But it should not
> make the evolution of the code for all filters more complex, and
> therefore it should not be mixed with the common code.

_Should_ is very hard word here.

>
>> And it would be nice if you and paul could come up with some solution
>> which you both are happy with
>> because i dont think arbitration by someone is going to make both of
>> you happy. No matter what the result, likely one at least would be
>> unhappy
>
> There is no way I will work with Paul after all the abuse I receive from
> him, and I resent you a little for suggesting it.

Does this means you will not push your patches that I approve on mailing list?

All abuse came from my frustration with dealing with Nicolas behavior,
specifically
his tactics and strategy when he is reviewing my patches.
Paul B Mahol July 21, 2020, 9:58 p.m. UTC | #28
On 7/20/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Thu, Jun 11, 2020 at 08:31:57PM +0200, Nicolas George wrote:
>> Paul B Mahol (12020-06-11):
>> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > ---
>> >  libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++-----
>> >  libavfilter/filters.h  | 17 ++++++++++++
>> >  2 files changed, 72 insertions(+), 6 deletions(-)
>>
>> Still no. It makes a very important function much more complex, it would
>> be simpler implemented in the filters directly since they are not using
>> the full features of the queue.
>
> shouldnt code which is used by multiple filters be shared and available
> to all filters ?
> but maybe iam missing something
>
> And it would be nice if you and paul could come up with some solution
> which you both are happy with
> because i dont think arbitration by someone is going to make both of
> you happy. No matter what the result, likely one at least would be unhappy

Preferring status quo make Nicolas happy.

>
> Thanks
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Dictatorship naturally arises out of democracy, and the most aggravated
> form of tyranny and slavery out of the most extreme liberty. -- Plato
>
Paul B Mahol Aug. 11, 2020, 9:56 a.m. UTC | #29
Ping!

On 7/21/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 7/20/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> On Thu, Jun 11, 2020 at 08:31:57PM +0200, Nicolas George wrote:
>>> Paul B Mahol (12020-06-11):
>>> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> > ---
>>> >  libavfilter/avfilter.c | 61
>>> > +++++++++++++++++++++++++++++++++++++-----
>>> >  libavfilter/filters.h  | 17 ++++++++++++
>>> >  2 files changed, 72 insertions(+), 6 deletions(-)
>>>
>>> Still no. It makes a very important function much more complex, it would
>>> be simpler implemented in the filters directly since they are not using
>>> the full features of the queue.
>>
>> shouldnt code which is used by multiple filters be shared and available
>> to all filters ?
>> but maybe iam missing something
>>
>> And it would be nice if you and paul could come up with some solution
>> which you both are happy with
>> because i dont think arbitration by someone is going to make both of
>> you happy. No matter what the result, likely one at least would be
>> unhappy
>
> Preferring status quo make Nicolas happy.
>
>>
>> Thanks
>>
>> [...]
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Dictatorship naturally arises out of democracy, and the most aggravated
>> form of tyranny and slavery out of the most extreme liberty. -- Plato
>>
>
diff mbox series

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index dd8074e462..b1db92de4b 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1147,6 +1147,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;
@@ -1158,7 +1159,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;
@@ -1188,18 +1192,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;
@@ -1520,7 +1526,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);
@@ -1528,6 +1534,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.