mpetroff / pannellum

Pannellum is a lightweight, free, and open source panorama viewer for the web.
https://pannellum.org/
MIT License
4.23k stars 722 forks source link

Performance issue with dynamic (still canvas image as source that rarely changes) and texImage2D #1135

Open bartzy opened 1 year ago

bartzy commented 1 year ago

Hi,

I have a performance issue with dynamic content that I think I narrowed down to an unnecessary texImage2D call here: https://github.com/mpetroff/pannellum/blob/master/src/js/libpannellum.js#L851

I'll explain my scenario: I have a still image that changes only very rarely. When it changes, I want to inform Pannellum to update. To do that, I'm giving pannellum a canvas that I control, I pass dynamic: true in the config, and whenever I (rarely) want to update, I call viewer.setUpdate(false). In desktop Chrome, this works very well. In iOS Safari, the performance when interacting with Pannellum (moving in the image) is pretty bad (10-15FPS). I narrowed this down to the texImage2D call happening on each frame.

This is weird to me - Why would Pannellum upload the image (it's the same one) on each frame?

Am I using the dynamic flag wrong in some way?

Thanks! Bar

mpetroff commented 1 year ago

You're using viewer.setUpdate incorrectly. With viewer.setUpdate(true), it updates every frame, which is required for, e.g., video, while with viewer.setUpdate(false) it does not update the texture and only renders frames when the viewer is panned / zoomed.

However, you're correct about the implementation being non-optimal (the dynamic update functionality was written to support video, but I tried to make it more generic). While setUpdate(false) prevents the viewer from rendering frames while idle, it did not stop the renderer from reuploading the texture every frame. I changed this in 6c639ae033272912bfe9ed3df2e510ea1b222e2b.

bartzy commented 1 year ago

Thanks!

I think I'm using viewer.setUpdate correctly - I don't want it to update every frame, just when I call viewer.setUpdate(false) explicitly (my source is a canvas that I rarely change. When I change it - I can call setUpdate). Should I change anything?

With your change in https://github.com/mpetroff/pannellum/commit/6c639ae033272912bfe9ed3df2e510ea1b222e2b, should I now call viewer.setUpdate(true) so update will be true for a single render, the new image will be uploaded to the GPU, and then call viewer.setUpdate(false) to turn update to false again so it won't render on every frame?

mpetroff commented 1 year ago

With your change in https://github.com/mpetroff/pannellum/commit/6c639ae033272912bfe9ed3df2e510ea1b222e2b, should I now call viewer.setUpdate(true) so update will be true for a single render, the new image will be uploaded to the GPU, and then call viewer.setUpdate(false) to turn update to false again so it won't render on every frame?

Yes, but that is also what you should have been doing before the change.

bartzy commented 1 year ago

Why is that what I should have been doing all along? I just want to tell Pannellum "Please take a new image from my source", but only once.

How should I turn off setUpdate(true)?

// let's say I have myCanvas which is what I give pannellum as my image
// I then rarely do this, let's say on a button click or whatever:

changeSomethingInMyCanvas()
viewer.setUpdate(true) // I want pannellum to take a new image from my canvas
// How/when do I call `viewer.setUpdate(false)`? in my own rAF? Something else?

Edit: Just to add, I think I understand why I used setUpdate(false). It's because it calls animateInit, so I used this as "please render now" and it worked. Calling setUpdate(false) to just mark update = false internally is not only doing that. It will also render one more time, which is a bit of a waste (I agree it's a very small cost)

mpetroff commented 1 year ago

Okay, yes, setUpdate(false) previously worked but only because it wasn't optimized and caused another frame to be rendered.

I just added a new updateOnce() function in 204241e7152c6b848353f380ef2fd4d47e6e88e6, which will cause the texture to be updated in the next frame and only the next frame. I think this will do what you need.

I also optimized setUpdate(false) to not render a new frame.

bartzy commented 1 year ago

@mpetroff Nice, thank you! I will try this out today and let you know if there are any issues.

bartzy commented 1 year ago

@mpetroff I'm trying to use viewer.updateOnce (without ever calling/using setUpdate), and I think there's an issue with it: viewer.updateOnce sets updateOnce to true, and then immediately calls viewer.setUpdate(true), which in turn immediately sets updateOnce to false. I saw you made this addition in https://github.com/mpetroff/pannellum/commit/3aab61fd2abe5f9d0109e6dafb21929a5b1d90bb, what bug did you encounter?

mpetroff commented 1 year ago

I didn't encounter a bug, but I did realize that if setUpdate(true) was called immediately after updateOnce(), it might not stick, and the panorama might only be updated once. I obviously didn't retest the updateOnce() function after making the change. I made further changes in 3d271c83d5dc4bf19c9ca7ba8ca80da554bf5cf6, which should resolve this.

bartzy commented 1 year ago

@mpetroff Thanks! I think updateOnce is still not working, because it doesn't set the update flag, only updateOnce. So animate doesn't even call requestAnimationFrame, and dynamic: update is false when calling renderer.render. Right?

mpetroff commented 1 year ago

Yes, you're absolutely right. I added that now, so hopefully it works.

I apologize for the lack of testing of the changes on my part. I'm currently working in Antarctica with only a couple hours of very slow internet access a day (a geostationary satellite connection shared with dozens of other people).