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

Submitted by Muhammad Faiz on April 26, 2017, 10:16 a.m.

Details

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

Commit Message

Muhammad Faiz April 26, 2017, 10:16 a.m.
This should fix Ticket6349

Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
generate unaligned frame data.

Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
---
 libavcodec/libmp3lame.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

wm4 April 26, 2017, 10:34 a.m.
On Wed, 26 Apr 2017 17:16:05 +0700
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>
> ---
>  libavcodec/libmp3lame.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
> index 5e26743..cd505bb 100644
> --- a/libavcodec/libmp3lame.c
> +++ b/libavcodec/libmp3lame.c
> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
>              break;
>          case AV_SAMPLE_FMT_FLTP:
> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
> -                return AVERROR(EINVAL);
> -            }
>              for (ch = 0; ch < avctx->channels; ch++) {
> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
> +                    float *src = (float *) frame->data[ch];
> +                    float *dst = s->samples_flt[ch];
> +                    int k;
> +
> +                    for (k = 0; k < frame->nb_samples; k++)
> +                        dst[k] = src[k] * 32768.0f;
> +                } else {
>                  s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
>                                             (const float *)frame->data[ch],
>                                             32768.0f,
>                                             FFALIGN(frame->nb_samples, 8));
> +                }
>              }
>              ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
>              break;

Looks like the right solution is fixing framequeue.

I don't think encoders generally allow lax alignment, and this might
not be the only case.
Muhammad Faiz April 26, 2017, 1:09 p.m.
On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Wed, 26 Apr 2017 17:16:05 +0700
> 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>
>> ---
>>  libavcodec/libmp3lame.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
>> index 5e26743..cd505bb 100644
>> --- a/libavcodec/libmp3lame.c
>> +++ b/libavcodec/libmp3lame.c
>> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
>>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
>>              break;
>>          case AV_SAMPLE_FMT_FLTP:
>> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
>> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
>> -                return AVERROR(EINVAL);
>> -            }
>>              for (ch = 0; ch < avctx->channels; ch++) {
>> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
>> +                    float *src = (float *) frame->data[ch];
>> +                    float *dst = s->samples_flt[ch];
>> +                    int k;
>> +
>> +                    for (k = 0; k < frame->nb_samples; k++)
>> +                        dst[k] = src[k] * 32768.0f;
>> +                } else {
>>                  s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
>>                                             (const float *)frame->data[ch],
>>                                             32768.0f,
>>                                             FFALIGN(frame->nb_samples, 8));
>> +                }
>>              }
>>              ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
>>              break;
>
> Looks like the right solution is fixing framequeue.
>
> I don't think encoders generally allow lax alignment, and this might
> not be the only case.

This requirement is not documented. Also some filters may also
generate unaligned data, for example crop filter.
Michael Niedermayer April 26, 2017, 1:46 p.m.
On Wed, Apr 26, 2017 at 05:16:05PM +0700, Muhammad Faiz wrote:
> This should fix Ticket6349
> 
> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
> generate unaligned frame data.
> 
> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> ---
>  libavcodec/libmp3lame.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
> index 5e26743..cd505bb 100644
> --- a/libavcodec/libmp3lame.c
> +++ b/libavcodec/libmp3lame.c
> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
>              break;
>          case AV_SAMPLE_FMT_FLTP:
> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
> -                return AVERROR(EINVAL);
> -            }
>              for (ch = 0; ch < avctx->channels; ch++) {
> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
> +                    float *src = (float *) frame->data[ch];
> +                    float *dst = s->samples_flt[ch];
> +                    int k;
> +
> +                    for (k = 0; k < frame->nb_samples; k++)
> +                        dst[k] = src[k] * 32768.0f;

If this solution is choosen then
this should produce a one time log message to inform about
the sub optimal alignment

[...]
wm4 April 26, 2017, 1:53 p.m.
On Wed, 26 Apr 2017 20:09:58 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Wed, 26 Apr 2017 17:16:05 +0700
> > 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>
> >> ---
> >>  libavcodec/libmp3lame.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
> >> index 5e26743..cd505bb 100644
> >> --- a/libavcodec/libmp3lame.c
> >> +++ b/libavcodec/libmp3lame.c
> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
> >>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
> >>              break;
> >>          case AV_SAMPLE_FMT_FLTP:
> >> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
> >> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
> >> -                return AVERROR(EINVAL);
> >> -            }
> >>              for (ch = 0; ch < avctx->channels; ch++) {
> >> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
> >> +                    float *src = (float *) frame->data[ch];
> >> +                    float *dst = s->samples_flt[ch];
> >> +                    int k;
> >> +
> >> +                    for (k = 0; k < frame->nb_samples; k++)
> >> +                        dst[k] = src[k] * 32768.0f;
> >> +                } else {
> >>                  s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
> >>                                             (const float *)frame->data[ch],
> >>                                             32768.0f,
> >>                                             FFALIGN(frame->nb_samples, 8));
> >> +                }
> >>              }
> >>              ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
> >>              break;  
> >
> > Looks like the right solution is fixing framequeue.
> >
> > I don't think encoders generally allow lax alignment, and this might
> > not be the only case.  
> 
> This requirement is not documented. Also some filters may also
> generate unaligned data, for example crop filter.

