Re: [question/testing] SoundFileView improvements

A couple optimization hints -

1. p.drawLine( x + i, min, x + i, max );
    p.drawLine( x + i, minRMS[i], x + i, maxRMS[i] );
These might be faster if they were a 1px wide fillRect.

2. compositionMode of the pen should be QPainter::CompositionMode_Source

3. p.setClipping(true); p.setClipRect( x, -halfChH, x+width, chHeight );
I believe it would be cheaper to guarantee the ranges of our values either
when we calculate in displayData, or inline in our inner loop.

4. Drawing vertical lines is bad for cache coherence and eliminates
possibilities for vectorization. It MIGHT be worth experimenting with
rotating the pixmap 90 degrees and drawing that way (so we have horizontal
scanlines), and then rotating back to the correct orientation when we do
drawPixmap, in hopes that Qt has either optimized the 90 degree rotation
case or vectorized horizontal line drawing.

Dynamic allocation is pretty trivial, as is pen setting (though drawing RMS
and min/max in separate passes might be a minor improvement).

The thing to think about here: drawLine calls will touch on average say 3/4
of pixels - half for min+max, another 1/4 for rms, so nearly O(n). These
will involve an "is this pixel under the line" calculation / loop, plus a
blend calculation, plus a clipping rect calculation, plus a pixel write.
Then, blending the overall pixmap is also a blend for every pixel - O(n).
Background-filling your view is another O(n), though it's a cheap one.
Whereas: setting a pen is probably copying ~16 bytes on the stack (less if
pens are ptrs), and the allocation is probably just re-using recently freed
memory (since I assume we only have 1 or 2 of each buffer around at a time
- the lifetime of them is non-obvious), which would NEVER be close to O(n)
unless we're leaking memory. None of the per-vertical-scanline branches in
the inner loop are going to be nearly as expensive as the many per-pixel
branches when actually painting pixels. Basically, anything happening
per-scanline or per-overall-draw will be dwarfed by even small extra costs
in the O(n) operations.

- Scott C



On Wed, Apr 4, 2018 at 5:06 PM <[email protected]> wrote:

> > The critical loop makes two calls two setPen for each point. Each of
> those calls will result in a nontrivial amount of instructions - from what
> I can tell, at least a few branches, a couple assignments, and an atomic
> ref count update on the Pen pimpl. Dynamically allocating on every redraw
> probably hurts as well.
>
> I decided to try this out for fun, and ended up seeing very little
> performance difference! Never mind then, I guess Qt is better at this than
> I thought. :)
>
> -Brian
>
> On Wed, Apr 4, 2018 at 7:26 PM, brian heim <[email protected]> wrote:
>
>> > I think this is a rather severe performance penalty for a relatively
>> rare use case. What do you think?
>>
>> You should do some testing to see where exactly the performance penalty
>> is coming from. I'm suspicious of the switch to QPointF.
>>
>> But more than that, floating point arithmetic is slow (see this post
>> <https://stackoverflow.com/questions/2550281/floating-point-vs-integer-calculations-on-modern-hardware?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa>),
>> and nobody has screens to display that level of precision. Think about it -
>> a short can represent about 65k values. Even with I think using floats in
>> the critical display loop is probably going to hurt you no matter what.
>> Even a larger integral type would probably be better.
>>
>> Honestly, looking at this implementation, it's not exactly perfectly
>> designed in the first place. (At least, the true branch of the if at line
>> 774). The critical loop makes two calls two setPen for each point. Each of
>> those calls will result in a nontrivial amount of instructions - from what
>> I can tell, at least a few branches, a couple assignments, and an atomic
>> ref count update on the Pen pimpl. Dynamically allocating on every redraw
>> probably hurts as well.
>>
>> > I see the following solutions:
>> > - leave SoundFileView as is, and note that it will only display data
>> between -1 and 1
>> > - modify SoundFileView to process everything in floats, which makes it
>> noticeably slower for everyone
>> > - create a companion class, maybe SoundFileViewF, that processes
>> everything in floats, and provides the option on the SC side to use that
>> instead, when larger range is needed
>>
>> You could also try changing back to short, but change the normalization
>> instead, and give the user control over that (or better yet allow it to be
>> automatically calculated from the data).
>>
>> -Brian
>>
>> On Wed, Apr 4, 2018 at 5:51 PM, <[email protected]> wrote:
>>
>>> Hello Devs,
>>>
>>> Currently SoundFileView does not display data outside of the -1 to 1
>>> range.
>>> This is generally OK in most cases (as we often use integer soundfile
>>> formats), but there are cases where one might need to display data outside
>>> of that range (floating point files, displaying non-audio data).
>>>
>>> The current implementation processes data as "short" integers, clipping
>>> the out-of-range values to prevent overflow. In attempt to achieve
>>> displaying arbitrary ranges, I changed processing to floats and,
>>> unsurprisingly, the performance got worse. I was a little surprised though
>>> how much worse it got - 3.5x longer to display similar data :/
>>>
>>> I think this is a rather severe performance penalty for a relatively
>>> rare use case. What do you think?
>>> I see the following solutions:
>>> - leave SoundFileView as is, and note that it will only display data
>>> between -1 and 1
>>> - modify SoundFileView to process everything in floats, which makes it
>>> noticeably slower for everyone
>>> - create a companion class, maybe SoundFileViewF, that processes
>>> everything in floats, and provides the option on the SC side to use that
>>> instead, when larger range is needed
>>>
>>> If anyone would like to give it a try, the floating-point soundfileview
>>> version is here:
>>> https://github.com/dyfer/supercollider/tree/topic/soundfileview-fixes
>>>
>>> Here's some code that shows the problem with the current implementation,
>>> as well as a simple benchmark.
>>>
>>> (
>>> r = 48000;
>>> m = 1.5; //minmax value
>>> w = Window.new.front;
>>> w.view.layout=HLayout();
>>> a = SoundFileView.new(w.view).alloc(r * 1, 1, r);
>>> a.set(0, (r).asInteger.collect({|i| i.linlin(0, r, m.neg, m)}));
>>> )
>>> a.yZoom_(0.5); //rescale to check
>>> a.yZoom_(1); //rescale
>>>
>>> //benchmark
>>> (
>>> {
>>> 10.do({
>>> a.set(0, (r).asInteger.collect({rrand(m.neg, m)}));
>>> });
>>> }.bench
>>> )
>>>
>>> I'd appreciate your input on this! Thanks,
>>> Marcin
>>>
>>>
>>
>