refactor(sdk): Allow to send waveform for any audio message #5732
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. |
|
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.
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. |
|
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. |
|
No problem, I'll rebase this after the Matrix conference then to solve the conflicts. |
3 participants
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
u16between 0 and 1024 to a list off32between 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.