[FFmpeg-devel] [PATCH] libgme support

[Stefano Sabatini]

On date Saturday 2013-06-29 17:52:08 +0200, wm4 encoded:
> On Sat, 29 Jun 2013 13:30:04 +0200
> Stefano Sabatini <stefasab at gmail.com> wrote:
> 
> > You might add some documentation in ffmpeg-formats.texi with a few
> > examples, so people can get a clue about how to use it.
> 
> I'm not sure where. libmodplug isn't documented anywhere either.
> 
> How to use it? Well, duh, enable it at compilation and just play a file?
> Here's a test file:
>   http://game-music-emu.googlecode.com/svn/trunk/test.nsf

Well at least the options could be documented a bit more, then you
could add the usual blurb about configuration.

> 
> > > +/*
> > > + * This file is part of FFmpeg.
> > 
> > Missing copyright?
> 

> libmodplug.c served as template. I didn't change the copyright header,
> and I'm not sure whether I really should put my name on there, if
> that's what you want.

I don't think we *require* contributions not to be anonymous (and
there is no way to verify attribution anyway). On the other hand
having a file with no copyiright is a bit weird (but I won't insist on
putting a name if you don't want it for whatever reason).

> I changed the probing scores a little. Clément Bœsch's suggestions are
> included as well. Also changed the n_samples calculation, which fixes
> distorted sound (oops). If there are multiple tracks, add the track
> count as metadata. 
> 

> I would prefer to add the tracks as streams, but since you can play
> only one track at a time, this is perhaps not possible. I wonder if
> there is some way to change the current track at runtime? The GME API
> allows it.

I don't think so. You could check the track option at each iteration,
and change stream if the option was set to a new value. Not very clean
but at the moment we lack a different system. A possibility would be
to add a command API (like the one in libavfilter).

> From 64054d77d680ebe41630aa4be997e0570f0be34d Mon Sep 17 00:00:00 2001
> From: wm4 <nfxjfg at googlemail.com>
> Date: Fri, 28 Jun 2013 10:36:00 +0200
> Subject: [PATCH] lavf: add support for libgme
> 
> ---
>  configure                |   4 +
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/libgme.c     | 201 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 207 insertions(+)
>  create mode 100644 libavformat/libgme.c
> 
[...]
> diff --git a/libavformat/libgme.c b/libavformat/libgme.c
> new file mode 100644
> index 0000000..95f5dde
> --- /dev/null
> +++ b/libavformat/libgme.c
> @@ -0,0 +1,201 @@
> +/*
> + * 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
> + */
> +
> +/**
> +* @file
> +* libgme demuxer
> +*/
> +
> +#include <gme/gme.h>
> +#include "libavutil/avstring.h"
> +#include "libavutil/eval.h"
> +#include "libavutil/opt.h"
> +#include "avformat.h"
> +#include "internal.h"
> +
> +typedef struct GMEContext {
> +    const AVClass *class;
> +    Music_Emu *music_emu;
> +    gme_info_t *info;   ///< selected track
> +
> +    /* options */
> +    int track_index;
> +    int sample_rate;
> +    int64_t max_size;
> +} GMEContext;
> +
> +#define OFFSET(x) offsetof(GMEContext, x)
> +#define A AV_OPT_FLAG_AUDIO_PARAM
> +#define D AV_OPT_FLAG_DECODING_PARAM
> +static const AVOption options[] = {
> +    {"track_index", "set track that should be played",  OFFSET(track_index), AV_OPT_TYPE_INT, {.i64 = 0}, 0,       INT_MAX, A|D},
> +    {"sample_rate", "set sample rate", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.i64 = 44100}, 1000, 999999, A|D},

> +    {"max_size",        "set max file size supported (in bytes)",

Nit++: weird align

> +     OFFSET(max_size), AV_OPT_TYPE_INT64, {.i64 = 50 * 1024 * 1024}, 0, SIZE_MAX, A|D},
> +    {NULL}
> +};
> +

> +static void add_meta(AVFormatContext *s, const char *name, const char *value)

nit: you could inline this

> +{
> +    if (value && value[0])
> +        av_dict_set(&s->metadata, name, value, 0);
> +}
> +
> +static int load_metadata(AVFormatContext *s)
> +{
> +    GMEContext *gme = s->priv_data;
> +    gme_info_t *info = gme->info;

> +    int n_tracks = gme_track_count(gme->music_emu);

nit++: int track_count for consistency

> +
> +    add_meta(s, "system",       info->system);
> +    add_meta(s, "game",         info->game);
> +    add_meta(s, "song",         info->song);
> +    add_meta(s, "author",       info->author);
> +    add_meta(s, "copyright",    info->copyright);
> +    add_meta(s, "comment",      info->comment);
> +    add_meta(s, "dumper",       info->dumper);
> +
> +    if (n_tracks > 1) {
> +        char buf[30];
> +        snprintf(buf, sizeof(buf), "%d", n_tracks);
> +        add_meta(s, "tracks", buf);
> +    }
> +
> +    return 0;
> +}
> +
> +#define AUDIO_PKT_SIZE 512
> +
> +static int read_header_gme(AVFormatContext *s)
> +{
> +    AVStream *st;
> +    AVIOContext *pb = s->pb;
> +    GMEContext *gme = s->priv_data;
> +    int64_t sz = avio_size(pb);
> +    char *buf;
> +    char dummy;
> +
> +    if (sz < 0) {
> +        av_log(s, AV_LOG_WARNING, "Could not determine file size\n");
> +        sz = gme->max_size;
> +    } else if (gme->max_size && sz > gme->max_size) {
> +        sz = gme->max_size;
> +    }
> +
> +    buf = av_malloc(sz);
> +    if (!buf)
> +        return AVERROR(ENOMEM);
> +    sz = avio_read(pb, buf, sz);
> +

> +    // Data left means our buffer (the max_size option) is too small

> +    if (avio_read(pb, &dummy, 1) == 1)
> +        return AVERROR_BUFFER_TOO_SMALL;

Here you might provide some operative instruction for the user
(e.g. "try to increase max_size").

[...]

No more comments from me.
-- 
FFmpeg = Fostering and Frightening MultiPurpose Efficient Gigant