August 2005 - Wine-Devel - list.winehq.org

Re: DSOUND: prevent assertion in mixer.c

by Alex Villacís Lasso

Felix Nawothnig wrote: > >> Since the error of "length not a multiple of block size" is nonfatal, > > > Yes it is fatal. Errors mean that something which should not have > happened did happen - and we're not talking about something the API is > supposed to handle... there is wrong code in Wine, internal states > might have gone corrupted and you can no-longer be sure that Wine is > working correctly. > > The difference to an assert() is that it's probably possible to repair > the corrupted state but there is no guarantee for that since the error > might be caused by completly unrelated code. > > And this is exactly what we you do in your code - you dunno the reason > why mixpos became greater than buflen but you know how to handle it, > that's why you should put an ERR there. > Hmmm... I have a moderate collection of Japanese RPG games, for the sole purpose of testing them under Wine, and most, if not all of them reliably display the "length not a multiple of block size" when I run them. This error looks now more serious than I thought at first. I really need to look into it. When I said "nonfatal", I meant that the error does not force the program to terminate. I meant nothing about the seriousness of the error. >> unlike the assertion, I submitted the patch as it is. I will need to >> read more of both DirectSound and the source code in order to >> understand the implications of the non-multiple error, since there is >> another game from the same source (LittleWitch/FloatingFrameDirector) >> that displays the same non-multiple errors without triggering the >> assertion. What kind > > > In the game I tested the assertion failure was not really > reproducable, sometimes I played through the whole game (a demo) > without problems - it also seems to be timing related... In the game I tested, the assertion happened at the exact same point into the game (the shot of the girl in the water tank). I provided the link to the problematic demo along with the patch (http://www.littlewitch.jp/sirotume.exe) BTW, all the tests I do are made with the ALSA driver. The other drivers do not exercise the code I am trying to debug in DirectSound.

Aug. 2, 2005

Re: DSOUND: prevent assertion in mixer.c

by Felix Nawothnig

Alex Villacís Lasso wrote: >> I guess this is preluded by some "length not a multiple of block size" >> errors? I've been experiencing those errors with the same failed >> assertion in another game and came up with a similar patch but didn't >> submit since I think this just hides another bug as it should *not* >> happen that buf_mixpos becomes greater than buflen (and this >= above >> should probably be ==) at any time. >> >> IMHO you should at least add an ERR to that branch. > The "length not a multiple of block size" errors you mention is what > happens *instead* of the failed assertion when this patch is applied. That's strange. I thought the failed assertion was caused by mixpos being increased by the fragment size rounded up to the next block size multiple... (the blocksize was 2 so mixpos was always buflen+1 when the assertion failed) > Since the error of "length not a multiple of block size" is nonfatal, Yes it is fatal. Errors mean that something which should not have happened did happen - and we're not talking about something the API is supposed to handle... there is wrong code in Wine, internal states might have gone corrupted and you can no-longer be sure that Wine is working correctly. The difference to an assert() is that it's probably possible to repair the corrupted state but there is no guarantee for that since the error might be caused by completly unrelated code. And this is exactly what we you do in your code - you dunno the reason why mixpos became greater than buflen but you know how to handle it, that's why you should put an ERR there. > unlike the assertion, I submitted the patch as it is. I will need to > read more of both DirectSound and the source code in order to understand > the implications of the non-multiple error, since there is another game > from the same source (LittleWitch/FloatingFrameDirector) that displays > the same non-multiple errors without triggering the assertion. What kind In the game I tested the assertion failure was not really reproducable, sometimes I played through the whole game (a demo) without problems - it also seems to be timing related... > of setup or error needs to happen in an DirectSound application in order > to trigger this non-multiple warning? Well, fragments of the wrong size are mixed in... but I dunno where they come from since I dunno anything about the DSound API. :-) >> And shouldn't it be "%= dsb->buflen;"? I'd think that this causes >> looping until new stuff is mixed in... > I tested with the particular game, and it made no difference. The > comment is there so it is evident that I tested both. Maybe new samples are immediatly mixed in? Felix

Aug. 2, 2005

Re: DSOUND: prevent assertion in mixer.c

by Alex Villacís Lasso

