| Message ID | 20170520104433.25151-1-mfcc64@gmail.com |
|---|---|
| State | Accepted |
| Commit | fc3a03fcf9cd7eafe7342e2508e6128888efa0bb |
| Headers | show |
On Sat, May 20, 2017 at 5:44 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: > Modifying data pointer when skipping samples may make it unaligned. > Workaround for Ticket6349. > > This should fix the crash of ticket's testcase and a crash/regression > with avxsynth (reported by Michael Niedermayer). > > 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@gmail.com> > --- > libavfilter/avfilter.c | 8 +++++++- > libavfilter/framequeue.c | 2 ++ > libavfilter/framequeue.h | 5 +++++ > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c > index 08b86b0..e60b024 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,12 @@ int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe) > *rframe = NULL; > if (!ff_inlink_check_available_frame(link)) > return 0; > + > + if (link->fifo.samples_skipped) { > + frame = ff_framequeue_peek(&link->fifo, 0); > + return ff_inlink_consume_samples(link, frame->nb_samples, frame->nb_samples, rframe); > + } > + > 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; > > /** > -- > 2.9.3 > I will push this soon. Thank's.
On Sat, May 20, 2017 at 7:33 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Sat, May 20, 2017 at 5:44 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: >> Modifying data pointer when skipping samples may make it unaligned. >> Workaround for Ticket6349. >> >> This should fix the crash of ticket's testcase and a crash/regression >> with avxsynth (reported by Michael Niedermayer). >> >> 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@gmail.com> >> --- >> libavfilter/avfilter.c | 8 +++++++- >> libavfilter/framequeue.c | 2 ++ >> libavfilter/framequeue.h | 5 +++++ >> 3 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c >> index 08b86b0..e60b024 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,12 @@ int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe) >> *rframe = NULL; >> if (!ff_inlink_check_available_frame(link)) >> return 0; >> + >> + if (link->fifo.samples_skipped) { >> + frame = ff_framequeue_peek(&link->fifo, 0); >> + return ff_inlink_consume_samples(link, frame->nb_samples, frame->nb_samples, rframe); >> + } >> + >> 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; >> >> /** >> -- >> 2.9.3 >> > > I will push this soon. > > Thank's. Pushed and backported to 3.3 branch. Thank's.
Le primidi 1er prairial, an CCXXV, Muhammad Faiz a écrit : > > I will push this soon. > Pushed and backported to 3.3 branch. Well, I was probably ok with this patch, but we will never know, will we? Can you explain the concept behind posting a patch for review and then pushing it before anybody could possibly review? Regards,
On Sun, May 21, 2017 at 1:12 AM, Nicolas George <george@nsup.org> wrote: > Le primidi 1er prairial, an CCXXV, Muhammad Faiz a écrit : >> > I will push this soon. >> Pushed and backported to 3.3 branch. > > Well, I was probably ok with this patch, but we will never know, will > we? Thank's. > > Can you explain the concept behind posting a patch for review and then > pushing it before anybody could possibly review? I'm sorry for that. I was tired and I thought that it had already been reviewed before. Thank's.
On Sat, 20 May 2017 20:12:15 +0200 Nicolas George <george@nsup.org> wrote: > Le primidi 1er prairial, an CCXXV, Muhammad Faiz a écrit : > > > I will push this soon. > > Pushed and backported to 3.3 branch. > > Well, I was probably ok with this patch, but we will never know, will > we? > > Can you explain the concept behind posting a patch for review and then > pushing it before anybody could possibly review? I think you had enough opportunity to hold back a fix for this, so I think it's ok that this got finally pushed.
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index 08b86b0..e60b024 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,12 @@ int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe) *rframe = NULL; if (!ff_inlink_check_available_frame(link)) return 0; + + if (link->fifo.samples_skipped) { + frame = ff_framequeue_peek(&link->fifo, 0); + return ff_inlink_consume_samples(link, frame->nb_samples, frame->nb_samples, rframe); + } + 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; /**
Modifying data pointer when skipping samples may make it unaligned. Workaround for Ticket6349. This should fix the crash of ticket's testcase and a crash/regression with avxsynth (reported by Michael Niedermayer). 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@gmail.com> --- libavfilter/avfilter.c | 8 +++++++- libavfilter/framequeue.c | 2 ++ libavfilter/framequeue.h | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-)