terminalizer

terminalizer copied to clipboard

Fixes https://github.com/faressoft/terminalizer/issues/96 - sometimes rendering fails when GPU is used. This update first tries rendering with GPU, and if that fails it retries with disabled GPU.

This is a workaround for electron issue. If you know a way to detect if this error would happen before the actual electron invocation, please let me know.

@maricn Just skimmed through your PR Here are a few things I noticed; • After recreating the rendering process, you don't attach the event listeners to it. • So there's no telling whether or not that process failed or succeeded. • The promise never properly resolves from this not rejects from a caught error. • Date.now() is analogous to (new Date()).getTime() but with fewer CPU cycles, it's about twice as fast jsperf.com/new-date-gettime-vs-date-now. • Also, you should call the kill() method on the legacy render process like in the original script. • Don't forget to call that on yours too.

Could you maybe detect the OpenGL error GL_INVALID_OPERATION : glBufferData and only then, respond to that by recreating the process without GPU.

Because the process could've caught a non-GPU related error but would still retry in spite of that. (this could be critical)

@miraclx Thanks for your review and constructive comments. I noticed the last two but was lazy to fix them, and they shouldn't affect the operation "critically", I believe. And I didn't check the first two, but they worked on my machine. So, here's a revised update.

In the following commit I think I've addressed all the points. Let me know if you think that's better. Also, that's the only reasonable way I've found on checking for the specific error. Would be better if we could preemptively check if error would occur, but this should suffice.

Honestly, I am not happy with these spaghetti promises, but I am not very experienced with this coding style and I tried to keep it analogous to the rest of the code.. Hope it's readable. I'm happy to hear further comments.

Looks good to me.

Here's a couple of extra suggestions to this; • https://github.com/faressoft/terminalizer/pull/97/commits/ee30de6339e3d9b150f64dd9189d182cde7242cf#r419170961 • https://github.com/faressoft/terminalizer/pull/97/commits/ee30de6339e3d9b150f64dd9189d182cde7242cf#r419171099 • And maybe time the merger process too

Here's another idea, maybe we shouldn't act on data streamed into stderr immediately. These could be mere warnings. Like the one about the GPU.

(yet to be tested) but I don't think that GPU warning would've caused the rendering to fail. Chromium would've fallen back to skipping the use of a GPU. It's the method for handling data from error streams that infers a failure.

Best to let the process run it's course and only after can we check that we have a non-zero return code and promptly reject the rendering promise.

Here's another idea, maybe we shouldn't act on data streamed into stderr immediately. These could be mere warnings. Like the one about the GPU.

(yet to be tested) but I don't think that GPU warning would've caused the rendering to fail. Chromium would've fallen back to skipping the use of a GPU. It's the method for handling data from error streams that infers a failure.

Best to let the process run it's course and only after can we check that we have a non-zero return code and promptly reject the rendering promise.

The problem with that is that I'm not sure that electron would return non-zero exit code.

But, you're right, instead of this solution, we can just not kill the render process whenever something pops up on stderr. The rendering will continue and finish successfully.

Also, that can fix the anti-pattern of tracking rendering success through its UI component - progress bar.

Nice work so far @maricn.

Thanks @maricn for taking the time to fix this. Lots of Debian users on the 4.x kernel will now be able to use this package.

when do you think this can get merged?

@faressoft , can you kindly consider merging this PR ?

@faressoft Could you please merge this PR?

please... merge this.. (fedora)

Why is this just sitting here?

Hi, using this PR does not fail on the warning on my system, but now hangs on it. I have little experience with either terminalizer or the environments it uses, so please excuse my ignorance and feel free to pester me for specific information.

The warning:

[render] [16189:1107/101325.856888:ERROR:buffer_manager.cc(488)] [.DisplayCompositor]GL ERROR :GL_INVALID_OPERATION : glBufferData: <- error from previous GL command

Some system details:

