jean-emmanuel / open-stage-control

Libre and modular OSC / MIDI controller
https://openstagecontrol.ammd.net
GNU General Public License v3.0
720 stars 91 forks source link

[Bug] frame widget becomes blank sometimes #589

Closed cyberic99 closed 4 years ago

cyberic99 commented 4 years ago

version: git 89afc65, on linux debian.

I have a frame widget containing a logo. it has some (quite heavy) css to add some animation to the picture.

When switching between tabs, the frame content sometimes vanishes.

I tried to capture this the best I could in this video:

osc_frame_blank

It seems it happens with images too sometimes, but I am not sure

thx

jean-emmanuel commented 4 years ago

The browser reloads the iframe everytime the tab is opened, and the frame's url may not respond. While the 1st problem is indeed related to o-s-c, I can't think of anything that could fix it : closed tabs are detached from the document for performance reasons and that's triggering the iframe reloading.

jean-emmanuel commented 4 years ago

closed tabs are detached from the document for performance reasons

This choice was made a long time ago (v0.4.0), I'll need to test if it's still relevant, if it's not then reverting it would fix the issue.

jean-emmanuel commented 4 years ago

Would you mind trying 483a991 with your session and tell me if you see a change in performances (loading time, interaction, tab switching...) ? If you have a mobile device I'm interested in the results too.

cyberic99 commented 4 years ago

I tested 58aa2f3048da50fb762b141faa00632a233fa148

The tab switching seems faster and I couldn't reproduce my problem.

However, the CPU usage raised, from minimum +5% to +25%, when I display the tab with 'intensive' graphics...

I know it might be a cause of the electron upgrade and you can do nothing about it...

My video card is very old too, but at comit 1c069188734c62b4b03bac65313d464f2e14221c (0.48.8), the cpu usage was capped at 50% max

now it goes from 55 to 75%

I can retest with an older commit if you want, to see if it was caused by electron upgrade or not.

Also, I was just using top, if you want me to test other tools I can do that too.

thx

jean-emmanuel commented 4 years ago

Did you try with --disable-vsync or --disable-gpu ?

jean-emmanuel commented 4 years ago

I just added the --force-gpu switch that may be more relevant in your case, can you try it ?

cyberic99 commented 4 years ago

OK I'll test it. thanks.

do I have a way to see in the developer tools or elesewhere, if my GPU is blacklisted or not?

cyberic99 commented 4 years ago

commit 1c069188734c62b4b03bac65313d464f2e14221c, v0.48.8: 1 electron process using 60% cpu, the other 15%

commit 0e4ba29dd9f3094be8850ecca4e55093ae26edb8, after 0.49.0-rc2: 1 electron process using 80% cpu, the other 30%

--force-gpu doesn't change anything.

jean-emmanuel commented 4 years ago

Could you try 0e4ba29 and alter the electron version manually in package.json from 7.1.0 to 1.8.8 (and rerun npm install && npm run build) ? edit: thanks for testing

cyberic99 commented 4 years ago

