Re: [fluid-dev] FluidSynth backend for SDL_mixer
Hi David, Thanks for replying. If you don't mind, I'll take this conversation (about MIDI buffers) to the other thread (Making MIDI player read from a buffer), and stop CCing James, Sam and Ryan. PS. Ryan, if you're reading this, I heard your FLOSS Weekly interview a couple of years back (on SDL), and it was really inspiring. I took your advice and wrote Tetris :) ___ fluid-dev mailing list fluid-dev@nongnu.org http://lists.nongnu.org/mailman/listinfo/fluid-dev
Re: [fluid-dev] Making MIDI player read from a buffer
Hi David,
In response to your post here:
http://lists.nongnu.org/archive/html/fluid-dev/2011-01/msg3.html
Well, for the memory allocation issue it sounds like we both are leaning
towards #5 as in copying the memory. I don't think the
inefficiency is an issue and it gives FS the most options to change later.
Agreed. OK. I will change the code so it copies the buffer, and the
user can free it immediately as soon as fluid_player_add_mem returns.
As for the rest of the patch - well, haven't gone through it letter by letter
but it looks good to me. Assuming that it works, I think you've
done a good job with it!
Thanks! If you (or anyone) wants to test it, I have added a code
snippet to the front page of the documentation which includes a small
sample in-memory MIDI file. I don't know what more-rigorous testing we
could use (I have tried it on some full-size MIDI files and it works).
Is there a test suite I should add some cases for?
A question about the fluid_midi_file struct:
typedef struct {
const char* buffer; /* Entire contents of MIDI file (borrowed) */
const char* buf_end; /* One-past-end of contents buffer */
const char* buf_ptr; /* Current read position in contents buffer */
int eof; /* The end of file condition */
int running_status;
int c;
int type;
int dtime;
} fluid_midi_file;
First, is the eof variable necessary or is that the same as buf_ptr =
buf_end ?
Yes it is necessary to have a separate variable. With the first four
fields (buffer, buf_end, buf_ptr and eof), I am trying to emulate the
stdio FILE type as closely as possible, since the original code worked
directly with a FILE*. I have provided a new in-memory version of
each of the stdio file operations, such as fgetc, fread, fseek and
feof, and tried to implement the same semantics as the man pages for
those functions.
I remember being confused by this as well. The way feof works is not
to check if the *next* fgetc operation would fail. It tests the
end-of-file indicator -- checking whether the *previous* fgetc
operation failed. This end-of-file indicator is set by any file
operation which attempts to read past the EOF. A getc of the last byte
in a file does not set the indicator; only a subsequent getc would set
it. Therefore, it implies some state beyond what is the position in
the file? You also need to know have any previous operations
failed?
This is an important distinction in its usage at the end of
fluid_midi_file_read_track. There is a call to fluid_midi_file_eof (in
the unpatched code, a call to feof), which errors out in the EOF case.
If it were merely checking for buf_ptr = buf_end, it would raise an
error for the last track in the file, because the last byte of the
file has been read at that point. We need it to only be an error if
the file caused any operation to read too far past the end.
This is summarised in a comment in fluid_midi_file_eof:
/* Note: This does not simply test whether the file read pointer is past
* the end of the file. It mimics the behaviour of feof by actually
* testing the stateful EOF condition, which is set to TRUE if getc or
* fread have attempted to read past the end (but not if they have
* precisely reached the end), but reset to FALSE upon a successful seek.
*/
Second, I would probably have done int buf_length and int buf_pos (as
indices to the buffer array) instead of buf_end and buf_ptr,
but I guess that's mostly a matter of taste. Nothing to reject a patch for.
True. I agree with this. It makes it clear that there is only one
buffer, not three, and briefly looking at the code, would probably
simplify it in several places. I will make this change at the same
time as I do the memory allocation change.
Matt
___
fluid-dev mailing list
fluid-dev@nongnu.org
http://lists.nongnu.org/mailman/listinfo/fluid-dev
Re: [fluid-dev] Propose changes for drum channels support
Hi Jimmy,
I am not a FluidSynth developer, just an interested person, so my opinions
don't represent the view of the FluidSynth project.
This seems like a valuable generalisation of a previously hard-coded value.
Given that your new flag will default to 0 for all channels and 1 for
channel 9, it won't change anything but add a useful new feature.
A few points: Firstly, in the future could you attach your patches as
attachments instead of pasting them at the end. It makes them much easier to
read and apply.
Add a flag to struct:
_fluid_channel_t
to indicate it is a drum channel, using int type, with value:
0: melodic channel
1: drum channel
Since this is a boolean (the name is is_drum_channel), your code should
use the constants FALSE and TRUE instead of 0 and 1 (existing FluidSynth
code uses these constants, but uses the type 'int' instead of 'bool').
Might it be better to generalise to an enum?
enum fluid_channel_type
{
CHANNEL_TYPE_MELODIC,
CHANNEL_TYPE_DRUM
};
And call the field fluid_channel_type channel_type.
It's questionable whether there would ever be more channel types in the
future other than melodic and drum, but at least it would make code clearer
to see, for instance, chan-is_drum_channel = (9 == num) ?
CHANNEL_TYPE_DRUM : CHANNEL_TYPE_MELODIC; instead of chan-is_drum_channel
= (9 == num) ? 1 : 0;. Can anybody think of any reason why, in the future,
there would be more channel types than melodic or drum?
+
+ /* Drum channel flag, 0 for melodic channel, 1 for drum channel.
+ * Previously, even for 32, 48, 64 channels only channel 9 can be drum,
+ * channel 25, 41, 57 were not treated as drum channel at all, even
though
+ * ALSA shows 2, 3, 4 ports (each with 16 midi channels).
+ * We can optionally initialize channel 25, 41, 57 be GM drum channel if
we
+ * want for each of those 16 midi channels -- need to change channel
+ * initialization, but may break existing apps unless they explicitly
set
+ * these channels.
+ * Moreover, GM2 allows 2 drum channels 10 and 11 (9, 10 with
zero-indexing).
+ * Some older XG may use drum channels 9 and 10 (8, 9 with
zero-indexing).
+ * Added at end of structure, hopefully existing apps using this
structure may
+ * still work without having to recompile for the time being. */
+ int is_drum_channel;
+
This comment reads more like a commit log than a comment -- the changes you
have made and the rationale for them (and discussion of breakages) don't
belong in comments in the code itself. I would delete all but the first line
of this comment.
As for the issues raised in there, I think we should definitely only set
channel 9 to drum, for backwards compatibility.
- *
- * FIXME - Shouldn't hard code bank selection for channel 10. I think
this
- * is a hack for MIDI files that do bank changes in GM mode. Proper way
to
- * handle this would probably be to ignore bank changes when in GM mode.
- JG
*/
I don't think this FIXME has been fully resolved. I don't quite understand
it, but it says that the proper way to handle it would be to ignore bank
changes, which isn't what your patch is doing. Can you explain what this
comment means and why it's OK to remove it?
Good work on that patch.
Matt
___
fluid-dev mailing list
fluid-dev@nongnu.org
http://lists.nongnu.org/mailman/listinfo/fluid-dev
