MIDI file decoder #1004
MIDI file decoder #1004
Conversation
|
Nice! With a dog I hope!! :-) :-) I'm busy on the changes you've suggested but they're going to take a little while, so happy moving .. |
No dogs yet :) but we have two cats!
Thanks and no stress 👍 |
- fixed remaining snake-cased event names in tests and examples (cf. wader#1004 (comment)) - report 'status' and 'event' seperately (cf. wader#1004 (comment)) - decoded TimeSignature metaevent as length + byte fields (cf. wader#1004 (comment)) - decoded SequencerSpecificEvent metaevent as length + byte fields (cf. wader#1004 (comment)) - reworked Errorf's as Fatalf's for data reads (cf. wader#1004 (comment))
…(cf. wader#1004 (comment)) - restructured and simplified metaevent decoding - restructured and simplified MIDI event decoding - added sample events for remaining metaevents - moved MIDI test and sample files to testdata/midi - fixed fuzzing fail 8450de010e750ed5
- delineated NoteOn and NoteOff events - simplified MIDI event decoding with function map - decoded MIDI event and channel as nibbles - reworked system exclusive event decoding - fixed peekEvent to correctly decode end of file events
- reduced format 0, format 1 and format 2 test files to basic MIDI skeleton - reworked reference.mid file as a test/development sample only - added fq tests for the single event MIDI files - updated examples to use twinkle.mid
|
<smile> for when the cats have forgiven you for moving them! I've restructured the decoding along the lines you suggested and TBH am a lot happier with it - the code is a lot cleaner and the output makes more sense. But .. see what you think :-). |
One of them recovered fast but the other one is still a bit grumpy, he also has some teeths removed some days ago so that did not help :)
Yeap looks better i think 👍 i've been in similar situations with decodes, overthinking things a bit and using value/sythenetic fields a bit too much, good to step back and try be more "basic" maybe use more structs and/or split into a bit more. I guess a good guideline is always use normal fields if there is at least some sense of "these bits are involved to produce the value" and only value fields for derived or "extra" things that don't correspond to anything in the source bits. Will have a deeper look later in the week |
format/midi/midi.go
Outdated
| // ... decode tracks | ||
| d.FieldArray("tracks", func(d *decode.D) { | ||
| for d.BitsLeft() > 0 { | ||
| if err := skipTo(d, "MTrk"); err != nil { |
There was a problem hiding this comment.
So a midi files can have "garbage" or unknown things between tracks?
There was a problem hiding this comment.
There is d.(Try)PeekFind function that can be used to peek in various ways, maybe can be used?
There was a problem hiding this comment.
Not garbage no - it's a little bit ambiguous but at the very least it has to be a valid chunk with a 4 character tag and a length. The theory being that it would allow extensions and customisations without breaking existing MIDI file decoders.
But .. I've only ever seen this referenced once and can't remember having ever come across a MIDI file that had anything other than MThd and MTrk chunks, so I've removed it. If anybody ever raises an issue and can supply a MIDI file with other chunks it's easy enough to put back.
Removed in 9f057b6.
There was a problem hiding this comment.
I see, could it make sense to have a case that decodes unknown tags as just tag+data?
There was a problem hiding this comment.
Ok, just to make sure I dug out the spec and this is what it says:
MIDI Files are made up of chunks. Each chunk has a 4-character type and a 32-bit length,
which is the number of bytes in the chunk. This structure allows future chunk types to be
designed which may easily be ignored if encountered by a program written before the chunk
type is introduced. Your programs should expect alien chunks and treat them as if they
weren't there.
So tag + data makes sense - it's not a string so I've also included the length:
| | | tracks[0:5]: 0xe-0x56 (72)
| | | [0]{}: other 0xe-0x1e (16)
0x00| 4d 54| MT| tag: "MTrx" 0xe-0x12 (4)
0x10|72 78 |rx |
0x10| 00 00 00 08 | .... | length: 8 0x12-0x16 (4)
0x10| 01 23 45 67 89 ab cd ef | .#Eg.... | data: raw bits 0x16-0x1e (8)
| | | [1]{}: track 0x1e-0x2a (12)
0x10| 4d 54| MT| tag: "MTrk" 0x1e-0x22 (4)
Not sure that it's really correct to call the section "tracks" any longer though - what do you think?
There was a problem hiding this comment.
Went with content in the end - as in header + content. It seemed to make the most sense :-)
Fixed in 9c7f7f9.
- Removed handling for non-MIDI chunks (cf. wader#1004 (comment)) - Removed redundant chunk length asserts (cf. wader#1004 (comment)) - Removed redundant header FieldArray (cf. wader#1004 (comment)) - Replaced FieldUintFn with FieldUnn for SMTPEOffsetEvent (cf. wader#1004 (comment)) - Added note to midi.md re. supported chunks
- Reworked event types as snake-case (cf. wader#1004 (comment)) - Decoded 'alien' chunks (cf. wader#1004 (comment))
|
Quick question - I'm considering removing any remaining checks that aren't actually catastrophic and deferring them to something like a --validate option (to be implemented later), e.g.: Any thoughts? |
Yeah i think that is better. The original idea was to keep decoder code free from as much explicit error checking as possible to make them be very straight forward. But think i forgot a bit about making the common error reporting nicer, i think it's a bit of a mess atm, feel free to look into :) i remember i spent some time on trying to improve on preserving "partial" decode trees on errors but it gets a bit tricky to combine with panic/recover control flow. BTW here is a overflow of how requre/asser/validate works https://github.com/wader/fq/blob/master/pkg/decode/decode_gen.go.tmpl#L89-L123 (note fq was started pre generics :) i have some ideas how to make the decode API nicer by using generic but is quite a bit of effort, we'll see!) |
|
I think it's starting to look ready for merge, something you want to add? (except possible rename to chunks) |
- Renamed 'tracks' section to 'content' (cf. wader#1004 (comment)) - Removed 'validation only' errors (cf. wader#1004 (comment)) - Decoded MThd divisions field with SMPTE timecode
|
Agreed, it makes much more sense not to fail because at least one use case for this thing is finding and fixing errors in files:
re. merge. I've run it on a folder of about 100 files and it seems fine and even found one file with an unsuspected error, and fuzzing it for about an hour turned up nothing (whew!) and I can't think of anything else I want to add/change, so unless you find anything it's probably good to go ... but maybe let me give it another once through just in case? |
|
Sounds good, just let me know when you think it's ready. I might spend some time reviewing if i get some time. Also remember that we can always do additional PR:s later on. |
|
And that's it from me I think - I tightened up the status/event decoding logic which was the thing that was subconsciously nagging at me. Anything else is largely cosmetic and I'm sure your cats would like to have your attention :-). |
|
👍 Let's merge! thanks and of course feel tree to open more PRs! |
|
Woohoo :-) ... thanks for all the reviews and comments! A second set of eyes makes such a difference .. |
|
No problem, always nice to help and learn a bit about a new format. Also it's a weird satisfying feeling seeing what every bit in a file is used for 😄 |
Hi,
First draft of a MIDI file decoder for your comments and suggestions - I've decoded the file in the way that seemed most useful to me but there are a couple things I'm a little undecided about so it seemed like a good time to get some feedback :-).