[Bug 42546] New: DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Bug ID: 42546
           Summary: DSOUND_PrimaryOpen() incorrect set buf size
           Product: Wine
           Version: 2.2
          Hardware: x86
                OS: FreeBSD
            Status: UNCONFIRMED
          Severity: major
          Priority: P2
         Component: directx-dsound
          Assignee: [e-mail]
          Reporter: [e-mail]

Wine (1.x, 2.x) crashes on FreeBSD 11 release with wineoss.drv
With winealsa.drv on FreeBSD 11 - OK.
With winoss.drv on FreeBSD 10 - OK.

I made some investigation and make patch to fix problem.
I test it and it work. But looks like dsound needs review.

DSOUND_ReopenDevice() call DSOUND_PrimaryOpen() with forcewave = FALSE
DSOUND_PrimaryOpen()
calculate:
    new_buflen = device->buflen;
    new_buflen -= new_buflen % wfx->nBlockAlign;
(wrong alig code, but newer mind)
Then calculated
        DWORD alloc_len = frames * sizeof(float);
and allocated buffer
        if (device->buffer)
            newbuf = HeapReAlloc(GetProcessHeap(), 0, device->buffer,
alloc_len);
        else
            newbuf = HeapAlloc(GetProcessHeap(), 0, alloc_len);
final: save new buffer pointer and size:
    device->buffer = newbuf;
    device->buflen = new_buflen;
!!!
We allocate 6144 (0x00001800) bytes but set buf size to 65536.
DSOUND_MixToPrimary() and norm16() in DSOUND_mixthread use device->buflen and
corrupt heap after first call.

First I make more proper alignment:
        new_buflen = (device->buflen + wfx->nBlockAlign);
        new_buflen -= (new_buflen % wfx->nBlockAlign);
and change calculation to:
        DWORD alloc_len = ((new_buflen / wfx->nBlockAlign) * sizeof(float));
It works.

Next I replace alloc_len->new_buflen and return original calc code:
        new_buflen = (frames * sizeof(float));
This work but sound with a bit noise and game crash after some time.

Finnaly:
        new_buflen = ((frames + 1) * wfx->nBlockAlign * sizeof(float));
This work OK.
This not looks like proper buf size.

Also in DSOUND_ReopenDevice() after DSOUND_PrimaryOpen():
    device->fraglen = frag_frames * wfx->nBlockAlign;
    device->aclen = aclen_frames * wfx->nBlockAlign;

device->buflen should somehow be synced with this.

606723.622:0034:0035:trace:heap:RtlAllocateHeap (0x110000,70000062,00000030):
returning 0x11d438
606723.622:0034:0035:trace:oss:AudioClient_Start (0x1ecb38) now playing...
606723.622:0034:0035:trace:oss:AudioClient_GetStreamLatency
(0x1ecb38)->(0x335f590)
606723.622:0034:0035:trace:oss:AudioClient_GetBufferSize
(0x1ecb38)->(0x335f58c)
606723.622:0034:0035:trace:oss:AudioClient_GetBufferSize buffer size: 3840
606723.622:0034:0035:trace:dsound:DSOUND_ReopenDevice period 11 ms fraglen 2048
buflen 6144
606723.622:0034:0035:trace:dsound:DSOUND_PrimaryOpen (0x11e4d8)
606723.622:0034:0035:trace:heap:RtlAllocateHeap (0x110000,70000062,00001800):
returning 0x1fc1e8
606723.622:0034:0035:trace:dsound:DSOUND_PrimaryOpen buflen: 65536, fraglen: 0
606723.622:0034:0035:trace:oss:AudioClient_IsFormatSupported (0x1ecb38)->(0,
0x335f650, 0x335f64c)
...
606723.688:0034:003e:trace:dsound:DSOUND_mixthread (0x11e4d8)
606723.688:0034:003e:trace:dsound:DSOUND_PerformMix (0x11e4d8)
606723.688:0034:003e:trace:oss:AudioClient_GetCurrentPadding
(0x1ecb38)->(0x1448f6b8)
606723.688:0034:003e:trace:oss:AudioClient_GetCurrentPadding pad: 0
606723.688:0034:003e:warn:dsound:DSOUND_PerformMix Probable buffer underrun
606723.688:0034:003e:trace:oss:AudioRenderClient_GetBuffer (0x1ecb38)->(1536,
0x1448f6b0)
606723.690:0034:003e:trace:heap:RtlAllocateHeap (0x110000,70000062,00001810):
returning 0x141908b0
606723.690:0034:003e:trace:dsound:DSOUND_MixToPrimary (0,6144)
606723.690:0034:003e:trace:dsound:norm16 0x1fc1e8 - 0x141908b0 6144
606723.690:0034:003e:trace:oss:AudioRenderClient_ReleaseBuffer
(0x1ecb38)->(1536, 0)
606723.690:0034:003e:trace:oss:AudioRenderClient_ReleaseBuffer writen: 6144
606723.690:0034:003d:err:heap:HEAP_ValidateInUseArena Heap 0x110000: block
0x1fc1e8 tail overwritten at 0x1fd9e8 (byte 0/8 == 0x00)
Heap: 0x110000
...
Sub-heap 0x110014: base=0x110000 size=00110000 committed=00110000

