My thumbnails are getting cut


#1

I updated an audioThumbnail to read memory mapped the other day. Immediately I was spanked by an assert about not having mapped the file sufficiently, although I had mapped the entire file. It turned out the thumbnail requested a few more samples than was available in the file, don't know if beacuse of my code or the inner beings of the thumbnail. Anyway, doing so was ok before when I didn't use memrymapping but read straight from the wav file. I don't think that's fair so I suggest a small alteration in MemoryMappedWavReader::readMaxLevels to silently ignore any extra trailing samples outside the file content:
 

void readMaxLevels (int64 startSampleInFile, int64 numSamples, Range<float>* results, int numChannelsToRead) override
    {
         int numSamplesInFile = dataLength / bytesPerFrame;
         numSamples = jmin(numSamples, numSamplesInFile - startSampleInFile);
        
         if (map == nullptr || numSamples <= 0 || !mappedSection.contains(Range<int64>(startSampleInFile, startSampleInFile + numSamples)))
       {
             jassertfalse; // you must make sure that the window contains all the samples you're going to attempt to read.

            for (int i = 0; i < numChannelsToRead; ++i)
                results[i] = Range<float>();

            return;
        }

        switch (bitsPerSample)
        {
            case 8:     scanMinAndMax<AudioData::UInt8> (startSampleInFile, numSamples, results, numChannelsToRead); break;
            case 16:    scanMinAndMax<AudioData::Int16> (startSampleInFile, numSamples, results, numChannelsToRead); break;
            case 24:    scanMinAndMax<AudioData::Int24> (startSampleInFile, numSamples, results, numChannelsToRead); break;
            case 32:    if (usesFloatingPointData) scanMinAndMax<AudioData::Float32> (startSampleInFile, numSamples, results, numChannelsToRead);
                        else                       scanMinAndMax<AudioData::Int32>   (startSampleInFile, numSamples, results, numChannelsToRead); break;
            default:    jassertfalse; break;
        }
    }

#2

(Sorry, just saw this post)

Ok, valid point, but I don't think that limitiing it to the mapped area makes sense.. Maybe this instead?


    void readMaxLevels (int64 startSampleInFile, int64 numSamples, Range<float>* results, int numChannelsToRead) override
    {
        numSamples = jmin (numSamples, lengthInSamples - startSampleInFile);
        
        if (map == nullptr || numSamples <= 0 || ! mappedSection.contains (Range<int64> (startSampleInFile, startSampleInFile + numSamples)))
        {
            jassert (numSamples <= 0); // you must make sure that the window contains all the samples you're going to attempt to read.
            for (int i = 0; i < numChannelsToRead; ++i)
                results[i] = Range<float>();
            return;
        }
        switch (bitsPerSample)
        {
            case 8:     scanMinAndMax<AudioData::UInt8> (startSampleInFile, numSamples, results, numChannelsToRead); break;
            case 16:    scanMinAndMax<AudioData::Int16> (startSampleInFile, numSamples, results, numChannelsToRead); break;
            case 24:    scanMinAndMax<AudioData::Int24> (startSampleInFile, numSamples, results, numChannelsToRead); break;
            case 32:    if (usesFloatingPointData) scanMinAndMax<AudioData::Float32> (startSampleInFile, numSamples, results, numChannelsToRead);
                        else                       scanMinAndMax<AudioData::Int32>   (startSampleInFile, numSamples, results, numChannelsToRead); break;
            default:    jassertfalse; break;
        }
    }

Here I'm limiting it to the total file length, but it should still give a warning if you've not fully mapped the file and try to read beyond the section that's been mapped.


#3

If you have a 10kB file and map the first 1000bytes and happen to request 1001, doesn't it make more sense to return the levels of the mapped 1000 bytes and zero the the 1001:th than returning all 1001 bytes zeroed?!

It's at least more in line with the handling of whole file requests.


#4

No... If you try to read past the end of a mapped sub-area, then it should assert and fail, because that would be a silly thing to do.

But the original bug you reported was that it was incorrectly asserting if you read past the end of the entire file, right? That's what I was trying to fix.


#5

My point is, I guess, that it's not more silly to read past the mapped area than past the file end, which you (mostly) get away with with not as much as an assert if you read none memory mapped. In AudiosubsectionReader you're even served with having the extra bytes cleared with clearSamplesBeyondAvailableLength().

And, the function does not really *fail* (disregarding the assert, which is only there in debug mode), but merely returns bogus values, Range (0,0), i.e the same result as a silent file and if you think that's more silly than returning the values that you *do* have, the I don't agree.

My initial post was about getting incorrect asserts for not having mapped the entire file AND for not getting any valid values back at all if trying to read past the file end, whereas when using none memory mapped files it's perfectly legal to do so. But looking at it now, I guess I found a cleverer solution in the end that also dealt with partial mapped files.

But even so, if we skip the partial mapped files, you still have the difference between memory mapped reading and none memory mapped.

normal files -> alright to read past file end, you even get the extraneous bytes cleared out for free
memory mapped files -> you get an assert (acceptable), but bogus values in return if reading as much as a tiny byte past file end


It's this difference in behaviour I think might be annoying for someone pimping up his/hers code with memory mapped readers.

Well, enough of arguing from me now, reading all text above might give a wrong impression of the importance I give the subject, I'm fully confident in your decisions, whatever they'll be...

 


#6

I think my summary here would be that this should behave as other readers do when you read beyond the end of the whole file (i.e. it works correctly without complaining). You're right that the old code didn't do this, so I'll fix that.

But it's a precondition of using a memory-mapped reader that you must map the area you're reading from. If you don't, there's an assertion, and you into dreaded undefined behaviour territory. That should apply to getting the max levels as much as it does to reading samples.

If you want to use a subsection of your file as a reader, then the thing to use would be to wrap it in an AudioSubsectionReader.