Fix ASR pipeline mono conversion for channels-last audio (fixes #47886) - #47888
Conversation
Rocketknight1
left a comment
There was a problem hiding this comment.
The idea seems okay, and I agree that silently averaging a dim of 48000 is very suboptimal, but I think we should be more rigorous to make sure we don't accidentally select other dimensions. In particular, inputs.ndim != 1 seems like it's written in cases where ndim might be 3 or larger, in which case the argmin might catch just about anything. A better solution might be simply to keep the existing code but throw an error if the mean() dim has a size other than 1/2.
| inputs = F.resample( | ||
| torch.from_numpy(inputs) if isinstance(inputs, np.ndarray) else inputs, | ||
| in_sampling_rate, | ||
| self.feature_extractor.sampling_rate, | ||
| ).numpy() |
There was a problem hiding this comment.
This fix runs after some steps like F.resample(). We might have to back up and do the fix earlier, or add a fix in that path too
| # on that layout averages across *time*, collapsing the waveform to one | ||
| # value per channel. Infer the channel axis instead: real audio has far | ||
| # more samples than channels, so the shorter axis is the channel axis. | ||
| channel_axis = int(np.argmin(inputs.shape)) |
There was a problem hiding this comment.
This feels extremely dangerous to me! If we want to be robust to either (samples, channels) or (channels, samples) then we should be much more explicit; and probably only do this for dims of 1 or 2 - if you have 5.1 audio then you can handle downmixing yourself 馃槄
|
Thanks, that's better than what I had. I pushed a version with the guessing taken out. If the array is 2-D, the first axis is treated as channels, so one or two channels get averaged down, and anything more raises an error that suggests transposing the array if it happens to be the other way round. Any other number of dimensions raises as well. I also moved the downmix so that it happens before the resample. It turns out resample wasn't the problem, since it handles either layout fine. The step that actually needed mono input was the stride calculation below it. The tests cover all of those cases. |
|
CI is red here but I don't think this PR caused it. The one failure is The commit history rules it out. The two commits carrying real code changes both passed. The red run is
Same shard, same 5,905 tests both times, and Happy to rebase if a re-run is easier that way. |
The pipeline averaged multi-channel input over a hardcoded axis 0, which assumes channels-first. soundfile.read, librosa.load(mono=False) and scipy.io.wavfile.read all return channels-last, so a stereo waveform was averaged across time and collapsed to one value per channel: 48000 samples became 2, were padded back to silence, and transcribed as " you" with only a warning saying the conversion had succeeded. Infer the channel axis from the shorter dimension instead, and report the actual shape rather than ndim, which always printed 2 for any 2-D input.
Per review: inferring the channel axis with argmin is unsafe. It picks an arbitrary axis for ndim >= 3, and it silently downmixes 5.1 audio where the weighting should be the caller's choice. Keep the documented (channels, samples) layout, which is what torchcodec returns, and raise when the input does not conform. A channels-last array gets an error naming the transpose, rather than a transcription of two samples. Also move the conversion ahead of F.resample. The resampler operates on the last axis, so a multi-channel array had to be reduced first, and the stride arithmetic reads shape[0] as samples.
ec8cc80 to
0f76ab6
Compare
CI recapDashboard: View test results in Grafana |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
What does this PR do?
Fixes #47886.
AutomaticSpeechRecognitionPipelineaveraged multi-channel input over a hardcodedaxis=0, which assumes channels-first(channels, samples).soundfile.read,librosa.load(..., mono=False)andscipy.io.wavfile.readall return channels-last(samples, channels), so on that layout the mean ran across time: a 3-second stereoclip collapsed to 2 numbers, was padded back to 30s of silence, and transcribed as
' you'.The pipeline logged a warning saying the conversion had happened. It had, along the
wrong axis.
The fix
Infer the channel axis rather than assume it. Real audio has far more samples than
channels, so the shorter axis is the channel axis.
(2, N)keeps working exactly asbefore;
(N, 2)stops being destroyed.Also report
tuple(inputs.shape)in the warning instead ofinputs.ndim. The oldmessage interpolated
ndim, so it printed "got 2" for every 2-D input regardless ofchannel count, and the number that would actually help you debug never appeared.
Why not raise instead
That is a reasonable alternative and I would switch if you prefer it. The two sibling
pipelines already reject multi-channel outright:
So the same stereo array raises a clear
ValueErrorin two pipelines and silentlytranscribes as
' you'in the third. I kept the conversion because it is existingdocumented behaviour and removing it would break callers who currently pass
(2, N)successfully, but making ASR raise like its siblings is a three-line change if that is
the preferred direction.
I left
ndim > 2alone deliberately; that is a separate question from the reported bug.Tests
test_multichannel_mono_conversion_is_layout_agnosticcovers(N, 2),(2, N), and2-D mono in both orientations, asserting all four match the 1-D baseline.
It fails on
mainwithAssertionError: choose a window size 400 that is [2, 2]fromtorchaudio, which is the bug surfacing as a 2-sample waveform.
Identical failures before and after, all pre-existing in my environment
(
test_return_timestamps_ctc_fast,test_pipeline_generation_kwargsand friends), sono regressions introduced.
ruff checkandruff format --checkboth clean.Who can review?
@ylacombe @eustlb (audio pipelines)