nfroidure / midifile

A MIDI file parser/writer using ArrayBuffers
http://karaoke.insertafter.com
MIT License
199 stars 30 forks source link

Merge improvements (pitch bend, `parseSong()`, ...) from `webaudiofont` #34

Open page200 opened 1 year ago

page200 commented 1 year ago

There are various improvements in a copy of MIDIFile.js in https://github.com/surikov/webaudiofont/blame/master/examples/MIDIFile.js such as support for pitch bends (slides).

It might be good to merge those improvements into this repo, and replace MIDIFile.js in that repo by a link to this repo, as discussed at https://github.com/nfroidure/midifile/issues/33#issuecomment-1655372411

mk-pmb commented 1 year ago

Unfortunately they seem to use a stricter license (GNU GPL v3) than we do (MIT), so we cannot take code from their repo. We'd have to track down the relevant patch author(s) and request explicit permission from them.

Most of the code lines that have the word "slide" seem to come from this commit by user @surikov but I cannot find a PR for that. Also the commit title "test" looks like the patch might not be a single feature, so we'd need to figure out whether we need other patches as well to support slides.

If your patch from https://github.com/surikov/webaudiofont/pull/95 works without slides, feel free to submit it as PR here under MIT license. (Assuming you do have all required copyrights, of course.)

page200 commented 1 year ago

@surikov's patches introduce pitch bend etc., and my patch https://github.com/surikov/webaudiofont/pull/95 adds support for MIDI files with custom pitch-bend range, so that the pitch bend is correct for them.

What form of permission is needed, as comments on GitHub? You have my permission, and I think @surikov also suggests here that @nfroidure's repos should be modified accordingly (and that @surikov's patches to MIDIFile.js aren't the focus of his repo anyway, but are merely a small example).

mk-pmb commented 1 year ago

What form of permission is needed, as comments on GitHub?

Obligatory disclaimer: I'm not a lawyer, and the following is just how I as a layman understand (US) copyright and (European) authorship protection.

We need an MIT license grant. A comment stating the grant would work in theory, but we'd have to keep that around somehow. The easier way is to submit a PR for a branch that is

The 2nd condition acts as implied license grant under the existing license. It's only effective if you're legally capable of granting the license, but this is most usually the case for your own creations.

mk-pmb commented 1 year ago

I checked https://github.com/surikov/webaudiofont/pull/95 and it looks like the patch there isn't directly compatible, even if I adjust the file paths. For example, it tries to modify a function addSlide but that code seems to not exist on our master branch. My quick attempt to trace where addSlide comes from, failed. I might try again later.

Edit: If with

surikov's patches introduce pitch bend etc.,

you mean he is the author of the patches for those features, we'd have to ask him to please PR his changes into our repo. However, they may require even more patches from other authors, can't currently check that.

page200 commented 1 year ago

My patch fixes the pitch-bend range. @nfroidure's repos don't have pitch bend yet (as far as I know / in the form we're discussing). @surikov's repo does.

Here's the blame history for addSlide: https://github.com/surikov/webaudiofont/blame/8d3bbebcb1b3fa5642b931bf1eff9e7740795c51/examples/MIDIFile.js#L1009-L1025

The contributors (@surikov and me) are listed on the left side of the code and on the top right.

The code was added in subsequent commits after "forking" the original MIDIFile.js.

mk-pmb commented 1 year ago

Thanks! So it seems the other code all comes from the "test" commit. Back then, the repo seems to have been under MIT license still, so we might be in luck. I've started negotiations.

Once we have proper permissions on that code, we'll have to figure out stuff like what this magic number means:

if (track.notes[i].duration == 0.0000001

and what conparison operator to use instead, probably <= or ===.

Edit: Also there is a suspicious change from binary | to boolean || near the /127 division.

mk-pmb commented 11 months ago

The issue where I asked how to credit him was closed without comment, so I guess he doesn't care. My approach would be to use the "From:" header of the "test" commit as a patch file as the author attribution. Would you other maintainers agree?

page200 commented 8 months ago

Yes, that sounds like an appropriate author attribution. And feel free to attribute me for the pitch-bend range functionality, if that makes things easier. I welcome a license that benefits the project and its widespread use. Thanks!

mk-pmb commented 8 months ago

And feel free to attribute me for the pitch-bend range functionality,

Are you refering to parts of the "test" commit? Are you the real author of part of that? If so, can you PR those parts first?

Edit: I see now that you probably meant the git-blame you linked above:

Here's the blame history for addSlide: […]

I'll take that into account.

page200 commented 8 months ago

I meant https://github.com/surikov/webaudiofont/pull/95.

I'll take that into account.

Whatever benefits a widespread use of the project.

Thanks!