search

atteneder / KtxUnity

Load KTX and Basis Universal textures at runtime
Apache License 2.0
223 stars 42 forks source link

Only one transcoder instance is needed for the entire library #1

Closed Saticmotion closed 5 years ago

Saticmotion commented 5 years ago

Maybe one per thread if the transcoder functions are not thread safe. But my guess is, they are. Every transcoder function receives the file, and returns you a transcoded texture. Codebooks are only read from as far as I can tell, so they're probably thread safe too.

atteneder commented 5 years ago

Thanks for pointing it out!

The basis readme says:

transcode_image_level() and transcode_slice() are thread safe, i.e. you can decompress multiple images/slices from multiple threads.

But it says nothing about the other functions. How can we certify?

An alternative would be to create a pool of transcoders, namely one per thread. What do you think?

atteneder commented 5 years ago

We're probably fine with one transcoder if we do just the transcoding on threads and the rest on the main thread (getHasAlpha, getImageWidth, etc.).

Saticmotion commented 5 years ago

I just read through all functions in the basisu_transcoder class. As far as I can tell the following are definitely pure functions, they only use data from function parameters:

validate_file_checksums
validate_header_quick
validate_header
get_texture_type
get_userdata
get_total_images
get_image_info
get_total_image_levels
get_image_level_desc
get_image_level_info
get_file_info
find_first_slice_index
find_slice

trancode_slice and transcode_image_level are confirmed by Rich.

That leaves start_transcoding, which I wasn't able to verify yet because it uses the low level transcoder.

I also noticed that although basisu_transcoder has private members const void *m_pFile_data and uint32_t m_file_data_size, they are never used in any functions.

The low level transcoder is quite a bit more complex, and it definitely uses its private data. But I can't tell right away if that private data is specific to a file -- my guess is it is. So in that case we would need a low level transcoder per file. Maybe we can ask Rich how we should handle the low level transcoder?

Saticmotion commented 5 years ago

Oh and if BASISU_DEVEL_ERROR() is active, any error output is obviously going to be weird in a multithreaded scenario.

Saticmotion commented 5 years ago

Rich clarified which parts of the transcoder are thread safe: https://github.com/BinomialLLC/basis_universal/issues/25

atteneder commented 5 years ago

That's great to know, thanks for asking! But that means we'd have to create multiple transcoder instance once we go threaded. Do you think it still makes sense to merge this one now? I'd like to look into this later and make it threaded.

Saticmotion commented 5 years ago

Considering Rich's feedback, we're probably best off making one transcoder per worker thread, or making a pool of transcoders that get reused.