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

[Michael Niedermayer]

On Wed, Apr 26, 2017 at 10:05:40PM +0200, wm4 wrote:
> 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 agree
in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db)
within the API of that time to avfilter

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170427/efd85f78/attachment.sig>