meshopt_encodeVertex* variants with version arguments

[ondys]

trafficstars

While encoding / decoding works fine in multi-threaded environments, one issue we found is that vertex / index codecs are using global variables for bitstream versioning, namely gEncodeVertexVersion and gEncodeIndexVersion. These variables are then used in the main encoding functions such as meshopt_encodeVertexBuffer or meshopt_encodeIndexBuffer which makes these calls harder to parallelize when an explicit bitstream version is requested via meshopt_encodeVertexVersion or meshopt_encodeIndexVersion. Currently, these functions can be called only once otherwise data races may occur (in a multi-threaded environment), which requires either explicit synchronization between threads or initialization before the threading starts which is often not feasible. A simple solution would be to provide new overloads for meshopt_encodeVertexBuffer or meshopt_encodeIndexBuffer that would take the version as an input which would remove all dependencies on the shared global variables.

[zeux]

The intent here is that you really don't need to change these except for once at startup, just like the memory allocation function.

[zeux]

... in fact it's really time to change the defaults so that you just don't need to call these functions in general unless the application is doing something incredibly specific.

[ondys]

I understand the intent but in practice it complicates deployment on multi-threaded systems where the initialization step is pretty much independent on the worker threads. If the initialization happens when the workers are already running an explicit synchronization is needed. Of course the situation is solvable but it just unnecessarily complicates things and it is bug prone because it is not immediately obvious from the API point of view that it can lead to data races (the only reason we found out was TSAN). As for us, we actually want to be able to lock the bitstream on a specific version so even when the library gets updated it doesn't change what the encoders produce (because decoders are deployed in environments where they can't be updated that easily). I can even imagine that we may want in future produce different bitstream versions at the same time (e.g. for different clients devices) which would be actually impractical with the current design (in a multi-threaded environment, we would have to basically put a mutex around the entire meshopt_encodeVertexBuffer function).

[zeux]

Can you explain why it's impractical to call the function exactly once before any worker threads start?

edit: assuming, that is, a single encoded version. If you want to encode for multiple versions simultaneously I understand why the current interface isn't sufficient.

[ondys]

It's mostly about the overall architecture of the system. Basically the worker threads are calling libraries that call other libraries and so on. Meshoptimizer is integrated in a very deep layer of this system and the scheduler that runs the worker threads doesn't know anything about it and most of the libraries in between don't really have any initialization API to begin with. We can't even put meshoptimizer initialization directly to the scheduler (which would be a rather poor solution anyway) because the library where meshoptimizer lives is already initializing it and it would overwrite the previous initialization. All this could be solved on our end by refactoring of the system but frankly that is not really feasible, especially when other, much easier, solutions are available (which includes explicit synchronization but all I'm saying is that it would be much easier if we didn't have to do that).

[zeux]

If this is in C++, would it be possible to simply set the version before the call once? eg using C++11's safe statics:

static int once = (meshopt_encodeVertexVersion(1), 0);

I'm basically trying to reconcile:

• it should be easy to specify a single fixed version of the encoding in the app
• while it's indeed a problem if you want to encode multiple versions, right now there's no valid use case to encode multiple versions in the same app (IMO) - simply using the fixed version should work just fine. This may change if the new versions are being added, but we could simply wait until then to introduce different APIs and I'd rather have this as an actual use case that exists in production to act on this
• at the same time, of course it's not that big of a deal to simply add two more APIs

Here's the original decision framework for this btw (early 2020): https://gist.github.com/zeux/ace5c2372ab05d8e1590a824120f36cf