danigb / smplr

A web audio sampler instrument
https://danigb.github.io/smplr/
186 stars 19 forks source link

Support for using standarized-audio-context #50

Closed Seanitzel closed 1 year ago

Seanitzel commented 1 year ago

Hello,

Thanks for this awesome library! I have a pretty large project that also uses ToneJS(mainly for the transport control), but it seems that it's immpossible to use them both together.

The reason seems to be stemming from the fact that Tone uses Standarized Audio Context, and your project uses native audio nodes by default.

The reason i'm creating this issue here is to maybe consider finding a way of supporting using standarized audio context as well with this library, I think it could really benefit from it.

Relevant issue in ToneJS

danigb commented 1 year ago

Yes, I'd love to add support to it. Do you know what is the specific issue? Have you tried to create a standardized audio context and pass it to the smplr constructor?

Seanitzel commented 1 year ago

Yay! So what i've tried is passing the audio context that was created using sac(standarized-audio-context), using both methods:

const piano = new SplendidGrandPiano(getContext().rawContext as AudioContext, { decayTime: 0.5 });
const marimba = new Soundfont(getContext().rawContext as AudioContext, { instrument: 'marimba' });

Getting the following error for marimba:

 TypeError: Failed to construct 'GainNode': parameter 1 is not of type 'BaseAudioContext'.

And for the piano, it actually throws no errors - but when I call load and try to play a note:

 piano.load.then(() => {
  piano.start({ note: 'C4', velocity: 80, time: 0, duration: 1 });
});

I get the following:

 TypeError: Cannot set properties of undefined (setting 'value')

I'm pretty sure both of these are caused by the issue with the audio context, after spending 2 full days trying to find the best way to play soundfonts inside my project...

I've also looked and tried soundfont-player and spessasynth, but with both I encounter the exact same issue.

I should also add that it works immediately and perfectly ifi I send a new audio context and not the one from sac... That's why i'm sure that's what causing the issue lol

danigb commented 1 year ago

Not thoroughly tested, but seems to work: https://github.com/danigb/smplr#usage-with-standardized-audio-context

Seanitzel commented 1 year ago

Hahaha I can't believe, I spent all day trying to make it work myself as well, and came to this.

Just finally made it work, was about to update and then I see your commit with the similar changes... I fried my brain and actually also learned a lot of stuff.

Also, I must say, beautiful codebase :)

One thing that may cause issues is the detune property of BufferSource, it is currently not implemented in sac.

My commit actually adds sac to the library, but I guess it's better to leave it to the consumer whether he wants to use it or not for his project rather than force the extra bundle size.

You are awesome, thanks for the super quick responses and updates, really appreciate it 😄 🙏🏻

P.S Small API change suggestion for the future v 1.0 - making .load a method rather than a property, as it returns a Promise. IMO it makes more sense, but it doesn't really matter too much and it's mostly a matter of taste 😆

danigb commented 1 year ago

Hehehe... yes, my first attempt was very similar to your changes. But I didn't like so many changes for something that should be more or less compatible, so I ended with the minimal set of changes that could work... Anyway, thanks for your PR attempt and glad it worth the effort even if it's not finally merged.

BTW, for the detune property I made a fallback to a playbackRate (see https://github.com/danigb/smplr/pull/52/files#diff-2218ebcdd9ad212d2d9639bce8879879ebc04fb6c9121955e9ee0d340f7aaff2R60) so it should work.

Also, at the very beginning load was a function, but I think it was confusing because people may think that you need to call it to load the instrument, which is not true. Anyway, open to suggestions.

Seanitzel commented 1 year ago

Yea, your commit looks great.

And about the detune - I missed it previously, that's perfect!

About load - I see what you mean, and actually agree, can't think of a better alternative...

I actually have a few more unrelated questions if it's ok -

  1. How can I dispose of an instrument and free it's resources?
  2. Currently it fetches all samples from github, which is not suitable for a production app since there are rate limits. I can see that for SoundFont we can provide an instrument URL, but is there anything similar to do in case I wanna use SplendidGrandPiano?
  3. For SoundFont - I don't see the option to load a custom sf2 file, are there plans to add that option?
  4. From what I understand(correct me if i'm wrong) to connect an instrument to an output I need to use the addInsert method on the output property? that doesn't seem very intuitive 🤔 I think maybe adding a simple connect method on the instrument API or the output can be good
danigb commented 1 year ago

Hi, some quick answers:

  1. There's a disconnect method, but is not documented. I'll add it to my tasklist
  2. All instruments should accept a url. If not, please raise an issue.
  3. The only option is to upload converted soundfont files. sf2 not planned (not sure even if possible: I don't know enough details about that format)
  4. The way to connect an instrument to something is passing a destination option in the constructor. Probably documentation can be better.

Feel free to open issues if you find something incorrect or PR if you want to improve something :-)

Seanitzel commented 1 year ago

Cool, thanks so much for taking the time to answer everything, really appreciate it! :)