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

[Muhammad Faiz]

On Thu, May 18, 2017 at 10:32 PM, Muhammad Faiz <mfcc64 at gmail.com> wrote:
> On Thu, May 18, 2017 at 9:59 PM, Nicolas George <george at nsup.org> wrote:
>> 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.
>
> IMHO, the solution is to document it properly to not mix
> ff_inlink_consume_samples with ff_inlink_consume_frame, similar to
> av_buffersink_get_frame vs av_buffersink_get_samples.
>
> Thank's.