Exception from std::clamp

Windows
1

We’re getting an exception from std::clamp when loading certain files into an AudioThumbnail. Windows only.

It seems that std::clamp does not like negative values, I assume from when the reader is near the end of the file.

It breaks at the following line:

_STL_REPORT_ERROR("invalid bounds arguments passed to std::clamp");

Fixed with the following patch:

diff --git a/modules/juce_audio_utils/gui/juce_AudioThumbnail.cpp b/modules/juce_audio_utils/gui/juce_AudioThumbnail.cpp
--- a/modules/juce_audio_utils/gui/juce_AudioThumbnail.cpp
+++ b/modules/juce_audio_utils/gui/juce_AudioThumbnail.cpp
@@ -111,5 +111,5 @@
         const auto numAvailableSamples = (int) ((int64) buffer->getNumSamples() - startSampleInFile);
-        const auto numSamplesToCopy = std::clamp (numAvailableSamples, 0, numSamples);
+        const auto numSamplesToCopy = std::clamp (numAvailableSamples, 0, std::abs(numSamples));
 
         if (numSamplesToCopy == 0)
             return true;
2

Well, that means you are calling readSamples with a negative number of samples.

I would look for the error further up the call stack

1 Like
3

std::clamp works fine with negative numbers, but the lo parameter must be less than hi. Isn’t the actual bug the fact that numAvailableSamples is negative? The question is, why is startSampleInFile greater than the number of samples? Or is it an overflow issue with the cast to int?

1 Like
4

Not numAvailableSamples but the number of samples requested (numSamples) when calling the read function.

We can guess, but I think the debugger is the better tool.

Could be trying to draw a fragment of the thumbnail with a higher startTimeSeconds than endTimeSeconds when calling drawChannel().

It could also be an inconsistency in the audio file, where totalNumSamples is different from the actual number of samples, although I don’t know if that would be the place to break.

5

I checked up the call stack and all the values look legit until the call to readSamples, where numSamples becomes negative.

It looks like this happens in the following function which is called at the start of readSamples:

    /** Used by AudioFormatReader subclasses to clear any parts of the data blocks that lie
        beyond the end of their available length.
    */
    static void clearSamplesBeyondAvailableLength (int* const* destChannels, int numDestChannels,
                                                   int startOffsetInDestBuffer, int64 startSampleInFile,
                                                   int& numSamples, int64 fileLengthInSamples)
    {
        if (destChannels == nullptr)
        {
            jassertfalse;
            return;
        }

        const int64 samplesAvailable = fileLengthInSamples - startSampleInFile;

        if (samplesAvailable < numSamples)
        {
            for (int i = numDestChannels; --i >= 0;)
                if (destChannels[i] != nullptr)
                    zeromem (destChannels[i] + startOffsetInDestBuffer, (size_t) numSamples * sizeof (int));

            numSamples = (int) samplesAvailable;
        }
    }
6

Oh, @cpr2323 actually already checked one level deeper.

This is how numSamples can become negative:

So the startSampleInFile seems to be beyond fileLengthInSamples.

Could this be the reason?

1 Like
7

Yes that is the reason.

Working out how it’s getting into that state…

8

Maybe it should be clamped in that method

1 Like
9

I would prefer

jassert (startSampleInFile <= fileLengthInSamples);

since calling it with a start outside the file size is clearly an error.
Don’t try to recover from nonsense input IMHO.

10

I agree @Daniel but I guess there is a reason for that clamp that is there already. For now I’m adding it here just to patch this issue. Not ideal but works for now.

diff --git a/modules/juce_audio_formats/format/juce_AudioFormatReader.h b/modules/juce_audio_formats/format/juce_AudioFormatReader.h
--- a/modules/juce_audio_formats/format/juce_AudioFormatReader.h
+++ b/modules/juce_audio_formats/format/juce_AudioFormatReader.h
@@ -319,4 +318,4 @@
-        const int64 samplesAvailable = fileLengthInSamples - startSampleInFile;
+        const int64 samplesAvailable = std::clamp(fileLengthInSamples - startSampleInFile, (int64) 0, fileLengthInSamples);
 
         if (samplesAvailable < numSamples)
         {
11

I’m not a fan of assertions only in these cases, as you might have an edge case that only appears on a special file on a user session in a live situation and there is nothing that guards against that in release.

1 Like
12

I really would be interested if the call from outside the juce code is legit, because there is a chance of a bug, in which case it would be nice to find a proper fix.

1 Like
13

There’s definitely something going on, as there are two calls to draw the thumbnail in quick succession, and it’s the second time that the negative value appears.
Adding a lock or atomic guard did not fix the issue. Definitely needs resolving properly.

14

I think I’ve seen this in other AudioFormatReader types, I thought I got them all a little while ago, but evidently not, maybe it didn’t make it through review I’ll go back and check. If memory serves me correctly after clearSamplesBeyondAvailableLength I think it should have…

if (numSamples <= 0)
    return true;

I can’t remember the details but my conclusion was that as a precaution that should just always be the first thing to do.

1 Like
15

I see a bit of int mixed with int64. All sample-length related values should be kept in int64 to allow for larger and larger files. 10+ years ago, a 4 GB audio file was unthinkable, not so today. Storage is cheap, and people sometimes do crazy things.

1 Like
16

I think it is consistent as in, if it refers to a full file it uses int64, but for AudioBuffer offsets it uses int.

It feels the right thing to do, although you are right, there are situations where you have to double check type propagation. And following the trend, I guess we will see more warnings coming in that realm.