[Bug 28723] Sound stutter in Rage when emulated windows version is set to "Windows 7" (XAudio2 -> mmdevapi sound output path)

[wine-bugs at winehq.org]

http://bugs.winehq.org/show_bug.cgi?id=28723

Andrew Eikum <aeikum at codeweavers.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37943|0                           |1
        is obsolete|                            |

--- Comment #84 from Andrew Eikum <aeikum at 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?

-- 
Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email
Do not reply to this email, post in Bugzilla using the
above URL to reply.
------- You are receiving this mail because: -------
You are watching all bug changes.