-
-
Notifications
You must be signed in to change notification settings - Fork 161
Improve efficiency and fix concurrent issue of audio dumping #291
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
Conversation
UnityPy/export/AudioClipConverter.py
Outdated
| return dump_samples(audio, audio_data, convert_pcm_float) | ||
|
|
||
|
|
||
| system_lock = Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of restricting the configuration to one instance, it would also work to simply use a dictionary of tuples to cache the different instances with their locks.
So
PYFMODEX_INSTANCES = {}
def get_pyfmodex_system_instance(channels: int, flags: int):
global pyfmodex, PYFMODEX_INSTANCES
instance_key = (channels, flags)
if instance_key in PYFMODEX_INSTANCES:
return PYFMODEX_INSTANCES[instance_key]
instance = pyfmodex.System()
instance.init(channels, flags, None)
instance_lock = Lock()
PYFMODEX_INSTANCES[instance_key] = (instance, instance_lock)
return instance, instance_lockThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also came up with whis trick that maintaining a dict "system_params => system_instance". I will work around to test whether this approach is okay.
But there is a concern that, this may cause additional memory usage. Maybe we should maintaining a length-restricted dict (e.g. allow 2 or 3 instances to coexist)? What's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good consideration, but I think it's not really worth the trouble.
Most games I came across only use dual or mono channels, with only a handful using more.
As you already mentioned later, the memory consumption of a system instance isn't too bad.
I will probably add a handler for the length restriction in version 2 for a (super) low memory consumption mode for low memory devices like Raspberry Pis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Although Fmod supports up to 4096 channels, most games only use dual or mono channels. So it'll be okay if we don't use length-restricted dict, since adding a length-restricted dict seems too troublesome.
Thank you for your UnityPy! : )
K0lb3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and thanks for the detailed explanation.
I would just like to see the single mentioned change, and then it's ready to be merged.
|
Now the cache strategy was optimized, allowing multiple instances. Additionally, a "global lock" was introduced to prevent asynchronous access to I have tested the memory consumption, it is acceptable. I also tested using multithreading to extract audio files that have different channels, all worked well. |
K0lb3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot 👍
Summary
First, this PR modified
AudioClipConverter.py, adding a reuse strategy forpyfmodex.Systeminstance to improve the efficiency ofdump_samples()method. In the best case, this improvement lead to 5x more efficiency.Second, a
threading.Lockwas added in order to protect thepyfmodex.Systemto avoid concurrent issue.Part 1. Efficiency Improvement
Before the improvement was introduced, we investigated the time consumption distribution in
dump_samples()method. The following time-tests results are based on audio clips whose duration ranging from 1s to 20s.As you can see, the system initialization and release takes the 89% time of the audio dumping. It is important to see that we need to re-init a system only if the
clip.m_Channelschanged. So we can use a reuse trick to avoid meaningless initializations.In the best case where all clips have the same num_channels, we can get 5x more efficiency because the system only needs to be inited once.
Part 2. Concurrent Issue Fix
When we use multithreading to dump audio files, an
FmodError: MEMORYwill be raise atpyfmodex.System()the creation method. This is not caused by insufficient memory but asynchronous access to the pyfmodex.To fix this problem, we introduce a threading lock to prevent the asynchronous access. This approach will hardly affect performance.