For some audio buffers, I get a RangeError.

unetInfo commented 8 months ago

Expected Behavior

Error-free sample rate conversion of an audiobuffer.

Current Behavior

Uncaught RangeError: offset is out of bounds

Steps to Reproduce

This only happens on certain audio buffers, with certain conversion requests. I'm not sure how to make it reproducible for you without sharing a big .wav file. If it's helpful, I can provide you with a link to my web app and instructions for how to reproduce the problem there.

Possible Solution

If I change:

        return Math.ceil(O.length * this.ratio) > r ? this._chunkAndResample(O) : (this.sourceArray.set(O),

to

        return Math.ceil(O.length * this.ratio) > r ? this._chunkAndResample(O) : (this.sourceArray.set(O.slice(0, r)),

the error isn't raised, but I'm not convinced that's the right fix. I just took a stab :-).

Context

Straightforward sr conversion

no

Your Environment

Operating System and version : Mac OS 14.2.1 / Chrome Version 120.0.6099.234 (Official Build) (arm64)
Link to your project : https://unet.run:8443/apps/golden

aolsenjazz commented 8 months ago

Thanks for submitting. I see you shared a link to a project - is this the correct one?

how big is the .wav file you’re trying to convert? Are you using the full or simple API?

if it’s an issue that only arises with large files, that gives a great hint as to what might be going on. Lmk!

btw - was unable to successfully load the page for the link you provided on mobile. Will try on computer when I get a chance

aolsenjazz commented 8 months ago

Oh - and please do provide steps to reproduce. Thanks again!

unetInfo commented 8 months ago

Greetings!

Thanks for submitting. I see you shared a link to a project - is this the correct one?

how big is the .wav file you’re trying to convert? Are you using the full or simple API?

The .wav file is 7.8 mb. I’m using the simple API. I’m definitely able to convert much bigger files without a problem.

Yes, I’m afraid that app isnt very mobile friendly. Using Chrome on a Desktop, you should be able to go to:

https://unet.run:8443/apps/golden/ Golden v 0.40 Jan 2024 unet.run

It can take a LONG time to load depending on your bandwidth.

I’ve saved a preset that will reproduce the crash for you (I’ve undone my hacky “slice” fix in libsamplerate.js for now).

Look on the panel on the top right — near the bottom is a select menu that says “mise”. If you click on that, at the bottom you’ll see a preset called “libSampleRateCrash”. If you choose that, it will try to load the .wav in question and you’ll see the error in the console.

Thanks for your patience, let me know if I can document this in a more useful way.

Best,

Andy

On Jan 21, 2024, at 8:44 AM, aolsenjazz @.***> wrote:

Oh - and please do provide steps to reproduce. Thanks again!

— Reply to this email directly, view it on GitHub https://github.com/aolsenjazz/libsamplerate-js/issues/140#issuecomment-1902691401, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHDBV3F6HDY4FVSFO7EUDBTYPVAWPAVCNFSM6AAAAABCDUNWVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSGY4TCNBQGE. You are receiving this because you authored the thread.

aolsenjazz commented 8 months ago

After review some of the code there, it looks like the problem may be with anticipating the size of the output. It looks like this block in the JS:

const projectedSize = Math.ceil(dataIn.length * this.ratio);
if (projectedSize > BUFFER_LENGTH) return this._chunkAndResample(dataIn);

this.sourceArray.set(dataIn);

is getting API selection wrong; because your piece of audio is 7.2mb (over the 4mb threshold), it should use the full API, but maybe because of the predicted resampled size, it's selecting the simple API instead. That would happen if you're downsampling and just above the 4mb threshold.

Can you confirm source and target sample rates? If it's something like 96k to 48k we should have our culprit.

Will have some downtime while travelling tomorrow and be able to properly look into this.

unetInfo commented 8 months ago

That makes sense — in the error-producing example, the source and target sample rates are 44100 and 28665. I’m (ab)using the library to slow down and speed up audio buffers in various musical contexts, so I’ll often end up with unusual ratios like this. Sorry if I’m an edge case farm :-).

I’ve just spotted something in my code that may be incorrect. I am reusing the converter instance. Is this okay?

Because my input stereo audio buffer is in a nested array (as opposed to interleaved samples) I figured I should just process each channel separately:

      var converterType = LibSampleRate.ConverterType.SRC_SINC_BEST_QUALITY;
      var nChannels = this.player.original_buffer.numberOfChannels;

      var inputSampleRate = this.player.buffer.sampleRate; // Use the original sample rate
      var outputSampleRate = newSR;

      let audioDataRight, audioDataLeft = this.player.original_buffer.getChannelData(0); // Get left channel audio data
      if (nChannels > 1)
        audioDataRight = this.player.original_buffer.getChannelData(1); // Get right channel audio data

      try {
        // Resample each channel separately
        LibSampleRate.create(1, inputSampleRate, outputSampleRate, {
          converterType: converterType,
        }).then((src) => {
          let resampledDataLeft = src.simple(audioDataLeft);
          let resampledDataRight = nChannels > 1 ? src.simple(audioDataRight) : resampledDataLeft;
          src.destroy();

On Jan 21, 2024, at 1:49 PM, aolsenjazz @.***> wrote:

After review some of the code there, it looks like the problem may be with anticipating the size of the output. It looks like this block in the JS:

const projectedSize = Math.ceil(dataIn.length * this.ratio); if (projectedSize > BUFFER_LENGTH) return this._chunkAndResample(dataIn);

this.sourceArray.set(dataIn); is getting API selection wrong; because your piece of audio is 7.2mb (over the 4mb threshold), it should use the full API, but maybe because of the predicted resampled size, it's selecting the simple API instead. That would happen if you're downsampling and just above the 4mb threshold.

Can you confirm source and target sample rates? If it's something like 96k to 48k we should have our culprit.

Will have some downtime while travelling tomorrow and be able to properly look into this.

— Reply to this email directly, view it on GitHub https://github.com/aolsenjazz/libsamplerate-js/issues/140#issuecomment-1902777528, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHDBV3HN55VGVAQB3RCA3VLYPWEPXAVCNFSM6AAAAABCDUNWVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSG43TONJSHA. You are receiving this because you authored the thread.

aolsenjazz commented 8 months ago

Edge cases are good! Glad you raised this. This will also be a super easy fix moving forward.

With the simple API, chunks <4mb there's no risk.

With converting streams using the full API (as you likely know, invoking simple API with big streams tacitly invokes full), there is a bit of a risk. IIRC, when SRC calculates new chunks of resampled audio with the full API, it uses data from the previously-resampled chunk to determine number of samples created and values of samples create, e.g. converting from one sample rate to another would generate a fractional value for a sample at the beginning or end of the array. That fractional sample is used to inform the value of the nextResampledChunk[0]. Does that make sense?

In reality, this is probably an unnoticeable change. Hard for me to say with certainty though - I would advise using channels as intended to be safe.

aolsenjazz commented 8 months ago

Fixed in b01693d8f7aad8155bb0ba36c1c938fabacfe9e3

Would you mind cloning main and testing on your end as well before I push new version to NPM?

unetInfo commented 8 months ago

Fixed! I can no longer reproduce that problem, and I’ve just run a giant set of random tests without an issue.

Thanks for your patience and quick response — sounding SO GOOD again.

On Jan 22, 2024, at 7:45 AM, aolsenjazz @.***> wrote:

Fixed in b01693d https://github.com/aolsenjazz/libsamplerate-js/commit/b01693d8f7aad8155bb0ba36c1c938fabacfe9e3 Would you mind cloning and testing on your end as well before I push new version to NPM?

— Reply to this email directly, view it on GitHub https://github.com/aolsenjazz/libsamplerate-js/issues/140#issuecomment-1904280482, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHDBV3DWLXFGPBG2MSC5OJ3YP2CQ5AVCNFSM6AAAAABCDUNWVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBUGI4DANBYGI. You are receiving this because you authored the thread.

aolsenjazz commented 8 months ago

Awesome! Glad to help. v2.1.1 published to NPM.

aolsenjazz / libsamplerate-js