From: Florian J. <flo...@we...> - 2011-11-26 15:56:32
|
Hi in node.cpp, in function AudioTrack::copyData(), the basic code structure is like this: if (processed()) { for(i = 0; i < srcTotalOutChans; ++i) buffer[i] = outBuffers[i]; } else { // First time here during this process cycle. // Point the input buffers at a temporary stack buffer. float data[nframes * srcTotalOutChans]; for(i = 0; i < srcTotalOutChans; ++i) buffer[i] = data + i * nframes; // NOT neccesarily a return. this can go on behind the closig else-"}"! } // happily read from buffer. valgrind tells me that there are invalid reads from buffer in node.cpp, line 722 (float val = *sp++) i added print statements which showed that muse all the time (not just one or two times: MANY times per second!) goes through the "else" branch, leaves it and continues with the function. the problem is now, that the "float data[]" temporary stack array is deleted as soon as we hit the closing "}"-bracket. but buffer still points to the now invalid memory locations, and the code happily uses it and reads from buffer, creating all these invalid read messages. and it does that all the time. so, what is actually causing this? why is that else branch hit so many times? and how can we fix it? i'd propose to either call malloc() or posix_memalign() to allocate the memory which currently is allocated on the stack via data[], and find a way to free it again at the end of the function... or will that be too slow? if so, what can we do instead? maybe allocate such an array ONE time, and then reuse it all the time? that is, make data a global float-pointer initially set to NULL. if NULL, then allocate it. after that (or after not allocating it because it wasn't NULL), set buffer to &data[blah] accordingly. the problem is that i don't understand what the code actually does. yeah, i think my proposal would be: static float* AudioTrack::temp_data = NULL; and at the place where that stacked data is created, replace it by the following: if (!temp_data) posix_memalign(&temp_data, 16, required_size_plus_some_safety_margin?); for(i = 0; i < srcTotalOutChans; ++i) buffer[i] = temp_data + i * nframes; is that okay? or can you see any problems with that? tim? then i've another question: why do we use posix_memalign in audiotrack.cpp for allocating the outBuffers? "only" because it makes stuff faster, or is there another reason for it? btw, AudioTrack::addData seems to do the same garbage on the first glance, will have a look at it as well. especially (but not limited to) tim: please tell me if my proposal will be okay. greetings flo |
From: Tim E. R. <ter...@ro...> - 2011-11-26 19:39:19
|
On November 26, 2011 4:56:23 PM Florian Jung wrote: > Hi > > in node.cpp, in function AudioTrack::copyData(), the basic code > structure is like this: > > > if (processed()) > { > for(i = 0; i < srcTotalOutChans; ++i) > buffer[i] = outBuffers[i]; > } > else > { > // First time here during this process cycle. > > // Point the input buffers at a temporary stack buffer. > float data[nframes * srcTotalOutChans]; > for(i = 0; i < srcTotalOutChans; ++i) > buffer[i] = data + i * nframes; > > // NOT neccesarily a return. this can go on behind the closig else-"}"! > } > > // happily read from buffer. > > > valgrind tells me that there are invalid reads from buffer in node.cpp, > line 722 (float val = *sp++) > i added print statements which showed that muse all the time (not just > one or two times: MANY times per second!) goes through the "else" > branch, leaves it and continues with the function. > > the problem is now, that the "float data[]" temporary stack array is > deleted as soon as we hit the closing "}"-bracket. but buffer still > points to the now invalid memory locations, and the code happily uses it > and reads from buffer, creating all these invalid read messages. and it > does that all the time. Ooh, I see. Will check. > > so, what is actually causing this? why is that else branch hit so many > times? > > and how can we fix it? > i'd propose to either call malloc() or posix_memalign() to allocate the > memory which currently is allocated on the stack via data[], and find a > way to free it again at the end of the function... > > or will that be too slow? if so, what can we do instead? maybe allocate > such an array ONE time, and then reuse it all the time? > that is, make data a global float-pointer initially set to NULL. if > NULL, then allocate it. after that (or after not allocating it because > it wasn't NULL), set buffer to &data[blah] accordingly. > > the problem is that i don't understand what the code actually does. Yep, it's the main audio engine. Welcome to hell. Put your thinking cap on and meditate. That's where data is gathered from all things connected, via routes, to an audio track. copyData and addData are called many times per second, even multiple times during one audio process cycle. So don't be alarmed at the copious messages. > > yeah, i think my proposal would be: > static float* AudioTrack::temp_data = NULL; > > and at the place where that stacked data is created, replace it by the > following: > > if (!temp_data) posix_memalign(&temp_data, 16, > required_size_plus_some_safety_margin?); > for(i = 0; i < srcTotalOutChans; ++i) buffer[i] = temp_data + i * nframes; > > > is that okay? or can you see any problems with that? tim? > OK thanks, I'm actually working more in those same functions right now. More fixes and surprises coming... > > then i've another question: why do we use posix_memalign in > audiotrack.cpp for allocating the outBuffers? "only" because it makes > stuff faster, or is there another reason for it? I borrowed that method from muse evolution. Its main purpose is to align the memory on a specified boundary, in this case a nice round even 16 bytes. Memory alignment can have an affect sometimes. I've seen it. So I thought it would be good to ensure memory is aligned. > > btw, AudioTrack::addData seems to do the same garbage on the first > glance, will have a look at it as well. Yes, it is identical to copyData, except that it adds data on top of the data gathered by copyData. > > especially (but not limited to) tim: please tell me if my proposal will > be okay. Don't touch just yet. My department. Wait till my next commit, we'll see if I can take care of it. Tim. > > greetings > flo > |
From: Tim E. R. <ter...@ro...> - 2011-11-26 23:01:19
|
Woo hoo! Thanks Florian. You and I were coincidentally on the exact same page yesterday. How many stupid hours I spent yesterday because simply adding some printf lines right there, at post-fader code, was corrupting the audio buffers! And so I'm like "Wtf? This can't be." It was right under my nose! My bad mistake. I simply moved that offending line to the top: float data[nframes * srcTotalOutChans]; (Here in the realtime audio context, one should not allocate using alloc or new etc. Always try to set up any allocated buffers outside of the RT code.) --- Stay tuned for some good audio fixes. OK - it works now, I can say it: I fixed DSSI synth audio inputs. :) This means you can now use dssi synths such as vocoders, effects, samplers, anything that needs audio inputs, yet is still classified as a 'synthesizer', not an 'effect'. Currently, 'effects' end up in our LADSPA browser instead of the list of available synthesizers. But some DSSI plugins have programs! But rack plugins have no programs! So I've known for a while that simply playing with a few lines in plugin.cpp and dssihost.cpp allows the DSSI 'effect' plugins to instead load as synthesizer tracks in MusE. Complete with programs, controllers etc. Works great. Yeah, it's a bit weird, effects as synths, but what can we do? Need programs. I'm testing/thinking it over... Anyway as a side note, the only DSSI synth with audio inputs I could find was Calf Rotary Speaker. I spent hours adding MusE code, all the time working with CRS, thinking "why the heck is the L/R audio being added together?". To prove that only CRS adds L/R, I was forced to allow DSSI 'effects' as MusE 'synthesizer' tracks as I mentioned above, so I could use other plugins to test the theory. Calf Flanger DSSI effect plugin works fine, for example. And it's got programs. And so do a lot of DSSI and VST plugins... Re-wrote/simplified metering code: Panning a mono audio track now does not affect meters, but still allows panning of the output. Have a good weekend. Tim. On November 26, 2011 2:39:00 PM Tim E. Real wrote: > On November 26, 2011 4:56:23 PM Florian Jung wrote: > > Hi > > > > in node.cpp, in function AudioTrack::copyData(), the basic code > > structure is like this: > > > > > > if (processed()) > > { > > > > for(i = 0; i < srcTotalOutChans; ++i) > > > > buffer[i] = outBuffers[i]; > > > > } > > else > > { > > > > // First time here during this process cycle. > > > > // Point the input buffers at a temporary stack buffer. > > float data[nframes * srcTotalOutChans]; > > for(i = 0; i < srcTotalOutChans; ++i) > > > > buffer[i] = data + i * nframes; > > > > // NOT neccesarily a return. this can go on behind the closig > > else-"}"!> > > } > > > > // happily read from buffer. > > > > > > valgrind tells me that there are invalid reads from buffer in node.cpp, > > line 722 (float val = *sp++) > > i added print statements which showed that muse all the time (not just > > one or two times: MANY times per second!) goes through the "else" > > branch, leaves it and continues with the function. > > > > the problem is now, that the "float data[]" temporary stack array is > > deleted as soon as we hit the closing "}"-bracket. but buffer still > > points to the now invalid memory locations, and the code happily uses it > > and reads from buffer, creating all these invalid read messages. and it > > does that all the time. > > Ooh, I see. Will check. > > > so, what is actually causing this? why is that else branch hit so many > > times? > > > > and how can we fix it? > > i'd propose to either call malloc() or posix_memalign() to allocate the > > memory which currently is allocated on the stack via data[], and find a > > way to free it again at the end of the function... > > > > or will that be too slow? if so, what can we do instead? maybe allocate > > such an array ONE time, and then reuse it all the time? > > that is, make data a global float-pointer initially set to NULL. if > > NULL, then allocate it. after that (or after not allocating it because > > it wasn't NULL), set buffer to &data[blah] accordingly. > > > > the problem is that i don't understand what the code actually does. > > Yep, it's the main audio engine. Welcome to hell. Put your thinking cap on > and meditate. That's where data is gathered from all things connected, > via routes, to an audio track. > > copyData and addData are called many times per second, even > multiple times during one audio process cycle. > > So don't be alarmed at the copious messages. > > > yeah, i think my proposal would be: > > static float* AudioTrack::temp_data = NULL; > > > > and at the place where that stacked data is created, replace it by the > > following: > > > > if (!temp_data) posix_memalign(&temp_data, 16, > > required_size_plus_some_safety_margin?); > > for(i = 0; i < srcTotalOutChans; ++i) buffer[i] = temp_data + i * > > nframes; > > > > > > is that okay? or can you see any problems with that? tim? > > OK thanks, I'm actually working more in those same functions right now. > More fixes and surprises coming... > > > then i've another question: why do we use posix_memalign in > > audiotrack.cpp for allocating the outBuffers? "only" because it makes > > stuff faster, or is there another reason for it? > > I borrowed that method from muse evolution. > Its main purpose is to align the memory on a specified boundary, in this > case a nice round even 16 bytes. > > Memory alignment can have an affect sometimes. I've seen it. > So I thought it would be good to ensure memory is aligned. > > > btw, AudioTrack::addData seems to do the same garbage on the first > > glance, will have a look at it as well. > > Yes, it is identical to copyData, except that it adds data on top of the > data gathered by copyData. > > > especially (but not limited to) tim: please tell me if my proposal will > > be okay. > > Don't touch just yet. My department. Wait till my next commit, we'll see > if I can take care of it. > > Tim. > > > greetings > > flo > |
From: Florian J. <flo...@we...> - 2011-11-27 13:57:50
|
Am 27.11.2011 00:00, schrieb Tim E. Real: > Woo hoo! Thanks Florian. > > You and I were coincidentally on the exact same page yesterday. > > How many stupid hours I spent yesterday because simply adding some > printf lines right there, at post-fader code, was corrupting the audio > buffers! And so I'm like "Wtf? This can't be." > > It was right under my nose! My bad mistake. > > I simply moved that offending line to the top: > float data[nframes * srcTotalOutChans]; > > (Here in the realtime audio context, one should not allocate > using alloc or new etc. Always try to set up any allocated buffers > outside of the RT code.) > oops, okay. i fixed it by using new[] and committed that to release_2_0. i fixed my fix now, please verify. greetings flo |
From: Florian J. <flo...@we...> - 2011-11-27 13:35:55
|
fixed the particular problem. tim, can you verify please? |
From: Florian J. <flo...@we...> - 2011-11-27 13:35:16
|
i fixed both addData and copyData. i'm now creating a temp buffer via new[] for each function call. using a global buffer would interfere with recursive calls which may happen. if you're _sure_ that a global buffer will be no problem, feel free to use one. but it's not neccessary, it seems to have no performance impact. do you think i should use posix_memalign instead? if so, watch out for the many delete[] temp_data lines. but it wasn't memaligned before as well, so this should be not a real problem... i considered this as fixed, do you as well? greetings flo |