[Bug 18916] New: Thief 2 crashes when bringing up in-game menu

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

Summary: Thief 2 crashes when bringing up in-game menu
           Product: Wine
           Version: 1.1.23
          Platform: PC
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: -unknown
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Created an attachment (id=21762)
 --> (http://bugs.winehq.org/attachment.cgi?id=21762)
Transcript of game run, complete with backtrace

After the commit below, Thief 2 crashes with the attached backtrace when
bringing up the in-game menu.  This menu involves switching screen resolution
to 640x480.

This appears to have been generated by the following commit:

commit f8c4832276fb740e527fed87aa27975f91546d26
Author: Henri Verbeet 
Date:   Fri Jun 5 10:10:48 2009 +0200

wined3d: Use GL_DEPTH24_STENCIL8_EXT for depth stencil formats.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

Michael Abbott  changed:

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

--- Comment #1 from Michael Abbott   2009-06-12 14:51:05 ---
Unable to CC Henri Verbeet  as e-mail not recognised
by bug tracker, trying gmail address instead.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |regression

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #2 from Michael Abbott   2009-06-13 09:36:33 ---
Created an attachment (id=21772)
 --> (http://bugs.winehq.org/attachment.cgi?id=21772)
Run log with  WINEDEBUG=+ddraw,+d3d,+d3d_caps

This is running current git with traces as suggested by Henri Verbeet.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #3 from Michael Abbott   2009-06-13 09:39:04 ---
I've checked the Thief 2 demo ... and alas, it seems to be *completely* broken,
I can't seem to get it to work at all on any 1.x version of Wine (haven't tried
0.9x versions), so we can't use this as a reference -- it crashes as soon as I
try to start a fresh game (and I have no saved games to try, so it probably
never worked very well...).

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #4 from Henri Verbeet   2009-06-15 04:02:43 ---
Created an attachment (id=21802)
 --> (http://bugs.winehq.org/attachment.cgi?id=21802)
revert

This should revert the patch (except for D24FS8, but that one isn't used by
Thief 2). Note that the backtrace looks somewhat similar to bug 18636, although
it isn't quite the same. Could be the same underlying issue.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #5 from Henri Verbeet   2009-06-15 06:14:24 ---
Could you add +d3d_surface to the trace?

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #6 from Michael Abbott   2009-06-15 06:35:09 ---
(In reply to comment #5)
> Could you add +d3d_surface to the trace?

Sure -- I'll do it this evening, and I'll report back on the effect of your
revert patch.  Do you want all four traces in the log?  (It'll be big!)

Is it worth looking at the Thief 2 demo?  I know that that isn't a regression
(as far as I can tell my copy has never worked), but presumably it uses the
same underlying technology ... so its failure might be quite instructive.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #7 from Henri Verbeet   2009-06-15 06:43:50 ---
(In reply to comment #6)
> (In reply to comment #5)
> > Could you add +d3d_surface to the trace?
>
> Sure -- I'll do it this evening, and I'll report back on the effect of your
> revert patch.  Do you want all four traces in the log?  (It'll be big!)

Yes, but the last 200 lines or so should do. I think we're calling
surface_upload_data() on the surface, but the data we're giving it isn't
actually in GL_UNSIGNED_INT_24_8_EXT format, so it'll probably try to read past
the end of the buffer.
> Is it worth looking at the Thief 2 demo?  I know that that isn't a regression
> (as far as I can tell my copy has never worked), but presumably it uses the
> same underlying technology ... so its failure might be quite instructive.

It might be worth looking at on its own, but it's probably not needed for this
bug.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #8 from Michael Abbott   2009-06-15 10:54:54 ---
Created an attachment (id=21813)
 --> (http://bugs.winehq.org/attachment.cgi?id=21813)
WINEDEBUG=+ddraw,+d3d,+d3d_caps,+d3d_surface log

Last 10,000 lines of crash run with
WINEDEBUG=+ddraw,+d3d,+d3d_caps,+d3d_surface

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #9 from Michael Abbott   2009-06-15 10:59:43 ---
(In reply to comment #4)
> Created an attachment (id=21802)

--> (http://bugs.winehq.org/attachment.cgi?id=21802) [details]
> revert
>
> This should revert the patch (except for D24FS8, but that one isn't used by
> Thief 2). Note that the backtrace looks somewhat similar to bug 18636, although
> it isn't quite the same. Could be the same underlying issue.

With this patch it seems to work fine.  At least this confirms the issue.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #10 from Henri Verbeet   2009-06-16 03:20:29 ---
I think http://www.winehq.org/pipermail/wine-patches/2009-June/074312.html
should fix this.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #11 from Michael Abbott   2009-06-16 05:30:21 ---
(In reply to comment #10)
> I think http://www.winehq.org/pipermail/wine-patches/2009-June/074312.html
> should fix this.

I'm a bit troubled by this computation:

WORD d15 = source[x] & 0xfffe;
    DWORD d24 = d15 * 0x100 + (d15 * 0xff80 + 0x3fff80) / 0x7fff00;

If I multiply the arithmetic out as if this was floating point, this comes to

d24 = 256.0078 * d15 + 0.5

Don't get it.  You're not relying on d15*0xff80 overflowing and doing something
evil ... are you?  Certainly if the top bit of d15 is set then this overflows
as an unsigned value ... and then the division, in this inner loop, is not
going to speed things up.

Why not just

DWORD d24 = (source[x] & 0xfffe) << 8;

?  What is the conversion rule that's really being applied here?

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #12 from Henri Verbeet   2009-06-16 06:02:38 ---
(In reply to comment #11)
> (In reply to comment #10)
> > I think http://www.winehq.org/pipermail/wine-patches/2009-June/074312.html
> > should fix this.
>
> I'm a bit troubled by this computation:
> WORD d15 = source[x] & 0xfffe;
> DWORD d24 = d15 * 0x100 + (d15 * 0xff80 + 0x3fff80) / 0x7fff00;
>
> If I multiply the arithmetic out as if this was floating point, this comes to
>
> d24 = 256.0078 * d15 + 0.5
>
> Don't get it.  You're not relying on d15*0xff80 overflowing and doing something
> evil ... are you?  Certainly if the top bit of d15 is set then this overflows
> as an unsigned value ... and then the division, in this inner loop, is not
> going to speed things up.
>
> Why not just
> DWORD d24 = (source[x] & 0xfffe) << 8;
>
> ?  What is the conversion rule that's really being applied here?

"DWORD d24 = (source[x] & 0xfffe) << 8;" would map 0x7fff to 0xfffe00 rather
than 0xffffff. We essentially need to multiply by (0xffffff/0x7fff) and round
correctly, which would result in something like "d24 = (d15 >> 1) * 512.0156 +
0.5", which in turn corresponds to the "d24 = 256.0078 * d15 + 0.5" you got
above.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #13 from Henri Verbeet   2009-06-16 06:06:30 ---
(And we can't simply do ((d15 >> 1) * 0xffffff) / 0x7fff because that would
either need long long or overflow.)

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #14 from Michael Abbott   2009-06-16 07:59:35 ---
(In reply to comment #12)
> We essentially need to multiply by (0xffffff/0x7fff) and round correctly

Ok, I get it.

How about this then (I take it WORD is unsigned, otherwise this won't quite
work):

WORD d15 = source[x] & 0xfffe;
    DWORD d24 = (d15 << 8) + (d15 >> 7);

?  Saves multiplies and more importantly divides and is pretty accurate:
effectively I've rewritten

1/(2^15-1) ~~ 2^-15 + 2^-30

(the 2^-45 term won't interest us) and so

(2^24-1)/(2^15-1) * d15/2  ~~  2^8 d15 + 2^-7 d15 .

Certainly this works properly for d15 = 0xfffe.  I'm not worried about the
dangling half bit.  I can do a detailed error analysis if you're not convinced,
the worst error is just under half a bit -- if we really care, I can try to
figure out how to get it back, but I'm sure we don't ;-)

I do this kind of nonsense a lot on the ARM, where divide is to be studiously
avoided in inner loops!

I'll test your patch this evening.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #15 from Henri Verbeet   2009-06-16 08:44:28 ---
(In reply to comment #14)
> How about this then (I take it WORD is unsigned, otherwise this won't quite
> work):
> WORD d15 = source[x] & 0xfffe;
> DWORD d24 = (d15 << 8) + (d15 >> 7);
>
> ?  Saves multiplies and more importantly divides and is pretty accurate:
> effectively I've rewritten
> 1/(2^15-1) ~~ 2^-15 + 2^-30
>
> (the 2^-45 term won't interest us) and so
> (2^24-1)/(2^15-1) * d15/2  ~~  2^8 d15 + 2^-7 d15 .
>
> Certainly this works properly for d15 = 0xfffe.  I'm not worried about the
> dangling half bit.  I can do a detailed error analysis if you're not convinced,
> the worst error is just under half a bit -- if we really care, I can try to
> figure out how to get it back, but I'm sure we don't ;-)

That half bit does get rounded up to a whole bit on occasion of course, but I
can live with that. As far as I'm concerned, feel free to submit that as a
patch. You might as well just write "dest[x] = (((source[x] & 0xfffe) << 16) +
((source[x] & 0xff80) << 1)) | (source[x] & 0x1);" though.
> I do this kind of nonsense a lot on the ARM, where divide is to be studiously
> avoided in inner loops!

I don't think it's quite as expensive on modern x86, but it doesn't hurt to
avoid.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #16 from Michael Abbott   2009-06-16 12:10:13 ---
(In reply to comment #10)
> I think http://www.winehq.org/pipermail/wine-patches/2009-June/074312.html
> should fix this.

Seems to work ok.  Not quite sure how to submit a patch to your patch, to be
honest -- seems I have to wait for it to go into git first, and then I can
patch against git.  wine-patches is rather a busy list...

(In reply to comment #15)
> You might as well just write "dest[x] = (((source[x] & 0xfffe) << 16) +
> ((source[x] & 0xff80) << 1)) | (source[x] & 0x1);" though.

Seems uglier, but I take your point.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #17 from Henri Verbeet   2009-06-16 12:15:41 ---
(In reply to comment #16)
> Seems to work ok.  Not quite sure how to submit a patch to your patch, to be
> honest -- seems I have to wait for it to go into git first, and then I can
> patch against git.

Yeah. It's in as 23231d5a62b2c23d0ec336b9e4b24a346eef3527 now though.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #18 from Michael Abbott   2009-06-16 17:25:05 ---
(In reply to comment #17)
> (In reply to comment #16)
> > Seems to work ok.  Not quite sure how to submit a patch to your patch, to be
> > honest -- seems I have to wait for it to go into git first, and then I can
> > patch against git.
>
> Yeah. It's in as 23231d5a62b2c23d0ec336b9e4b24a346eef3527 now though.

My patch submitted as
http://www.winehq.org/pipermail/wine-patches/2009-June/074355.html

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

--- Comment #19 from Michael Abbott   2009-06-17 03:21:33 ---
This is all completely irrelevant now, but I was puzzling over how to get that
last bit of rounding right in this calculation:
> WORD d15 = source[x] & 0xfffe;
> DWORD d24 = (d15 << 8) + (d15 >> 7);

This produces a worst error of +-0.9825; if we allow for the fact that we have
to round (so can subtract 0.5 from this error), we have an error of +-0.4825.

Here's the best I can do (really not suggesting this goes into the code, this
is just one of those details that bugs me until I can write it out):

DWORD d15 = (source[x] & 0xfffe) << 7;
    DWORD d32 = (d15 << 9) + (d15 >> 6) - (d15 >> 15) + (1 << 7)
    DWORD d24 = d32 >> 8;

This isn't perfect, but we *nearly* get the bottom bit right -- in fact, this
only get it wrong 166 times out of 32768.  The worst error is -0.5106..+0.5029,
or after compensating for rounding, -0.0106..+0.0029.

Ah well -- your original calculation gets this perfect (error +-0.4999, or 0
after rounding).  These elusive last bits in simplified division are annoying,
I have this annoyance elsewhere (again, in a context where the proper response
is "who cares?").

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

Michael Abbott  changed:

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

--- Comment #20 from Michael Abbott   2009-06-17 07:02:08 ---
Commit 23231d5a62b2c23d0ec336b9e4b24a346eef3527 fixes this.

[Bug 18916] Thief 2 crashes when bringing up in-game menu

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

Alexandre Julliard  changed:

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

--- Comment #21 from Alexandre Julliard   2009-06-19 11:10:25 ---
Closing bugs fixed in 1.1.24.