Add FAudioCreateCollector to create a FAPO that captures samples into a buffer #377

Merged

kg merged 5 commits from collector into master

2025-04-21 18:22:27 -05:00

kg commented

2025-03-31 22:15:04 -05:00

(Migrated from github.com)

I previously did this with a custom callback from outside but Collector makes it much easier and eliminates the possibility of crashing/hanging the mixer thread.

Simple and easy to use. Hand it a ring buffer and it will fill it with mono samples (so you don't need to think about how many channels there are), and you can feed that into an FFT or something else in order to do a realtime spectrum analyzer, etc. I previously did this with a custom callback from outside but Collector makes it much easier and eliminates the possibility of crashing/hanging the mixer thread.

flibitijibibo commented

2025-04-01 19:11:29 -05:00

(Migrated from github.com)

Overall looks good - let's add this as a full extension and add a text document in the extensions/ folder.

kg commented

2025-04-01 19:18:06 -05:00

(Migrated from github.com)

Do you think it's necessary to have some sort of mechanism to allow the user to figure out where the write position is so they can avoid processing the same data twice? I had that originally but it felt like unnecessary complication, so I stripped it out.
Another option would be to have two buffers and a flag that tells you which one is "ready", I guess.

Do we have any documentation on the extension process anywhere? I'm not sure if I need to do anything beyond documenting it.

Do you think it's necessary to have some sort of mechanism to allow the user to figure out where the write position is so they can avoid processing the same data twice? I had that originally but it felt like unnecessary complication, so I stripped it out. Another option would be to have two buffers and a flag that tells you which one is "ready", I guess. Do we have any documentation on the extension process anywhere? I'm not sure if I need to do anything beyond documenting it.

flibitijibibo commented

2025-04-01 19:57:56 -05:00

(Migrated from github.com)

I think VolumeMeter has something similar but if not I wouldn't worry about it. I usually just copy another extension file when making a new one, it's usually pretty short unless there's something super finicky about how it works.

kg commented

2025-04-01 20:38:34 -05:00

(Migrated from github.com)

Updated with an extension description txt along with some other stuff I overlooked. Not sure I got everything.

flibitijibibo (Migrated from github.com) requested changes

2025-04-03 15:17:32 -05:00

flibitijibibo (Migrated from github.com) left a comment

Finally was able to sit down and look at this in more detail - the write position should be done with FAudioVoice_GetEffectParameters instead of a new export since that will avoid race conditions on the XAPO's internal variable:

// FAudioFX.h
typedef struct FAudioFXCollectorParametersEXT
{
    uint32_t writeOffset;
} FAudioFXCollectorParametersEXT;

// FAudioFX_collector.c
void FAudioFXCollector_GetParameters(
	FAudioFXCollector *fapo,
	FAudioFXCollectorParametersEXT *pParameters,
	uint32_t ParameterByteSize
) {
	FAudioFXCollectorParametersEXT *levels = (FAudioFXCollectorParametersEXT*)
		fapo->base.m_pCurrentParameters;
	FAudio_assert(ParameterByteSize == sizeof(FAudioFXCollectorParametersEXT));

	pParameters->writeOffset = fapo->writeOffset;
}

This may also be a better place to fill a user buffer, for basically the same reason - I think this is why VolumeMeter has its own ring buffer internally.

Finally was able to sit down and look at this in more detail - the write position should be done with `FAudioVoice_GetEffectParameters` instead of a new export since that will avoid race conditions on the XAPO's internal variable: ``` // FAudioFX.h typedef struct FAudioFXCollectorParametersEXT { uint32_t writeOffset; } FAudioFXCollectorParametersEXT; // FAudioFX_collector.c void FAudioFXCollector_GetParameters( FAudioFXCollector *fapo, FAudioFXCollectorParametersEXT *pParameters, uint32_t ParameterByteSize ) { FAudioFXCollectorParametersEXT *levels = (FAudioFXCollectorParametersEXT*) fapo->base.m_pCurrentParameters; FAudio_assert(ParameterByteSize == sizeof(FAudioFXCollectorParametersEXT)); pParameters->writeOffset = fapo->writeOffset; } ``` This _may_ also be a better place to fill a user buffer, for basically the same reason - I think this is why VolumeMeter has its own ring buffer internally.

kg commented

2025-04-04 00:06:24 -05:00

(Migrated from github.com)

Finally was able to sit down and look at this in more detail - the write position should be done with FAudioVoice_GetEffectParameters instead of a new export since that will avoid race conditions on the XAPO's internal variable:
// FAudioFX.h
typedef struct FAudioFXCollectorParametersEXT
{
    uint32_t writeOffset;
} FAudioFXCollectorParametersEXT;

// FAudioFX_collector.c
void FAudioFXCollector_GetParameters(
	FAudioFXCollector *fapo,
	FAudioFXCollectorParametersEXT *pParameters,
	uint32_t ParameterByteSize
) {
	FAudioFXCollectorParametersEXT *levels = (FAudioFXCollectorParametersEXT*)
		fapo->base.m_pCurrentParameters;
	FAudio_assert(ParameterByteSize == sizeof(FAudioFXCollectorParametersEXT));

	pParameters->writeOffset = fapo->writeOffset;
}
This may also be a better place to fill a user buffer, for basically the same reason - I think this is why VolumeMeter has its own ring buffer internally.

There shouldn't be a race condition with the existing way. I'm not sure I see the advantage of this change, is it just for consistency?

> Finally was able to sit down and look at this in more detail - the write position should be done with `FAudioVoice_GetEffectParameters` instead of a new export since that will avoid race conditions on the XAPO's internal variable: > > ``` > // FAudioFX.h > typedef struct FAudioFXCollectorParametersEXT > { > uint32_t writeOffset; > } FAudioFXCollectorParametersEXT; > > // FAudioFX_collector.c > void FAudioFXCollector_GetParameters( > FAudioFXCollector *fapo, > FAudioFXCollectorParametersEXT *pParameters, > uint32_t ParameterByteSize > ) { > FAudioFXCollectorParametersEXT *levels = (FAudioFXCollectorParametersEXT*) > fapo->base.m_pCurrentParameters; > FAudio_assert(ParameterByteSize == sizeof(FAudioFXCollectorParametersEXT)); > > pParameters->writeOffset = fapo->writeOffset; > } > ``` > > This _may_ also be a better place to fill a user buffer, for basically the same reason - I think this is why VolumeMeter has its own ring buffer internally. There shouldn't be a race condition with the existing way. I'm not sure I see the advantage of this change, is it just for consistency?

flibitijibibo commented

2025-04-04 08:34:30 -05:00

(Migrated from github.com)

The audio thread may be modifying data at the time that the new function is being called - note the lock we have in the GetEffectParameters implementation.

👍 1

kg commented

2025-04-19 13:24:59 -05:00

(Migrated from github.com)