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 _______________________________________________ fluid-dev mailing list fluid-dev@nongnu.org http://lists.nongnu.org/mailman/listinfo/fluid-dev