User Details
- User Since
- Nov 25 2021, 6:20 PM (168 w, 11 h)
Tue, Feb 11
Put braces around if block as suggested.
Mon, Feb 10
Sun, Feb 2
I have a proposal to simplify this code, may be worth considering. The general idea would be to
Mon, Jan 27
Sun, Jan 26
Sat, Jan 25
Reapprove.
Reapprove, I already accepted this.
Definitely better. The initial values of vchanformat and vchanrate are still off, but now they adopt the real channel parameters once set via sysctl. We can fix the initial values in a separate patch.
Thu, Jan 23
To the best of my knowledge, this should fix the test on i386. I haven't spotted any other test failures yet on CI, but that's not conclusive - some architectures seem to have build failures and won't run the tests.
Wed, Jan 22
Ok, the CI builds already found something. I'll post an update for review this evening.
Tue, Jan 21
Aren't vchanrate and vchanformat now completely disconnected from the actual parameters of the primary channel, until a vchan is opened? This would mean there's no useful feedback for the user until a channel is opened, at which point the parameters could suddenly change to something different?
I see now that there's a chn_reset() following this for primary channels, which means the defaults here may be overridden if not compatible. Also my concern was in conjunction with D48435, but I think we have to fix it there.
Sun, Jan 19
Sat, Jan 18
Fri, Jan 17
Hopefully the last update, sorry for the breakage in the commits based on this. If there's no further issues, we should get this into CI for all supported architectures - then verify that this test passes everywhere before merging the followup commits with the refactorings.
Extend tests and restructure test data:
- Label each row of test data explicitly.
- Replace normalized test values with non-UB normalize function.
- Test read / write macros that use 24 bit samples for 32 bit calculation.
- Rename some helper functions for clarity.
Jan 13 2025
Jan 12 2025
There's actually three different sample value types involved here:
This currently shifts 32bit samples out of the 32bit range in case SND_PCM_64 is not set. sample << PCM_FXSHIFT happens twice. The root cause is in D47932 though.
Jan 11 2025
Unfortunately the last two changes leave the vchanformat uninitialized until the channel's first use:
Jan 10 2025
Address some of the comments:
- Turn test data into a const array.
- Read samples from the local buffer, as intended.
- Test u-law and A-law sample formats.
- Extra test case to check the AFMT_BIT macro for correctness.
- Add a high level comment on the scope of the tests.
Jan 9 2025
I guess that's because of D47917, which assigns vchanrate to VCHAN_DEFAULT_RATE (48000), during attach in pcm_init(). Perhaps we should initialize these values once the primary channels have been created, and use their rates instead of defaulting to VCHAN_DEFAULT_RATE. I suppose the same should apply to the format.
Jan 8 2025
Jan 7 2025
No problem, I suppose you just wanted to test the attention of us reviewers ;-)
With this stack of changes I'm not able to set a different vchanrate than the 48000 default value. Didn't investigate yet, Christos can you reproduce this?
Jan 6 2025
Christos, I hope it's ok for me to "hijack" your refactoring efforts with this unit test, but I think it's the proper way forward with D47932. If this test succeeds now on all supported architectures, I'll be much more confident about the correctness of the test and test data. We can then adapt it to test your refactored pcm_sample_read() and pcm_sample_write() functions.
Jan 3 2025
Please note that I'm working on a unit test which exercises the original sample read / write macros, to be committed before this refactoring. This way we can verify the test procedure and test data with the original code, then adapt it to the refactored code. It's still WIP, but I can already tell that we definitely have some regressions here.
Dec 31 2024
Dec 30 2024
Dec 29 2024
Dec 28 2024
Not a consequence of your changes here, but it seems we only set the vchan settings on the first channel per direction. There could be multiple, right?
Dec 27 2024
Dec 26 2024
I stumbled upon that repeated list traversal when reviewing D48185. The implementation is roughly based on SLIST_REMOVE(). Compared to CHN_REMOVE(), the difference in performance should be only minuscule, doing one additional NULL pointer check per iteration. Therefore we may want to consider using the safe variant everywhere.
Dec 24 2024
I can confirm that this fixes the panic experienced with D47917, and I don't see anything else that could be affected by the change in return value. Thank you!
Dec 19 2024
First of all, could you please update these reviews when you rebase? The diff here still has D47868 included, and thus fails to apply cleanly.
Dec 17 2024
Ok, I did some more debugging, my current theory is this:
Dec 16 2024
FYI, the panic also happens on close:
Dec 15 2024
Dec 13 2024
Same as D48032, you may want to call your function with literal format parameters, for performance reasons.
If you want to make your function equal to the macro in terms of performance, you have to call it with literal format parameters somehow. E.g. through a switch on the format parameters.
There's more #ifdef FEEDMATRIX_GENERIC code which references this, no?
Why not just cache it in a local variable? That's probably cheaper, even though the "computation" here is trivial anyway.
Hm, PCM_CLAMP_S32 does something different when the SND_PCM_64 option is not set?
Dec 12 2024
I have a hard time to apply these interdependent patches cleanly to my tree. @christos, do you update all affected patches in the series when doing a rebase?
Dec 6 2024
Overall I like the direction of this change, but I still see some major issues:
Dec 5 2024
Dec 3 2024
I have to admit the centralization through sound.h goes against my intuition - usually I would only include what is needed, where it's actually used. While this change reduces the number of includes, it makes it even harder to see the dependencies and layers in the sound module. But that's just my opinion, I suppose @markj and @emaste may be the better judges in this case.