On Sun, Sep 27, 2015 at 12:42:42AM +0800, Bruno Jesus wrote:
Signed-off-by: Bruno Jesus 00cpxxx@gmail.com
Tomb Raider 3 uses a single archive with all wav files inside, when playing some specific sounds it passes incorrect lengths of data that reach the next wav file header. This causes an assert in the code that is now gracefully handled.
diff --git a/dlls/msadp32.acm/msadp32.c b/dlls/msadp32.acm/msadp32.c index 2c61575..0924163 100644 --- a/dlls/msadp32.acm/msadp32.c +++ b/dlls/msadp32.acm/msadp32.c @@ -249,9 +249,17 @@ static void cvtSSms16K(const ACMDRVSTREAMINSTANCE *adsi, { const unsigned char* in_src = src;
assert(*src <= 6);
/* Catch a problem from Tomb Raider III (bug 21000) where it passes* invalid data after a valid sequence of blocks */if (*src > 6 || *(src + 1) > 6){/* Recalculate the amount of used output buffer. We are not changing* nsrc, let's assume the bad data was parsed */*ndst -= nblock * nsamp_blk * adsi->pwfxDst->nBlockAlign;ERR("Invalid ADPCM data, stopping conversion\n");
Looks reasonable, thanks. I think you should change that message to a WARN. Audio methods are often called thousands of times per second, so spamming an ERR can be very obnoxious.
Andrew
On Monday, September 28, 2015, Andrew Eikum aeikum@codeweavers.com wrote:
On Sun, Sep 27, 2015 at 12:42:42AM +0800, Bruno Jesus wrote:
Signed-off-by: Bruno Jesus <00cpxxx@gmail.com javascript:;>
Tomb Raider 3 uses a single archive with all wav files inside, when playing some specific sounds it passes incorrect lengths of data that reach the next wav file header. This causes an assert in the code that is now gracefully handled.
...
Looks reasonable, thanks. I think you should change that message to a WARN. Audio methods are often called thousands of times per second, so spamming an ERR can be very obnoxious.
Will do, thanks for the review. Does this mean I should add a Signed-off-by with your name too? Or that is not related to patch reviewing.
On Mon, Sep 28, 2015 at 09:38:38PM +0800, Bruno Jesus wrote:
Will do, thanks for the review. Does this mean I should add a Signed-off-by with your name too? Or that is not related to patch reviewing.
I'm not 100% sure, but I think I should add the S-o-b as a reply to your email to wine-patches. Unless it's for sharing authorship, I don't think you can add my S-o-b, since you are not me.
Andrew
On 28.09.2015 15:42, Andrew Eikum wrote:
On Mon, Sep 28, 2015 at 09:38:38PM +0800, Bruno Jesus wrote:
Will do, thanks for the review. Does this mean I should add a Signed-off-by with your name too? Or that is not related to patch reviewing.
I'm not 100% sure, but I think I should add the S-o-b as a reply to your email to wine-patches. Unless it's for sharing authorship, I don't think you can add my S-o-b, since you are not me.
Andrew
Was just about to answer the same. Do not add S-o-b yourself, unless you use it to track multiple authors, or you are 100% sure that Andrew is _really_ fine with it. In this case, when you are doing additional modifications, you can never be sure that this is the case. ;)
At WineConf there was also the decision that an answer like "the patch looks good" is not equivalent to a S-o-b. The latter is more strong because it implies that the person is also taking responsibility for it, and that it was a full review including running the tests, not just a quick look.
Regards, Sebastian
-
Andrew Eikum -
Bruno Jesus -
Sebastian Lackner