-
Notifications
You must be signed in to change notification settings - Fork 333
refactor(sdk): Allow to send waveform for any audio message #5732
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
base: main
Are you sure you want to change the base?
Conversation
By moving the waveform declaration into `BaseAudioInfo`. Signed-off-by: Kévin Commaille <[email protected]>
Most clients will probably work with values between 0 and 1 and need to convert it just to send it, so we can move that conversion into the SDK. This is also more forwards-compatible, because MSC3246 now has a different max value for the amplitude, so when this becomes stable, the only change needed will be in the SDK. Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
CodSpeed Performance ReportMerging #5732 will not alter performanceComparing Summary
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5732 +/- ##
==========================================
- Coverage 88.40% 88.40% -0.01%
==========================================
Files 359 359
Lines 99048 99046 -2
Branches 99048 99046 -2
==========================================
- Hits 87566 87562 -4
- Misses 7344 7346 +2
Partials 4138 4138 ☔ View full report in Codecov by Sentry. |
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.
The changes look good to me. I'm a bit annoyed by the regression with the waveform format, but since it's a different type and it's documented in the CHANGELOG
, I think it's fine.
Re-running the benchmarks. I suspect the result is misleading and could be due to noise. @zecakeh can you fix the conflicts please? |
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.
Thanks, we're on board with this change, and as you said it'll make the switch to the new MSC simpler. We might delay the landing of this PR, since we want to limit landing breaking changes at the FFI layer until the Matrix conference.
One comment I would have is: why to use f32
and not f64
? We'd have a much better precision when it comes to the [0; 1] range, leading to a better precision in the normalized range; and I don't think we're getting any notable speedup for using 32 bits instead of 64 bits here.
Would it make sense to change to a f64 in this patch?
/// The waveform of the audio clip. | ||
pub waveform: Option<Vec<u16>>, | ||
/// | ||
/// Must only includes values between 0 and 1. |
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.
nit: include
I don't mind changing to f64 but I chose f32 because it is precise enough to be converted to an integer value between 0 and 1024 and it should be enough to draw it too so I don't see what we gain by increasing the precision? |
I wary of using float32s, because they can't represent finely small values, viz. values in the [0; 1] range. That is, many small float32 values would be normalized to the same int in the [0; 1024] range, so if you looked at a flat projection of the f32 range to the int10 range, you might see holes in the int range. Since float64 have so much more precision, it's unlikely that this problem would happen with those. |
I am not convinced that this is true, because f32 has a precision of 6 significant digits, and for integer values between 0 and 1024 we only need 4. |
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've double-checked that with a program, and agree with your conclusion; good point.
Approving, but we've been requested to not merge this immediately.
No problem, I'll rebase this after the Matrix conference then to solve the conflicts. |
We plan to use this in Fractal to always be able to present a waveform for an audio message.
The second commit might be more controversial: it changes the format of the waveform from a list of
u16
between 0 and 1024 to a list off32
between 0 and 1. This is done because the value between 0 and 1024 used in the event is quite arbitrary (and they have changed in MSC3246 since then), most clients should end up with values between 0 and 1, and need to convert it for sending it. So this centralizes this conversion in the SDK.