Fixing blocks read with overlap for files shorter than block size

[illesguy]

Summary

The blocks method to read an audio in frame blocks with an optional overlap has a bug in the edge case when the file has less frames than the block size specified and the overlap specified is not 0. The full file and an overlap number of random values are returned in the output in this case.

Explanation

When the method is called without a provided out array, it creates an empty array to store the output with the np.empty method with the shape corresponding to the block size argument (and the number of channels but that is not relevant for this problem). The default values in an array created by np.empty are not deterministic.

The file is then read into this out array up to the block size or the end of the file, whichever is shorter. The number of frames in the file plus the overlap size is copied into a block from the out array which is returned.

Example

Let's take an example of a mono file with 5 frames, a block size of 10 and an overlap of 1.

• First an output array of size 10 is produced: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0].
• Then the entire file is read into this array (since the file length is shorter than the block size) so it becomes:
  [1, 1, 1, 1, 1, 0, 0, 0, 0, 0] (assuming all frames in the mono file were 1)
• Then the number of frames in the file plus the overlap size is taken from the output array as the block which is 5+1=6. The output block we get therefore is: [1, 1, 1, 1, 1, 0].
• We get an extra frame at the end of our output array which in this example is 0 but can be a random number produced by np.empty.

The test_block_longer_than_file_with_overlap_mono test was added to cover this case. It did fail as expected before adding the fix.

What happens when the block size is smaller than the file length but there is an overlap?

In case the where the file has multiple blocks in it, the error doesn't occur because at the end of each block read, the overlapping frames are added to the beginning of the out array and the next frames for the file are read on top of that. Due to this, when returning the final block, we will again return the number of frames left in the file plus the overlap frames, but in this case the overlap frames will be the frames added to the beginning of the output in the previous run so this case isn't impacted by the bug.

The fix

When creating the initial out array (in case it was not provided as an argument) we just need to make sure to create it with a length that is equal to the block size OR the number of frames in the file, whichever is smaller. If the block size is smaller than the number of frames in the file, it will work as previously and as discussed above, that case is not affected by the bug. If however the block size is greater, the output array will be equal to the number of frames in the file but in this case there's only going to be one block anyway so having an overlap does not make sense.

[bastibe]

Congratulations! It's quite rare that someone finds a real bug in this project.

And thank you for fixing it right away as well!

[illesguy]

Thanks for being responsive and for the merge! Will there be a new version released with this commit?

[bastibe]

There are still a few cross-compilation issues with the CI that need to be resolved before I'll cut a new version. Unfortunately, this is not my area of expertise, and I don't have a whole lot of time available to devote to this. I can't promise any particular time frame, but I hope to find time for it during the Christmas holidays at the latest.

[bmcfee]

Sorry to jump into this thread late. This PR is changing the test behavior in librosa's downstream block processing, and I wanted to check in here to better understand whether the new behavior is fully as intended (and librosa needs to change) or if a regression has been introduced.

It seems to me that the change introduced in this PR is now ignoring the fill_value parameter, which I understand is meant to pad out incomplete blocks to the desired length. The API documentation reads as follows:

Yields: numpy.ndarray or type(out) – Blocks of audio data. If out was given, and the requested frames are not an integer multiple of the length of out, and no fill_value was given, the last block will be a smaller view into out.

Reading this pedantically, it seems like all conditions (out given, requested frames not integer multiple, no fill value given) must be satisfied for the last block to be short. If any of those conditions fail, then the last block should be the same length as out.

The issue mentioned above (librosa/librosa#1895 ) provides a link to a test log from a librosa branch running on the 0.13 release. The test failures, as far as I can tell, correspond to cases where out is not given, but a fill value is provided. Previously (0.12 series) these tests passed, but they are now failing in a way that appears to be ignoring the fill_value setting.

If this is intended behavior, we can change the tests downstream, but I'd prefer to verify that before we start changing things.

[illesguy]

Hi, I'm not the owner of the repo but I did make this particular change. I had a look now and it looks like the fill value is indeed ignored but only in cases when the the entire file read is shorter than the block size itself. I tested in cases where only once getting to the last block, the remainder of the file was shorter than the block size, but in those cases the fill_value worked as expected.

For the cases where the entire file being read is shorter than the block size, this does seem like an oversight on my part and I think it would make sense to fix in soundfile (if the owner of the repo also agrees). I'll probably have time to work on it at some point this week.

These are the test I added to the test_soundfile.py (only locally for now) file to confirm this, the file in this test is 5 frames long, the first 2 tests pass, the 2nd two fail:

def test_blocks_with_fill_value_mono():
    blocks = list(sf.blocks(filename_mono, blocksize=3, dtype='int16',
                            fill_value=0))
    assert_equal_list_of_arrays(blocks, [[0, 1, 2], [-2, -1, 0]])


def test_blocks_with_overlap_and_fill_value_mono():
    blocks = list(sf.blocks(filename_mono, blocksize=4, dtype='int16',
                            overlap=2, fill_value=0))
    assert_equal_list_of_arrays(blocks, [[0, 1, 2, -2], [2, -2, -1, 0]])

def test_block_longer_than_file_with_fill_value_mono():
    blocks = list(sf.blocks(filename_mono, blocksize=10, dtype='int16',
                            overlap=2, fill_value=0))
    assert_equal_list_of_arrays(blocks, [[0, 1, 2, -2, -1, 0, 0, 0, 0, 0]])


def test_block_longer_than_file_with_overlap_and_fill_value_mono():
    blocks = list(sf.blocks(filename_mono, blocksize=10, dtype='int16',
                            overlap=2, fill_value=0))
    assert_equal_list_of_arrays(blocks, [[0, 1, 2, -2, -1, 0, 0, 0, 0, 0]])

[illesguy]

I created a new PR to fix the above issue. Hopefully a new version will be created from it soon.

[bastibe]

Thank you for so thoroughly testing soundfile, beyond our own test suite. And sorry for not responding earlier, the last week has been busy.

@bmcfee, does the latest change by @illesguy make sense from your end? It looks good to me. If you're happy, too, I'll cut a bugfix release with this change soon.

[bmcfee]

@bastibe @illesguy LGTM! All of our downstream tests pass on this PR build. These include shape checks for various configurations, with and without fill_value set, so I think it's good to go.

Thanks again @illesguy !

[bastibe]

Thank you, I'll cut a new version then. My apologies for the inconvenience.