No - and it's inconsistently handled. But in general, everything seems
to prefer aligned data, and blows up or becomes slow otherwise. Keep in
mind that all data is allocated with large alignment in the first place.

The crop filter is sort of a special case too.
Muhammad Faiz April 26, 2017, 6:41 p.m.
On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Wed, 26 Apr 2017 20:09:58 +0700
> Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote:
>> > On Wed, 26 Apr 2017 17:16:05 +0700
>> > 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>
>> >> ---
>> >>  libavcodec/libmp3lame.c | 13 +++++++++----
>> >>  1 file changed, 9 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
>> >> index 5e26743..cd505bb 100644
>> >> --- a/libavcodec/libmp3lame.c
>> >> +++ b/libavcodec/libmp3lame.c
>> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
>> >>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
>> >>              break;
>> >>          case AV_SAMPLE_FMT_FLTP:
>> >> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
>> >> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
>> >> -                return AVERROR(EINVAL);
>> >> -            }
>> >>              for (ch = 0; ch < avctx->channels; ch++) {
>> >> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
>> >> +                    float *src = (float *) frame->data[ch];
>> >> +                    float *dst = s->samples_flt[ch];
>> >> +                    int k;
>> >> +
>> >> +                    for (k = 0; k < frame->nb_samples; k++)
>> >> +                        dst[k] = src[k] * 32768.0f;
>> >> +                } else {
>> >>                  s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
>> >>                                             (const float *)frame->data[ch],
>> >>                                             32768.0f,
>> >>                                             FFALIGN(frame->nb_samples, 8));
>> >> +                }
>> >>              }
>> >>              ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
>> >>              break;
>> >
>> > Looks like the right solution is fixing framequeue.
>> >
>> > I don't think encoders generally allow lax alignment, and this might
>> > not be the only case.
>>
>> This requirement is not documented. Also some filters may also
>> generate unaligned data, for example crop filter.
>
> No - and it's inconsistently handled. But in general, everything seems
> to prefer aligned data, and blows up or becomes slow otherwise. Keep in
> mind that all data is allocated with large alignment in the first place.

Based on looking the code, it seems that volume filter cannot handle
unaligned data, probably others too.
Having this restriction without documenting it is considered bug.

>
> The crop filter is sort of a special case too.

As consequence, video filters/codecs can handle it consistently.
Note that frame.h only states that linesize should be multiple of
16/32. It does not state about data pointer. And crop filter does not
modify linesize, and generating unaligned data pointer is legitimate.
wm4 April 26, 2017, 8:05 p.m.
On Thu, 27 Apr 2017 01:41:04 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Wed, 26 Apr 2017 20:09:58 +0700
> > Muhammad Faiz <mfcc64@gmail.com> wrote:
> >  
> >> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote:  
> >> > On Wed, 26 Apr 2017 17:16:05 +0700
> >> > 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>
> >> >> ---
> >> >>  libavcodec/libmp3lame.c | 13 +++++++++----
> >> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
> >> >> index 5e26743..cd505bb 100644
> >> >> --- a/libavcodec/libmp3lame.c
> >> >> +++ b/libavcodec/libmp3lame.c
> >> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
> >> >>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
> >> >>              break;
> >> >>          case AV_SAMPLE_FMT_FLTP:
> >> >> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
> >> >> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
> >> >> -                return AVERROR(EINVAL);
> >> >> -            }
> >> >>              for (ch = 0; ch < avctx->channels; ch++) {
> >> >> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
> >> >> +                    float *src = (float *) frame->data[ch];
> >> >> +                    float *dst = s->samples_flt[ch];
> >> >> +                    int k;
> >> >> +
> >> >> +                    for (k = 0; k < frame->nb_samples; k++)
> >> >> +                        dst[k] = src[k] * 32768.0f;
> >> >> +                } else {
> >> >>                  s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
> >> >>                                             (const float *)frame->data[ch],
> >> >>                                             32768.0f,
> >> >>                                             FFALIGN(frame->nb_samples, 8));
> >> >> +                }
> >> >>              }
> >> >>              ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
> >> >>              break;  
> >> >
> >> > Looks like the right solution is fixing framequeue.
> >> >
> >> > I don't think encoders generally allow lax alignment, and this might
> >> > not be the only case.  
> >>
> >> This requirement is not documented. Also some filters may also
> >> generate unaligned data, for example crop filter.  
> >
> > No - and it's inconsistently handled. But in general, everything seems
> > to prefer aligned data, and blows up or becomes slow otherwise. Keep in
> > mind that all data is allocated with large alignment in the first place.  
> 
> Based on looking the code, it seems that volume filter cannot handle
> unaligned data, probably others too.
> Having this restriction without documenting it is considered bug.
> 
> >
> > The crop filter is sort of a special case too.  
> 
> As consequence, video filters/codecs can handle it consistently.
> Note that frame.h only states that linesize should be multiple of
> 16/32. It does not state about data pointer. And crop filter does not
> modify linesize, and generating unaligned data pointer is legitimate.

