Re: [FFmpeg-devel] PATCH - libmad MP3 decoding support

[David Fletcher <David@megapico.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 10150 bytes --]

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










[-- Attachment #2: libmad_v2-git-master.patch --]
[-- Type: application/octet-stream, Size: 9250 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".