Skip to content

Handle non-contiguous input in process() - #5

Draft
sanerdemirel wants to merge 1 commit into
gregogiudici:mainfrom
sanerdemirel:fix/noncontiguous-process-input
Draft

Handle non-contiguous input in process()#5
sanerdemirel wants to merge 1 commit into
gregogiudici:mainfrom
sanerdemirel:fix/noncontiguous-process-input

Conversation

@sanerdemirel

Copy link
Copy Markdown

Fixes #3.

The problem

process() copied each channel's samples with std::copy under the assumption
that the input buffer was C-contiguous:

std::copy(inData + i*inputLength, inData + (i+1)*inputLength, inputChannels[i]);

A transposed array — which is what librosa.load(..., mono=False) commonly
hands you — is a view with the channel and sample strides swapped relative to a
freshly allocated one. Copying through it as if it were still contiguous reads
the wrong samples, so the output is silently corrupted rather than failing.

The change

process() now reads each sample through the array's own strides instead of
assuming a layout. The input was already being copied into internally-owned
buffers before processing, contiguous or not, so this costs nothing extra for
the contiguous case — it just also gives the right answer for the
non-contiguous one.

While in there, process() also accepts 1-D (mono) input by promoting it to a
single channel instead of raising TypeError, matching the layout handling
proposed on the issue.

How I checked it

  • Contiguous input is bit-identical to before, across 6 configurations
    (mono and stereo, several time factors).
  • With a positive control, because "bit-identical" is worthless if the
    comparison could not have detected a difference in the first place: I
    confirmed the harness could tell the old and new builds apart when a
    difference was deliberately introduced, before trusting the runs where it
    reported none.
  • Non-contiguous input now matches contiguous input for the same audio.

tests/test_noncontiguous_input.py covers the transposed-input case in both
mono and stereo.

Note

Opened as a draft. Happy to adjust anything about the approach or the tests.

process() copied each channel's samples with std::copy under the
assumption that the input buffer was C-contiguous. A transposed array,
which is what librosa.load(..., mono=False) commonly returns, has its
channel and sample strides swapped relative to a contiguous array, so
that assumption produced silently corrupted output (gregogiudici#3).

process() now reads through the array's own strides instead of
assuming a fixed layout. The buffer it reads from is always copied
into internally-owned memory before processing regardless of layout,
so this costs nothing extra for the already-contiguous case; it just
also gives the right answer for the non-contiguous one.

While in there, process() now also accepts a 1-D (mono) input by
promoting it to a single channel, instead of raising a TypeError,
matching the layout handling proposed on the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-contiguous memory layout in stereo input causes corrupted output

1 participant