Well yes, these things are not entirely documented. As a user I'd
actually expect unaligned things to work, even if there's a performance
impact.

But still:
- framequeue should probably not trigger low-performance code paths
  intentionally
- it's hard to fix all the cases where unaligned things cause crashes

AVPacket actually documented an alignment and padding requirement. I
suppose AVFrame is less-well documented, because it has many use-cases
associated (encoding, filtering, as direct rendering buffer for
decoding, etc.).

Where should we go from here? Personally I'd claim framequeue should be
changed to produce aligned output. For encoders and filter input, we
should probably add a flag whether unaligned input is supported, and if
not, have the generic code before it copy the frame to make it aligned.

I feel uneasy about that inconsistency being introduced by the
framequeue thing, and adding another inconsistency by only fixing
mp3lame... but maybe I'm making everything too complicated again.
Michael Niedermayer April 27, 2017, 2:20 a.m.
On Wed, Apr 26, 2017 at 10:05:40PM +0200, wm4 wrote:
> On Thu, 27 Apr 2017 01:41:04 +0700
> Muhammad Faiz <mfcc64@gmail.com> wrote:
> 
> > On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > > On Wed, 26 Apr 2017 20:09:58 +0700
> > > Muhammad Faiz <mfcc64@gmail.com> wrote:
> > >  
> > >> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote:  
> > >> > On Wed, 26 Apr 2017 17:16:05 +0700
> > >> > 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>
> > >> >> ---
> > >> >>  libavcodec/libmp3lame.c | 13 +++++++++----
> > >> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> > >> >>
> > >> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
> > >> >> index 5e26743..cd505bb 100644
> > >> >> --- a/libavcodec/libmp3lame.c
> > >> >> +++ b/libavcodec/libmp3lame.c
> > >> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
> > >> >>              ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
> > >> >>              break;
> > >> >>          case AV_SAMPLE_FMT_FLTP:
> > >> >> -            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
> > >> >> -                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
> > >> >> -                return AVERROR(EINVAL);
> > >> >> -            }
> > >> >>              for (ch = 0; ch < avctx->channels; ch++) {
> > >> >> +                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
> > >> >> +                    float *src = (float *) frame->data[ch];
> > >> >> +                    float *dst = s->samples_flt[ch];
> > >> >> +                    int k;
> > >> >> +
> > >> >> +                    for (k = 0; k < frame->nb_samples; k++)
> > >> >> +                        dst[k] = src[k] * 32768.0f;
> > >> >> +                } else {
> > >> >>                  s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
> > >> >>                                             (const float *)frame->data[ch],
> > >> >>                                             32768.0f,
> > >> >>                                             FFALIGN(frame->nb_samples, 8));
> > >> >> +                }
> > >> >>              }
> > >> >>              ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
> > >> >>              break;  
> > >> >
> > >> > Looks like the right solution is fixing framequeue.
> > >> >
> > >> > I don't think encoders generally allow lax alignment, and this might
> > >> > not be the only case.  
> > >>
> > >> This requirement is not documented. Also some filters may also
> > >> generate unaligned data, for example crop filter.  
> > >
> > > No - and it's inconsistently handled. But in general, everything seems
> > > to prefer aligned data, and blows up or becomes slow otherwise. Keep in
> > > mind that all data is allocated with large alignment in the first place.  
> > 
> > Based on looking the code, it seems that volume filter cannot handle
> > unaligned data, probably others too.
> > Having this restriction without documenting it is considered bug.
> > 
> > >
> > > The crop filter is sort of a special case too.  
> > 
> > As consequence, video filters/codecs can handle it consistently.
> > Note that frame.h only states that linesize should be multiple of
> > 16/32. It does not state about data pointer. And crop filter does not
> > modify linesize, and generating unaligned data pointer is legitimate.
> 
> Well yes, these things are not entirely documented. As a user I'd
> actually expect unaligned things to work, even if there's a performance
> impact.
> 
> But still:
> - framequeue should probably not trigger low-performance code paths
>   intentionally
> - it's hard to fix all the cases where unaligned things cause crashes
> 
> AVPacket actually documented an alignment and padding requirement. I
> suppose AVFrame is less-well documented, because it has many use-cases
> associated (encoding, filtering, as direct rendering buffer for
> decoding, etc.).
> 
> Where should we go from here? Personally I'd claim framequeue should be
> changed to produce aligned output.

