-
Notifications
You must be signed in to change notification settings - Fork 349
Audio: Sound Dose: Add new component #10058
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
f038d75 to
710ed6d
Compare
lgirdwood
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.
LGTM, just some opens.
6f6db17 to
a608928
Compare
lgirdwood
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.
LGTM, think this looks ready for review
kv2019i
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.
I see you went with "notifications with timestamp" instead of a compressed stream of data. I guess this works as well, doesn't take DMA resources, and we can always add a DMA option later one. Added one comment about the timestamp accuracy if this is put in the common header.
src/include/user/audio_feature.h
Outdated
| }; | ||
|
|
||
| struct sof_audio_feature { | ||
| int64_t stream_time_ms; |
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.
If this is a common feature header, I wonder if "ms" is accurate enough? Maybe even report just number of frames since start at let user-space/app convert. For sound does, this would be 1ms increments.
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.
Number of frames depends on content sample rate, so I thought the time as relative from start is more generic. Not sure if sound dose user space part has connection to the part that decodes stream. But we could have both in the struct?
Looks microseconds would work too, wrap in 580000 years so it would be more generic if need for high accuracy.
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.
Done now
c03beba to
def60f6
Compare
lgirdwood
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.
just minor comments, needs more documentation, I would add a small ReadMe.md describing the flow and linking to the Android docs.
| <include/components/tdfb.conf> | ||
| <include/components/template_comp.conf> | ||
| <include/components/micsel.conf> | ||
| <include/components/sound_dose.conf> |
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.
Can the commit message describe how test topology a bit more, e.g. the pipeline used to test.
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.
Added
| ipc_msg_send(cd->msg, NULL, false); | ||
| } | ||
|
|
||
| /* IPC4 controls handler */ |
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.
since this is the main transport for data to the host, can you describe the flow in more detail.
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.
I added comments text
3aaf3a2 to
7f4e26d
Compare
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.
Pull Request Overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| #include <stdint.h> | ||
|
|
||
| #define SOUND_DOSE_DEBUG 0 |
Copilot
AI
Sep 18, 2025
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.
[nitpick] Consider using a Kconfig option instead of a hardcoded debug flag to allow runtime configuration of debug output without recompilation.
| #include <stdint.h> | |
| #define SOUND_DOSE_DEBUG 0 | |
| #include <stdint.h> | |
| #include <sof/common.h> | |
| #ifdef CONFIG_SND_SOC_SOF_SOUND_DOSE_DEBUG | |
| #define SOUND_DOSE_DEBUG 1 | |
| #else | |
| #define SOUND_DOSE_DEBUG 0 | |
| #endif |
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.
Kconfig is a recompile and I'd not like to expose it globally. I find it easier to set it from this header file than going through "ninja -C build-xxx menuconfig"
| /* TODO: 96 kHz and 192 kHz with integer decimation factor. The A-weight is not | ||
| * defined above 20 kHz, so high frequency energy is not needed. Also it will | ||
| * help keep the module load reasonable. |
Copilot
AI
Sep 18, 2025
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.
This TODO comment should include a ticket number or timeline for when this feature will be implemented, as it affects the component's supported sample rates.
| /* TODO: 96 kHz and 192 kHz with integer decimation factor. The A-weight is not | |
| * defined above 20 kHz, so high frequency energy is not needed. Also it will | |
| * help keep the module load reasonable. | |
| /* TODO (TICKET-1234): 96 kHz and 192 kHz with integer decimation factor. The A-weight is not | |
| * defined above 20 kHz, so high frequency energy is not needed. Also it will | |
| * help keep the module load reasonable. | |
| * See TICKET-1234 for progress and timeline. |
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.
I need to discuss this with my manager, it depends on overall priorities.
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.
This is fine, keep the TODO.
| sample = sat_int16(Q_MULTSR_32X32((int64_t)cd->gain, *x, | ||
| 30, 15, 15)); |
Copilot
AI
Sep 18, 2025
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.
[nitpick] The magic numbers for Q format (30, 15, 15) should be defined as named constants to improve code readability and maintainability.
| sample = sat_int32(Q_MULTSR_32X32((int64_t)cd->gain, *x, | ||
| 30, 31, 31)); |
Copilot
AI
Sep 18, 2025
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.
[nitpick] Similar to the S16 case, the Q format constants (30, 31, 31) should be defined as named constants for better maintainability.
kv2019i
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.
No blockers!
c702744 to
f21c100
Compare
| normal topology file. Or alternative use module options | ||
| tplg_filename and tplg_path for module snd_sof. | ||
|
|
||
| Get kcontrol_events from https://github.com/ujfalusi/kcontrol_events |
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.
@ujfalusi please upstream this test too in here in a follow up PR. Thanks !
85f5ff2 to
f53a92d
Compare
|
@lrudyX ping - looks like CI got stuck ? |
1452a41 to
81ec41b
Compare
|
@lgirdwood No changes, just pushed a rebase to restart CI checks. |
|
@lrudyX The two required CI checks seem to have got stuck since 19 hours when I re-pushed. The previous failure seemed to be CI internal, not caused by these code changes. |
81ec41b to
1989a26
Compare
|
Now I added toml for wcl platform, not sure, hoping it was the issue with rimage in build. |
|
There's a fail in https://sof-ci.01.org/sof-pr-viewer/#/build/PR10058/build14749499 with MTL TestRtcAec. I wonder how this PR can impact it. |
The exported header file is missing the include for stdint.h for the used uint32_t type. Without it some builds fail to warning. Also the copyright text is updated. This patch also adds static const to the array declaration. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
Need to drop the TLV header (used for Linux kernel bytes control) from data to keep the existing format after the header was added to sof-ctl generated data header. This header is not passed to firmware, only the kernel that this usage of filter coefficients passes, is using it. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch adds script sof_sound_dose_time_domain_filters.m that exports IIR and FIR coefficients to approximate A-weight function. The current choice is IIR only for lower MCPS load. The script sof_sound_dose_blobs.m creates a few control blobs to test the sound_dose component. A simple script sof_sound_dose_ref.m to compute dBFS and MEL for a wav file is added to compare with firmware reported values. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The added plot is useful especially with parametric IIR equalizer tuning to see achieved response error vs. target. A numerical mean(abs()) value of error is printed to help see filter parameters change impact. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The build topologies are - sof-hda-benchmark-sound_dose32.tplg - sof-mtl-sdw-benchmark-sound_dose32-sdw0.tplg - sof-mtl-sdw-benchmark-sound_dose32-simplejack.tplg The controls are for example initialized to - sensitivity 100 dB, 0 dBFS equals 100 dBSPL (a worst case loudness) - volume 0 dB, assumes codec gain for headphones is set to max - gain 0 dB, user's music playback is not attenuated Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
1989a26 to
fdc91b9
Compare
| # ---------------------------------------- | ||
|
|
||
| config COMP_SOUND_DOSE | ||
| default y |
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.
If the CI issue persists, @lgirdwood proposed to limit the feature to newer platforms. @singalsu You can remove this bit completely and leave default as not set. For all the newer Intel platforms (ace20 and newer), sound dose will get enabled as a module.
OTOH, I don't see how enabling the component on MTL could break the stub loadable modules...
This patch adds a new SOF component Sound Dose. The purpose is to calculate for audio playback MEL values (momentary sound exposure level) to provide to user space the data to compute the sound dose CSD as defined in EN 50332-3. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
fdc91b9 to
b940776
Compare
|
Looks like the last change has significant impact on test result. Now we have PASS on internal CI. |
No description provided.