[FFmpeg-devel] avfilter: align data frame when needed

Submitted by Muhammad Faiz on May 5, 2017, 6:01 a.m.

Details

Message ID 20170505060131.3103-1-mfcc64@gmail.com
State New
Headers show

Commit Message

Muhammad Faiz May 5, 2017, 6:01 a.m.
This should fix Ticket6349.
Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
generate unaligned frame data.

Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
---
 libavfilter/avfilter.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Nicolas George May 5, 2017, 10 a.m.
Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
> This should fix Ticket6349.
> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
> generate unaligned frame data.
> 
> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> ---
>  libavfilter/avfilter.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Sorry, but no.

According to the C standard and FFmpeg's developer documentation, the
current output of libavfilter is correctly aligned, and libmp3lame is
misbehaving.

Therefore, you have only two options :

- fix libmp3lame ;

- change the documentation to shift the burden elsewhere.

Note that changing libavfilter is not an option. If you change the
documentation tu put the burden on libavfilter, then this patch can go
in, but the documentation change must come first.

Note that I will argue for a good documentation change, not something
rushed and inconvenient.

Regards,
wm4 May 5, 2017, 10:11 a.m.
On Fri, 5 May 2017 12:00:02 +0200
Nicolas George <george@nsup.org> wrote:

> Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
> > This should fix Ticket6349.
> > Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
> > generate unaligned frame data.
> > 
> > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> > ---
> >  libavfilter/avfilter.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)  
> 
> Sorry, but no.
> 
> According to the C standard and FFmpeg's developer documentation, the
> current output of libavfilter is correctly aligned, and libmp3lame is
> misbehaving.

FFmpeg doesn't follow standard C alignment rules for media data.

Please stop bullshitting. Fix the crashes, somehow. Or at least don't
prevent others from doing it.

> Therefore, you have only two options :
> 
> - fix libmp3lame ;
> 
> - change the documentation to shift the burden elsewhere.
> 
> Note that changing libavfilter is not an option. If you change the
> documentation tu put the burden on libavfilter, then this patch can go
> in, but the documentation change must come first.
> 
> Note that I will argue for a good documentation change, not something
> rushed and inconvenient.
> 
> Regards,
>
Paul B Mahol May 5, 2017, 10:52 a.m.
On 5/5/17, Nicolas George <george@nsup.org> wrote:
> Le sextidi 16 floreal, an CCXXV, Muhammad Faiz a ecrit :
>> This should fix Ticket6349.
>> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
>> generate unaligned frame data.
>>
>> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
>> ---
>>  libavfilter/avfilter.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> Sorry, but no.

Come on, I will report this to the committe.
Nicolas George May 5, 2017, 11:05 a.m.
Le sextidi 16 floréal, an CCXXV, Paul B Mahol a écrit :
> Come on, I will report this to the committe.

If you have something useful to say, please do so.
Paul B Mahol May 5, 2017, 11:55 a.m.
On 5/5/17, Nicolas George <george@nsup.org> wrote:
> Le sextidi 16 floreal, an CCXXV, Paul B Mahol a ecrit :
>> Come on, I will report this to the committe.
>
> If you have something useful to say, please do so.

There is crash reproducible with filters and not just libmp3lame.
Nicolas George May 5, 2017, 1:38 p.m.
Le sextidi 16 floréal, an CCXXV, Paul B Mahol a écrit :
> There is crash reproducible with filters and not just libmp3lame.

Then these filters are bogus since they are making invalid assumptions
about their inputs. Or quote the docs saying otherwise.
Muhammad Faiz May 5, 2017, 2:46 p.m.
On Fri, May 5, 2017 at 5:00 PM, Nicolas George <george@nsup.org> wrote:
> Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
>> This should fix Ticket6349.
>> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
>> generate unaligned frame data.
>>
>> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
>> ---
>>  libavfilter/avfilter.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> Sorry, but no.
>
> According to the C standard and FFmpeg's developer documentation, the
> current output of libavfilter is correctly aligned, and libmp3lame is
> misbehaving.

If documentation says that data should be aligned to 32-bytes but I
only align it to 4-bytes, then I violate the doc. But if documentation
says that data should be aligned to 4-bytes but I align it to
32-bytes, then there are no violations at all. So what's wrong with
this patch?

This patch does not mean to state that framequeue is wrong. This is a
workaround.
So I add locally to the commit message and comment in code:
'Workaround for filters and codecs that assume simd aligned data.'