> For encoders and filter input, we
> should probably add a flag whether unaligned input is supported, and if
> not, have the generic code before it copy the frame to make it aligned.

I agree
in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db)
within the API of that time to avfilter

[...]
Nicolas George April 27, 2017, 8:51 a.m.
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.

Regards,
Muhammad Faiz April 30, 2017, 4:55 p.m.
On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george@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.

Thank's
Nicolas George April 30, 2017, 5:06 p.m.
Le primidi 11 floréal, an CCXXV, Muhammad Faiz a écrit :
> Are you working on these? Because currently I'm not.

There is nothing to work on yet: the message you answer to is raising a
question about the global design of the internal API. That question
needs an answer before any work can be done, and I can not decide alone.

Regards,
wm4 April 30, 2017, 5:16 p.m.
On Sun, 30 Apr 2017 23:55:02 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george@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.
Muhammad Faiz May 1, 2017, 6:08 a.m.
On Mon, May 1, 2017 at 12:16 AM, wm4 <nfxjfg@googlemail.com> wrote:
> On Sun, 30 Apr 2017 23:55:02 +0700
> Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george@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
Muhammad Faiz May 1, 2017, 6:12 a.m.
On Mon, May 1, 2017 at 12:06 AM, Nicolas George <george@nsup.org> wrote:
> Le primidi 11 floréal, an CCXXV, Muhammad Faiz a écrit :
>> Are you working on these? Because currently I'm not.
>
> There is nothing to work on yet: the message you answer to is raising a
> question about the global design of the internal API. That question
> needs an answer before any work can be done, and I can not decide alone.

Probably, better to post a patch, choosing one of the designs, and
then it will be reviewed
rather than waiting an answer here.

Thank's.
Paul B Mahol May 1, 2017, 8:18 a.m.
On 4/30/17, Nicolas George <george@nsup.org> wrote:
> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>> Are you working on these? Because currently I'm not.
>
> There is nothing to work on yet: the message you answer to is raising a
> question about the global design of the internal API. That question
> needs an answer before any work can be done, and I can not decide alone.
>

How nice, introducing bug that causes crash and then claiming there is
not such bug.

Which filters you consider deemed worthy to not crash?
Kyle Swanson May 1, 2017, 4:22 p.m.
Hi,

On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>> Are you working on these? Because currently I'm not.
>>
>> There is nothing to work on yet: the message you answer to is raising a
>> question about the global design of the internal API. That question
>> needs an answer before any work can be done, and I can not decide alone.
>>
>
> How nice, introducing bug that causes crash and then claiming there is
> not such bug.
>
> Which filters you consider deemed worthy to not crash?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
until API stuff is sorted. This should also be backported to 3.3
because these issues are present in that release.

Kyle
Ronald S. Bultje May 1, 2017, 4:44 p.m.
Hi,

On Sun, Apr 30, 2017 at 1:16 PM, wm4 <nfxjfg@googlemail.com> wrote:

> 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.


I don't like the patch because it essentially duplicates the C
implementation of the DSP function. If we do that here, are we going to do
that everywhere? Will we get 2 DSP functions, aligned and unaligned? I
foresee a mess.

Michael already said that too, I believe.

Ronald
Ronald S. Bultje May 1, 2017, 4:45 p.m.
Hi,

On Mon, May 1, 2017 at 12:44 PM, Ronald S. Bultje <rsbultje@gmail.com>
wrote:

> Hi,
>
> On Sun, Apr 30, 2017 at 1:16 PM, wm4 <nfxjfg@googlemail.com> wrote:
>
>> 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.
>
>
> I don't like the patch because it essentially duplicates the C
> implementation of the DSP function. If we do that here, are we going to do
> that everywhere? Will we get 2 DSP functions, aligned and unaligned? I
> foresee a mess.
>
> Michael already said that too, I believe.
>

