MIDI file decoder

[twystd]

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 :-).

[wader]

Hey, nice! had a quick look and generally good i think, will review in the evening! but i will approve so that the CI tests runs, maybe it finds something.

[twystd]

Thanks for the comments - think I've addressed all of them but take a look and see what you think.

The failing CI builds don't seem MIDI related but may have been related to the leftover debug stuff that was printing out a ">>" .. apologies for that :-(.

[wader]

CI errors now:

• test TestFormats/all/all.fqtest is not happy, i've thought about removing this test or redo it in a more maintance friendly way, but for now just add midi to the list
• Linter is complaining about some redundant conversion, see the review comment it added

[wader]

Just a heads up: i'm moving to a house so i might be a bit slow at replying the next few days... but knowing myself i will probably squeeze in some coding time anyway

[twystd]

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 ..

[wader]

Nice! With a dog I hope!! :-) :-)

No dogs yet :) but we have two cats!

I'm busy on the changes you've suggested but they're going to take a little while, so happy moving ..

Thanks and no stress 👍

[twystd]

<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 :-).

[wader]

for when the cats have forgiven you for moving them!

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 :)

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 :-).

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

[twystd]

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.:

...
    d.FramedFn(length*8, func(d *decode.D) {
        format := d.FieldU16("format")
        if format != 0 && format != 1 && format != 2 {
            d.Errorf("invalid MThd format %v (expected 0,1 or 2)", format)
        }
...

Any thoughts?

[wader]

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.:

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!)

[wader]

I think it's starting to look ready for merge, something you want to add? (except possible rename to chunks)

[twystd]

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:

• I've removed the validation errors
• And also tweaked the division field in the header to be decoded more correctly as a struct

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?

[wader]

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.

[twystd]

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 :-).

[wader]

👍 Let's merge! thanks and of course feel tree to open more PRs!

[twystd]

Woohoo :-) ... thanks for all the reviews and comments! A second set of eyes makes such a difference ..

[wader]

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 😄

[wader]

https://fosstodon.org/@wader/113102715574487149