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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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