Let me expand a little bit further:
- if we agree that the API does not require alignment, then we should fix
the DSP function (and others in other filters) to allow non-aligned or
differently-aligned input data.
- if not, then the data alignment should be fixed.

Ronald
Muhammad Faiz May 1, 2017, 5:40 p.m.
On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote:
> Hi,
>
> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
>> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>>> Are you working on these? Because currently I'm not.
>>>
>>> There is nothing to work on yet: the message you answer to is raising a
>>> question about the global design of the internal API. That question
>>> needs an answer before any work can be done, and I can not decide alone.
>>>
>>
>> How nice, introducing bug that causes crash and then claiming there is
>> not such bug.
>>
>> Which filters you consider deemed worthy to not crash?
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
> until API stuff is sorted. This should also be backported to 3.3
> because these issues are present in that release.
>
> Kyle

Of course no. Reverting it will make more bug.

Unless https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html
and  https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html
are also applied.

Thank's
Paul B Mahol May 1, 2017, 5:45 p.m.
On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote:
>> Hi,
>>
>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>>>> Are you working on these? Because currently I'm not.
>>>>
>>>> There is nothing to work on yet: the message you answer to is raising a
>>>> question about the global design of the internal API. That question
>>>> needs an answer before any work can be done, and I can not decide alone.
>>>>
>>>
>>> How nice, introducing bug that causes crash and then claiming there is
>>> not such bug.
>>>
>>> Which filters you consider deemed worthy to not crash?
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
>> until API stuff is sorted. This should also be backported to 3.3
>> because these issues are present in that release.
>>
>> Kyle
>
> Of course no. Reverting it will make more bug.
>
> Unless
> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html
> and
> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html
> are also applied.

One of those are already reviewed, other looks like is not needed at all.
Could you elaborate why it is needed?
Muhammad Faiz May 1, 2017, 5:55 p.m.
On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote:
> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote:
>>> Hi,
>>>
>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>>>>> Are you working on these? Because currently I'm not.
>>>>>
>>>>> There is nothing to work on yet: the message you answer to is raising a
>>>>> question about the global design of the internal API. That question
>>>>> needs an answer before any work can be done, and I can not decide alone.
>>>>>
>>>>
>>>> How nice, introducing bug that causes crash and then claiming there is
>>>> not such bug.
>>>>
>>>> Which filters you consider deemed worthy to not crash?
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
>>> until API stuff is sorted. This should also be backported to 3.3
>>> because these issues are present in that release.
>>>
>>> Kyle
>>
>> Of course no. Reverting it will make more bug.
>>
>> Unless
>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html
>> and
>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html
>> are also applied.
>
> One of those are already reviewed, other looks like is not needed at all.
> Could you elaborate why it is needed?

The code before the patch write to unwritable frame.

