http://bugs.winehq.org/show_bug.cgi?id=28723
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37943|0 |1 is obsolete| |
--- Comment #84 from Andrew Eikum aeikum@codeweavers.com 2011-12-13 13:56:49 CST --- Created attachment 37960 --> http://bugs.winehq.org/attachment.cgi?id=37960 ALSA fixes patchset #3
(In reply to comment #83)
Here's my mini-review. 0003 alsa_period_us = 10000; If a constant, use DefaultPeriod. But that's wrong. It should be mmdev_period after due clamping of values. In exclusive mode, the app could ask for a period not 10ms.
0002/0004 -/+ TRACE("alsa_period_frames... The first patch should move it once and for all.
0004 Use MulDiv for both duration and period
- This->mmdev_period_frames = (fmt->nSamplesPerSec * This->mmdev_period_rt)
/ 10000000.;
- This->bufsize_frames = MulDiv(duration, rate, 10000000);
Why rate here vs nSamplesPerPsec there? bufsize_frames=GetBufferSize should be computed with app-visible units, i.e. nSamplesPerSec, not ALSA's actual internal rate.
0002 Continue using the same variable (who remembers to_write == written?):
- if(This->held_frames && (to_write < write_limit)){
min(This->held_frames, write_limit - written), This);
All of these are done in the attached patchset. As usual, it passes all tests with and without PA.
0002 In alsa_write_data I found the logic not clear enough. I suggest /* try to keep 3 ... */ max_period = 3 * max(); if(in_alsa >= max_period) return; and would get rid of the "while () write_limit+=;" loop which is bad with alsa_period=8.
I added an "if(write_limit == 0)" early return. I don't understand your other objection.
Overall, I don't like the complexity that 0002 adds to alsa_write_data. It would be preferable to use set_hw_params_buffer_size_max(3xboth_periods) because that's what the code in effect realizes! Often enough, set_period_near returns the period that ALSA chose, even before set_buffer_time|size or the final set_hw_params. (Another idea would be to iterate twice through set_hw_params. Such initialisation overhead is still preferable to overhead in the audio loop).
We can't set max(3xperiod) because then we will get too small a buffer on someone's system. We can't set exact 3xperiod because it will fail on someone's system. If we set min(3xperiod) we must also have the 3-period limiter in alsa_write_data because it will give us too large a buffer on someone's system. So that's what we have to do. (Aside: of what use is an API that makes no guarantees?)
Note that I tweaked the buffer size call slightly in this version. We now use set_buffer_size_min() instead of _near(). We also only request a minimum of 3 MMDevAPI periods, instead of max() between that & duration. It would be 3 of either period size, but as you pointed out, we would have to do two passes to get that information. While ALSA often surprises me, I'm sure they already have at least 3 ALSA periods in their buffer by default...... right?