Re: [PATCH 4/5] wined3d: Reject unsupported pitches in wined3d_surface_update_desc().
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2015-02-18 um 14:20 schrieb Matteo Bruni:
+ if (pitch & (texture_resource->format->byte_count - 1)) + { + WARN("Pitch unsupported, not a multiple of the texture format byte width.\n"); + return WINED3DERR_INVALIDCALL; + } I'd prefer this to be an ERR instead, and the return statement removed. The idea is that since we don't ever expect this check to be true it shouldn't result in different behavior and can be compiled out in a build where debugging is disabled.
The basic idea originated from Henri: https://www.winehq.org/pipermail/wine-devel/2013-August/100974.html . -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU5KrDAAoJEN0/YqbEcdMwdj0P/0kphTArNNtcxtIbt5noekiX wTwi7X6YhxkjryHENqvYKKO8ckBftBoxJHo9o4I82Cc+IRvj9GvrvvuZ5FuiMsJO 6dGmo5JSHnGGNG3rl/oZt31v3P/1LAba3GoFAgRwBfXCULue6gNRyC8+57ahlbLy xVWc2m249URRP3ML/O9+bXlzA61oQdS+5q54T+jmYv3X8K8AgDHATu114mcpxaAq hkHIKmW2WCoigCBrNHQTzcxMG3lsavMbnuMxSvQ/01/mwrKvmYYkvvBtR/AVI6W+ I7HLF4KZlz5BLZnS71joW2LLL35/z/3qtmaHi8dq/kCMaajow9dBQmstcUFvwJ4p UknSxgcCOgKreAZH3yTOH5LNgf8qZvB21wQ5Cn0NGEBW7bZKdUxfRftFN8IQY1Ht NhGB9cmvo3Ozv0z+i7wMfcXjntT9wL/2H77RqC4Xb9Z/igo+E2LJN0XqQYvwvtj7 wG4Fw5rH7vrO4g/IcfFYvbT7CMUYrPWtFaTfiMn5IOQZ4mx2gacktHcpZJnCUjw5 /uTQo+089YosnM+xY/WvDnjjI13svZt5c3gUMlvha30RjsX7PDfih/vNrr0qR5Y/ C6TLk8ffbmPOX6TTobndfZrWdb2sP3d7x1T1tx9nMAKJG0HFy5wvFoygEDOzFRYU tG/x7wnrmr3fYfUSEPJY =LuAf -----END PGP SIGNATURE-----
On 18 February 2015 at 16:07, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Am 2015-02-18 um 14:20 schrieb Matteo Bruni:
+ if (pitch & (texture_resource->format->byte_count - 1)) I don't think this would do the right thing for formats where byte_count isn't a power of two. I also think the check belongs in wined3d_texture_update_desc(), since otherwise the texture would be in an inconsistent state if it fails. (That actually also applies to failing to create the DIB section, but that one seems much harder to avoid.)
+ { + WARN("Pitch unsupported, not a multiple of the texture format byte width.\n"); + return WINED3DERR_INVALIDCALL; + } I'd prefer this to be an ERR instead, and the return statement removed. The idea is that since we don't ever expect this check to be true it shouldn't result in different behavior and can be compiled out in a build where debugging is disabled.
It's a bit debatable in this case, since that depends on assumptions about callers outside of wined3d.
2015-02-18 16:44 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 18 February 2015 at 16:07, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Am 2015-02-18 um 14:20 schrieb Matteo Bruni:
+ if (pitch & (texture_resource->format->byte_count - 1)) I don't think this would do the right thing for formats where byte_count isn't a power of two. I also think the check belongs in wined3d_texture_update_desc(), since otherwise the texture would be in an inconsistent state if it fails. (That actually also applies to failing to create the DIB section, but that one seems much harder to avoid.)
In theory there are no non-power of two byte_count texture formats from ddraw and d3d9ex always passes 0 as pitch. Actually initially I wrote the check as: if (WARN_ON(d3d) && pitch % texture_resource->format->byte_count) I can go back to something like that, it's probably more consistent with the point below.
+ { + WARN("Pitch unsupported, not a multiple of the texture format byte width.\n"); + return WINED3DERR_INVALIDCALL; + } I'd prefer this to be an ERR instead, and the return statement removed. The idea is that since we don't ever expect this check to be true it shouldn't result in different behavior and can be compiled out in a build where debugging is disabled.
It's a bit debatable in this case, since that depends on assumptions about callers outside of wined3d.
Yes, that was my reasoning for using WARN. I can change that though. I'm certainly going to move the check to wined3d_texture_update_desc() as you suggest.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2015-02-18 um 17:10 schrieb Matteo Bruni:
2015-02-18 16:44 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>:
It's a bit debatable in this case, since that depends on assumptions about callers outside of wined3d.
Yes, that was my reasoning for using WARN. I can change that though. I'm certainly going to move the check to wined3d_texture_update_desc() as you suggest. Fwiw, I don't have strong feelings about this, so feel free to do what you think fits best. My thinking is that the entire patch is based on assumptions about the client libraries. If this assumption breaks it's worth making the resulting message more verbose than a WARN. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2
iQIcBAEBAgAGBQJU5LziAAoJEN0/YqbEcdMwD3oP/1ebOCguJuQqAVCjnGmihlVZ 8+imGRXdiRl83qvz+7g/D0j++DIpMLQdAP0GwkYsVKzkGh3lubkB8dm+/pRCB4ug xBXE8rDDlLYhlBgBQ6pZ2RkfSdGgmEwv1kZB5HXVSkolSM4L5UyUrfXG9zUjW6ki e4WyhYvpoJlK5wpJlndcqtGFoCocDE/3kGrIc6HvXNciY6wlRIu262anjXzNmFey sFZyvRhLTD10wx1CipG6VUY5aAEnmOALKA8N0Jw6CBnBi2DCvKcwEtmoZAAHMo68 fUBwC+W/E8pqApfF1FNju/3ukh24S1pgWzrb7LX+iiLRBeCdhV+UswbV5iL5QPrf v3r/wE5aL7QreP05vcnRqiSw+6/2b33rZ2vM4pdz+TOkH25DyBhUtYC0SsE4r8c1 7Vr0ofcJi/nzaEpmVdVbNtEFi0lNaxN7qA7HG7enaKYWnRooDVq/IVPfFoVuhtFl yXdcAt2byCQbuy9rSHvqXdtLejXqdgw1My1XOpSrJ+LiVncDoZEvi0TPa5M7q3Fx FJuZ/8jptJIxCvITbRkSCFMAQe8Nb5Ebj4Jdj77ePJrzco9Rmvf6CzyJk66P7Sw0 xq4kZTN9sp6yjdpj/wzMxEVx3J0p5wwIq7cBVvHfFIhtSdOqXpZqYqxTCqekEW2x cOFtmivT1TluHiUYstKd =E7V4 -----END PGP SIGNATURE-----
On 18 February 2015 at 17:25, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Fwiw, I don't have strong feelings about this, so feel free to do what you think fits best. My thinking is that the entire patch is based on assumptions about the client libraries. The conceptual difference is that if we just reject the pitch in wined3d it becomes part of the API, so if e.g. ddraw tries to set an invalid pitch that's ddraw's problem. While if we don't, it's like we're pretending that pitches like that are supported, but we really hope nothing ever uses them, so if something sets an invalid pitch it's wined3d's problem to make it work somehow. The distinction is of course a bit artificial because everything is under our control, but the former seems more proper to me.
If this assumption breaks it's worth making the resulting message more verbose than a WARN. Possibly, although at least ddraw_surface_create() will already print an ERR if wined3d_texture_update_desc() fails.
participants (3)
-
Henri Verbeet -
Matteo Bruni -
Stefan Dösinger