test-case:
ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp,
asplit [a][b];
[a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
[b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
[a1][b1] vstack'

the data will be corrupted because fixed=on enables partial_buf_size stuff

Compare that without fixed=on.

Thank's
Paul B Mahol May 1, 2017, 6:05 p.m.
On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote:
>> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote:
>>>> Hi,
>>>>
>>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>>>>>> Are you working on these? Because currently I'm not.
>>>>>>
>>>>>> There is nothing to work on yet: the message you answer to is raising
>>>>>> a
>>>>>> question about the global design of the internal API. That question
>>>>>> needs an answer before any work can be done, and I can not decide
>>>>>> alone.
>>>>>>
>>>>>
>>>>> How nice, introducing bug that causes crash and then claiming there is
>>>>> not such bug.
>>>>>
>>>>> Which filters you consider deemed worthy to not crash?
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
>>>> until API stuff is sorted. This should also be backported to 3.3
>>>> because these issues are present in that release.
>>>>
>>>> Kyle
>>>
>>> Of course no. Reverting it will make more bug.
>>>
>>> Unless
>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html
>>> and
>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html
>>> are also applied.
>>
>> One of those are already reviewed, other looks like is not needed at all.
>> Could you elaborate why it is needed?
>
> The code before the patch write to unwritable frame.
>
> test-case:
> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp,
> asplit [a][b];
> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
> [a1][b1] vstack'
>
> the data will be corrupted because fixed=on enables partial_buf_size stuff
>
> Compare that without fixed=on.

Hmm, so it seems Nicolas blocked [1/2] patch which is mandatory for [2/2] one,
time without providing alternative solution.
Muhammad Faiz May 1, 2017, 6:14 p.m.
On Tue, May 2, 2017 at 1:05 AM, Paul B Mahol <onemda@gmail.com> wrote:
> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>>>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>>>>>>> Are you working on these? Because currently I'm not.
>>>>>>>
>>>>>>> There is nothing to work on yet: the message you answer to is raising
>>>>>>> a
>>>>>>> question about the global design of the internal API. That question
>>>>>>> needs an answer before any work can be done, and I can not decide
>>>>>>> alone.
>>>>>>>
>>>>>>
>>>>>> How nice, introducing bug that causes crash and then claiming there is
>>>>>> not such bug.
>>>>>>
>>>>>> Which filters you consider deemed worthy to not crash?
>>>>>> _______________________________________________
>>>>>> ffmpeg-devel mailing list
>>>>>> ffmpeg-devel@ffmpeg.org
>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
>>>>> until API stuff is sorted. This should also be backported to 3.3
>>>>> because these issues are present in that release.
>>>>>
>>>>> Kyle
>>>>
>>>> Of course no. Reverting it will make more bug.
>>>>
>>>> Unless
>>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html
>>>> and
>>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html
>>>> are also applied.
>>>
>>> One of those are already reviewed, other looks like is not needed at all.
>>> Could you elaborate why it is needed?
>>
>> The code before the patch write to unwritable frame.
>>
>> test-case:
>> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp,
>> asplit [a][b];
>> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
>> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
>> [a1][b1] vstack'
>>
>> the data will be corrupted because fixed=on enables partial_buf_size stuff
>>
>> Compare that without fixed=on.
>
> Hmm, so it seems Nicolas blocked [1/2] patch which is mandatory for [2/2] one,
> time without providing alternative solution.

The alternative was 383057f8e744efeaaa3648a59bc577b25b055835, of
course. I approved it at that time.
Paul B Mahol May 1, 2017, 6:17 p.m.
On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Tue, May 2, 2017 at 1:05 AM, Paul B Mahol <onemda@gmail.com> wrote:
>> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>> On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote:
>>>>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote:
>>>>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit :
>>>>>>>>> Are you working on these? Because currently I'm not.
>>>>>>>>
>>>>>>>> There is nothing to work on yet: the message you answer to is
>>>>>>>> raising
>>>>>>>> a
>>>>>>>> question about the global design of the internal API. That question
>>>>>>>> needs an answer before any work can be done, and I can not decide
>>>>>>>> alone.
>>>>>>>>
>>>>>>>
>>>>>>> How nice, introducing bug that causes crash and then claiming there
>>>>>>> is
>>>>>>> not such bug.
>>>>>>>
>>>>>>> Which filters you consider deemed worthy to not crash?
>>>>>>> _______________________________________________
>>>>>>> ffmpeg-devel mailing list
>>>>>>> ffmpeg-devel@ffmpeg.org
>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>
>>>>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted
>>>>>> until API stuff is sorted. This should also be backported to 3.3
>>>>>> because these issues are present in that release.
>>>>>>
>>>>>> Kyle
>>>>>
>>>>> Of course no. Reverting it will make more bug.
>>>>>
>>>>> Unless
>>>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html
>>>>> and
>>>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html
>>>>> are also applied.
>>>>
>>>> One of those are already reviewed, other looks like is not needed at
>>>> all.
>>>> Could you elaborate why it is needed?
>>>
>>> The code before the patch write to unwritable frame.
>>>
>>> test-case:
>>> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp,
>>> asplit [a][b];
>>> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
>>> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
>>> [a1][b1] vstack'
>>>
>>> the data will be corrupted because fixed=on enables partial_buf_size
>>> stuff
>>>
>>> Compare that without fixed=on.
>>
>> Hmm, so it seems Nicolas blocked [1/2] patch which is mandatory for [2/2]
>> one,
>> time without providing alternative solution.
>
> The alternative was 383057f8e744efeaaa3648a59bc577b25b055835, of
> course. I approved it at that time.

This is all one big mess.
Muhammad Faiz May 2, 2017, 9:44 a.m.
On Mon, May 1, 2017 at 11:45 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Mon, May 1, 2017 at 12:44 PM, Ronald S. Bultje <rsbultje@gmail.com>
> wrote:
>
>> Hi,
>>
>> On Sun, Apr 30, 2017 at 1:16 PM, wm4 <nfxjfg@googlemail.com> wrote:
>>
>>> 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.
>>
>>
>> I don't like the patch because it essentially duplicates the C
>> implementation of the DSP function. If we do that here, are we going to do
>> that everywhere? Will we get 2 DSP functions, aligned and unaligned? I
>> foresee a mess.
>>
>> Michael already said that too, I believe.
>>
>
> Let me expand a little bit further:
> - if we agree that the API does not require alignment, then we should fix
> the DSP function (and others in other filters) to allow non-aligned or
> differently-aligned input data.
> - if not, then the data alignment should be fixed.

My opinion:
- for short term, the data alignment should be fixed.
- for long term, the framework should support non-aligned data. No needs for
  the DSP function to support non-aligned data. Instead ensure the framework
  to send aligned data to filters/codecs which require it. As Michael suggested,
  this can be achieved by using flags/required_alignment similar to
need_writable.

Thank's
Nicolas George May 2, 2017, 2:16 p.m.
Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit :
> This is all one big mess.

It is, but I will not take responsibility when it is not mine.

Libavfilter was in need of a serious overhaul. I do not see you denying
it, and you suffered from the limitations it was causing as much as
anybody else. I have undertaken that overhaul, and the help I received
from other developers in that project has been scarce, to say the least.

I do not complain, I can manage on my own; it will take longer, but I am
in no rush. But I will not let the complaints of the inspectors of
finished work trouble me.

An overhaul of this magnitude is sure to introduce bugs. This one did,
and I made a point of fixing them. My work tree is currently set up to
investigate the problem with shortest and dualinput. Once again, feel
free to offer help.

An overhaul of this magnitude is also sure to reveal bugs that were
already present in the code base and just did not manifest due to
undocumented specifics of the old implementation.

The crash we are discussing is caused by the new code violating the
unwritten un-agreed-upon rule that it must keep the data extra-aligned.
Except there is no such thing as an "unwritten un-agreed-upon rule". The
cause of the crash is the code in libmp3lame relying on this unwritten
un-agreed-upon rule. I strongly suspect it can be triggered by an
application without using libavfilter at all.

For now, fixing libmp3lame is the correct solution, and this patch does
it. I think it is not the best long-term solution, but that is not for
me to say.

Another solution would be to write the rule. If somebody does, then I
will implement the code needed to respect it in libavfilter.

But I will not raise another finger to write the rule, because whatever
I do I will be blamed, and I am fed up with that attitude.

Regards,
wm4 May 2, 2017, 5:18 p.m.
On Tue, 2 May 2017 16:16:35 +0200
Nicolas George <george@nsup.org> wrote:

> Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit :
> > This is all one big mess.  
> 
> It is, but I will not take responsibility when it is not mine.
> 
> Libavfilter was in need of a serious overhaul. I do not see you denying
> it, and you suffered from the limitations it was causing as much as
> anybody else. I have undertaken that overhaul, and the help I received
> from other developers in that project has been scarce, to say the least.

That doesn't free you from the responsibility to keep things in a
reasonably working state.

> I do not complain, I can manage on my own; it will take longer, but I am
> in no rush. But I will not let the complaints of the inspectors of
> finished work trouble me.

I can understand that attitude, but if a fix takes "longer" and
meanwhile certain filters or encoders crash left and right in git
master and even in the latest FFmpeg release, there's a need to act
quickly, even if it means reverting certain patches.

I don't want to blame anyone (and I ask you not to blame others either,
like you did above), but please fix/avoid crashing bugs?
Kyle Swanson May 4, 2017, 8:15 p.m.
Hi,

On Tue, May 2, 2017 at 12:18 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Tue, 2 May 2017 16:16:35 +0200
> Nicolas George <george@nsup.org> wrote:
>
>> Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit :
>> > This is all one big mess.
>>
>> It is, but I will not take responsibility when it is not mine.
>>
>> Libavfilter was in need of a serious overhaul. I do not see you denying
>> it, and you suffered from the limitations it was causing as much as
>> anybody else. I have undertaken that overhaul, and the help I received
>> from other developers in that project has been scarce, to say the least.
>
> That doesn't free you from the responsibility to keep things in a
> reasonably working state.
>
>> I do not complain, I can manage on my own; it will take longer, but I am
>> in no rush. But I will not let the complaints of the inspectors of
>> finished work trouble me.
>
> I can understand that attitude, but if a fix takes "longer" and
> meanwhile certain filters or encoders crash left and right in git
> master and even in the latest FFmpeg release, there's a need to act
> quickly, even if it means reverting certain patches.

I believe this is still broken on git master and is present on release
3.3. If a proper fix is going to take time, the patches that caused
this should probably just be reverted for the time being, IMHO.

> I don't want to blame anyone (and I ask you not to blame others either,
> like you did above), but please fix/avoid crashing bugs?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks,
Kyle
Nicolas George May 4, 2017, 8:24 p.m.
Le quintidi 15 floréal, an CCXXV, Kyle Swanson a écrit :
> I believe this is still broken on git master and is present on release

