[FFmpeg-devel] [PATCH] lavf: add libopenmpt demuxer

Clément Bœsch u at pkh.me
Sun Jun 12 22:08:10 CEST 2016


On Sun, Jun 12, 2016 at 02:43:10AM +0100, Josh de Kock wrote:
> ---
>  configure                |   4 +
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/libopenmpt.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 191 insertions(+)
>  create mode 100644 libavformat/libopenmpt.c
> 
> diff --git a/configure b/configure
> index 7c463a5..a74aaab 100755
> --- a/configure
> +++ b/configure
> @@ -248,6 +248,7 @@ External library support:
>    --enable-libopencv       enable video filtering via libopencv [no]
>    --enable-libopenh264     enable H.264 encoding via OpenH264 [no]
>    --enable-libopenjpeg     enable JPEG 2000 de/encoding via OpenJPEG [no]

> +  --enable-libopenmpt      enable decoding tracked mod files via libopenmpt [no]

libopenmpt is supposed to replace libmodplug support all kind of trackers,
not only MOD.

>    --enable-libopus         enable Opus de/encoding via libopus [no]
>    --enable-libpulse        enable Pulseaudio input via libpulse [no]
>    --enable-librubberband   enable rubberband needed for rubberband filter [no]
> @@ -1495,6 +1496,7 @@ EXTERNAL_LIBRARY_LIST="
>      libopencv
>      libopenh264
>      libopenjpeg
> +    libopenmpt
>      libopus
>      libpulse
>      librtmp
> @@ -2741,6 +2743,7 @@ libopencore_amrwb_decoder_deps="libopencore_amrwb"
>  libopenh264_encoder_deps="libopenh264"
>  libopenjpeg_decoder_deps="libopenjpeg"
>  libopenjpeg_encoder_deps="libopenjpeg"
> +libopenmpt_demuxer_deps="libopenmpt"
>  libopus_decoder_deps="libopus"
>  libopus_encoder_deps="libopus"
>  libopus_encoder_select="audio_frame_queue"
> @@ -5633,6 +5636,7 @@ enabled libopenjpeg       && { check_lib openjpeg-2.1/openjpeg.h opj_version -lo
>                                 check_lib openjpeg-1.5/openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC ||
>                                 check_lib openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC ||
>                                 die "ERROR: libopenjpeg not found"; }

> +enabled libopenmpt        && require libopenmpt libopenmpt/libopenmpt.h openmpt_module_create -lopenmpt -lstdc++

openmpt is distributed with pkg-config so please use require_pkg_config.
And then you shouldn't need -lstdc++ if the .pc is properly done.

[...]
> +/**
> +* @file
> +* libopenmpt demuxer
> +*/

not really useful

> +
> +#include <libopenmpt/libopenmpt.h>
> +#include "libavutil/avstring.h"

> +#include "libavutil/eval.h"

doesn't look necessary (maybe check the other includes)