Block    Arena   Stat   Size    Id
...
0x1f0118 00bedead pend 00000430
0x1f0550 00bedead pend 00002c18
0x1f3170 00bedead pend 00000430
0x1f35a8 00bedead pend 00008c30
0x1fc1e0 00455355 used 00001808
0x1fd9f0 00000000 pend 00000000
0x1fd9f8 00000000 pend 00000000
0x1fda00 00000000 pend 00000000
0x1fda08 00000000 pend 00000000

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Ivan_83  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #1 from Bruno Jesus  ---
Not sure if it is related but today earlier a patch related to this function
was sent to the list.
http://source.winehq.org/patches/data/130782

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #2 from Ivan_83  ---
Created attachment 57448
  --> https://bugs.winehq.org/attachment.cgi?id=57448
Quick crash fix + more trace info

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #3 from Ivan_83  ---
(In reply to Bruno Jesus from comment #1)
> Not sure if it is related but today earlier a patch related to this function
> was sent to the list.
> http://source.winehq.org/patches/data/130782

new_buflen = frames * sizeof(float);
as I write before - crash after a little time of work, sound with noise - looks
like this is not enough and some little mem peace after buf used in this case.

new_buflen = ((frames + 1) * wfx->nBlockAlign * sizeof(float));
My version - ok.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Ivan_83  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Andrew Eikum  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #4 from Andrew Eikum  ---
Created attachment 57459
  --> https://bugs.winehq.org/attachment.cgi?id=57459
Use the same buffer length for intermediate buffer

Ivan, thanks for the excellent analysis. Can you try applying Huw's two patches
(or, wait for them to be merged into Wine soon), and then apply this diff on
top? Let me know if it helps.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #5 from Ivan_83  ---
Yes, I will try it.

But this patch will allocate less then:
new_buflen = ((frames + 1) * wfx->nBlockAlign * sizeof(float));
and
new_buflen = (frames  * sizeof(float)); - I have test it and it cause crash.

wfx->nBlockAlign - in most cases I see =2, sizeof(float)=4.
And it does not change:
    device->fraglen = frag_frames * wfx->nBlockAlign;
    device->aclen = aclen_frames * wfx->nBlockAlign;
so device->fraglen and device->aclen will be bigger (in some cases) than
device->buflen and allocated mem size.
Next, device->fraglen and device->aclen will be used some where and corrupt
heap...again.

IMHO, refactoring needed and DSOUND_PrimaryOpen() should be included into
DSOUND_ReopenDevice() and all sizes should be calculated and set in ONE(!!!)
place.
All playing with patch DSOUND_PrimaryOpen() is waste of time.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #6 from Huw Davies  ---
Yes, I think there are clearly still problems.  An example is in
DSOUND_PerformMix() where in the normfunction != NULL case, the call to
DSOUND_MixToPrimary() uses device->buffer but maxq doesn't seem to be
constrained to buflen.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #7 from Huw Davies  ---
Actually, shouldn't new_buflen = frames * wfx->nChannels * sizeof(float) ?

I don't get what fraglen is doing, but this at least would make it match aclen
(with the appropriate conversion to deal with the change of type).

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #8 from Ivan_83  ---
(In reply to Huw Davies from comment #7)
> Actually, shouldn't new_buflen = frames * wfx->nChannels * sizeof(float) ?
>
> I don't get what fraglen is doing, but this at least would make it match
> aclen (with the appropriate conversion to deal with the change of type).

I think not.
nChannels never used in code.
Buf size always in frames (but frames count some where include nChannels) and
other weird (for me) things like aclen and fraglen.

To fix crash and prevent in future regression:
    device->buffer = newbuf;
    device->buflen = new_buflen;
    device->fraglen = frag_frames * wfx->nBlockAlign;
    device->aclen = aclen_frames * wfx->nBlockAlign;
must be grouped into one place and calculated correctly.

I wrote:
    new_buflen = ((frames + 1) * wfx->nBlockAlign * sizeof(float));
just to get buf size is bugger than later set in device->fraglen,
device->aclen.
wfx->nBlockAlign - just to not wrote some magic constant like 128 to bee 100%
that allocated mem is bigger that can be set as size some where later.

Stop playing around new_buflen in DSOUND_PrimaryOpen() and do it right once for
long time.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #9 from Huw Davies  ---
(In reply to Ivan_83 from comment #8)
> Stop playing around new_buflen in DSOUND_PrimaryOpen() and do it right once
> for long time.

Where it's set is essentially irrelevant, what matters is the correct value.
While I agree the code is a mess as it stands, it doesn't seem too unreasonable
to me to have the buffer allocation in a separate helper function.

Now, back to the actual discussion in hand.
https://msdn.microsoft.com/en-us/library/windows/desktop/dd370866(v=vs.85).a...
states that:

The size in bytes of an audio frame is calculated as
   the number of channels in the stream multiplied by the
   sample size per channel. For example, the frame size is
   four bytes for a stereo (2-channel) stream with 16-bit samples.

i.e. a frame contains nChannels samples.  So if we're allocating memory for n
frames stored as floats, we need n * nChannels * sizeof(float) bytes.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #10 from Ivan_83  ---
> Now, back to the actual discussion in hand.
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd370866(v=vs.85).
> aspx
> states that:
>
> The size in bytes of an audio frame is calculated as
>    the number of channels in the stream multiplied by the
>    sample size per channel. For example, the frame size is
>    four bytes for a stereo (2-channel) stream with 16-bit samples.
>
> i.e. a frame contains nChannels samples.  So if we're allocating memory for
> n frames stored as floats, we need n * nChannels * sizeof(float) bytes.

Looks OK.
But
    device->fraglen = frag_frames * wfx->nBlockAlign;
    device->aclen = aclen_frames * wfx->nBlockAlign;
steel have values that look a little weird.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Ivan_83  changed:

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

--- Comment #11 from Ivan_83  ---
Created attachment 57472
  --> https://bugs.winehq.org/attachment.cgi?id=57472
Crash fix + refactoring

Well, DSOUND_PrimaryOpen() is done.
All works fine.
Please review and merge it.

But mixer module need more checks.
I m move up check in DSOUND_PerformMix()
    if (maxq > device->buflen)
        maxq = device->buflen;
and then make a test: set
            new_buflen = sizeof(float) * (max(aclen, (3 * fraglen)) /
wfx->nBlockAlign);
and got small noises and crash on exit.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

--- Comment #12 from Huw Davies  ---
(In reply to Ivan_83 from comment #11)
> But mixer module need more checks.
> I m move up check in DSOUND_PerformMix()
>    if (maxq > device->buflen)
>    	maxq = device->buflen;
> and then make a test: set
>             new_buflen = sizeof(float) * (max(aclen, (3 * fraglen)) /
> wfx->nBlockAlign);
> and got small noises and crash on exit.

The problem with this is that maxq corresponds to bytes
in the audio client buffer, while buflen is bytes in the
float buffer, so you're not comparing like with like.

I've sent in four patches which tidy this up by using
frame counts.  The final one of them fixes an issue with
the initial assignment of maxq (now called 'frames').

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #13 from [e-mail] ---
(In reply to Huw Davies from comment #12)
> (In reply to Ivan_83 from comment #11)
> > But mixer module need more checks.
> > I m move up check in DSOUND_PerformMix()
> >    if (maxq > device->buflen)
> >    	maxq = device->buflen;
> > and then make a test: set
> >             new_buflen = sizeof(float) * (max(aclen, (3 * fraglen)) /
> > wfx->nBlockAlign);
> > and got small noises and crash on exit.
>
> The problem with this is that maxq corresponds to bytes
> in the audio client buffer, while buflen is bytes in the
> float buffer, so you're not comparing like with like.
>
> I've sent in four patches which tidy this up by using
> frame counts.  The final one of them fixes an issue with
> the initial assignment of maxq (now called 'frames').

Were the patches merged or pending? I don't know an easy way to look for old
patches.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Alistair Leslie-Hughes  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #14 from Alistair Leslie-Hughes  ---
(In reply to winetest from comment #13)
> > the initial assignment of maxq (now called 'frames').
>
> Were the patches merged or pending? I don't know an easy way to look for old
> patches.

The only one not applied.
https://www.winehq.org/pipermail/wine-patches/2017-March/158830.html

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

joaopa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #15 from joaopa  ---
Does the bug still occurs with wine-4.0-rc2?

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Ivan_83  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #16 from Ivan_83  ---
(In reply to joaopa from comment #15)
> Does the bug still occurs with wine-4.0-rc2?

Can be closed, all works fine now.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Ivan_83  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #17 from Ivan_83  ---
https://github.com/wine-mirror/wine/commit/134b684fd90423490e7eb2395db88df68...

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |134b684fd90423490e7eb2395db
                   |                            |88df683d8fdc1
             Status|CLOSED                      |RESOLVED

--- Comment #18 from Austin English  ---
(In reply to Ivan_83 from comment #17)
> https://github.com/wine-mirror/wine/commit/
> 134b684fd90423490e7eb2395db88df683d8fdc1#diff-
> b4bc0648b1d177a4cc62e61f8158b614

Thanks, but we'll close with the next Wine release.

[Bug 42546] DSOUND_PrimaryOpen() incorrect set buf size

https://bugs.winehq.org/show_bug.cgi?id=42546

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #19 from Alexandre Julliard  ---
Closing bugs fixed in 4.0-rc3.