-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Use ImageBitmap when possible #1131
base: master
Are you sure you want to change the base?
Conversation
Bug report for Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=877083 |
ecd9bfe
to
0addbca
Compare
Is this still a viable PR? |
I believe we're waiting for a bugfix in Chrome. In a browser that works correctly, this is an improvement. |
@CendioOssman Are there any plans to merge this? I've tried it with up-to-date code here godsic@5f0f226 and it indeed reduces |
Eventually. Would need to re-benchmark it and make sure it isn't causing any regression on one of the browsers. |
@CendioOssman Good, looking forward to. In the meantime, I've properly integrated that promised image into rendering queue, see godsic@2300bb6 . So now it renders correctly. |
what is the status of this PR? |
Needs a retest to see how it performs on current browsers. Might still be a problem on some browsers. |
@vbabenko You can give it a try at https://github.com/godsic/noVNC . |
Can this be rebased off of current |
The render queue doesn't need to know how to wait for images to load.
Otherwise each element isn't considered a single byte. This breaks conversion to other types, e.g. Blob.
These are much faster than Image object in some browsers, so prefer them when possible.
0addbca
to
8804b91
Compare
Sure. It's now rebased. |
I have been playing with this patch and was pleased with the performance improvement. also forced img.close(); in drawImage and a.img = null; in _scanRenderQ() but not sure it impacted the GC |
I was also playing around with this (not knowing of this pull) and implemented some other changes as well. I've created a gist with the changes I made to display.js you can find here: https://gist.github.com/Mr-Shibari/70dfd6a576a3bbbd060833982e11a9cc It works great in my tests but I observe the same issue with the Firefox garbage collector causing janks (sudden frame drops) as symbalis did. The problem seems to be caused by createImageBitmap not releasing all resources, even after calling bitmap.close() on the generated bitmap causing the GC to kick in periodically. Not calling createImageBitmap solves this so it has te be that function causing it. |
These are much faster than Image object in some browsers, so prefer them when possible.
Found this whilst profiling #1108. Noticed that we were spending a lot of time base64 encoding images and drawing them. So I tried using the newer ImageBitmap objects instead.
This gives a massive performance increase in Firefox (2.7 seconds instead of 5 seconds for my test recording). Unfortunately it seems Chrome has screwed something up here and I get a performance regression there (8 seconds vs 5.5 seconds before).
More investigation is needed, but it looks like it is something worth pursuing.