[FFmpeg-devel] PATCH - libmad MP3 decoding support

Please find attached a patch adding support for MP3 decoding using
libmad. This is against release ffmpeg-5.0.1, and works for libmad
0.15.1b (the most recent available).

I hope that this is useful. Some context - I found the FFmpeg inbuild
fixed point MP3 decoder produced very distorted audio for low bitrate
streams (56k/s and lower). This was on an ARMv4 CPU for which I needed
to make some adaptation to the fixed point maths routines which
currently target more recent CPUs. I was not able to resolve the
distortion issue and it was easier to integrate libmad decoding which is
already optimised for very fast decoding on the ARMv4 CPU. More info
about this application here:
http://www.megapico.co.uk/sharpfin/mediaserver.html.

Best regards, David.

Only patches for master are accepted.

No more wrappers.

Thanks.

David Fletcher (12022-05-01):
> I hope that this is useful. Some context - I found the FFmpeg inbuild
> fixed point MP3 decoder produced very distorted audio for low bitrate
> streams (56k/s and lower).

Is there a trac ticket? If not, please fill one: we would not want to
keep that bug.

Regards,

On 2/5/2022, "Paul B Mahol" <onemda@gmail.com> wrote:
>Only patches for master are accepted.
>
>No more wrappers.
>
>Thanks.

Hi Paul,

Please find attached the libmad MP3 decoding patch against the master
(ffmpeg-master-b67572c).

I'm not sure what "No more wrappers" means?

Best regards, David.

On 2/5/2022, "Nicolas George" <george@nsup.org> wrote:
>Is there a trac ticket? If not, please fill one: we would not want to
>keep that bug.
>
>Regards,
>
>-- 
>  Nicolas George

Hi Nicolas,

I'll prepare a test case to demonstrate the issue and fill in a ticket.
As far as I can tell this is an issue affecting older ARM hardware, it
cannot be reproduced on x86 hardware. I now have some additional ARM
hardware with a more capable CPU which should help pin down the problem.

Best regards, David.

On 02.05.2022 19:45, David Fletcher wrote:
> On 2/5/2022, "Nicolas George" <george@nsup.org> wrote:
>> Is there a trac ticket? If not, please fill one: we would not want to
>> keep that bug.
>>
>> Regards,
>>
>> -- 
>>   Nicolas George
> 
> Hi Nicolas,
> 
> I'll prepare a test case to demonstrate the issue and fill in a ticket.
> As far as I can tell this is an issue affecting older ARM hardware, it
> cannot be reproduced on x86 hardware. I now have some additional ARM
> hardware with a more capable CPU which should help pin down the problem.
> 
> Best regards, David.

That sounds like a pretty serious issue to me, and I'd rather see that 
fixed than an entire other decoder library pulled in, just to work 
around it.

On Mon, May 2, 2022 at 1:42 PM David Fletcher <David@megapico.co.uk> wrote:
>
> On 2/5/2022, "Paul B Mahol" <onemda@gmail.com> wrote:
> >Only patches for master are accepted.
> >
> >No more wrappers.
> >
> >Thanks.
>
> Hi Paul,
>
> Please find attached the libmad MP3 decoding patch against the master
> (ffmpeg-master-b67572c).
>
> I'm not sure what "No more wrappers" means?
>

Don't worry about it. He doesn't like seeing the use of codec libraries.

That said, it's concerning that the mp3 codec is so broken in ffmpeg.
It would be good to have a bug report so someone can look at fixing
it.



--
真実はいつも一つ！/ Always, there's only one truth!

On Mon, 2 May 2022, David Fletcher wrote:

> On 2/5/2022, "Nicolas George" <george@nsup.org> wrote:
>> Is there a trac ticket? If not, please fill one: we would not want to
>> keep that bug.
>>
>> Regards,
>>
>> --
>>  Nicolas George
>
> Hi Nicolas,
>
> I'll prepare a test case to demonstrate the issue and fill in a ticket.
> As far as I can tell this is an issue affecting older ARM hardware, it
> cannot be reproduced on x86 hardware. I now have some additional ARM
> hardware with a more capable CPU which should help pin down the problem.