When the framework is ready for simd unaligned data, this patch can be reverted.

Thank's.
Hendrik Leppkes May 5, 2017, 2:53 p.m.
On Fri, May 5, 2017 at 3:38 PM, Nicolas George <george@nsup.org> wrote:
> Le sextidi 16 floréal, an CCXXV, Paul B Mahol a écrit :
>> There is crash reproducible with filters and not just libmp3lame.
>
> Then these filters are bogus since they are making invalid assumptions
> about their inputs. Or quote the docs saying otherwise.

Blocking crash fixes on silly arguments doesn't help anyone. Send a
doc update yourself or stop blocking valid crash fixes.
Every ffmpeg developer knows that we have alignment requirements,
trying to badger someone into writing docs and holding crash fixes
hostage in the process is not the way to go.

- Hendrik
Nicolas George May 5, 2017, 4:17 p.m.
Le sextidi 16 floréal, an CCXXV, Hendrik Leppkes a écrit :
> Blocking crash fixes on silly arguments doesn't help anyone. Send a

My argument is not silly: the code to align the frame will be needed,
but NOT HERE.

> Every ffmpeg developer knows that we have alignment requirements,

Well, I do not, so please enlighten me.
Nicolas George May 5, 2017, 4:22 p.m.
Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
>						    But if documentation
> says that data should be aligned to 4-bytes but I align it to
> 32-bytes, then there are no violations at all. So what's wrong with
> this patch?

Useless code performing wasteful operations in speed-critical sections.

Write the doc, and we can discuss.
James Almer May 5, 2017, 4:38 p.m.
On 5/5/2017 1:22 PM, Nicolas George wrote:
> Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
>> 						    But if documentation
>> says that data should be aligned to 4-bytes but I align it to
>> 32-bytes, then there are no violations at all. So what's wrong with
>> this patch?
> 
> Useless code performing wasteful operations in speed-critical sections.
> 
> Write the doc, and we can discuss.

No, you write the documentation if that's what you think should be done.

Don't block a patch that fixes stuff you broke. Either you fix it the
way you think it should be fixed, or stay out of this and don't bother
those that are trying to fix it for you.
Nicolas George May 5, 2017, 5:23 p.m.
Le sextidi 16 floréal, an CCXXV, Hendrik Leppkes a écrit :
> Blocking crash fixes on silly arguments doesn't help anyone. Send a
> doc update yourself or stop blocking valid crash fixes.
> Every ffmpeg developer knows that we have alignment requirements,
> trying to badger someone into writing docs and holding crash fixes
> hostage in the process is not the way to go.

Now that I have a little more time, let me elaborate.

When a patch fixes the obvious trigger of a bug without fixing the
actual cause, you are one of the first to object, even in strong terms.
And you are right.

Well, this instance is exactly a case like that. If the alignment
requirements are not documented, then an application can do things in a
way similar to take_samples() right now, and result in a crash. And
there is no blaming the application, it followed the documentation.

The bug is not in take_samples(), the bug is in all the filters and
codecs that require alignment without documenting it.

Therefore, the correct fix is to either change the filters to not
require the alignment, or to document it. The second option is obviously
the best.

Now, I will NOT do the documentation myself, for two reasons. First
because I do not know the code that requires alignment. Second, because
I am quite fed up with the attitude of a few developers here who
constantly bash their peers who undertake hard but necessary overhauls.

As is, this patch is rejected because it is unnecessary and wasteful,
and only hides the crash in it most obvious manifestation. This is not a
tantrum, this is proper development. I want a proper fix, and usually,
so do you.

Regards,
Kieran Kunhya May 5, 2017, 5:53 p.m.
>
> Well, this instance is exactly a case like that. If the alignment
> requirements are not documented, then an application can do things in a
> way similar to take_samples() right now, and result in a crash. And
> there is no blaming the application, it followed the documentation.
>
> The bug is not in take_samples(), the bug is in all the filters and
> codecs that require alignment without documenting it.
>

I sent a patch about this nearly three years ago, it wasn't applied for
whatever reason.
This is pretty common if you don't use av_malloc.

Kieran
Nicolas George May 6, 2017, 9:20 a.m.
Le sextidi 16 floréal, an CCXXV, Kieran Kunhya a écrit :
> I sent a patch about this nearly three years ago, it wasn't applied for
> whatever reason.
> This is pretty common if you don't use av_malloc.