OS: Ubuntu 20.04.1 LTS x86_64 
Kernel: 5.4.0-52-generic 
Uptime: 2 hours, 54 mins 
Packages: 2788 (dpkg), 15 (snap) 
Shell: fish 3.1.2 
Resolution: 1920x1080 
DE: GNOME 
WM: Mutter 
WM Theme: Adwaita 
Theme: Yaru [GTK2/3] 
Icons: Yaru [GTK2/3] 
Terminal: gnome-terminal 
CPU: Intel Pentium G3258 (2) @ 3.200GHz 
GPU: AMD ATI Radeon HD 7770/8760 / R7 250X 
Memory: 3334MiB / 7900MiB 

Tool	Version
npm	6.14.8
electron	10.1.5
node	14.15.0

@Honno, Is this the same behavior with master?

If so, this isn't a bug with this PR

@miraclx Nope, sorry should of clarified.

On master, render just exits straight away.

Error: 
  Error: [43288:1108/213958.894749:ERROR:buffer_manager.cc(488)] [.DisplayCompositor]GL ERROR :GL_INVALID_OPERATION : glBufferData: <- error from previous GL command


Hint:
  Use the --help option to get help about the usage

On this PR, it prints the error (as expected), but just hangs on my machine.

[render] [16189:1107/101325.856888:ERROR:buffer_manager.cc(488)] [.DisplayCompositor]GL ERROR :GL_INVALID_OPERATION : glBufferData: <- error from previous GL command

Hi, I can't seem to be able to reproduce this

Could you perhaps insert log lines within the following methods https://github.com/faressoft/terminalizer/blob/d2d5ff1c23c4ecf785b29435335b73515d83c2db/commands/render.js#L141 https://github.com/faressoft/terminalizer/blob/d2d5ff1c23c4ecf785b29435335b73515d83c2db/commands/render.js#L130 https://github.com/faressoft/terminalizer/blob/d2d5ff1c23c4ecf785b29435335b73515d83c2db/commands/render.js#L153 To sort of understand what statements the control flows through?

Also, try logging out the path to the electron binary the render process spawns and test-launching that manually.

At this stage, I can speculatively infer that the onClose method isn't called because the spawned child-process doesn't exit, so it's more likely a problem with electron.

@miraclx Logging the three functions in render.js, the only method to trigger is onError() (once with the GL error).

I tried launching the path manually with electron , but nothing happened, either before or after execution of render (I just used electron <path>, if that's what you meant).

Log the di.electron value here to get the binary path https://github.com/faressoft/terminalizer/blob/d2d5ff1c23c4ecf785b29435335b73515d83c2db/commands/render.js#L124

Once you have that, manually launch it from a terminal

It should launch an electron demo window

If not, this is a problem with electron

Perhaps an update is in order

Ah I see thanks. The di.electron path does indeed launch, with a familiar warning.

[98337:1111/133530.962570:ERROR:buffer_manager.cc(488)] [.DisplayCompositor]GL ERROR :GL_INVALID_OPERATION : glBufferData: <- error from previous GL command

~~Is this package just abandoned now?~~ Shame that such a critical PR would be left hanging.

RIP all the time I spent just getting it to install properly on node v12. At least the play function works... Guess I'll just have to use LICEcap to make a gif.

@teauxfu, you could just use https://github.com/asciinema/asciinema to create asciicasts and then proceed to using https://github.com/asciinema/asciicast2gif to make the render to gif.

I apologize for the delay in following up with the pull requests. I will merge it as soon as the conflicts are resolved.

@maricn @miraclx

@faressoft thank you for the follow up, i resolved the conflicts but building locally doesn't work for me unless i clean up the package-json.lock and update css-loader to 4.3.0 or later (where they support webpack 5).. is there another PR to fix the build system?

I created a PR for the dependencies (https://github.com/faressoft/terminalizer/pull/225), but feel free to reject that if for whatever reason i missed the point.

@maricn I don't believe there is a PR to fix the build system. Does your other PR fixes it? Have you tested it manually?

@maricn Thanks for your contribution. Your fix is tested and released with v0.10.0