That sounds like there's an issue with some of the ARM implementations of 
DSP functions, that only exists in implementations for less featureful ARM 
instruction sets.

(It could theoretically be a compiler bug also but I think it's much more 
probable that it's a bug in a seldom used codepath in ffmpeg.)

If you pass "-cpuflags 0" to ffmpeg, does the issue vanish? (If it does, 
it would suggest an issue in e.g. an armv5/armv6 specific implementation 
of some DSP functions.) Does if vanish if you configure with 
--arch=generic? (That would omit all ARM specific DSP routines and just 
use plain C code.)

On a more modern ARM cpu, if it works correctly normally, does the issue 
appear if you use "-cpuflags 0"? That would indicate an issue in one of 
the ARM specific routines that are enabled without cpuflags (e.g. that 
don't require anything above armv4).

// Martin

On 2/5/2022, "Martin Storsjö" martin at martin.st  wrote:
> That sounds like there's an issue with some of the ARM implementations of 
> DSP functions, that only exists in implementations for less featureful ARM 
> instruction sets.
> 
> (It could theoretically be a compiler bug also but I think it's much more 
> probable that it's a bug in a seldom used codepath in ffmpeg.)
> 
> If you pass "-cpuflags 0" to ffmpeg, does the issue vanish? (If it does, 
> it would suggest an issue in e.g. an armv5/armv6 specific implementation 
> of some DSP functions.) Does if vanish if you configure with 
> --arch=generic? (That would omit all ARM specific DSP routines and just 
> use plain C code.)
> 
> On a more modern ARM cpu, if it works correctly normally, does the issue 
> appear if you use "-cpuflags 0"? That would indicate an issue in one of 
> the ARM specific routines that are enabled without cpuflags (e.g. that 
> don't require anything above armv4).
> 

Hi Martin,

Thanks for these tips. I've posted a bug tracker about this issue, and
also created some sample audio files that demonstrate things working
well on ARMv7, but with distortion on ARMv4.
https://trac.ffmpeg.org/ticket/9764

So far I've been using the libav libraries rather than ffmpeg so it's
not so easy to pass "-cpuflags 0". But this is I think doing the same
thing I've been doing with when configuring to compile FFmpeg. I tried
a range of  --disable-vfp --disable-neon --enable-asm or --disable-asm.
I could not find a combination that makes the issue go away. As you say,
that could well indicate something in an ARM routine that always runs,
and is not superseded by an ARM5 or higher alternative when the CPU can
handle that.

I've not tried --arch=generic. I'm cross-compiling on x86 targeting
ARMv4 so the cross-compile will stop working properly if I take out
--arch=armv4t.

I'll do some further investigation and see if I can create ffmpeg
through the cross-compilation rather than the transcode program I've
been using (this is based on the example transcode_aac).

Many thanks, David.

Following today's posts about help with submitting patches I realised I
sent the libmad patch yesterday in the wrong format. Apologies, I was
not familiar with the git format patches.

Hopefully the attached version is now in the correct format against the
current master branch.

The bug report about why this exists is at the following link, including
a link to sample distorted audio from decoding an mp3 stream on ARMv4
hardware: https://trac.ffmpeg.org/ticket/9764

Best regards, David.

David Fletcher:
> Following today's posts about help with submitting patches I realised I
> sent the libmad patch yesterday in the wrong format. Apologies, I was
> not familiar with the git format patches.
> 
> Hopefully the attached version is now in the correct format against the
> current master branch.
> 
> The bug report about why this exists is at the following link, including
> a link to sample distorted audio from decoding an mp3 stream on ARMv4
> hardware: https://trac.ffmpeg.org/ticket/9764
> 
> Best regards, David.
> 

> 
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index c47133aa18..e3df6178c8 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -744,6 +744,7 @@ extern const FFCodec ff_libcodec2_decoder;
>  extern const FFCodec ff_libdav1d_decoder;
>  extern const FFCodec ff_libdavs2_decoder;
>  extern const FFCodec ff_libfdk_aac_encoder;
> +extern const AVCodec ff_libmad_decoder;
>  extern const FFCodec ff_libfdk_aac_decoder;
>  extern const FFCodec ff_libgsm_encoder;
>  extern const FFCodec ff_libgsm_decoder;

This should look weird to you.

> 
> diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h
> index 8b317fa121..be70f4a71c 100644
> --- a/libavcodec/codec_id.h
> +++ b/libavcodec/codec_id.h
> @@ -519,6 +519,7 @@ enum AVCodecID {
>      AV_CODEC_ID_FASTAUDIO,
>      AV_CODEC_ID_MSNSIREN,
>      AV_CODEC_ID_DFPWM,
> +    AV_CODEC_ID_LIBMAD,
>  
>      /* subtitle codecs */
>      AV_CODEC_ID_FIRST_SUBTITLE = 0x17000,          ///< A dummy ID pointing at the start of subtitle codecs.

This makes no sense: Your decoder is still expected to decode MP3 and
not a new, previously unsupported format.

> diff --git a/libavcodec/libmaddec.c b/libavcodec/libmaddec.c
> new file mode 100644
> index 0000000000..7082c53f4d
> --- /dev/null
> +++ b/libavcodec/libmaddec.c
> @@ -0,0 +1,181 @@
> +/*
> + * MP3 decoder using libmad
> + * Copyright (c) 2022 David Fletcher
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <mad.h>
> +
> +#include "libavutil/channel_layout.h"
> +#include "libavutil/common.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +#include "decode.h"
> +
> +#define MAD_BUFSIZE (32 * 1024)
> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
> +
> +typedef struct libmad_context {
> +    uint8_t input_buffer[MAD_BUFSIZE+MAD_BUFFER_GUARD];
> +    struct mad_synth  synth; 
> +    struct mad_stream stream;
> +    struct mad_frame  frame;
> +    struct mad_header header;
> +    int got_header;
> +}libmad_context;		
> +
> +/* utility to scale and round samples to 16 bits */
> +static inline signed int mad_scale(mad_fixed_t sample)
> +{
> +     /* round */
> +     sample += (1L << (MAD_F_FRACBITS - 16));
> + 
> +     /* clip */
> +     if (sample >= MAD_F_ONE)
> +         sample = MAD_F_ONE - 1;
> +     else if (sample < -MAD_F_ONE)
> +         sample = -MAD_F_ONE;
> +    
> +     /* quantize */
> +     return sample >> (MAD_F_FRACBITS + 1 - 16);
> +}
> +
> +static av_cold int libmad_decode_init(AVCodecContext *avc)
> +{
> +     libmad_context *mad = avc->priv_data;
> +
> +     mad_synth_init  (&mad->synth);
> +     mad_stream_init (&mad->stream);
> +     mad_frame_init  (&mad->frame);
> +     mad->got_header = 0;
> +
> +     return 0;
> +}
> +
> +static av_cold int libmad_decode_close(AVCodecContext *avc)
> +{
> +     libmad_context *mad = avc->priv_data;
> +
> +     mad_synth_finish(&mad->synth);
> +     mad_frame_finish(&mad->frame);
> +     mad_stream_finish(&mad->stream);
> +
> +     mad = NULL;

This is pointless as the lifetime of this pointer ends with returning
from this function anyway.

> +    
> +     return 0;
> +}
> +
> +static int libmad_decode_frame(AVCodecContext *avc, void *data,
> +                          int *got_frame_ptr, AVPacket *pkt)
> +{
> +     AVFrame *frame = data;
> +     libmad_context *mad = avc->priv_data;
> +     struct mad_pcm *pcm;
> +     mad_fixed_t const *left_ch;
> +     mad_fixed_t const *right_ch;
> +     int16_t *output;
> +     int nsamples;
> +     int nchannels;
> +     size_t bytes_read = 0;
> +     size_t remaining = 0;
> +     
> +     if (!avc)
> +	 return 0;

A codec can presume the AVCodecContext to not be NULL.

> +     
> +     if (!mad)
> +	 return 0;
> +     

Similarly, every codec can presume the Codec's private data to be
allocated (and be retained between calls to the same AVCodecContext
instance).
(Furthermore, the initialization of mad presumes avc to be not NULL,
which makes the above check pointless.)

> +     remaining = mad->stream.bufend - mad->stream.next_frame;
> +     memmove(mad->input_buffer, mad->stream.next_frame, remaining);
> +     bytes_read = MIN(pkt->size, MAD_BUFSIZE - remaining);
> +     memcpy(mad->input_buffer+remaining, pkt->data, bytes_read);
> +     
> +     if (bytes_read == 0){
> +	 *got_frame_ptr = 0;
> +	 return 0;
> +     }
> +     
> +     mad_stream_buffer(&mad->stream, mad->input_buffer, remaining + bytes_read);
> +     mad->stream.error = 0;
> +     
> +     if(!mad->got_header){
> +	 mad_header_decode(&mad->header, &mad->stream);
> +	 mad->got_header = 1;
> +	 avc->frame_size = 32 * (mad->header.layer == MAD_LAYER_I ? 12 : \
> +				 ((mad->header.layer == MAD_LAYER_III && \
> +				   (mad->header.flags & MAD_FLAG_LSF_EXT)) ? 18 : 36));
> +	 avc->sample_fmt = AV_SAMPLE_FMT_S16;
> +	 if(mad->header.mode == MAD_MODE_SINGLE_CHANNEL){
> +	     avc->channel_layout = AV_CH_LAYOUT_MONO;
> +	     avc->channels = 1;
> +	 }else{
> +	     avc->channel_layout = AV_CH_LAYOUT_STEREO;
> +	     avc->channels = 2;
> +	 }
> +     }
> +     
> +     frame->channel_layout = avc->channel_layout;
> +     frame->format = avc->sample_fmt;
> +     frame->channels = avc->channels;
> +     frame->nb_samples = avc->frame_size; 
> +     
> +     if ((ff_get_buffer(avc, frame, 0)) < 0)
> +	 return 0;

Return the error.

> +     
> +     if (mad_frame_decode(&mad->frame, &mad->stream) == -1) {
> +	 *got_frame_ptr = 0;
> +	 return mad->stream.bufend - mad->stream.next_frame;
> +     }
> +     
> +     mad_synth_frame (&mad->synth, &mad->frame);
> +     
> +     pcm = &mad->synth.pcm;
> +     output = (int16_t *)frame->data[0];
> +     nsamples = pcm->length;
> +     nchannels = pcm->channels;
> +     left_ch = pcm->samples[0];
> +     right_ch = pcm->samples[1];
> +     while (nsamples--) {
> +	 *output++ = mad_scale(*(left_ch++));
> +	 if (nchannels == 2) {
> +	     *output++ = mad_scale(*(right_ch++));
> +	 }
> +	 //Players should recognise mono and play through both channels
> +	 //Writing the same thing to both left and right channels here causes
> +	 //memory issues as it creates double the number of samples allocated.
> +     }
> +     
> +     *got_frame_ptr = 1;
> +     
> +     return mad->stream.bufend - mad->stream.next_frame;
> +}
> +
> +AVCodec ff_libmad_decoder = {
> +    .name           = "libmad",
> +    .long_name      = NULL_IF_CONFIG_SMALL("libmad MP3 decoder"),
> +    .wrapper_name   = "libmad",
> +    .type           = AVMEDIA_TYPE_AUDIO,
> +    .id             = AV_CODEC_ID_MP3,
> +    .sample_fmts    = (const enum AVSampleFormat[]) { AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_NONE },
> +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_CHANNEL_CONF,
> +    .priv_data_size = sizeof(libmad_context),
> +    .init           = libmad_decode_init,
> +    .close          = libmad_decode_close,
> +    .decode         = libmad_decode_frame
> +};

This should not even compile with latest master.

- Andreas