Well, nobody fixed it.

> 3.3. If a proper fix is going to take time,

There is no reason it should, the issue is rather simple.

>					      the patches that caused
> this should probably just be reverted for the time being, IMHO.

Absolutely not, these change were there for a reason, that reason is
still valid.

Regards,
wm4 May 4, 2017, 9:53 p.m.
On Thu, 4 May 2017 22:24:15 +0200
Nicolas George <george@nsup.org> wrote:

> Le quintidi 15 floréal, an CCXXV, Kyle Swanson a écrit :
> > I believe this is still broken on git master and is present on release  
> 
> Well, nobody fixed it.
> 
> > 3.3. If a proper fix is going to take time,  
> 
> There is no reason it should, the issue is rather simple.
> 
> >					      the patches that caused
> > this should probably just be reverted for the time being, IMHO.  
> 
> Absolutely not, these change were there for a reason, that reason is
> still valid.

So I guess our users are supposed to tolerate those crashes?

Revert seems like the right thing to do. Your changes can be reapplied
once the other issues are fixed. Or you come up with a fix now.
Nicolas George May 4, 2017, 10:05 p.m.
Le quintidi 15 floréal, an CCXXV, Nicolas George a écrit :
> Absolutely not, these change were there for a reason, that reason is
> still valid.

