Skip to content

DRC code refactor #8593

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

Merged
lgirdwood merged 2 commits into from
Dec 19, 2023
Merged

DRC code refactor #8593

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
2b6c517
audio: drc: move drc header file from include path to module folder
btian1 Dec 8, 2023
846ffdb
Audio: drc: remove incompatible inline for three functions
btian1 Dec 8, 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
19 changes: 10 additions & 9 deletions src/audio/drc/drc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
// Author: Pin-chih Lin <johnylin@google.com>

#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/drc/drc.h>
#include <sof/audio/drc/drc_algorithm.h>
#include <sof/audio/buffer.h>
#include <sof/audio/component.h>
#include <sof/audio/data_blob.h>
Expand Down Expand Up @@ -35,6 +33,9 @@
#include <stddef.h>
#include <stdint.h>

#include "drc.h"
#include "drc_algorithm.h"

LOG_MODULE_REGISTER(drc, CONFIG_SOF_LOG_LEVEL);

/* b36ee4da-006f-47f9-a06d-fecbe2d8b6ce */
Expand All @@ -43,7 +44,7 @@ DECLARE_SOF_RT_UUID("drc", drc_uuid, 0xb36ee4da, 0x006f, 0x47f9,

DECLARE_TR_CTX(drc_tr, SOF_UUID(drc_uuid), LOG_LEVEL_INFO);

inline void drc_reset_state(struct drc_state *state)
void drc_reset_state(struct drc_state *state)
{
int i;

Expand All @@ -67,9 +68,9 @@ inline void drc_reset_state(struct drc_state *state)
state->max_attack_compression_diff_db = INT32_MIN;
}

inline int drc_init_pre_delay_buffers(struct drc_state *state,
size_t sample_bytes,
int channels)
int drc_init_pre_delay_buffers(struct drc_state *state,
size_t sample_bytes,
int channels)
{
size_t bytes_per_channel = sample_bytes * CONFIG_DRC_MAX_PRE_DELAY_FRAMES;
size_t bytes_total = bytes_per_channel * channels;
Expand All @@ -90,9 +91,9 @@ inline int drc_init_pre_delay_buffers(struct drc_state *state,
return 0;
}

inline int drc_set_pre_delay_time(struct drc_state *state,
int32_t pre_delay_time,
int32_t rate)
int drc_set_pre_delay_time(struct drc_state *state,
int32_t pre_delay_time,
int32_t rate)
{
int32_t pre_delay_frames;

Expand Down
5 changes: 3 additions & 2 deletions src/include/sof/audio/drc/drc.h → src/audio/drc/drc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@

#include <stdint.h>
#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/drc/drc_plat_conf.h>
#include <sof/audio/buffer.h>
#include <sof/platform.h>
#include <user/drc.h>

#include "drc_plat_conf.h"
#include "drc_user.h"

struct audio_stream;
struct comp_dev;
Expand Down
5 changes: 3 additions & 2 deletions src/include/sof/audio/drc/drc_algorithm.h → src/audio/drc/drc_algorithm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
#define __SOF_AUDIO_DRC_DRC_ALGORITHM_H__

#include <stdint.h>
#include <sof/audio/drc/drc.h>
#include <sof/platform.h>
#include <user/drc.h>

#include "drc_user.h"
#include "drc.h"

/* drc reset function */
void drc_reset_state(struct drc_state *state);
Expand Down
7 changes: 4 additions & 3 deletions src/audio/drc/drc_generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
// Author: Pin-chih Lin <johnylin@google.com>

#include <sof/audio/component.h>
#include <sof/audio/drc/drc.h>
#include <sof/audio/drc/drc_algorithm.h>
#include <sof/audio/drc/drc_math.h>
#include <sof/audio/format.h>
#include <sof/math/decibels.h>
#include <sof/math/numbers.h>
#include <stdint.h>

#include "drc.h"
#include "drc_algorithm.h"
#include "drc_math.h"

#if DRC_GENERIC

#define ONE_Q20 Q_CONVERT_FLOAT(1.0f, 20) /* Q12.20 */
Expand Down
7 changes: 4 additions & 3 deletions src/audio/drc/drc_hifi3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
// Author: Pin-chih Lin <johnylin@google.com>

#include <sof/audio/component.h>
#include <sof/audio/drc/drc.h>
#include <sof/audio/drc/drc_algorithm.h>
#include <sof/audio/drc/drc_math.h>
#include <sof/audio/format.h>
#include <sof/math/decibels.h>
#include <sof/math/numbers.h>
#include <stdint.h>

#include "drc.h"
#include "drc_algorithm.h"
#include "drc_math.h"

#if DRC_HIFI3

#include <xtensa/tie/xt_hifi3.h>
Expand Down
7 changes: 4 additions & 3 deletions src/audio/drc/drc_hifi4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
// Author: Pin-chih Lin <johnylin@google.com>

#include <sof/audio/component.h>
#include <sof/audio/drc/drc.h>
#include <sof/audio/drc/drc_algorithm.h>
#include <sof/audio/drc/drc_math.h>
#include <sof/audio/format.h>
#include <sof/math/decibels.h>
#include <sof/math/numbers.h>
#include <stdint.h>

#include "drc.h"
#include "drc_algorithm.h"
#include "drc_math.h"

#if DRC_HIFI4

#include <xtensa/tie/xt_hifi4.h>
Expand Down
3 changes: 2 additions & 1 deletion src/include/sof/audio/drc/drc_math.h → src/audio/drc/drc_math.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@

#include <stddef.h>
#include <stdint.h>
#include <sof/audio/drc/drc_plat_conf.h>
#include <sof/audio/format.h>
#include <sof/math/numbers.h>
#include <sof/math/trig.h>

#include "drc_plat_conf.h"

/* Unmark this define to use cordic arc sine implementation. */
/* #define DRC_USE_CORDIC_ASIN */

Expand Down
3 changes: 2 additions & 1 deletion src/audio/drc/drc_math_generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
//
// Author: Pin-chih Lin <johnylin@google.com>

#include <sof/audio/drc/drc_math.h>
#include <sof/audio/format.h>
#include <sof/math/decibels.h>
#include <sof/math/numbers.h>
#include <sof/math/trig.h>

#include "drc_math.h"

#define q_mult(a, b, qa, qb, qy) ((int32_t)Q_MULTSR_32X32((int64_t)(a), b, qa, qb, qy))
#define q_multq(a, b, q) ((int32_t)Q_MULTSR_32X32((int64_t)(a), b, q, q, q))

Expand Down
2 changes: 1 addition & 1 deletion src/audio/drc/drc_math_hifi3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
//
// Author: Pin-chih Lin <johnylin@google.com>

#include <sof/audio/drc/drc_math.h>
#include <sof/math/numbers.h>
#include "drc_math.h"

#if DRC_HIFI3 || DRC_HIFI4

Expand Down
0 src/include/sof/audio/drc/drc_plat_conf.h → src/audio/drc/drc_plat_conf.h
View file
Open in desktop
File renamed without changes.
0 src/include/user/drc.h → src/audio/drc/drc_user.h
View file
Open in desktop
File renamed without changes.
3 changes: 2 additions & 1 deletion src/audio/multiband_drc/multiband_drc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/crossover/crossover_algorithm.h>
#include <sof/audio/drc/drc_algorithm.h>
#include <sof/audio/buffer.h>
#include <sof/audio/format.h>
#include <sof/audio/ipc-config.h>
Expand All @@ -32,6 +31,8 @@
#include <errno.h>
#include <stddef.h>
#include <stdint.h>

#include "../drc/drc_algorithm.h"
Copy link
Collaborator

@lyakh lyakh Dec 11, 2023 •
edited by marc-hb
Loading

Choose a reason for hiding this comment

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

as @marc-hb also has pointed out elsewhere[*], such includes aren't very pretty, would be better to keep shared between modules headers in an "include" directory

[*]

Copy link
Collaborator

@kv2019i kv2019i Dec 11, 2023

Choose a reason for hiding this comment

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

This isn't a fully clearcut case either. Here multiband_drc is directly building on top of drc (and also uses crossover).

I think we should also ensure this type of cross-module resuse is easy to do and code stays maintainable.

@lgirdwood Any input, should we keep such reused headers (like drc_algorithm.h) in common code?

I think I'd personally still code with the approach in this PR. The modules are deeply dependent on each other, so having a "../drc/drc_algorithm.h" makes sense. Having alone drc_algorithm.h in sof/audio/ seems harder to understand ("why is this header here but not others?").

Copy link
Collaborator

@lyakh lyakh Dec 12, 2023

Choose a reason for hiding this comment

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

I don't have a huge problem with this approach, just looking for the best one. As alternatives we could either

  1. have an include directory under src/audio/include for such shared headers or
  2. add src/audio to the list of include paths

I think I'd find (2) preferable

Copy link
Contributor Author

@btian1 btian1 Dec 13, 2023

Choose a reason for hiding this comment

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

Seppo suggested move the common part to math folder in a separate PR, I will do it once those PRs get approval and merged.

Copy link
Collaborator

@marc-hb marc-hb Dec 15, 2023

Choose a reason for hiding this comment

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

The modules are deeply dependent on each other, so having a "../drc/drc_algorithm.h" makes sense.

I measured how frequent are these "dot-dot" includes across all the default zephyr/west.yml repos. It's about 1.5%. So it's not "exotic" but it's definitely not "mainstream" either. As I just mentioned in #8630, there is zephyr_library_include_directories() and friends.

$ west grep '^[[:blank:]]*#[[:blank:]]*include[[:blank:]]*[<"]\.\.' | wc -l
2914
$ west grep '^[[:blank:]]*#[[:blank:]]*include[[:blank:]]*[<"][[:alnum:]]' | wc -l
228292

Copy link
Contributor Author

@btian1 btian1 Dec 18, 2023

Choose a reason for hiding this comment

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

how about non-zephyr cases? i.e: CONFIG_ZEPHYR_NATIVE_DRIVERS is not defined, will other build also support zephyr syntax?

#include "multiband_drc.h"

LOG_MODULE_REGISTER(multiband_drc, CONFIG_SOF_LOG_LEVEL);
Expand Down
3 changes: 2 additions & 1 deletion src/audio/multiband_drc/multiband_drc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@

#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/crossover/crossover.h>
#include <sof/audio/drc/drc.h>
#include <sof/math/iir_df2t.h>
#include <sof/audio/component.h>
#include <sof/audio/data_blob.h>
#include <sof/platform.h>
#include <stdint.h>

#include "../drc/drc.h"
#include "user/multiband_drc.h"

/**
Expand Down
3 changes: 2 additions & 1 deletion src/audio/multiband_drc/multiband_drc_generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
// Author: Pin-chih Lin <johnylin@google.com>

#include <stdint.h>
#include <sof/audio/drc/drc_algorithm.h>
#include <sof/audio/format.h>
#include <sof/math/iir_df2t.h>

#include "multiband_drc.h"
#include "../drc/drc_algorithm.h"

static void multiband_drc_default_pass(const struct processing_module *mod,
const struct audio_stream *source,
Expand Down
3 changes: 2 additions & 1 deletion src/audio/multiband_drc/user/multiband_drc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@

#include <stdint.h>
#include <user/crossover.h>
#include <user/drc.h>
#include <user/eq.h>

#include "../../drc/drc_user.h"

/* Maximum number of frequency band for Multiband DRC */
#define SOF_MULTIBAND_DRC_MAX_BANDS SOF_CROSSOVER_MAX_STREAMS

Expand Down