(just to give some news, I tested the same session on core-i7-8550U, which is far more powerful and have a faster video card than my home PC.

I get the same CPU load.

But maybe I am using the css properties too heavily...

So the graphic card perf does not seem to make a difference.

About the test:

thanks to you for spending time on this.

Unfortunately my nodejs skills are close to 0, so I ran into difficulties when downgrading electron:

I did that:

$ git diff

diff --git a/package.json b/package.json
index 5ed1126d..74b4a3b4 100644
--- a/package.json
+++ b/package.json
@@ -69,7 +69,7 @@
     "yargs": "15.1.0"
   },
   "optionalDependencies": {
-    "electron": "7.1.10",
+    "electron": "1.8.8",
     "electron-installer-debian": "3.0.0"
   },
   "scripts": {

Unfortunately i seems that it somehow also downgraded the node version??

I get this error:

App threw an error during load
open-stage-control-server.js:9377
        const foundPath = await runMatcher({...options, cwd: directory});
                                            ^^^

SyntaxError: Unexpected token ...
    at createScript (vm.js:74:10)
    at Object.runInThisContext (vm.js:116:10)
    at Module._compile (module.js:533:28)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (open-stage-control-old-electron/app/index.js:1:156)
A JavaScript error occurred in the main process
Uncaught Exception:
app/server/open-stage-control-server.js:9377
                const foundPath = await runMatcher({...options, cwd: directory});
                                                    ^^^

It seems caused by this module:

./node_modules/find-up/index.js:        const foundPath = await runMatcher({...options, cwd: directory});

I can also go back to https://github.com/jean-emmanuel/open-stage-control/commit/e5a696bbcf0d8cb90f5003821a406941d497dcd9

before upgrading the deps versions, and try to apply these patches:

https://github.com/jean-emmanuel/open-stage-control/commit/c3b9563adf079e78c7b8953fa6d86698c67e17c1 https://github.com/jean-emmanuel/open-stage-control/commit/699078e089093fbb2ec15942118a3d6525072b13

?

What I can do too is to make a simplified session that you can run on your own, and provide a client in python to trigger the graphics randomly.

Would you be interested in that?

thx for your support!

jean-emmanuel commented 4 years ago

tested the same session on core-i7-8550U [...] I get the same CPU load.

That's weird, maybe there is a css animation that's rendered differently in recent chromes. Have you tried removing css animations ?

I ran into difficulties when downgrading electron:

A dependency was causing this compatibility issue (now fixed). For testing purpose I just pushed another commit that lets you choosing whether a tab is hidden or detached when inactive. Could you pull the latest commit and compare the following cases with both electron 1.8.8 and 7.1.0 ?

jean-emmanuel commented 4 years ago

Just in case, make sure the debug option is disabled.

cyberic99 commented 4 years ago

Have you tried removing css animations ? I think that even attached, when the tab was hidden, the CPU would go down to 10% usage max

I'll retest all this, and try to see if attached or detached makes a difference

thx!

jean-emmanuel commented 4 years ago

tested the same session on core-i7-8550U [...] I get the same CPU load.

By the way I just remembered 100% in in top means "100% of 1 cpu core" so it's not that bad...

cyberic99 commented 4 years ago

By the way I just remembered 100% in in top means "100% of 1 cpu core"

Yes that's it

so it's not that bad...

I don't know... I am sending updates to 8 faders at 25fps

I would understand if the video card was the bottleneck, but the cpu. really? (even 1 core)

also, when switching to another tab, the cpu usages comes back to 10pct !

I'll try to see what happens with a web browser instead of electron...

also, maybe it is too hackish to work well... I am using a timestamp to create a new css animations each time an OSC event is received

And I limited my animation to 0.3s with 8 steps...

But... I changed nothing about these animations. and it seems that the latest electron (or other changes to o-s-c) did increate the cpu usage quite a bit...

Anyway, I keep you updated

(my ultimate goal it to have something like that: https://nightride.fm/?station=nightride#EQ, (the bars at the bottom))

I already have something very close, but it is not that fast

jean-emmanuel commented 4 years ago

I am sending updates to 8 faders at 25fps

If that's so you don't need any css animation ! Either send frequent update without transition or intermittent updates with transitions.

jean-emmanuel commented 4 years ago

But... I changed nothing about these animations. and it seems that the latest electron (or other changes to o-s-c) did increate the cpu usage quite a bit...

As I said the rendering pipeline may have changed in a way that's less favorable in your case.

jean-emmanuel commented 4 years ago

I just reverted a change in canvas rendering that was only affecting recent chrome versions, maybe you'll get better perfs with the latest commit (nothing to do with css animations though).

cyberic99 commented 4 years ago

If that's so you don't need any css animation ! Either send frequent update without transition or intermittent updates with transitions.

yes, at this rate you're right.

But the advantage is that I can fire my event and forget it, the bar will slowly go down on its own

And even if I lower the event rate, the animation will still look smooth

After these tests I'll try to see if disabling css animations lower the cpu usage

I just reverted a change in canvas rendering that was only affecting recent chrome versions, maybe you'll get better perfs with the latest commit (nothing to do with css animations though).

Thank you.

So to recap, what do you want me to test @jean-emmanuel ?

is this still this:

I should disable --debug, but should I use some specific flags?

jean-emmanuel commented 4 years ago

Just test the latest commit with electron 1.8.8 (manual downgrade) and 7.1.0, if the performance hit occurs only when you're on the heavy tab, fiddling with the detached option won't make a big difference.

jean-emmanuel commented 4 years ago

What I can do too is to make a simplified session that you can run on your own, and provide a client in python to trigger the graphics randomly.

Also this could help me track the issue yes

cyberic99 commented 4 years ago

It seems that your latest commit did mitigate the perf issues I was experiencing,

I'll try to prepare a kind of repro case, and I'll also investigate other ways of representing the bars

I think you can close this issue for now, thank you very much!

jean-emmanuel commented 4 years ago

Great !!

cyberic99 commented 4 years ago

To be perfectly clear, there is still a cpu usage difference between the older and newest version of electron. But the difference is smaller thanks to your latest commit. What I noticed too is that disabling css animation does improve the situation, but not that much.

It seems that the cpu spends time in drawing the faders (they are compact, no pip)

Also, I tried your suggestion of updating the values to handle the 'fall' of each band with OSC instead of css.

it works, but it is slower than a CSS solution, it seems less smooth and uses more CPU.

I'll try to replace the faders with a text widget and some animated ASCII art ;-)

hence I might ask some questions about them in the forum ;-)

I'll also try to send you a kind of OSC dump and a replayer, to repro my setup.

Thx for your time!

jean-emmanuel commented 4 years ago

I'll try to replace the faders with a text widget and some animated ASCII art ;-)

This can be pretty cpu intensive too actually, you might want to try that with the input widget instead that relies on the canvas for drawing.

cyberic99 commented 4 years ago

You mean a text or html widget will be more intensive than an input widget?

jean-emmanuel commented 4 years ago

Depending on the content, it's possible yes.

cyberic99 commented 4 years ago

Ok. I'll tell you updated on the discourse.

So about the original issue, will you keep the 'detach' as an option?

jean-emmanuel commented 4 years ago

I'm not sure yet. If perfs are not affected by this change I'd rather set it to false and hide it, but I need confirmation on mobile devices.

jean-emmanuel commented 4 years ago

To be perfectly clear, there is still a cpu usage difference between the older and newest version of electron

Unfortunately I'll probably stick with this change, keeping compatibility from chrome 50 to 78+ may become complicated at some point, plus in some cases things seem to perform better.

cyberic99 commented 4 years ago

Yes I understand that, of course.

breaking changes seem usual when it comes to Nodejs stuff, it seems (I do not speak about o-s-c here), and the web evolves at a fast pace, so keeping up to date seems mandatory...

I must say that with o-s-c you have a real policy, and session files never break on a newest version.

I know that not breaking existing setup/beheaviours is hard, but o-s-c does it well, so thank you for the extra effort you put in this.

I'll do some test on an android tablet next week or a bit after that

jean-emmanuel commented 4 years ago

breaking changes seem usual when it comes to Nodejs stuff

It seems so unfortunately, that's a real problem because I don't intend to increase the system requirements as o-s-c evolves !

cyberic99 commented 4 years ago

Yeah it is a problem, for sure.

The only thing you can do is to try to reduce the number of dependancies or fork (and maintain) them, but it adds a lot of work...

jean-emmanuel commented 4 years ago

v0.49.0 released with detached property to true by default to avoid edge effects.