| Message ID | 20200611161227.5622-2-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avfilter/af_biquads: switch to activate() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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.
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.
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.
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 >
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 >> >
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?
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 >
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.
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.
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.
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.
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.
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.
Paul B Mahol (12020-06-27): > Going to apply this, as not valid technical arguments are provided to > act differently. This is not true.
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.
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.
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.
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.
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.
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?
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 */
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 */
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,
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.
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 [...]
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,
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.
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 >
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 --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.
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavfilter/avfilter.c | 61 +++++++++++++++++++++++++++++++++++++----- libavfilter/filters.h | 17 ++++++++++++ 2 files changed, 72 insertions(+), 6 deletions(-)