Skip to content
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

Use a hidden canvas in memory to paint to avoid jank #72

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ankeetmaini
Copy link
Collaborator

Screenshot 2019-10-02 at 10 32 40 PM

So I played with this app a bit. To support OffScreenCanvas, I tried using an in-memory canvas (not drawn on the main screen) to draw and copy it to main screen. I think it mostly works, but somehow I see long continuous bars and not exact notes. I'm guessing it's not able to reset the buffer canvas. Attached an image.

Will update this.

@vercel
Copy link

vercel bot commented Oct 2, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/ritz078/synth/fh28wueo6
🌍 Preview: https://synth-git-fork-ankeetmaini-no-offscreen.ritz078.now.sh

@ankeetmaini
Copy link
Collaborator Author

I think it's okay now, let me know when you've sometime to look at this, I'll remove the hardcodings to always force into non offscreencanvas support.

// TODO: undo the ternary; not wrapping via Comlink to test offcanvas perf
// on latest chrome, too lazy to install anything else
const canvasProxyRef = useRef<any>(controlVisualizer);
// offScreenCanvasSupport === OFFSCREEN_2D_CANVAS_SUPPORT.SUPPORTED
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this, once you test the perf :P

offScreenCanvasSupport === OFFSCREEN_2D_CANVAS_SUPPORT.SUPPORTED
? canvasRef.current.transferControlToOffscreen()
: canvasRef.current;
const canvas: any = false //isOffscreenSupported
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this too

@ankeetmaini ankeetmaini changed the title [WIP] Use a hidden canvas in memory to paint to avoid jank Use a hidden canvas in memory to paint to avoid jank Oct 2, 2019
hiddenCanvasElement.height
);
// I think this is too agressive, for later, what's Clock?
requestAnimationFrame(paintToMainCanvas);
Copy link
Owner

@ritz078 ritz078 Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint.

The problem here is that we don't have that much time that we can wait till the next repaint. This will make the animation slow and hence out of sync. canvas doesn't trigger repaint and that's why they are faster so if we use requestAnimationFrame which actually depends on the paint timing, we are actually going back to the same constraints that div/svg have.

In this case setTimeout will give us better result but then again all the limitations of main thread will come into play which will eventually cause the delay.

I tested this PR and that confirms what I wrote above. You can open the PR deployment URL

@ritz078
Copy link
Owner

ritz078 commented Oct 2, 2019

You can check the prod build of this PR at https://synth-git-fork-ankeetmaini-no-offscreen.ritz078.now.sh

Let me know if you are not able to open it.

In case you had trouble on opening the app on browsers other than chrome, take a master pull.

hiddenCanvasElement.width,
hiddenCanvasElement.height
);
// I think this is too agressive, for later, what's Clock?
Copy link
Owner

@ritz078 ritz078 Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clock keeps track of time and progress of the song. It's separate because it provides better API that way. For eg. when you need to pause and song on canvas, you just have to stop that clock. So it's king of virtual clock which is in our control.

  • If we stop it, the canvas pauses because the progress (it's a number between 0-1 which depicts the song's progress) doesn't increase.
  • If we change the speed, the clock's speed should be increased.
  • If we skip to a particular position, the clock skips to that note's time.

Eventually based on all these metrics, it provide the song progress which is used by Visualizer utility to control the visualizations.

So if progress is moving faster, the canvas will be fast. If progress is stopped, the canvas remains at that position etc.

You can assume that the Clock actually is the time machine for the canvas which we can control.

@ritz078
Copy link
Owner

ritz078 commented Oct 3, 2019

Our first priority here is making this work on the main thread but in case that doesn't work (highly likely), I think that we can use the same logic in a worker and render on the main canvas after doing the calculations on a offscreen canvas. Might improve the performance even further.

For that we can create a OffscreenCanvas in the worker and do all the rendering on it. after everything for a single frame is done, we can draw the image on the main canvas (whose access we already have in the canvas).

@ankeetmaini
Copy link
Collaborator Author

I did some profiling, and I think main thread is busy because of something in Keyboard.ts, the profile showed that the browser is doing a lot in terms of recalc and layout too. Will do a deep dive this weekend.

@ankeetmaini
Copy link
Collaborator Author

I'm trying improve the profile perf on this with some trial & error experiments. I'll let you know when this is ready for review.

@ritz078 ritz078 added the WIP label Oct 8, 2019
@ritz078
Copy link
Owner

ritz078 commented Oct 8, 2019

sure, take your time.

> running for the first time

```
yarn build:midi
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants