Re: [fluid-dev] Making MIDI player read from a buffer

[Matt Giuca]

Hi David,

In response to your post here:
http://lists.nongnu.org/archive/html/fluid-dev/2011-01/msg00003.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