Also, the bug is not in libavfilter, so reverting changes in libavfilter
makes no sense.

Regards,
wm4 May 4, 2017, 10:12 p.m.
On Fri, 5 May 2017 00:05:35 +0200
Nicolas George <george@nsup.org> wrote:

> Le quintidi 15 floréal, an CCXXV, Nicolas George a écrit :
> > Absolutely not, these change were there for a reason, that reason is
> > still valid.  
> 
> Also, the bug is not in libavfilter, so reverting changes in libavfilter
> makes no sense.

Yes, it does, because it triggers the crash.

It amazes me that you don't think crashing bugs should be fixed
quickly. Nobody cares if your code is awesome and perfect if it trigger
crashes.
Muhammad Faiz May 5, 2017, 6:20 a.m.
On Fri, May 5, 2017 at 3:15 AM, Kyle Swanson <k@ylo.ph> wrote:
> Hi,
>
> On Tue, May 2, 2017 at 12:18 PM, wm4 <nfxjfg@googlemail.com> wrote:
>> On Tue, 2 May 2017 16:16:35 +0200
>> Nicolas George <george@nsup.org> wrote:
>>
>>> Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit :
>>> > This is all one big mess.
>>>
>>> It is, but I will not take responsibility when it is not mine.
>>>
>>> Libavfilter was in need of a serious overhaul. I do not see you denying
>>> it, and you suffered from the limitations it was causing as much as
>>> anybody else. I have undertaken that overhaul, and the help I received
>>> from other developers in that project has been scarce, to say the least.
>>
>> That doesn't free you from the responsibility to keep things in a
>> reasonably working state.
>>
>>> I do not complain, I can manage on my own; it will take longer, but I am
>>> in no rush. But I will not let the complaints of the inspectors of
>>> finished work trouble me.
>>
>> I can understand that attitude, but if a fix takes "longer" and
>> meanwhile certain filters or encoders crash left and right in git
>> master and even in the latest FFmpeg release, there's a need to act
>> quickly, even if it means reverting certain patches.
>
> I believe this is still broken on git master and is present on release
> 3.3. If a proper fix is going to take time, the patches that caused
> this should probably just be reverted for the time being, IMHO.
>
>> I don't want to blame anyone (and I ask you not to blame others either,
>> like you did above), but please fix/avoid crashing bugs?
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks,
> Kyle

A new patch is posted.

Thank's

Patch hide | download patch | download mbox

diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
index 5e26743..cd505bb 100644
--- a/libavcodec/libmp3lame.c
+++ b/libavcodec/libmp3lame.c
@@ -203,15 +203,20 @@  static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
             ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data);
             break;
         case AV_SAMPLE_FMT_FLTP:
-            if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) {
-                av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n");
-                return AVERROR(EINVAL);
-            }
             for (ch = 0; ch < avctx->channels; ch++) {
+                if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) {
+                    float *src = (float *) frame->data[ch];
+                    float *dst = s->samples_flt[ch];
+                    int k;
+
+                    for (k = 0; k < frame->nb_samples; k++)
+                        dst[k] = src[k] * 32768.0f;
+                } else {
                 s->fdsp->vector_fmul_scalar(s->samples_flt[ch],
                                            (const float *)frame->data[ch],
                                            32768.0f,
                                            FFALIGN(frame->nb_samples, 8));
+                }
             }
             ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt);
             break;