[FFmpeg-devel] [PATCH] avcodec/libmp3lame: properly handle unaligned frame data

[Muhammad Faiz]

On Mon, May 1, 2017 at 12:16 AM, wm4 <nfxjfg at googlemail.com> wrote:
> On Sun, 30 Apr 2017 23:55:02 +0700
> Muhammad Faiz <mfcc64 at gmail.com> wrote:
>
>> On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george at nsup.org> wrote:
>> > L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit :
>> >> I agree
>> >> in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db)
>> >> within the API of that time to avfilter
>> >
>> > It was not a bad idea, but it should not be limited to filters. A few
>> > comments.
>> >
>> > * First, the framequeue framework does not produce unaligned code.
>> > According to the C standard, the data it handles stay aligned. The
>> > alignment problems come from non-standard requirements by special
>> > processor features used by some filters and codecs, but not all.
>> >
>> > * That means a lot of the most useful codecs and filters will suffer
>> > from it, but not all. For many tasks, the alignment is just fine, and
>> > the extra copy would be wasteful.
>> >
>> > * The alignment requirements increase. Before AVX, it was up to 16, now
>> > it can be 32, and I have no doubt future processor will at some point
>> > require 64 or 128. But realigning buffers used with SSE to 32 would be
>> > wasteful too. Thus, we do not require a flag but a full integer.
>> >
>> > * The code that does the actual work of realigning a buffer should
>> > available as a stand-alone API, to be used by applications that work at
>> > low-level. I suppose something like that would be in order:
>> >
>> >         int av_frame_realign(AVFrame *frame, unsigned align);
>> >
>> > Or maybe:
>> >
>> >         int av_frame_realign(AVFrame *frame, unsigned align,
>> >                              AVBufferAllocator *alloc);
>> >
>> > where AVBufferAllocator is there to allocate possibly hardware or mmaped
>> > buffers.
>> >
>> > * It is another argument for my leitmotiv that filters and codecs are
>> > actually the same and should be merged API-wise.
>> >
>> > * It would be better to have the API just work for everything rather
>> > than documenting the alignment needs.
>> >
>> > As for the actual implementation, I see a lot of different approaches:
>> >
>> > - have the framework realing the frame before submitting it to the
>> >   filters and codecs: costly in malloc() and memcpy() but simple;
>> >
>> > - have each filter or codec call av_frame_realign() as needed; it may
>> >   seem less elegant than the previous proposal, but it may prove a
>> >   better choice in the light of what follows;
>> >
>> > - have each filter or codec copy the unaligned data into a buffer
>> >   allocated once and for all or on the stack, possibly by small chunks:
>> >   less costly in malloc() and refcounting overhead, and possibly better
>> >   cache-locality, but more complex code;
>> >
>> > - run the plain C version of the code on unaligned data rather than the
>> >   vectorized version, or the less-vectorized version (SSE vs AVX) on
>> >   insufficiently aligned data.
>> >
>> > Since all this boils down to a matter of performance and is related to
>> > the core task of FFmpeg, I think the choice between the various options
>> > should be done on a case-by-case basis using real benchmarks.
>>
>> Are you working on these? Because currently I'm not.
>
> I think it's probably ok to push your current patch, since the original
> author of the patch who broke this probably won't fix it.

As you said before, my patch didn't solve the problem. Here segfault
with volume filter:
./ffmpeg -filter_complex "aevalsrc=0:d=10, aformat=fltp,
firequalizer=fixed=on:delay=0.0013, volume=0.5" -f null -

Thank's