| Message ID | 20170505060131.3103-1-mfcc64@gmail.com |
|---|---|
| State | New |
| Headers | show |
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,
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, >
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.
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.
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.
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.
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.
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
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.
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.
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.
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,
> > 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
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,
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?
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.
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
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
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.
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.
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,
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
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.
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.
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.
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,
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.
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,
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.
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.
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
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.
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
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.
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.
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,
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.
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,
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.
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.
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;
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(-)