From 0a9435c034b725be5f5ddc0c5870946141832005 Mon Sep 17 00:00:00 2001 From: "Tim E. Real" Date: Mon, 24 Oct 2011 20:11:20 +0000 Subject: Fixed bug #3293339: Midi file export SMF format 0 broken. Please see ChangeLog. --- muse2/ChangeLog | 8 + muse2/muse/exportmidi.cpp | 100 ++++--- muse2/muse/midifile.cpp | 33 ++- muse2/muse/widgets/configmidifilebase.ui | 494 ++++++++++++++++--------------- 4 files changed, 364 insertions(+), 271 deletions(-) diff --git a/muse2/ChangeLog b/muse2/ChangeLog index bf5172f9..5d0ae536 100644 --- a/muse2/ChangeLog +++ b/muse2/ChangeLog @@ -1,3 +1,11 @@ +24.10.2010: + - Fixed bug #3293339: Midi file export SMF format 0 broken. (Tim) + Removed erroneous extra 2 bytes. Allow metas in SMF0 in MidiFile::writeEvent(). + Fixed very weird timing issue MidiFile::write(). See comments there (hidden problems lurking still?). + Use single MidiFileTrack for SMF0 in MusE::exportMidi(). Added info to export dialog indicating that + SMF 0 grabs the track name and comment from first midi track in arranger. Iterate full tracks list + instead of midis list in MusE::exportMidi(), so that user can rearrange tracks on the fly before exporting. + Some not fully understood useage of meta 0x0F/0x01 text events in MusE::exportMidi(). Hopefully correct... 22.10.2010: - Catch return in spin boxes and move focus away (rj) 20.10.2011: diff --git a/muse2/muse/exportmidi.cpp b/muse2/muse/exportmidi.cpp index b892e808..97029d9d 100644 --- a/muse2/muse/exportmidi.cpp +++ b/muse2/muse/exportmidi.cpp @@ -149,15 +149,28 @@ void MusE::exportMidi() return; MusECore::MidiFile mf(fp); - MusECore::MidiTrackList* tl = MusEGlobal::song->midis(); - int ntracks = tl->size(); + //MusECore::MidiTrackList* tl = MusEGlobal::song->midis(); + MusECore::TrackList* tl = MusEGlobal::song->tracks(); // Changed to full track list so user can rearrange tracks. + //int ntracks = tl->size(); MusECore::MidiFileTrackList* mtl = new MusECore::MidiFileTrackList; int i = 0; - for (MusECore::iMidiTrack im = tl->begin(); im != tl->end(); ++im, ++i) { - MusECore::MidiTrack* track = *im; - MusECore::MidiFileTrack* mft = new MusECore::MidiFileTrack; - mtl->push_back(mft); + MusECore::MidiFileTrack* mft = 0; + //for (MusECore::iMidiTrack im = tl->begin(); im != tl->end(); ++im, ++i) { + for (MusECore::ciTrack im = tl->begin(); im != tl->end(); ++im) { + + if(!(*im)->isMidiTrack()) + continue; + + MusECore::MidiTrack* track = (MusECore::MidiTrack*)(*im); + + //MusECore::MidiFileTrack* mft = new MusECore::MidiFileTrack; + if (i == 0 || (i != 0 && MusEGlobal::config.smfFormat != 0)) // Changed to single track. Tim + { + mft = new MusECore::MidiFileTrack; + mtl->push_back(mft); + } + MusECore::MPEventList* l = &(mft->events); int port = track->outPort(); int channel = track->outChannel(); @@ -200,14 +213,17 @@ void MusE::exportMidi() //--------------------------------------------------- // Write Coment // - QString comment = track->comment(); - if (!comment.isEmpty()) { - int len = comment.length(); - MusECore::MidiPlayEvent ev(0, port, MusECore::ME_META, (const unsigned char*)(comment.toLatin1().constData()), len); - ev.setA(0x1); - l->add(ev); - } - + if (MusEGlobal::config.smfFormat == 0) // Only for smf 0 added by Tim. FIXME: Is this correct? See below. + { + QString comment = track->comment(); + if (!comment.isEmpty()) { + int len = comment.length(); + MusECore::MidiPlayEvent ev(0, port, MusECore::ME_META, (const unsigned char*)(comment.toLatin1().constData()), len); + ev.setA(0x1); + l->add(ev); + } + } + //--------------------------------------------------- // Write Songtype SYSEX: GM/GS/XG // @@ -287,27 +303,38 @@ void MusE::exportMidi() // track name //----------------------------------- - if (!track->name().isEmpty()) { - QByteArray ba = track->name().toLatin1(); - const char* name = ba.constData(); - int len = strlen(name); - MusECore::MidiPlayEvent ev(0, port, MusECore::ME_META, (unsigned char*)name, len+1); - ev.setA(0x3); // Meta Sequence/Track Name - l->add(ev); - } - + if (i == 0 || (i != 0 && MusEGlobal::config.smfFormat != 0)) + { + if (!track->name().isEmpty()) { + QByteArray ba = track->name().toLatin1(); + const char* name = ba.constData(); + int len = strlen(name); + MusECore::MidiPlayEvent ev(0, port, MusECore::ME_META, (unsigned char*)name, len+1); + ev.setA(0x3); // Meta Sequence/Track Name + l->add(ev); + } + } + //----------------------------------- // track comment //----------------------------------- - if (!track->comment().isEmpty()) { - QByteArray ba = track->comment().toLatin1(); - const char* comment = ba.constData(); - int len = strlen(comment); - MusECore::MidiPlayEvent ev(0, port, MusECore::ME_META, (unsigned char*)comment, len+1); - ev.setA(0xf); // Meta Text - l->add(ev); - } + // FIXME: What are these 0x0F? All I found was that they are unspecified part of the sixteen text meta events. + // So why not use 0x01? And do we include it for all tracks, or only above 1? Tim. + //if (i == 0 || (i != 0 && MusEGlobal::config.smfFormat != 0)) + //if (i != 0 && MusEGlobal::config.smfFormat != 0) + if (MusEGlobal::config.smfFormat != 0) + { + if (!track->comment().isEmpty()) { + QByteArray ba = track->comment().toLatin1(); + const char* comment = ba.constData(); + int len = strlen(comment); + MusECore::MidiPlayEvent ev(0, port, MusECore::ME_META, (unsigned char*)comment, len+1); + ev.setA(0xf); // Meta Text + l->add(ev); + } + } + MusECore::PartList* parts = track->parts(); for (MusECore::iPart p = parts->begin(); p != parts->end(); ++p) { MusECore::MidiPart* part = (MusECore::MidiPart*) (p->second); @@ -315,7 +342,6 @@ void MusE::exportMidi() for (MusECore::iEvent i = evlist->begin(); i != evlist->end(); ++i) { MusECore::Event ev = i->second; int tick = ev.tick() + part->tick(); - switch (ev.type()) { case MusECore::Note: { @@ -400,11 +426,19 @@ void MusE::exportMidi() } } } + ++i; + } mf.setDivision(MusEGlobal::config.midiDivision); mf.setMType(MusEGlobal::song->mtype()); - mf.setTrackList(mtl, ntracks); + //mf.setTrackList(mtl, ntracks); + mf.setTrackList(mtl, i); mf.write(); + + // TESTING: Cleanup. I did not valgrind this feature in last memleak fixes, but I suspect it leaked. + //for(MusECore::iMidiFileTrack imft = mtl->begin(); imft != mtl->end(); ++imft) + // delete *imft; + //delete mtl; } } // namespace MusEGui diff --git a/muse2/muse/midifile.cpp b/muse2/muse/midifile.cpp index a94644ff..2db2855d 100644 --- a/muse2/muse/midifile.cpp +++ b/muse2/muse/midifile.cpp @@ -568,9 +568,9 @@ void MidiFile::writeEvent(const MidiPlayEvent* event) int nstat = event->type(); // we dont save meta data into smf type 0 files: - - if (MusEGlobal::config.smfFormat == 0 && nstat == ME_META) - return; + // Oct 16, 2011: Apparently it is legal to do that. Part of fix for bug tracker 3293339. + //if (MusEGlobal::config.smfFormat == 0 && nstat == ME_META) + // return; nstat |= c; // @@ -621,24 +621,45 @@ bool MidiFile::write() writeLong(6); // header len writeShort(MusEGlobal::config.smfFormat); if (MusEGlobal::config.smfFormat == 0) { - writeShort(1); + /* + //writeShort(1); // Removed. Bug tracker 3293339 MidiFileTrack dst; for (iMidiFileTrack i = _tracks->begin(); i != _tracks->end(); ++i) { MPEventList* sl = &((*i)->events); for (iMPEvent ie = sl->begin(); ie != sl->end(); ++ie) + { + // ALERT: Observed a problem here, apparently some of the events are being added too fast. + // The dump below tells me some of the events (sysex/meta) are missing from the list! + // Apparently it's a timing problem. Very puzzling. + // Attempting wild-guess fix now to eliminate multiple MidiFileTracks in MusE::exportMidi()... + // Nope. Didn't help. Now that it's a single MidiFileTrack, try skipping this section altogether... + // Yes that appears to have fixed it. Weird. What's the difference - the local 'dst' variable ? + // Or are there still lurking problems, or something more fundamentally wrong with Event or MPEvent? + printf("MidiFile::write adding event to dst:\n"); // REMOVE Tim. + ie->dump(); // REMOVE Tim. dst.events.add(*ie); + } } writeShort(1); writeShort(_division); writeTrack(&dst); + */ + + writeShort(1); + //writeShort(_division); + //if(!_tracks->empty()) + // writeTrack(*(_tracks->begin())); + } else { + + writeShort(ntracks); - + } writeShort(_division); for (ciMidiFileTrack i = _tracks->begin(); i != _tracks->end(); ++i) writeTrack(*i); - } +/// } return (ferror(fp) != 0); } diff --git a/muse2/muse/widgets/configmidifilebase.ui b/muse2/muse/widgets/configmidifilebase.ui index 920596ec..ca64f2d8 100644 --- a/muse2/muse/widgets/configmidifilebase.ui +++ b/muse2/muse/widgets/configmidifilebase.ui @@ -1,238 +1,268 @@ - - - - - ConfigMidiFileBase - - - - 0 - 0 - 548 - 353 - - - - MusE: Config Midi File Import/Export - - - true - - - - - - - 5 - 1 - 0 - 0 - - - - Import: - - - - - - Split tracks into &parts - - - Alt+P - - - Split tracks into parts, or one single part - - - - - + + ConfigMidiFileBase + + + + 0 + 0 + 548 + 353 + + + + MusE: Config Midi File Import/Export + + + true + + + + + + + 0 + 0 + + + + Import: + + + + + + Split tracks into parts, or one single part + + + Split tracks into &parts + + + Alt+P + + - - - - - 5 - 7 - 0 - 0 - - - - Export: - - - - - - - - - - 96 - - - - - 192 - - - - - 384 - - - - - - - - true - - - Enable extended smf format (currently not implemented) - - - - - - - Use &2-byte time signatures instead of standard 4 - - - Alt+2 - - - - - - - Copyright: - - - false - - - - - - - Format: - - - false - - - - - - - Division: - - - false - - - - - - - Save space by replacing note-offs with &zero velocity note-ons - - - Alt+Z - - - - - - - - 0 (single track) - - - - - 1 (multiple tracks) - - - - - - + + + + + + + + 0 + 0 + + + + Export: + + + + - - - - 0 - - - 6 - - - - - - 20 - 20 - - - - QSizePolicy::Expanding - - - Qt::Horizontal - - - - - - - &OK - - - - - - true - - - true - - - - - - - &Cancel - - - - - - true - - - - + + + + + 96 + + + + + 192 + + + + + 384 + + + + + + + true + + + Enable extended smf format (currently not implemented) + + + + + + + Use &2-byte time signatures instead of standard 4 + + + Alt+2 + + + + + + + Copyright: + + + false + + + + + + + Format: + + + false + + + + + + + + 0 + 0 + + + + Note: Format 0 uses the FIRST midi track's name/comment in the arranger + + + false + + + + + + + Division: + + + false + + + + + + + Save space by replacing note-offs with &zero velocity note-ons + + + Alt+Z + + + + + + + + 0 (single track) + + + + + 1 (multiple tracks) + + + + + + + + + + + 6 + + + 0 + + + + + Qt::Horizontal + + + QSizePolicy::Expanding + + + + 20 + 20 + + + + + + + + &OK + + + + + + true + + + true + + + + + + + &Cancel + + + + + + true + + + - - - - - buttonOk - clicked() - ConfigMidiFileBase - accept() - - - buttonCancel - clicked() - ConfigMidiFileBase - reject() - - + + + + + + + + buttonOk + clicked() + ConfigMidiFileBase + accept() + + + 20 + 20 + + + 20 + 20 + + + + + buttonCancel + clicked() + ConfigMidiFileBase + reject() + + + 20 + 20 + + + 20 + 20 + + + + -- cgit v1.2.3