Possible bug in AudioSampleBuffer


#1

I’ve been using the AudioSampleBuffer class as a container for pre-allocated external buffers, so its allocoatedBytes member is zero.

From the header:

    /** Copies another buffer.

        This buffer will make its own copy of the other's data, unless the buffer was created
        using an external data buffer, in which case boths buffers will just point to the same
        shared block of data.
    */
    AudioSampleBuffer (const AudioSampleBuffer& other) noexcept;

It looks like it’s meant to have the behaviour I expect, so I can use copy constructors to other functions and just get the pointers copied, but in the constructor it looks like a deep copy is always done:

AudioSampleBuffer::AudioSampleBuffer (const AudioSampleBuffer& other) noexcept
  : numChannels (other.numChannels),
    size (other.size)
{
    allocateData();

    for (int i = 0; i < numChannels; ++i)
        FloatVectorOperations::copy (channels[i], other.channels[i], size);
}

Should it be more like this?

AudioSampleBuffer::AudioSampleBuffer (const AudioSampleBuffer& other) noexcept
  : numChannels (other.numChannels),
    size (other.size),
    allocatedBytes (other.allocatedBytes)
{
    if (allocatedBytes == 0 )
    {
        allocateChannels (other.channels, 0);
    }
    else
    {
        allocateData();

        for (int i = 0; i < numChannels; ++i)
            FloatVectorOperations::copy (channels[i], other.channels[i], size);
    }
}

#2

Thanks Andy! Yes… strange that it doesn’t already do that, but I think you’re right - I’ll get on the case and have a good look at that, cheers!


#3

Also the behaviour of operator= should be consistent as well with what the copy constructor does, and perhaps have a specific duplicate / deep copy and shallow copy / reference call, even keeping the current names of copyFrom and setDataToReferTo but both taking const AudioSampleBuffer& as parameters.


#4

Hmm… I’d be reluctant to change that, in case people are relying on the copy operator to actually make a deep copy of the data. I was happy to change the constructor because like you pointed out, the comments clearly say that it won’t make a deep copy, but that’s not the case for the operator=, so a change could break legitimate code. Seems to me that having setDataToReferTo() to do a shallow copy, and operator= to do a deep copy should cover all the use-cases there.


#5

Having different behaviour for the copy constructor and equals operator is a dangerous game to play, and although everyone may be a c++ expert some may get confused thinking that they have a copy of the data when they do this:

AudioSampleBuffer b1 (externaldata, numchans, len); // point b1 to some external data
AudioSampleBuffer b2 = b1; // this will call the copy constructor and so make a shallow copy

but what they actually need to do is:

AudioSampleBuffer b1 (externaldata, numchans, len); // point b1 to some external data
AudioSampleBuffer b2;
b2 = b1; // this will call the equals operator and so make a deep copy

#6

Yes, I can’t argue with that - it’s a tricky choice. I think possibly the only really safe thing to do here would be to remove the operator= and replace it with a method explicitly called “createCopy” or something that explains what it’s going to do, but I’m not keen on doing that either.