Skip to content
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.

Sign up for GitHub

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

[DNM] Audio: Volume: Skip stream start fade as defined in Kconfig #8227

Closed
wants to merge 2 commits into from
Closed

[DNM] Audio: Volume: Skip stream start fade as defined in Kconfig #8227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61ea560
Audio: Kconfig: Add for volume options for start fade-in
singalsu Sep 19, 2023
7f0aa8f
Audio: Volume: Skip stream start fade as defined in Kconfig
singalsu Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/audio/module_adapter/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,26 @@ endif # Cadence

if COMP_VOLUME

config COMP_VOLUME_PLAYBACK_START_FADE
bool "Start playback with fade-in"
default n
help
This option is needed to help reduce audible pops on
some codecs but depending on requested ramp type it
can increase peak MCPS by up to 3.5 per channel. Pops
playback direction are rare, so this should be usually
Copy link
Collaborator

Choose a reason for hiding this comment

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

typoe: "Pops in playback direction"

disabled.


config COMP_VOLUME_CAPTURE_START_FADE
bool "Start capture with fade-in"
default y
help
This option is needed to help reduce audible pops on
some codecs but depending on requested ramp type it
can increase peak MCPS by up to 3.5 per channel. It is
recommended to enable this.

config COMP_VOLUME_WINDOWS_FADE
bool "Windows Fade shape volume transitions support"
help
Expand Down
75 changes: 58 additions & 17 deletions src/audio/volume/volume.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,46 @@ static void volume_set_alignment(struct audio_stream *source,
audio_stream_init_alignment_constants(byte_align, frame_align_req, sink);
}

/**
* \brief Prepare volume to start with fade-in ramp.
*/
#if CONFIG_COMP_VOLUME_PLAYBACK_START_FADE || CONFIG_COMP_VOLUME_CAPTURE_START_FADE
static void volume_prepare_start_with_fade(struct processing_module *mod)
{
struct vol_data *cd = module_get_private_data(mod);
int i;

for (i = 0; i < cd->channels; i++) {
cd->volume[i] = cd->vol_min;
volume_set_chan(mod, i, cd->tvolume[i], false);
if (cd->volume[i] != cd->tvolume[i])
cd->ramp_finished = false;
}

volume_prepare_ramp(mod->dev, cd);
}
#endif

/**
* \brief Prepare volume to start without ramp.
*/
#if !CONFIG_COMP_VOLUME_PLAYBACK_START_FADE || !CONFIG_COMP_VOLUME_CAPTURE_START_FADE
static void volume_prepare_start_without_fade(struct processing_module *mod)
{
struct vol_data *cd = module_get_private_data(mod);
int i;

cd->is_passthrough = true;
for (i = 0; i < cd->channels; i++) {
cd->volume[i] = cd->tvolume[i];
if (cd->volume[i] != VOL_ZERO_DB)
cd->is_passthrough = false;
}

set_volume_process(cd, mod->dev, false);
}
#endif

/**
* \brief Prepares volume component for processing.
* \param[in,out] mod Volume processing module handle
Expand All @@ -641,7 +681,6 @@ static int volume_prepare(struct processing_module *mod,
struct comp_buffer *source_c, *sink_c;
uint32_t sink_period_bytes;
int ret;
int i;

comp_dbg(dev, "volume_prepare()");

Expand Down Expand Up @@ -687,14 +726,6 @@ static int volume_prepare(struct processing_module *mod,
goto err;
}

/* Set current volume to min to ensure ramp starts from minimum
* to previous volume request. Copy() checks for ramp finished
* and executes it if it has not yet finished as result of
* driver commands. Ramp is not constant rate to ensure it lasts
* for entire topology specified time.
*/
cd->ramp_finished = false;

cd->channels = audio_stream_get_channels(&sink_c->stream);
if (cd->channels > SOF_IPC_MAX_CHANNELS) {
ret = -EINVAL;
Expand All @@ -706,14 +737,24 @@ static int volume_prepare(struct processing_module *mod,

buffer_release(sink_c);

for (i = 0; i < cd->channels; i++) {
cd->volume[i] = cd->vol_min;
volume_set_chan(mod, i, cd->tvolume[i], false);
if (cd->volume[i] != cd->tvolume[i])
cd->ramp_finished = false;
}

volume_prepare_ramp(dev, cd);
#if !CONFIG_COMP_VOLUME_PLAYBACK_START_FADE && !CONFIG_COMP_VOLUME_CAPTURE_START_FADE
volume_prepare_start_without_fade(mod);
#elif CONFIG_COMP_VOLUME_PLAYBACK_START_FADE && CONFIG_COMP_VOLUME_CAPTURE_START_FADE
volume_prepare_start_with_fade(mod);
#else
#if CONFIG_COMP_VOLUME_PLAYBACK_START_FADE
if (dev->direction == SOF_IPC_STREAM_PLAYBACK)
volume_prepare_start_with_fade(mod);
else
volume_prepare_start_without_fade(mod);
#endif /* CONFIG_COMP_VOLUME_PLAYBACK_START_FADE*/
#if CONFIG_COMP_VOLUME_CAPTURE_START_FADE
if (dev->direction == SOF_IPC_STREAM_CAPTURE)
volume_prepare_start_with_fade(mod);
else
volume_prepare_start_without_fade(mod);
#endif /* CONFIG_COMP_VOLUME_PLAYBACK_START_FADE*/
Copy link
Contributor

Choose a reason for hiding this comment

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

those changes will make volume more complicated, we may need investigate and discuss first before take action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested all kconfig permutations so they work, but yes, there's more complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is not performance critical, I'd put the checks in an inline function and have just one call to volume_prepare_start_with_fade() and one to volume_prepare_start_without_fade().

static inline bool start_with_fade(direction)
{
#if CONFIG_COMP_VOLUME_PLAYBACK_START_FADE
	if (direction == SOF_IPC_STREAM_PLAYBACK)
                return true;
#endif 
#if CONFIG_COMP_VOLUME_CAPTURE_START_FADE
	if (direction == SOF_IPC_STREAM_CAPTURE)
                return true;
#endif 

        return false;
}

I think we can spare one conditional check in prepare().

Copy link
Contributor

Choose a reason for hiding this comment

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

don't add new code if it is not must, our initial problem is high ramp mcps, now, with change to linear and optimize windows fade, we already get good results, as liam just mentioned in mail, we should keep module easy to read/understand, especially for volume such simply module.

If volume is complicated, how can we do for other real complicated modules.

#endif

/*
* volume component does not do any format conversion, so use the buffer size for source
Expand Down
Loading