---------- Forwarded message ----------
From: Tony Miller <mcfiredrill@...>
Date: Mon, Apr 5, 2010 at 11:42 PM
Subject: Re: [Musicpd-dev-team] [PATCH] GameMusicEmu decoder
To: Max Kellermann <max@...>
On Mon, Apr 05, 2010 at 11:20:18AM +0200, Max Kellermann wrote:
> On 2010/04/04 10:03, Tony Miller <mcfiredrill@...> wrote:
> > I've written a patch for a decoder that uses the Game Music Emulation
> > library. This library suports many video game music formats. The patch
> > is available in the latest commit in my repository:
> >
> > git@...
> >
> > More information on this library is available here:
> > http://www.fly.net/~ant/libs/audio.html
> >
> > Don't download it from there though, get the latest svn from here:
> > http://code.google.com/p/game-music-emu/source/checkout
>
> Ok, finally got this "cmake" tool to work - cmake throws very obscure
> error messages when you try to run it a second time with different
> parameters (install prefix).
>
> I havn't tried to run your plugin yet, though, but supporting more
> codecs is always a good idea, so I will merge your patch, after some
> formal changes:
>
> - See Avuton's mail on the typo.
>
> - Correct the number of dots:
>
> + echo " GME support ....................enabled"
> + echo " GME support ...................disabled"
>
Fixed.
> - Correct indentation:
>
> + cmd = decoder_data(decoder, NULL,
> + buf, GME_BUF_SIZE,
>
> Should be:
>
> + cmd = decoder_data(decoder, NULL,
> + buf, GME_BUF_SIZE,
>
Can i just put it all on the same line, since its under 80 chars? I see
other sources in src/decoder that do that.
> - Why not set enable_gme=auto by default? If this is a library with
> an unstable API, we should of course only enable it if the user
> explicitly wants it, so we don't break the build for others just in
> case GME changes its API.
>
My thinking exactly, I thought enable_gme=no would accomplish this.
I noticed the other plugins use enable_*=auto however.
> - I don't think your error handling is correct:
>
> + if((gme_err = gme_open_file(path_fs, &emu, sample_rate)) !=
NULL)
> + g_warning("%s\n", gme_err);
>
> I guess if gme_open_file() fails, you shouldn't be using the "emu"
> variable because it wasn't initialized -> you should bail out of the
> function.
>
> Same for other error handlers. Have you tried these code paths?
You're right, I definitely shouldn't try decoding the file if I can't even
open it! How about this in gme_file_decode():
+ if((gme_err = gme_open_file(path_fs, &emu, sample_rate)) != NULL){
+ g_warning("%s\n", gme_err);
+ gme_delete(emu);
+ return;
+ }
+ if((gme_err = gme_track_info(emu, &ti, 0)) != NULL){
+ g_warning("%s\n", gme_err);
+ gme_delete(emu);
+ return;
+ }
+ if((gme_err = gme_play(emu, GME_BUF_SIZE>>1, buf)) != NULL){
+ g_warning("%s\n", gme_err);
+ return;
+ }
And in tag dup:
+ if((gme_err = gme_open_file(path_fs, &emu, sample_rate)) != NULL){
+ g_warning("%s\n", gme_err);
+ gme_delete(emu);
+ return NULL;
+ }
+ if((gme_err = gme_track_info(emu, &ti, 0)) != NULL){
+ g_warning("%s\n", gme_err);
+ gme_delete(emu);
+ return NULL;
+ }
>
> - Use audio_format_init() or audio_format_init_checked() instead of
> manually initializing the audio_format. If we ever add another
> attribute to that type, we don't need to change all initializers in
> all plugins.
+#include "audio_check.h"
- //audio_format.sample_rate = sample_rate;
- //audio_format.format = SAMPLE_FORMAT_S16;
- //audio_format.channels = 2;
- //audio_format.reverse_endian = 0;
- //assert(audio_format_valid(&audio_format));
+ GError *error = NULL;
+ if(!audio_format_init_checked(&audio_format, sample_rate,
SAMPLE_FORMAT_S16,
+ 2, &error)){
+ g_warning("%s\n", error->message);
+ g_error_free(error);
+ gme_delete(emu);
+ gme_free_info(ti);
+ return;
+ }
>
> - Did you really implement seeking? Your seeking code looks like a
> no-op.
>
Yes, I forgot to add these lines in the commit(also made it a bit simpler):
if(cmd == DECODE_COMMAND_SEEK) {
float where = decoder_seek_where(decoder);
- int s = (int)(where*1000);
- s += gme_tell(emu);
+ if((gme_err = gme_seek(emu, (int)where*1000)) !=
NULL)
+ g_warning("%s\n", gme_err);
decoder_command_finished(decoder);
}
> - Remove the NULL struct initializers:
>
> + NULL, /* container scan */
>
> If you don't implement a method, don't mention it. If we ever
> remove a method (which you didn't implement anyway), or if we change
> their order, your plugin might break the build.
Done.
Will it work for you if I just push another commit with the changes, or
should I squash another commit with the fixes on top of the previous one?
|