[FFmpeg-devel] PATCH - libmad MP3 decoding support
David Fletcher
David at megapico.co.uk
Thu May 5 02:47:01 EEST 2022
> Andreas Rheinhardt wrote:
>
> 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.
Now you've pointed it out - yes, it does! This error was matched by a similar
use of AVCodec in place of FFCodec in the libmaddec.c file. It seems these
structures have enough in common that I'd got away with it running without
noticing this.
>
> >
> > 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.
This was left over from some development experiments while I was working out
how to integrate libmad. It was not used and needed to be removed. The
ff_libmad_decoder structure in libmaddec.c already used AV_CODEC_ID_MP3 and
never mentioned AV_CODEC_ID_LIBMAD.
>
> > 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.
Thanks for pointing this out - removed now.
>
> > +
> > + 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.)
I'd played it safe by including checks on things - but since you've confirmed
that AVCodecContext will not be NULL and private data will always be
allocated these can be removed.
>
> > + 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.
Modified to return the error now
>
> > +
> > + 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
Hi Andreas,
Many thanks for your review and help to improve this. I've inserted
comments/replies above, and an updated version of the patch file is attached.
It compiles and runs, and I've just converted a range of mp3 files with it.
I've realised I was suppressing some warnings when I compiled previously,
including about deprecated code in use of avc->channel_layout and
avc->channels. I've made updates to use the avc->ch_layout and
avc->ch_layout.nb_channels in the updated patch. Could you take another look?
Best regards, David.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libmad_v2-git-master.patch
Type: application/octet-stream
Size: 9250 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220505/d12febf9/attachment.obj>
More information about the ffmpeg-devel
mailing list