now OSS driver emulation -- and other drivers too -- works as well as OSS HW mode; block size error gone?

Hi,

if your app worked well in OSS HW driver mode but not in emulation mode,
the following patch may help you. For me, it fixed the following:
 - wrong playback speed in some apps (bug #19901);
 - spurious errors about block size:
   err:dsound:DSOUND_MixInBuffer length not a multiple of block size,
    len = 1579, block size = 2
 - an assertion failure and crash (bug #12349):
   mixer.c:489: DSOUND_MixInBuffer:
    Assertion `dsb->buf_mixpos + len <= dsb->tmp_buffer_len' failed.

As the MacOS CoreAudio driver produces sound effects much like OSS
emulation, a similar fix to wineCoreAudio.drv will certainly cure sound
on the Mac, lifting some apps' rating from garbage to gold!

diff --git a/dlls/wineoss.drv/audio.c b/dlls/wineoss.drv/audio.c
index ca79cd5..9ef0d5b 100644
--- a/dlls/wineoss.drv/audio.c
+++ b/dlls/wineoss.drv/audio.c
@@ -2071,6 +2071,13 @@ static DWORD wodOpen(WORD wDevID, LPWAVEOPENDESC lpDesc, DWORD dwFlags)
 	     wwo->waveFormat.Format.nSamplesPerSec) /
 	    wwo->waveFormat.Format.nChannels;
     }
+    if (wwo->waveFormat.Format.nBlockAlign != wwo->waveFormat.Format.nChannels *
+            wwo->waveFormat.Format.wBitsPerSample / 8) {
+        WARN("Fixing BlockAlign\n");
+        wwo->waveFormat.Format.nBlockAlign = wwo->waveFormat.Format.nChannels *
+	  wwo->waveFormat.Format.wBitsPerSample / 8;
+        lpDesc->lpFormat->nBlockAlign=wwo->waveFormat.Format.nBlockAlign; /* see DirectSoundDevice_Create */
+    }
     /* Read output space info for future reference */
     if (ioctl(wwo->ossdev.fd, SNDCTL_DSP_GETOSPACE, &info) < 0) {
 	ERR("ioctl(%s, SNDCTL_DSP_GETOSPACE) failed (%s)\n", wwo->ossdev.dev_name, strerror(errno));

The problem is that Wine depends on invariants about BlockAlign and
AvgBytesPerSec but does not enforce them when presented incorrect data.

MSDN says http://msdn.microsoft.com/en-us/library/bb669161(VS.85).aspx
"If wFormatTag is WAVE_FORMAT_PCM or WAVE_FORMAT_EXTENSIBLE, nBlockAlign
must be equal to the product of nChannels and wBitsPerSample divided by
8 (bits per byte).
If wFormatTag is WAVE_FORMAT_PCM, nAvgBytesPerSec should be equal to
the product of nSamplesPerSec and nBlockAlign."

In the source base up to now, Wine only corrects the format in the OSS
hardware mode.  That's why OSS HW works well.

It's too early to submit this as a patch as there are a few open issues:

- Does the bogus data originate from the app or is some other function
  in Wine returning this data?
A trace snippet from bug #19901 shows inconsistent data in the following call
to SetFormat following device creation:
PrimaryBufferImpl_Create (formattag=0x0001,chans=2,samplerate=44100,bytespersec=176400,blockalign=4,bitspersamp=16,cbSize=0)
DSOUND_PrimarySetFormat (formattag=0x0001,chans=1,samplerate=22050,bytespersec=88200,blockalign=4,bitspersamp=16,cbSize=0)

- Is there some central place in dsound for enforcing the invariant instead
  of identical code in the half dozen OSS/ALSA/CoreAudio/Jack etc. drivers?

- Is the modification of the WAVEOPENDESC structure allowed, as it seems
  to be a user-supplied structure, which should be handled read-only?

- Should the sound driver refuse to open when submitted such incorrect data?
Possibly not. MSDN says about SetFormat http://msdn.microsoft.com/en-us/library/bb206041(VS.85).aspx
"The method succeeds even if the hardware does not support the requested
format; DirectSound sets the buffer to the closest supported format.  To
determine whether this has happened, an application can call the GetFormat ..."
Thus, DS may set a format different from what was supplied. Presumably,
BlockAlign and AvgBytesPerSec play a minor role, as they can be derived
from the other values.

Experimentally, I had wineoss.drv/audio.c:supportedformat() refuse
inconsistent formats.  Wine nevertheless played the app's sound at bogus
speed after scanning all wineacm/mp3/xyz drivers (which it would not do
otherwise), taking an execution path I've not yet analysed.

- How does MS-Windows behave in the presence of bad data?
Additional tests are needed.

- I've not yet fixed the ALSA driver, but I expect similar improvements, at
  least in the emulation case, because the sound errors are the same in OSS
  and ALSA emulation modes.

This is exciting news about progress in Wine sound issues!
Bugzilla has errors about block size as old as 2005 still open.

Regards,
	Jörg Höhle

now OSS driver emulation -- and other drivers too -- works as well as OSS HW mode; block size error gone?

[Juan Lang]

Hi Jörg,

good news :)  Only one brief response, I don't know much about this area:

On Fri, Sep 4, 2009 at 5:09 AM, <Joerg-Cyril.Hoehle(a)t-systems.com> wrote:
> - Is there some central place in dsound for enforcing the invariant instead
>  of identical code in the half dozen OSS/ALSA/CoreAudio/Jack etc. drivers?

No, or, not to my knowledge.
--Juan

now OSS driver emulation -- and other drivers too -- works as well as OSS HW mode; block size error gone?

Juan Lang wrote:
> > - Is there some central place in dsound for enforcing the 
> > invariant instead of identical code in the half dozen
> > OSS/ALSA/CoreAudio/Jack etc. drivers?
> > No, or, not to my knowledge.

This is unfortunate.
Wine's dsound needs a channel back from winmm (which eventually calls
wineoss/winealsa etc. via wodOpen) to obtain the correct format data.
The OSS hardware mode does this (in wineoss.drv/audio.c) by updating
the supplied structure.  My patch does it for the OSS emulation mode.

However, testing on MS-Windows reveals that waveOutOpen() does not
modify the supplied WAVEFORMATEX structure.  Windows' wave*() seems to
completely ignore the incorrect nBlockAlign and nAvgBytesPerSample.
Apps can submit bogus data and still work.  So must Wine.

If Wine would not update the structure, there would be no channel back
to dsound.  So I'm wondering whether dsound/dsound.c can itself fix
the values.  Sadly I don't understand where the mapper and all these
adpcm/mp3/gsm drivers comes into play in dsound: Where can Wine set
  nBlockAlign == nChannels * wBitsPerSample/8 and
  nAvgBytesPerSec == nSamplesPerSec * nBlockAlign?
MSDN says in http://msdn.microsoft.com/en-us/library/bb669161(VS.85).aspx
"For non-PCM formats, this member must be computed according to the
manufacturer's specification for the format tag."
*Who* computes? The app? The mapper?

Note that msacm32.drv/wavemap.c:wodOpenHelper sets
 lpwfx->nBlockAlign and nAvgBytesPerSec
The line wodOpen:TRY: ... case MMSYSERR_NOERROR:
            wom->avgSpeedInner = wfx.nAvgBytesPerSec; goto found;
shows that nAvgBytesPerSec is considered an output variable after wodOpen().
The value need be correct, or crashes will likely follow.

In any case, I'm going to submit patches (in-place update) soon for OSS and
CoreAudio (ALSA later).  A cleaner solution is awaiting ideas.

Regards,
	Jörg Höhle
PS: While I've verified that MS-Windows' waveOutOpen does not modify the format
structure (given PCM), I've not tested what the dsound interface does.

now OSS driver emulation -- and other drivers too -- works as well as OSS HW mode; block size error gone?

Hi,

so I have submitted patches for OSS and coreaudio. ALSA will follow.

I also wrote a testcase, but am unsure about one issue.

The following makes sense (winmm/tests/wave.c):
todo_wine ok(origformat.nBlockAlign = format.nBlockAlign, "nBlockAlign fixed");

The problem is: whether or not I use todo_wine, the test will succeed
with some audio drivers and fails with others. How to deal with that?
Limit the test to wine_interactive testruns?

Should I submit this test now, knowing that it'll "fail" (to be more precise:
"succeed unexpectedly") with what people -- presumably -- use most on Linux: ALSA?

Thanks for your help,
	Jörg Höhle

[3/4] msi: GetDriveType and GetVolumeInformation require a path terminated by a slash.

Dan Kegel asked:
> Don't changes like this need corresponding conformance tests?
> There is one already for GetVolumeInformation in kernel32, see the commits made by Guy Albertelli in April this year.

Please see my mail from June where I located all uses of GetVolumeInformation and asked for review.
http://www.winehq.org/pipermail/wine-devel/2009-June/076733.html

In hindsight, my use of "pass" was misleading. It means "reviewers, please find out and check the callers of the code to see whether their supply the now required trailing slash."
As you can see from the list of commits, grepping for GetVolumeInformation, the change from April affected several places, some of which I found and fixed in June, another one Hans Leidekker now.
File msi/media.c which Hans changed now was listed in this mail.  This is unfortunate.

Looking at the mail above, this still leaves 5 places to check ("review" and "pass") in shell32, user32 and winedos.

Regards,
	Jörg Höhle