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

[wm4]

On Thu, 27 Apr 2017 01:41:04 +0700
Muhammad Faiz <mfcc64 at gmail.com> wrote:

> On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Wed, 26 Apr 2017 20:09:58 +0700
> > Muhammad Faiz <mfcc64 at gmail.com> wrote:
> >  
> >> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg at googlemail.com> wrote:  
> >> > On Wed, 26 Apr 2017 17:16:05 +0700
> >> > Muhammad Faiz <mfcc64 at gmail.com> wrote:
> >> >  
> >> >> This should fix Ticket6349
> >> >>
> >> >> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may
> >> >> generate unaligned frame data.
> >> >>
> >> >> Signed-off-by: Muhammad Faiz <mfcc64 at 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.