Conversation
| } | ||
|
|
||
| app.disableHardwareAcceleration(); | ||
| // app.disableHardwareAcceleration(); |
There was a problem hiding this comment.
Yes. WebGL does not work with hardware acceleration off. I will remove this rather than commenting out before this is merged.
|
Sometimes I'm just thinking about the idea of having all pre-rendered voltmeters states into a single image and changing the visible area of this image. |
|
@holiber I'm not sure that would be as efficient. Our original volmeter implementation was based on using CSS transforms, which ended up being less efficient than our most recent Canvas 2D implementation, and significantly less efficient than this WebGL implementation. But if you do have any other ideas you want to try I am all ears and certainly not married to this implementation. |
|
|
||
| window['startMeasure'] = () => { | ||
| this.measurements = []; | ||
| }; |
There was a problem hiding this comment.
I'll be backing these changes out of this PR. Was just using this to sanity check the performance increases I was seeing.
|
I mean if you have a set of pre-rendered frames in one image you can just change the position of the visible part of the image. So the computer renders the image only once. However, I can't say this way will be more efficient without testing. Just letting know that we didn't try it. |
|
Yeah essentially a sprite sheet. My gut tells me using sprites to animate the volmeters with be more expensive. But yes we haven't tried it. |
|
❌ build failed Build execution time has reached the maximum allowed time for your plan (60 minutes). |
|
Looks like our CI machines don't support hardware acceleration, so this will always raise an error there. I am either going to keep the old volmeters as a fallback or just disable it on our CI. We need to do more research on how many users are going to be on machines that won't support WebGL. |
|
On second thought I'm definitely going to keep the old code as a fallback. It's easy enough to do and will guarantee the best support for the most people. |
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| } | ||
|
|
||
| drawVolmeterChannel(peak: number, channel: number) { | ||
| private drawVolmeterChannelC2d(peak: number, channel: number) { |
There was a problem hiding this comment.
Function drawVolmeterChannelC2d has 33 lines of code (exceeds 30 allowed). Consider refactoring.
|
Ok should properly fall back to Canvas 2D now on unsupported machines. The 2 renderers produce visually identical results. |
|
Code Climate has analyzed commit 39f4916 and detected 0 issues on this pull request. View more on Code Climate. |
| this.drawVolmeterWebgl(volmeter.peak); | ||
| } else { | ||
| this.drawVolmeterC2d(volmeter.peak); | ||
| } |
There was a problem hiding this comment.
If we pay attention on performance in this PR, we should draw volmeters on requestAnimationFrame event. It allows skipping some frames if CPU is overloaded
There was a problem hiding this comment.
I don't think that helps us here. requestAnimationFrame executes the callback before the next paint. In an animation loop using requestAnimationFrame, you would queue up your next requestAnimationFrame from within your callback. This means that effectively your your callback interval matches your paint cycle. As that slows, so does your interval.
However, simply adding a call in this callback to requestAnimationFrame does nothing for us, because the interval is a set 40ms by OBS. Our callback will be called every 40ms by OBS regardless of the paint speed. requestAnimationFrame would not save us any work because we would be using it incorrectly.
A potential solution could be to separate the OBS callback and the render callback. So we simply store the latest data on the frontend, and then asynchronously run a traditional requestAnimationFrame loop that picks up that data and draws it with WebGL. However I'm skeptical that would actual cause any tangible benefit in CPU usage.
3 participants
We need to put this through a battery of benchmarks, but it looks like this could save us a bunch of CPU.