> +#include "libavutil/opt.h"
> +#include "avformat.h"
> +#include "internal.h"
> +
> +typedef struct OpenMPTContext {
> +    const AVClass *class;
> +    openmpt_module *module;
> +
> +    int channels;

> +    double length;

better call this duration for consistency

> +    /* options */
> +    int sample_rate;
> +} OpenMPTContext;
> +
> +#define OFFSET(x) offsetof(OpenMPTContext, x)
> +#define A AV_OPT_FLAG_AUDIO_PARAM
> +#define D AV_OPT_FLAG_DECODING_PARAM
> +static const AVOption options[] = {

> +    {"sample_rate", "set sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.i64 = 44100}, 1000, 999999, A|D},

INT_MAX instead of 999999?

> +    {NULL}
> +};
> +
> +static int probe_openmpt(AVProbeData *p)
> +{
> +    if (p->buf_size < 1084)
> +        return 0;
> +
> +    if (p->buf[1080] == 'M' && p->buf[1081] == '.' &&
> +        p->buf[1082] == 'K' && p->buf[1083] == '.')
> +        return AVPROBE_SCORE_MAX;
> +

So this is going to probe only one kind of tracker? It would be nice to
detect all the tracker files the library supports. With modplug we rely on
extension (bad); does openmpt support something better (something better
than trying to decode, which will be slow and slow down every other
probing)?

> +    return 0;
> +}
> +
> +static void openmpt_logfunc(const char *message, void *userdata)
> +{

> +    av_log((AVFormatContext*)userdata, AV_LOG_INFO, "%s\n", message);

The cast is not necessary. No loglevel to differenciate error from
warnings from info? That's a shame.

> +}
> +
> +#define add_meta(s, name, value)                  \
> +    if (value && value[0])                        \
> +       av_dict_set(&s->metadata, name, value, 0); \
> +

nit: scope this in a do { ... } while (0) form

> +static int read_header_openmpt(AVFormatContext *s)
> +{
> +    AVStream *st;
> +    OpenMPTContext *openmpt = s->priv_data;
> +    int64_t size = avio_size(s->pb);
> +    char *buf = av_malloc(size);
> +
> +    if (!buf)
> +        return AVERROR(ENOMEM);
> +    size = avio_read(s->pb, buf, size);
> +

> +    if (!(openmpt->module = openmpt_module_create_from_memory(buf, size, openmpt_logfunc, s, NULL))) {
> +        av_free(buf);
> +        return AVERROR_INVALIDDATA;
> +    }
> +    av_free(buf);

    openmpt->module = openmpt_module_create_from_memory(buf, size, openmpt_logfunc, s, NULL);
    av_freep(&buf);
    if (!openmpt->module)
        return AVERROR_EXTERNAL;

(or maybe AVERROR(ENOMEM)?)

> +    openmpt->channels = openmpt_module_get_num_channels(openmpt->module);

Only a number of channels; no channel layout provided?

> +    openmpt->length   = openmpt_module_get_duration_seconds(openmpt->module);
> +

> +    add_meta(s, "artist",  openmpt_module_get_metadata(openmpt->module, "artist"));
> +    add_meta(s, "title",   openmpt_module_get_metadata(openmpt->module, "title"));
> +    add_meta(s, "encoder", openmpt_module_get_metadata(openmpt->module, "tracker"));
> +    add_meta(s, "comment", openmpt_module_get_metadata(openmpt->module, "message"));

add_meta() is going to call the same function 3 times (you could have a
local const char *value in the local do { ... } while (0) suggested.)

> +
> +    st = avformat_new_stream(s, NULL);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +    avpriv_set_pts_info(st, 64, 1, 1000);
> +    if (st->duration > 0)
> +        st->duration = openmpt->length;

Is this really in double?

> +
> +    st->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
> +    st->codecpar->codec_id    = AV_NE(AV_CODEC_ID_PCM_S16BE, AV_CODEC_ID_PCM_S16LE);
> +    st->codecpar->channels    = openmpt->channels;
> +    st->codecpar->sample_rate = openmpt->sample_rate;
> +
> +    return 0;
> +}
> +
> +#define AUDIO_PKT_SIZE 2048
> +
> +static int read_packet_openmpt(AVFormatContext *s, AVPacket *pkt)
> +{
> +    OpenMPTContext *openmpt = s->priv_data;
> +    int n_samples = AUDIO_PKT_SIZE / (openmpt->channels ? openmpt->channels*2 : 2);
> +    int ret;
> +
> +    if ((ret = av_new_packet(pkt, AUDIO_PKT_SIZE)) < 0)
> +        return ret;
> +
> +    switch (openmpt->channels) {

> +    case 1: {
> +        ret = openmpt_module_read_mono(openmpt->module, openmpt->sample_rate,
> +                                       n_samples, (short *)pkt->data);
> +        break;
> +    }

you don't need to scope each case as you're not declaring ret (or any
other variable) inside them.

> +    case 2: {
> +        ret = openmpt_module_read_interleaved_stereo(openmpt->module, openmpt->sample_rate,
> +                                                    n_samples, (short *)pkt->data);
> +        break;
> +    }
> +    case 4: {
> +        ret = openmpt_module_read_interleaved_quad(openmpt->module, openmpt->sample_rate,
> +                                                   n_samples, (short *)pkt->data);
> +        break;
> +    }
> +    default:

> +        av_log(s, AV_LOG_ERROR, "Invalid amount of channels: %d", openmpt->channels);

I'm not a native speaker so correct me if I'm wrong but using "amount"
sounds weird here; you probably want "number".

I'd also use "Unsupported" instead of "Invalid" (and adjust the code
message).

> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (ret < 1 || (openmpt->length < openmpt_module_get_position_seconds(openmpt->module)))
> +        return AVERROR_EOF;
> +

> +    pkt->size = AUDIO_PKT_SIZE;

aren't you supposed to use pkt->size = ret?

> +
> +    return 0;
> +}
> +
> +static int read_close_openmpt(AVFormatContext *s)
> +{
> +    OpenMPTContext *openmpt = s->priv_data;
> +    openmpt_module_destroy(openmpt->module);
> +    return 0;
> +}
> +
> +static int read_seek_openmpt(AVFormatContext *s, int stream_idx, int64_t ts, int flags)
> +{
> +    OpenMPTContext *openmpt = s->priv_data;
> +    openmpt_module_set_position_seconds(openmpt->module, (double)ts/AV_TIME_BASE);
> +    return 0;
> +}
> +
> +static const AVClass class_openmpt = {
> +    .class_name = "libopenmpt",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
> +AVInputFormat ff_libopenmpt_demuxer = {
> +    .name           = "libopenmpt",
> +    .long_name      = NULL_IF_CONFIG_SMALL("Tracker MOD format (libopenmpt)"),
> +    .priv_data_size = sizeof(OpenMPTContext),
> +    .read_probe     = probe_openmpt,
> +    .read_header    = read_header_openmpt,
> +    .read_packet    = read_packet_openmpt,
> +    .read_close     = read_close_openmpt,
> +    .read_seek      = read_seek_openmpt,
> +    .priv_class     = &class_openmpt,
> +    .extensions     = "mod",
> +};

Note: please add "TODO: bump lavf minor" in the commit description so the
commiter doesn't forget. You should also reference the trac ticket.
And you can update the Changelog as well.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160612/d952894e/attachment.sig>


More information about the ffmpeg-devel mailing list