Felix Nawothnig wrote: > Alex Villacís Lasso wrote: > >> wine-pthread: mixer.c:386: DSOUND_MixInBuffer: Assertion >> `adjusted_remainder >= 0' failed. >> wine: Unhandled exception (thread 000a), starting debugger... >> WineDbg starting on pid 0x8 > > > I guess this is preluded by some "length not a multiple of block size" > errors? I've been experiencing those errors with the same failed > assertion in another game and came up with a similar patch but didn't > submit since I think this just hides another bug as it should *not* > happen that buf_mixpos becomes greater than buflen (and this >= above > should probably be ==) at any time. > > IMHO you should at least add an ERR to that branch. > The "length not a multiple of block size" errors you mention is what happens *instead* of the failed assertion when this patch is applied. Since the error of "length not a multiple of block size" is nonfatal, unlike the assertion, I submitted the patch as it is. I will need to read more of both DirectSound and the source code in order to understand the implications of the non-multiple error, since there is another game from the same source (LittleWitch/FloatingFrameDirector) that displays the same non-multiple errors without triggering the assertion. What kind of setup or error needs to happen in an DirectSound application in order to trigger this non-multiple warning? >> --- wine-20050725-cvs/dlls/dsound/mixer.c 2005-06-21 >> 04:43:29.000000000 -0500 >> +++ wine-20050725-cvs-patch/dlls/dsound/mixer.c 2005-08-01 >> 02:16:42.000000000 -0500 >> @@ -491,6 +491,7 @@ >> if (dsb->leadin && (dsb->startpos <= dsb->buf_mixpos)) >> dsb->leadin = FALSE; /* HACK: see above */ >> } >> + else dsb->buf_mixpos = 0; /* %= dsb->buflen; */ > > > And shouldn't it be "%= dsb->buflen;"? I'd think that this causes > looping until new stuff is mixed in... > > Felix > I tested with the particular game, and it made no difference. The comment is there so it is evident that I tested both.

Aug. 2, 2005

Re: Move Win16 relay messages to the relay16 channel

by Alexandre Julliard

Uwe Bonnes <bon(a)elektron.ikp.physik.tu-darmstadt.de> writes: > RelayFromInclude doesn't follow CreateProcess, if I understand things right... Not sure what you mean, it's in the registry so child processes will certainly use the same options. -- Alexandre Julliard julliard(a)winehq.org

Aug. 2, 2005

Re: Move Win16 relay messages to the relay16 channel

by Uwe Bonnes

>>>>> "Felix" == Felix Nawothnig <felix.nawothnig(a)t-online.de> writes: Felix> Uwe Bonnes wrote: Felix> How about implementing magic values like %native% and %win16% for Felix> it then? "option-like" debug channels seem very unintuitive to Felix> me... Good idea! Go ahead with implementing ;-) And let's add %deferred% -- Uwe Bonnes bon(a)elektron.ikp.physik.tu-darmstadt.de Institut fuer Kernphysik Schlossgartenstrasse 9 64289 Darmstadt --------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

Aug. 2, 2005

Re: Move Win16 relay messages to the relay16 channel

by Felix Nawothnig

Uwe Bonnes wrote: >>>>> Why? >>>> When you're debugging 16bit programs, sometimes you only want to see >>>> 16bit calls.:) >>> Isn't your proposal similar to the recent "nrelay" proposal, where >>> only calls from native code is logged, but not wine code calling >>> other wine code? >> Why exactly do we need this (both nrelay and relay16) when there >> is RelayFromInclude? > RelayFromInclude doesn't follow CreateProcess, if I understand things right... How about implementing magic values like %native% and %win16% for it then? "option-like" debug channels seem very unintuitive to me... Felix

Aug. 2, 2005

Re: Move Win16 relay messages to the relay16 channel

by Uwe Bonnes

>>>>> "Felix" == Felix Nawothnig <felix.nawothnig(a)t-online.de> writes: Felix> Uwe Bonnes wrote: >>>> Why? >>> When you're debugging 16bit programs, sometimes you only want to see >>> 16bit calls.:) >> Isn't your proposal similar to the recent "nrelay" proposal, where >> only calls from native code is logged, but not wine code calling >> other wine code? Felix> Why exactly do we need this (both nrelay and relay16) when there Felix> is RelayFromInclude? RelayFromInclude doesn't follow CreateProcess, if I understand things right... -- Uwe Bonnes bon(a)elektron.ikp.physik.tu-darmstadt.de Institut fuer Kernphysik Schlossgartenstrasse 9 64289 Darmstadt --------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

Aug. 2, 2005

Re: test case demonstrating PeekMessage give up timeslices

by Alexandre Julliard

Felix Nawothnig <felix.nawothnig(a)t-online.de> writes: > I meant that using shm generally would lower the cost of a server > request and doing that extra yield would no longer be necessary > (although we'd still have the other yield due to the request itself > unless the queue is put into shm). If you mean passing the parameters through shared memory instead of a pipe, then no, it doesn't really make a difference. The cost is not the few bytes we need to copy across, it's the context switches. -- Alexandre Julliard julliard(a)winehq.org

Aug. 2, 2005

Re: Move Win16 relay messages to the relay16 channel

by Felix Nawothnig

Uwe Bonnes wrote: >>> Why? >> When you're debugging 16bit programs, sometimes you only want >> to see 16bit calls.:) > Isn't your proposal similar to the recent "nrelay" proposal, where only > calls from native code is logged, but not wine code calling other wine code? Why exactly do we need this (both nrelay and relay16) when there is RelayFromInclude? Felix

Aug. 2, 2005

Re: DSOUND: prevent assertion in mixer.c

by Felix Nawothnig

Alex Villacís Lasso wrote: > wine-pthread: mixer.c:386: DSOUND_MixInBuffer: Assertion > `adjusted_remainder >= 0' failed. > wine: Unhandled exception (thread 000a), starting debugger... > WineDbg starting on pid 0x8 I guess this is preluded by some "length not a multiple of block size" errors? I've been experiencing those errors with the same failed assertion in another game and came up with a similar patch but didn't submit since I think this just hides another bug as it should *not* happen that buf_mixpos becomes greater than buflen (and this >= above should probably be ==) at any time. IMHO you should at least add an ERR to that branch. > --- wine-20050725-cvs/dlls/dsound/mixer.c 2005-06-21 04:43:29.000000000 -0500 > +++ wine-20050725-cvs-patch/dlls/dsound/mixer.c 2005-08-01 02:16:42.000000000 -0500 > @@ -491,6 +491,7 @@ > if (dsb->leadin && (dsb->startpos <= dsb->buf_mixpos)) > dsb->leadin = FALSE; /* HACK: see above */ > } > + else dsb->buf_mixpos = 0; /* %= dsb->buflen; */ And shouldn't it be "%= dsb->buflen;"? I'd think that this causes looping until new stuff is mixed in... Felix

Aug. 2, 2005