Skip to content

Conversation

@isHarryh
Copy link
Contributor

@isHarryh isHarryh commented Jan 17, 2025

Summary

First, this PR modified AudioClipConverter.py, adding a reuse strategy for pyfmodex.System instance to improve the efficiency of dump_samples() method. In the best case, this improvement lead to 5x more efficiency.

Second, a threading.Lock was added in order to protect the pyfmodex.System to 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.

  1. System instance initialization (64%)
system = pyfmodex.System()
system.init(clip.m_Channels, pyfmodex.flags.INIT_FLAGS.NORMAL, None)
  1. Sound creation and transformation (11%)
sound = system.create_sound(
    bytes(audio_data),
    pyfmodex.flags.MODE.OPENMEMORY,
    exinfo=pyfmodex.structure_declarations.CREATESOUNDEXINFO(
        length=len(audio_data),
        numchannels=clip.m_Channels,
        defaultfrequency=clip.m_Frequency,
    ),
)

# iterate over subsounds
samples = {}
for i in range(sound.num_subsounds):
    if i > 0:
        filename = "%s-%i.wav" % (clip.m_Name, i)
    else:
        filename = "%s.wav" % clip.m_Name
    subsound = sound.get_subsound(i)
    samples[filename] = subsound_to_wav(subsound, convert_pcm_float)
    subsound.release()
  1. Resource release (25%)
sound.release()
system.release()

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_Channels changed. 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: MEMORY will be raise at pyfmodex.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.

return dump_samples(audio, audio_data, convert_pcm_float)


system_lock = Lock()
Copy link
Owner

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_lock

Copy link
Contributor Author

@isHarryh isHarryh Jan 17, 2025

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?

Copy link
Owner

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.

Copy link
Contributor Author

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! : )

Copy link
Owner

@K0lb3 K0lb3 left a 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.

@isHarryh
Copy link
Contributor Author

Now the cache strategy was optimized, allowing multiple instances.

Additionally, a "global lock" was introduced to prevent asynchronous access to pyfmodex.System() function call.

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.

@isHarryh isHarryh requested a review from K0lb3 January 17, 2025 15:57
Copy link
Owner

@K0lb3 K0lb3 left a comment

Choose a reason for hiding this comment

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

Thanks a lot 👍

@K0lb3 K0lb3 merged commit e0d8154 into K0lb3:master Jan 18, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants