opensheetmusicdisplay / opensheetmusicdisplay

OpenSheetMusicDisplay renders sheet music in MusicXML format in your web browser based on VexFlow. OSMD is brought to you by PhonicScore.com.
https://opensheetmusicdisplay.org
BSD 3-Clause "New" or "Revised" License
1.46k stars 289 forks source link

Support for workers #396

Open ritz078 opened 6 years ago

ritz078 commented 6 years ago

tl;dr;

With the introduction of OffscreenCanvas API, we can now control canvas from the workers. This will result in faster rendering since the canvas is now moved to a worker. I have already been doing music visualization using the same concept and I really like the following flow:

I think technically the library just needs the file content and the canvas context. Currently, it asks for container element (which is not accessible in the worker) and due to that, I can't use it in the worker.

A lot of music libraries now have also started accepting ArrayBuffers for performance reasons.

bneumann commented 6 years ago

This sound very promising. How is the browser support? We could test this with our internal drawing stuff like lyrics and labels in general but for vexflow we would need to implement a specific offscreen canvas backend. Which is not impossible but has to be done in the vexflow repo

Update: I was just checking myself and that feature, although pretty cool and basically what we need, is still in experimental phase. So I would recommend to reject this for now and check on that later.

sschmidTU commented 6 years ago

This would be nice to have. But I'm not sure VexFlow (and by extension OSMD) can work with an OffscreenCanvas, that is, without an HTMLElement.

We need to create a Vex.Flow.Renderer, which needs an HTMLElement. Specifically, in Vexflow's Renderer.js, it does this:

this.element = document.getElementById(elementId);

Since the OffscreenCanvas works outside of the DOM, it seems like it can't provide an HTMLElement.

By the way, are there Typescript definition files for OffscreenCanvas? Doesn't look like it. Though there aren't too many methods and attributes to write into a custom definition file, judging from the API.

ritz078 commented 6 years ago

So I would recommend to reject this for now and check on that later.

Agreed. The browser support is limited. safari has already implemented it behind a flag and it's live on Chrome and Firefox though.

Since the OffscreenCanvas works outside of the DOM, it seems like it can't provide an HTMLElement.

I don't think you need to make a lot of changes. vexflow eventually extracts out the context from the element.

https://github.com/0xfe/vexflow/blob/6e13b8437107f6e85c03e6665667e2bafbdbe179/src/renderer.js#L141

So the only thing you need to do (from the code I can see) is accept canvas context and assign it directly rather than extracting it from the element. It will automatically be supported for web workers for canvas.

Though there aren't too many methods and attributes to write into a custom definition file, judging from the API.

You just need to add . transferControlToOffscreen. You won't need anything else. but I don't think this should be done in the repo. It's on the user how he/she wants to use it. If we just provide them a library that just works by accepting canvas context and arrayBuffer/string data, it can be used anywhere.

sschmidTU commented 6 years ago

Vexflow always looks for an HTML Element, so the Renderer constructor won't work without it. That's the line of code from my last comment. So we do have to change Vexflow if we want to accept a canvas context instead of HTML Element.

ritz078 commented 6 years ago

Yes, vexflow definitely needs modification on that part. I guess even if we leave the web worker part aside, accepting a canvas context is still an improvement.

sschmidTU commented 6 years ago

Vexflow's SVGContext also needs an HTMLElement right now. Seems like removing Vexflow's dependency on HTMLElements is not so easy.

@ritz078 If you could make a PR for Vexflow to remove the dependency on HTMLElements and make constructors optionally accept a Canvas Context (without making all other Vexflow users change how they use the constructors), that would be great. I could do it, but i haven't used OffscreenCanvas so it wouldn't be easy for me to test, and i'm working on other features right now. Once Vexflow doesn't rely on HTMLElements anymore, the same change to OSMD should be easy.

ritz078 commented 6 years ago

I will.

sschmidTU commented 6 years ago

That would be great, thanks!

ritz078 commented 6 years ago

I am still waiting for them to comment on my issue so that I can go ahead and open a PR.

sschmidTU commented 6 years ago

From my experience, they tend to react more to working PRs (though lately they've actually also been responding to issues a lot). Maybe you could just open the PR. But if the time isn't pressing, and for us it isn't, i guess it doesn't hurt to wait.

sschmidTU commented 5 years ago

People keep asking for Web worker support on Vexflow as well: https://github.com/0xfe/vexflow/issues/655

We also regularly get emails about it.

Someone should do it some day ;)

blueCat11 commented 5 years ago

Is there any workaround to get workers (or something else) to render music asynchronously? I have huge lags when rendering longer music pieces, and in production, the process gets terminated, so nothing is rendered at all.

sschmidTU commented 5 years ago

Can't you run OSMD asynchronously with a Promise (or task)? Here are some resources: https://glebbahmutov.com/blog/difference-between-promise-and-task/ https://medium.com/techtrument/multithreading-javascript-46156179cf9a https://codeburst.io/why-web-workers-make-javascript-more-than-a-single-thread-3d489ffad502

Unfortunately, the processing time is mainly in Vexflow draw calls, not in OSMD directly. btw, there's an old issue for performance, #352. though it doesn't have much info.

Maybe there is a performance parameter we can set in Vexflow via an option. I tried changing Flow.RESOLUTION: 16384 to 1024 in tables.js (in node_modules/vexflow/releases/vexflow-debug.js), but it didn't seem to impact performance.

Theoretically, you can split up the XML into smaller parts and render them one after another. In future, besides DrawUpToMeasureNumber: x we will have an OSMDOption DrawMeasures: [x,y], so it's easier to render only part of a score at a time.

So your options seem to be finding a performance parameter in Vexflow, implementing Worker support in Vexflow and/or OSMD, or splitting up the score and rendering parts of it at a time, maybe as an addition to OSMD (#482).

blueCat11 commented 5 years ago

Thanks for your quick response and many suggestions! I'm going to look into all those and see if I can get something to work.

turnerhayes commented 5 years ago

It looks like VexFlow supports OffscreenCanvas (see https://github.com/0xfe/vexflow/issues/655#issuecomment-493502770). It also sort of supports being run in Worker threads (if you hack in a global window variable). OSMD still needs a backend that supports just passing in an OffscreenCanvas or OffscreenCanvas2DRenderingContext, and to stop requiring a container div in that case. I tried hacking such a backend together, but there are other parts of the code that assume it's running in the main thread (for instance, the Cursor class calls document.createElement()) and so presumably there'd need to be a good deal of refactoring to get features that depend on being run in the DOM working. The better approach would probably to create an OSMD instance in the main thread and then have OSMD spin off worker threads for drawing.

bneumann commented 5 years ago

Pretty good analysis. I thought about splitting too but our code is not really split friendly. If I would rewrite it I'd put an drawing interface which can also somehow send drawing tasks to a worker. It's feasible but a lot of work. Unfortunately we are all pretty bound by our own projects right now.

sschmidTU commented 1 year ago

but there are other parts of the code that assume it's running in the main thread (for instance, the Cursor class calls document.createElement())

Just mentioning that the cursor shouldn't be an issue here, as it's a very simple class that's easy to adapt.