Thanks for pointing it.

As for the rest, I can observe that my hope that some people will start
acting like adult and spend their time in constructive ways instead of
spending it flaming scapegoats. And I am quite fed up to be the
designated scapegoat just because I happen to see flaws in other
people's design. This project is in sore need of an Architect (
http://esr.ibiblio.org/?p=7478
), but I do not see many around here.

So I have taken the moral high ground, and just posted a proposal for an
actual fix, despite my statement of not intending to do so.

Regards,
wm4 May 6, 2017, 9:56 a.m.
On Sat, 6 May 2017 11:20:13 +0200
Nicolas George <george@nsup.org> wrote:

> Le sextidi 16 floréal, an CCXXV, Kieran Kunhya a écrit :
> > I sent a patch about this nearly three years ago, it wasn't applied for
> > whatever reason.
> > This is pretty common if you don't use av_malloc.  
> 
> Thanks for pointing it.

Well, nobody expected that apparently you didn't know that you have to
use av_malloc for everything in FFmpeg exactly because of alignment
requirements. Usually every API user learns this the hard way. (I wonder
if you ever used the FFmpeg API outside of ffmpeg.c...)

> As for the rest, I can observe that my hope that some people will start
> acting like adult

I can't say your behavior was particularly adult like. You raised a fuzz
because a bug was found and we wanted it to be fixed.

Adults fix their bugs and don't blame them on others.

> and spend their time in constructive ways instead of
> spending it flaming scapegoats.

This isn't about "scapegoats" - we just tried to get rid of that bug.
(Still trying...)

Apropos "scapegoat" - you're blaming everyone here, without
acknowledging any responsibility yourself.

What we really don't want in this project is that every time a bug is
pointed out, someone freaks out and raises a shit storm.

> And I am quite fed up to be the
> designated scapegoat just because I happen to see flaws in other
> people's design.

"Seeing flaws in other people's design" is ok, but your behavior
otherwise was a little questionable.

By the way, I can't help to observe that you refused to fix the bug
since it was not your fault (even though your change triggered it).
Fine, but what exactly makes you expect others to fix that bug? These
"others" are even less responsible for the bug than you.

That would still have been fine, but then you rejected bug fix patches
by others. Now I'm the first to say that we should fix bugs properly,
instead of painting them over with hacks, but let me remind you that no
better solution was in sight (or anyone working on it), and the crash
still happened, so applying whatever short-term fixes we had would have
been fine. (Or alternatively, reverting the patch that triggered it.)

> This project is in sore need of an Architect (
> http://esr.ibiblio.org/?p=7478
> ), but I do not see many around here.

That's... rich. I hope you don't claim to be that Architect. If you
think that's an insult, be aware that you just insulted everyone else
here. (Fortunately you added a "many" so that the reader can chose
whether he should be affected by the insult or not. How funny of you.)

> So I have taken the moral high ground, and just posted a proposal for an
> actual fix, despite my statement of not intending to do so.

This is amazing. When I saw your patches I thought you started to
understand (although I wasn't impressed by the lack of implementation),
but here you go and turn that around into yet another passive
aggressive bout. Very adult-like.

Maybe we can avoid all this bullshit the next time?
Muhammad Faiz May 6, 2017, 2:13 p.m.
On Fri, May 5, 2017 at 11:22 PM, Nicolas George <george@nsup.org> wrote:
> Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
>>                                                   But if documentation
>> says that data should be aligned to 4-bytes but I align it to
>> 32-bytes, then there are no violations at all. So what's wrong with
>> this patch?
>
> Useless code performing wasteful operations in speed-critical sections.

This is not speed-critical section.
The command:
./ffmpeg -filter_complex "aevalsrc=0:d=1000, aformat=fltp,
firequalizer=fixed=on:delay=0.0013, volume=0.5" -f null null
segfault at eof. So, this section is rarely executed.
And code below this section do extensive copying.


>
> Write the doc, and we can discuss.
Ronald S. Bultje May 6, 2017, 2:35 p.m.
Hi,

On Fri, May 5, 2017 at 12:17 PM, Nicolas George <george@nsup.org> wrote:

> Le sextidi 16 floréal, an CCXXV, Hendrik Leppkes a écrit :
> > Blocking crash fixes on silly arguments doesn't help anyone. Send a
>
> My argument is not silly: the code to align the frame will be needed,
> but NOT HERE.


I actually think this is a fair point. I've hated the swscale frame
alignment code for ages, and this can effectively be used to clean that up
also. We could even start making padding requirements which would make
swscale SIMD writing a LOT easier.

Let's think about this a little bit guys. Nicolas may be overly aggressive,
but he has a good point.

Ronald
Ronald S. Bultje May 6, 2017, 2:36 p.m.
Hi Muhammad,

On Sat, May 6, 2017 at 10:13 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Fri, May 5, 2017 at 11:22 PM, Nicolas George <george@nsup.org> wrote:
> > Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
> >>                                                   But if documentation
> >> says that data should be aligned to 4-bytes but I align it to
> >> 32-bytes, then there are no violations at all. So what's wrong with
> >> this patch?
> >
> > Useless code performing wasteful operations in speed-critical sections.
>
> This is not speed-critical section.


Not in this case, no. But I believe we have multiple issues in this area
and some of the affected areas _are_ speed-critical.

Plus, we like speed even if it's pointless. There's no problem with that.

Ronald
Muhammad Faiz May 6, 2017, 3:49 p.m.
On Sat, May 6, 2017 at 9:36 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi Muhammad,
>
> On Sat, May 6, 2017 at 10:13 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Fri, May 5, 2017 at 11:22 PM, Nicolas George <george@nsup.org> wrote:
>> > Le sextidi 16 floréal, an CCXXV, Muhammad Faiz a écrit :
>> >>                                                   But if documentation
>> >> says that data should be aligned to 4-bytes but I align it to
>> >> 32-bytes, then there are no violations at all. So what's wrong with
>> >> this patch?
>> >
>> > Useless code performing wasteful operations in speed-critical sections.
>>
>> This is not speed-critical section.
>
>
> Not in this case, no. But I believe we have multiple issues in this area
> and some of the affected areas _are_ speed-critical.

I guess, those speed-critical areas are video frames. Because the data
is larger.

>
> Plus, we like speed even if it's pointless. There's no problem with that.

But it should not generate crash.

Thanks.
Muhammad Faiz May 6, 2017, 4:12 p.m.
On Sat, May 6, 2017 at 9:35 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Fri, May 5, 2017 at 12:17 PM, Nicolas George <george@nsup.org> wrote:
>
>> Le sextidi 16 floréal, an CCXXV, Hendrik Leppkes a écrit :
>> > Blocking crash fixes on silly arguments doesn't help anyone. Send a
>>
>> My argument is not silly: the code to align the frame will be needed,
>> but NOT HERE.
>
>
> I actually think this is a fair point. I've hated the swscale frame
> alignment code for ages, and this can effectively be used to clean that up
> also. We could even start making padding requirements which would make
> swscale SIMD writing a LOT easier.
>
> Let's think about this a little bit guys. Nicolas may be overly aggressive,
> but he has a good point.

+1

But, my point is:
Because the design of handling data alignment may take long time to
discuss, and also it may add new features which can't be backported,
we need temporal fix.
There are two choices:
 - fix libmp3lame (previous patch) and filters/codecs that need aligned data
 - fix framequeue (this patch).
And I think, fixing framequeue is more elegant.

I think, Nicolas was afraid that if the crash is fixed, people will
not be aware again with the alignment problem and will not discuss it.

Thank's.
Nicolas George May 6, 2017, 4:35 p.m.
Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit :
>  - fix framequeue (this patch).

Once again, this patch is not correct. In particular it is NOT ENOUGH
to fix the crash in all cases.

Regards,
Hendrik Leppkes May 6, 2017, 5:34 p.m.
On Sat, May 6, 2017 at 6:35 PM, Nicolas George <george@nsup.org> wrote:
> Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit :
>>  - fix framequeue (this patch).
>
> Once again, this patch is not correct. In particular it is NOT ENOUGH
> to fix the crash in all cases.
>

It is enough to restore the situation to the previous status before
the avfilter changes, which in the short term would fix a regression.
We can work on a more complete solution, but this particular
regression needs to be resolved in a simple manner capable of easy
backporting.

- Hendrik
James Almer May 6, 2017, 6:29 p.m.
On 5/5/2017 2:23 PM, Nicolas George wrote:
> Le sextidi 16 floréal, an CCXXV, James Almer a écrit :
>> 				 stuff you broke
> 
> Please stop spreading wm4's lies. The bug was already there.
> 
> Regards,
> 

Take a look at https://trac.ffmpeg.org/ticket/6346. That ticket reported
a regression in concatdec, where some file started to segfault where it
didn't before. A bisect showed the commit that started this was a commit
made by myself, even if mostly authored by someone else.
That commit was not the cause of the problem, but the one that exposed
the underlying bug, as i stated in that ticket.

You know what the underlying bug was? The autobsf feature in concatdec
was broken by *you* in commits 0cb19c30c6 and b8fa374fb6 (see
https://ffmpeg.org/pipermail/ffmpeg-devel/2017-April/210639.html).
Now, what was my reaction? I worked around the issue to restore the
original behavior. I didn't try to coerce you into fixing what you broke
a year ago. I worked around what you broke and i unknowingly exposed,
for immediate backporting purposes.
Eventually i did go and fixed the underlying bug, as mentioned in the
last comment from the ticket, but that's not important here.

So, my point? Even if you didn't cause the underlying alignment issue,
you exposed it and created a regression. Either work around it yourself,
or help others trying to do it for you. Have the decency of not blocking
every attempt others give at it.
You're right that the real fix needs to go deeper, but that requires a
design discussion, and the crashes in release/3.3 can't wait. They need
an immediate, and able to backport solution.
wm4 May 7, 2017, 4:11 a.m.
On Sat, 6 May 2017 15:29:18 -0300
James Almer <jamrial@gmail.com> wrote:

> On 5/5/2017 2:23 PM, Nicolas George wrote:
> > Le sextidi 16 floréal, an CCXXV, James Almer a écrit :  
> >> 				 stuff you broke  
> > 
> > Please stop spreading wm4's lies. The bug was already there.

Oh, now he's accusing me of lying?

Can we eject this guy from FFmpeg already? I see no reason why we should
deal with his bullshit.
Muhammad Faiz May 7, 2017, 7:15 a.m.
On Fri, May 5, 2017 at 1:01 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> This should fix Ticket6349.
> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
> generate unaligned frame data.
>
> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> ---
>  libavfilter/avfilter.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 08b86b0..504f5c6 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -1192,7 +1192,31 @@ 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 (frame->nb_samples >= min && frame->nb_samples < max) {
> -        *rframe = ff_framequeue_take(&link->fifo);
> +        int align = 32;
> +        if (!((align - 1) & (intptr_t)frame->data[0])) {
> +            *rframe = ff_framequeue_take(&link->fifo);
> +            return 0;
> +        }
> +
> +        buf = ff_get_audio_buffer(link, frame->nb_samples);
> +        if (!buf)
> +            return AVERROR(ENOMEM);
> +
> +        ret = av_frame_copy_props(buf, frame);
> +        if (ret < 0) {
> +            av_frame_free(&buf);
> +            return ret;
> +        }
> +
> +        ret = av_frame_copy(buf, frame);
> +        if (ret < 0) {
> +            av_frame_free(&buf);
> +            return ret;
> +        }
> +
> +        frame = ff_framequeue_take(&link->fifo);
> +        av_frame_free(&frame);
> +        *rframe = buf;
>          return 0;
>      }
>      nb_frames = 0;
> --
> 2.9.3
>

Is it ok to push this?

Thank's.
Nicolas George May 7, 2017, 8:33 a.m.
L'octidi 18 floréal, an CCXXV, Muhammad Faiz a écrit :
> Is it ok to push this?

Of course not. How would it be ok to push a partial fix when a complete
one is being discussed?

Regards,
James Almer May 7, 2017, 1:15 p.m.
On 5/7/2017 5:33 AM, Nicolas George wrote:
> L'octidi 18 floréal, an CCXXV, Muhammad Faiz a écrit :
>> Is it ok to push this?
> 
> Of course not. How would it be ok to push a partial fix when a complete
> one is being discussed?
> 
> Regards,

Will that complete fix be ready and be backport-enabled (AKA, no new
API) in the coming days? Because if not, then this patch here will have
to be committed for 3.3.1 to at least deal with some of the failures.
Nicolas George May 7, 2017, 1:18 p.m.
L'octidi 18 floréal, an CCXXV, James Almer a écrit :
> Will that complete fix be ready and be backport-enabled (AKA, no new
> API) in the coming days?

It can. Right now, I am waiting for a comment from Hendrik, who seemed
to have objections on the principle but that I hope to have convinced.
After that, the coding is quite straightforward.

Regards,
James Almer May 7, 2017, 1:19 p.m.
On 5/7/2017 10:18 AM, Nicolas George wrote:
> L'octidi 18 floréal, an CCXXV, James Almer a écrit :
>> Will that complete fix be ready and be backport-enabled (AKA, no new
>> API) in the coming days?
> 
> It can. Right now, I am waiting for a comment from Hendrik, who seemed
> to have objections on the principle but that I hope to have convinced.
> After that, the coding is quite straightforward.
> 
> Regards,

Alright, thank you.
Muhammad Faiz May 16, 2017, 11:23 p.m.
On Sun, May 7, 2017 at 8:18 PM, Nicolas George <george@nsup.org> wrote:
> L'octidi 18 floréal, an CCXXV, James Almer a écrit :
>> Will that complete fix be ready and be backport-enabled (AKA, no new
>> API) in the coming days?
>
> It can. Right now, I am waiting for a comment from Hendrik, who seemed
> to have objections on the principle but that I hope to have convinced.
> After that, the coding is quite straightforward.

I will push soon if there is no progress in that complete fix.

Thank's.
Ronald S. Bultje May 16, 2017, 11:33 p.m.
Hi,

On Tue, May 16, 2017 at 7:23 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Sun, May 7, 2017 at 8:18 PM, Nicolas George <george@nsup.org> wrote:
> > L'octidi 18 floréal, an CCXXV, James Almer a écrit :
> >> Will that complete fix be ready and be backport-enabled (AKA, no new
> >> API) in the coming days?
> >
> > It can. Right now, I am waiting for a comment from Hendrik, who seemed
> > to have objections on the principle but that I hope to have convinced.
> > After that, the coding is quite straightforward.
>
> I will push soon if there is no progress in that complete fix.


Push what patch exactly?

Ronald
Muhammad Faiz May 16, 2017, 11:43 p.m.
On Wed, May 17, 2017 at 6:33 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Tue, May 16, 2017 at 7:23 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Sun, May 7, 2017 at 8:18 PM, Nicolas George <george@nsup.org> wrote:
>> > L'octidi 18 floréal, an CCXXV, James Almer a écrit :
>> >> Will that complete fix be ready and be backport-enabled (AKA, no new
>> >> API) in the coming days?
>> >
>> > It can. Right now, I am waiting for a comment from Hendrik, who seemed
>> > to have objections on the principle but that I hope to have convinced.
>> > After that, the coding is quite straightforward.
>>
>> I will push soon if there is no progress in that complete fix.
>
>
> Push what patch exactly?

Push my patch (avfilter: align data frame when needed).

Thanks.
Ronald S. Bultje May 17, 2017, 2:28 a.m.
Hi,

On Tue, May 16, 2017 at 7:43 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Wed, May 17, 2017 at 6:33 AM, Ronald S. Bultje <rsbultje@gmail.com>
> wrote:
> > Hi,
> >
> > On Tue, May 16, 2017 at 7:23 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> >
> >> On Sun, May 7, 2017 at 8:18 PM, Nicolas George <george@nsup.org> wrote:
> >> > L'octidi 18 floréal, an CCXXV, James Almer a écrit :
> >> >> Will that complete fix be ready and be backport-enabled (AKA, no new
> >> >> API) in the coming days?
> >> >
> >> > It can. Right now, I am waiting for a comment from Hendrik, who seemed
> >> > to have objections on the principle but that I hope to have convinced.
> >> > After that, the coding is quite straightforward.
> >>
> >> I will push soon if there is no progress in that complete fix.
> >
> >
> > Push what patch exactly?
>
> Push my patch (avfilter: align data frame when needed).


Wasn't consensus that you should backport the new API, possibly with ff
prefix, and call it in relevant places, to make sure that the code in new
and old branch is identical?

Ronald
James Almer May 17, 2017, 3:47 a.m.
On 5/16/2017 11:28 PM, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, May 16, 2017 at 7:43 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> 
>> On Wed, May 17, 2017 at 6:33 AM, Ronald S. Bultje <rsbultje@gmail.com>
>> wrote:
>>> Hi,
>>>
>>> On Tue, May 16, 2017 at 7:23 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>>
>>>> On Sun, May 7, 2017 at 8:18 PM, Nicolas George <george@nsup.org> wrote:
>>>>> L'octidi 18 floréal, an CCXXV, James Almer a écrit :
>>>>>> Will that complete fix be ready and be backport-enabled (AKA, no new
>>>>>> API) in the coming days?
>>>>>
>>>>> It can. Right now, I am waiting for a comment from Hendrik, who seemed
>>>>> to have objections on the principle but that I hope to have convinced.
>>>>> After that, the coding is quite straightforward.
>>>>
>>>> I will push soon if there is no progress in that complete fix.
>>>
>>>
>>> Push what patch exactly?
>>
>> Push my patch (avfilter: align data frame when needed).
> 
> 
> Wasn't consensus that you should backport the new API, possibly with ff
> prefix, and call it in relevant places, to make sure that the code in new
> and old branch is identical?
> 
> Ronald

Yes, but as he said, if there's no progress in that complete fix, then
this partial fix should be pushed instead, at least for one point
release in the 3.3 branch before being replaced.
For that matter, it should have happened for 3.3.1, seeing the complete
fix couldn't be done in time even though it was supposed to.
Muhammad Faiz May 17, 2017, 6:37 a.m.
On Wed, May 17, 2017 at 10:47 AM, James Almer <jamrial@gmail.com> wrote:
> On 5/16/2017 11:28 PM, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Tue, May 16, 2017 at 7:43 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>
>>> On Wed, May 17, 2017 at 6:33 AM, Ronald S. Bultje <rsbultje@gmail.com>
>>> wrote:
>>>> Hi,
>>>>
>>>> On Tue, May 16, 2017 at 7:23 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>>>
>>>>> On Sun, May 7, 2017 at 8:18 PM, Nicolas George <george@nsup.org> wrote:
>>>>>> L'octidi 18 floréal, an CCXXV, James Almer a écrit :
>>>>>>> Will that complete fix be ready and be backport-enabled (AKA, no new
>>>>>>> API) in the coming days?
>>>>>>
>>>>>> It can. Right now, I am waiting for a comment from Hendrik, who seemed
>>>>>> to have objections on the principle but that I hope to have convinced.
>>>>>> After that, the coding is quite straightforward.
>>>>>
>>>>> I will push soon if there is no progress in that complete fix.
>>>>
>>>>
>>>> Push what patch exactly?
>>>
>>> Push my patch (avfilter: align data frame when needed).
>>
>>
>> Wasn't consensus that you should backport the new API, possibly with ff
>> prefix, and call it in relevant places, to make sure that the code in new
>> and old branch is identical?
>>
>> Ronald
>
> Yes, but as he said, if there's no progress in that complete fix, then
> this partial fix should be pushed instead, at least for one point
> release in the 3.3 branch before being replaced.
> For that matter, it should have happened for 3.3.1, seeing the complete
> fix couldn't be done in time even though it was supposed to.

Actually, one can argue that because we've already missed 3.3.1, we
should wait for 3.3.2. But I can argue that master branch also has
users, and keeping the crash unresolved at master branch is not good
idea at all.

Thank's.
Nicolas George May 17, 2017, 7:25 a.m.
L'octidi 28 floréal, an CCXXV, Muhammad Faiz a écrit :
> Actually, one can argue that because we've already missed 3.3.1, we
> should wait for 3.3.2. But I can argue that master branch also has
> users, and keeping the crash unresolved at master branch is not good
> idea at all.

The complete fix is almost in shape, it needs only a few adjustments. If
you are not happy with the time I can invest in it, feel free to take it
over. But pushing something that will conflict with it is not ok.

(The biggest blocker, apparently, is the lack of test for the alignment
of linesize, which is not in the partial fix either. Ha ha ha.)

Regards,
Muhammad Faiz May 17, 2017, 8:11 a.m.
On Wed, May 17, 2017 at 2:25 PM, Nicolas George <george@nsup.org> wrote:
> L'octidi 28 floréal, an CCXXV, Muhammad Faiz a écrit :
>> Actually, one can argue that because we've already missed 3.3.1, we
>> should wait for 3.3.2. But I can argue that master branch also has
>> users, and keeping the crash unresolved at master branch is not good
>> idea at all.
>
> The complete fix is almost in shape, it needs only a few adjustments. If
> you are not happy with the time I can invest in it, feel free to take it
> over. But pushing something that will conflict with it is not ok.

Of course, pushing the partial fix won't conflict with the complete
fix. The partial fix aligns audio data unconditionally, but the
complete fix aligns audio data when needed.

>
> (The biggest blocker, apparently, is the lack of test for the alignment
> of linesize, which is not in the partial fix either. Ha ha ha.)

Ha ha ha. Of course, that's why I prefer the partial fix. Because it
is simple. We assume that at first data and linesize are always
aligned, so we only need to check data[0] alignment. If data[0] is
aligned, then data[n] and linesize are also aligned.

On the contrary, the new API must be carefully designed. It must
handle all corner cases. So, that's not a trivial task. (I see more
problems in linesize handling).

Thank's.
Nicolas George May 17, 2017, 8:20 a.m.
L'octidi 28 floréal, an CCXXV, Muhammad Faiz a écrit :
> Of course, pushing the partial fix won't conflict with the complete
> fix.

Of course it will, it touches the same area of code.

> Ha ha ha. Of course, that's why I prefer the partial fix. Because it
> is simple.

It is simple because it is incomplete.

>		We assume that at first data and linesize are always
> aligned, so we only need to check data[0] alignment.

Untrue. You only need to check data[0] because you only want to fix a
tiny case of the more general bug.

Regards,
Muhammad Faiz May 17, 2017, 9:06 a.m.
On Wed, May 17, 2017 at 3:20 PM, Nicolas George <george@nsup.org> wrote:
> L'octidi 28 floréal, an CCXXV, Muhammad Faiz a écrit :
>> Of course, pushing the partial fix won't conflict with the complete
>> fix.
>
> Of course it will, it touches the same area of code.

No, your code doesn't check the alignment at take_samples.

>
>> Ha ha ha. Of course, that's why I prefer the partial fix. Because it
>> is simple.
>
> It is simple because it is incomplete.

But it is enough to fix ticket 6349.

>
>>               We assume that at first data and linesize are always
>> aligned, so we only need to check data[0] alignment.
>
> Untrue. You only need to check data[0] because you only want to fix a
> tiny case of the more general bug.

With the assumption that ff_framequeue_skip_samples is the only source
of unaligned audio data and the framework doesn't mix between
ff_inlink_consume_frame and ff_inlink_consume_samples, this partial
fix is enough to fix ticket 6349.

Thank's.
Muhammad Faiz May 18, 2017, 1:38 p.m.
On Wed, May 17, 2017 at 4:06 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Wed, May 17, 2017 at 3:20 PM, Nicolas George <george@nsup.org> wrote:
>> L'octidi 28 floréal, an CCXXV, Muhammad Faiz a écrit :
>>> Of course, pushing the partial fix won't conflict with the complete
>>> fix.
>>
>> Of course it will, it touches the same area of code.
>
> No, your code doesn't check the alignment at take_samples.
>
>>
>>> Ha ha ha. Of course, that's why I prefer the partial fix. Because it
>>> is simple.
>>
>> It is simple because it is incomplete.
>
> But it is enough to fix ticket 6349.
>
>>
>>>               We assume that at first data and linesize are always
>>> aligned, so we only need to check data[0] alignment.
>>
>> Untrue. You only need to check data[0] because you only want to fix a
>> tiny case of the more general bug.
>
> With the assumption that ff_framequeue_skip_samples is the only source
> of unaligned audio data and the framework doesn't mix between
> ff_inlink_consume_frame and ff_inlink_consume_samples, this partial
> fix is enough to fix ticket 6349.
>
> Thank's.

A better patch has been posted.

Thank's.

Patch hide | download patch | download mbox

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index 08b86b0..504f5c6 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1192,7 +1192,31 @@  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 (frame->nb_samples >= min && frame->nb_samples < max) {
-        *rframe = ff_framequeue_take(&link->fifo);
+        int align = 32;
+        if (!((align - 1) & (intptr_t)frame->data[0])) {
+            *rframe = ff_framequeue_take(&link->fifo);
+            return 0;
+        }
+
+        buf = ff_get_audio_buffer(link, frame->nb_samples);
+        if (!buf)
+            return AVERROR(ENOMEM);
+
+        ret = av_frame_copy_props(buf, frame);
+        if (ret < 0) {
+            av_frame_free(&buf);
+            return ret;
+        }
+
+        ret = av_frame_copy(buf, frame);
+        if (ret < 0) {
+            av_frame_free(&buf);
+            return ret;
+        }
+
+        frame = ff_framequeue_take(&link->fifo);
+        av_frame_free(&frame);
+        *rframe = buf;
         return 0;
     }
     nb_frames = 0;