Rev. 8875 (developm.) = changed move constructor / move ass.op. for AudioBuffer.channels crashes my app


#1

@hogilux: You changed the move constructor / move assignment for AudioBuffer.channels. I don’t know why, but since then I get random crashes in several parts of my app at startup. Could you have look into this again?


#2

For the records: hogilux is @Fabian :wink:


#3

Those methods look OK to me… Which revision are you talking about? (“8875” isn’t a git ID, is it?)


#4

Sorry, we’re using old fashioned svn. I’ll try to figure out the Git ID…


#5

No Git ID, but date/time: 2017/08/29 12:39:26


#6

Hmm. Fabian’s fix looks correct to me, and the previous version definitely had a bug in it. I can’t really see how the old one would work for you and the new one fail, unless you’ve got some kind of corrupted memory/dangling pointer issue and just happened to be lucky before!


#7

In some cases, I allocate an AudioBuffer on the Stack and copy it to a class member just like this:

someClassMemberAudioSampleBuffer = AudioSampleBuffer (2, samplesPerBlock);

This doesn’t work any more.

I think I now understand why:

In

AudioBuffer& operator= (AudioBuffer&& other) noexcept

this line

channels = numChannels < (int) numElementsInArray (preallocatedChannelSpace) ? preallocatedChannelSpace : other.channels;

is wrong. “other.channels” is never used, because (int) numElementsInArray (preallocatedChannelSpace) is 32. And preallocatedChannelSpace is an Array filled with nullptrs.

If I change it to

channels = numChannels < (int) numElementsInArray (preallocatedChannelSpace) ? other.channels : preallocatedChannelSpace;

it works again…
Could you verify this?


#8

OK… Your suggestion’s definitely not correct, you’re just re-introducing the bug that Fabian was trying to fix.

However, I can see an edge-case where if the source block had been shrunk it could copy the wrong thing into that preallocated data block… Does it fix it your problem if you just change this line:

        memcpy (preallocatedChannelSpace, other.channels, sizeof (preallocatedChannelSpace));

?


#9

Sorry, I missed the memcpy line…

Yes, indeed, now it works like expected. Thank you.


#10

OK, thanks - I’ll push a fix to develop shortly!


#11

Great.


#12

Requesting a unit test be added for that case. Presumably that issue could have been caught during a unit test running its course?

Edit: Unless I skimmed over it accidentally looking for UnitTest subclasses, it appears there isn’t an AudioBuffer<float> or AudioBuffer<double> UnitTest…


#13

Agreed.

(Though TBH we’d have been lucky if any unit-test had caught an edge case like this one)