Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Victor]

On Wednesday 30 July 2008 15:15:45 you wrote:
> Am Mittwoch, den 30.07.2008, 15:03 +0400 schrieb Victor:
> > > You also have to expand types properly, i.e. converting a
> > > component from 4-bit to 8-bit isn't just a shift.
> >
> > In all places where I saw pixel format conversion before, it was _always_
> > done by shifting components (even in MS-DOS vesa-based applications).
> > Your assumption that "converting isn't just a shift" requires link to
> > official documentation (i.e. proof), where it is clearly stated that
> > during Blt() (from which convert_unsigned_pixels is being called) between
> > different surface format, ddraw/d3d converts pixel format by means other
> > than simply shifting components. I doubt that in real software ddraw
> > implementation blt from 565 to 888 is done by completely recalculating
> > components using floating point operations.
>
> Floating point is overkill. The correct way for a 5 to 8 conversion is
> "divide by 31, multiply by 255". If you use
>   bits8 = bits5 << 3 | bits5 >> 2;
> you get "bits8 = bits5 * 8.25", which is always less than one off the
> correct result. What you do is to repeat the bit pattern infinitely
> instead of filling with zeroes on the right hand.
>
> Regards,
>   Michael Karcher
Fine, I've fixed that and resubmitted patch. But now conversion will be 
slower.

-- 
Виктор Ерёмин ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Michael Karcher]

Am Mittwoch, den 30.07.2008, 15:03 +0400 schrieb Victor:
> > You also have to expand types properly, i.e. converting a 
> > component from 4-bit to 8-bit isn't just a shift.
> In all places where I saw pixel format conversion before, it was _always_ 
> done 
> by shifting components (even in MS-DOS vesa-based applications). 
> Your assumption that "converting isn't just a shift" requires link to 
> official 
> documentation (i.e. proof), where it is clearly stated that during Blt() 
> (from which convert_unsigned_pixels is being called) between different 
> surface format, ddraw/d3d converts pixel format by means other than simply 
> shifting components. I doubt that in real software ddraw implementation blt 
> from 565 to 888 is done by completely recalculating components using floating 
> point operations.

Floating point is overkill. The correct way for a 5 to 8 conversion is
"divide by 31, multiply by 255". If you use
  bits8 = bits5 << 3 | bits5 >> 2;
you get "bits8 = bits5 * 8.25", which is always less than one off the
correct result. What you do is to repeat the bit pattern infinitely
instead of filling with zeroes on the right hand.

Regards,
  Michael Karcher

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Victor]

On Tuesday 29 July 2008 21:54:36 you wrote:
> Obviously not, 1-byte formats should be read as bytes, 2-byte format as
> WORDs, and 4-byte format as DWORDs. 
This will require several nearly identical read functions, or callbacks which 
means either code duplication or slowdown within Blt function, which isn't 
good idia, IMHO.

> That's of course only if the data is 
> supposed to be swapped on big-endian; if you assume that the data is
> always little-endian then you read byte by byte and rebuild DWORDs from
> that. Either way you don't access a DWORD mask as an array of bytes.
I"ve resubmitted patch and removed accessing DWORDs as bytes where it was 
possible (it was possible only in one place, actually). Accessing DWORD mask 
as array of bytes is required by algorithm. Accessing everything as DWORD is 
not possible, because, for example, accessing last pixel of r8g8b8 as DWORD 
will cause segfault, unless there is one additional byte available due to the 
surface alignment. Making several different functions which will read 1/2/3/4 
bytes seems clumsy/pointless to me.

> You also need to do this in a single pass, it's silly to go through the
> data 5 times, you should handle each pixel completely before moving on
> to the next. 
Conversion process for every channel is identical, and calling several if's 
for every pixel doesn't look like good idea (code duplication/slowdown). 
That's why surface is walked several times. Also, generic per-channel 
functions allows to add support for other formats later.

> You also have to expand types properly, i.e. converting a 
> component from 4-bit to 8-bit isn't just a shift.
In all places where I saw pixel format conversion before, it was _always_ done 
by shifting components (even in MS-DOS vesa-based applications). 
Your assumption that "converting isn't just a shift" requires link to official 
documentation (i.e. proof), where it is clearly stated that during Blt() 
(from which convert_unsigned_pixels is being called) between different 
surface format, ddraw/d3d converts pixel format by means other than simply 
shifting components. I doubt that in real software ddraw implementation blt 
from 565 to 888 is done by completely recalculating components using floating 
point operations.

-- 
Виктор Ерёмин ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Victor]

On Tuesday 29 July 2008 21:54:36 you wrote:
> Obviously not, 1-byte formats should be read as bytes, 2-byte format as
> WORDs, and 4-byte format as DWORDs. 
How it is supposed to be implemented inside pixel conversion loop?
As separate per-pixel callbacks for fetching data? Or as switch()?
The best way to do this, of course, would be compiling (on the fly) optimized 
conversion function on the fly from pre-generated parts, but I don't think 
that WINE is the right place to do stuff like this, especially for functions 
like this one.

> Either way you don't access a DWORD mask as an array of bytes. 
No, accessing DWORD masks as array of bytes still will be reasonable. Look at 
mask_set() and mask_erase() functions. It could be possible to set those 
values always as DWORDs, but that will cause segfault on last pixel for 
pixelsize < 4, if surface rows aren't 4-byte aligned.
In mask_copy accessing dword as array could be removed (will this be enough to 
make patch accepted, by the way?), but in mask_set/mask_erase it can't, and 
j-related macros will be still here.

> You also need to do this in a single pass, it's silly to go through the
> data 5 times, 
This would require if/else within pixel procession loop. I don't think that 
it's good, since for each channel there will be code duplication. Also, the 
data isn't always walked 5 times. Surface is walked through only if channel 
exists. If there is only RGB in both surfaces, data is walked only three 
times, if there is only L in both surfaces, then it's walked once.

> You also have to expand types properly,  
What exactly do you mean?

> i.e. converting a 
> component from 4-bit to 8-bit isn't just a shift.
Are you suggesting convert each shifted source component into float, divide it 
by source mask, multiply by shifted destination mask, then convert it back to 
int, preferably with dithering? This certainly wouldn't be fast.

For the sake of robustness it's quite safe to assume that shifted result is 
close enough. By the way, pixel colors were always converted by shifting 
them, not by calculating "truly correct" value (assuming that 0xFF in 8bit is 
equal 1.0 in floating point and is equal to 0xF in 4-bit).

-- 
Виктор Ерёмин ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Alexandre Julliard]

Victor <[EMAIL PROTECTED]> writes:

> On Tuesday 29 July 2008 12:21:23 you wrote:
>> No, you shouldn't be accessing the surface per byte either, the access
>> should depend on the format, i.e. for a 32-bit format you want to access
>> DWORD by DWORD.
> 1) As I understand, surface data should be stored in same way on both 
> big/little endian platforms, so for rgb8 blue compes first, then goes green, 
> then red, no matter which endianness. I think so because when windows program 
> access surface data it, data is supposed to be in same format, no matter how 
> program accesses it - by reading bytes, dwords and words. In this way 
> accessing r5g6b5 as dword/word on big-endian machine, will require 
> byte-swapping, otherwise middle channel will be split in two parts. I do not 
> have access to big-endian platform, so I can't check if that is true, and how 
> exactly surface data is stored on big-endian platform. 
>
> 2) Pixel formats have different sizes - 1..4 bytes. Reading 1-byte or 3-byte 
> data as dwords doesn't look right. 

Obviously not, 1-byte formats should be read as bytes, 2-byte format as
WORDs, and 4-byte format as DWORDs. That's of course only if the data is
supposed to be swapped on big-endian; if you assume that the data is
always little-endian then you read byte by byte and rebuild DWORDs from
that. Either way you don't access a DWORD mask as an array of bytes.

You also need to do this in a single pass, it's silly to go through the
data 5 times, you should handle each pixel completely before moving on
to the next. You also have to expand types properly, i.e. converting a
component from 4-bit to 8-bit isn't just a shift.

-- 
Alexandre Julliard
[EMAIL PROTECTED]

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Victor]

On Tuesday 29 July 2008 12:21:23 you wrote:
> No, you shouldn't be accessing the surface per byte either, the access
> should depend on the format, i.e. for a 32-bit format you want to access
> DWORD by DWORD.
1) As I understand, surface data should be stored in same way on both 
big/little endian platforms, so for rgb8 blue compes first, then goes green, 
then red, no matter which endianness. I think so because when windows program 
access surface data it, data is supposed to be in same format, no matter how 
program accesses it - by reading bytes, dwords and words. In this way 
accessing r5g6b5 as dword/word on big-endian machine, will require 
byte-swapping, otherwise middle channel will be split in two parts. I do not 
have access to big-endian platform, so I can't check if that is true, and how 
exactly surface data is stored on big-endian platform. 

2) Pixel formats have different sizes - 1..4 bytes. Reading 1-byte or 3-byte 
data as dwords doesn't look right. 

3) And I agree that this completely defeats purpose of the function. 

4) There will be ifdefs anyway. Especially if assumption in #1 is true, then 
each value will require byte-swapping, at least.

This won't work and I've already thought about accessing data as DWORDs, it 
doesn't look reasonable.

My suggestion is to accept patch as it is, until someone with access to 
big-endian machine will change it to be more elegant (although I don't see 
any way to do this).

-- 
Виктор Ерёмин ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

RE: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Stefan Dösinger]

> No, you shouldn't be accessing the surface per byte either, the access
> should depend on the format, i.e. for a 32-bit format you want to
> access
> DWORD by DWORD.
This defeats the point of a format-independent conversion function I think

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Alexandre Julliard]

Victor <[EMAIL PROTECTED]> writes:

> I've changed things a bit and resubmitted patch. Now there will be less 
> ifdefs.

Not much better I'm afraid.

> I'm not sure which "data types" are you talking about, since I'll need 
> per-byte surface access in any case, and It doesn't look like it can be 
> wrapped into endianness-independant datatype.

No, you shouldn't be accessing the surface per byte either, the access
should depend on the format, i.e. for a 32-bit format you want to access
DWORD by DWORD.

-- 
Alexandre Julliard
[EMAIL PROTECTED]

RE: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Stefan Dosinger]

> I've changed things a bit and resubmitted patch. Now there will be less
> ifdefs.
> I'm not sure which "data types" are you talking about, since I'll need
> per-byte surface access in any case, and It doesn't look like it can be
> wrapped into endianness-independant datatype.
Maybe you could use bitmasks to access the single bytes

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Victor]

On Monday 28 July 2008 19:09:03 you wrote:
> Victor <[EMAIL PROTECTED]> writes:
> > Well, the last attempt (with added big-endian support) was submitted
> > several days ago, and again - no feedback, and patch still isn't
> > accepted.
> >
> > What's wrong with patch now?
>
> The big endian support makes things worse than before. You need to use
> the proper data types so that you don't need any casts or #ifdefs.
I've changed things a bit and resubmitted patch. Now there will be less 
ifdefs.
I'm not sure which "data types" are you talking about, since I'll need 
per-byte surface access in any case, and It doesn't look like it can be 
wrapped into endianness-independant datatype.
-- 
Victor Eremin


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Alexandre Julliard]

Victor <[EMAIL PROTECTED]> writes:

> Well, the last attempt (with added big-endian support) was submitted several 
> days ago, and again - no feedback, and patch still isn't accepted.
>
> What's wrong with patch now?

The big endian support makes things worse than before. You need to use
the proper data types so that you don't need any casts or #ifdefs.

-- 
Alexandre Julliard
[EMAIL PROTECTED]

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Stefan Dösinger]

> Well, the last attempt (with added big-endian support) was submitted
several 
> days ago, and again - no feedback, and patch still isn't accepted.

> What's wrong with patch now?
Sorry, I must have missed this on Wine-patches again :-(

It look ok to me. The endianess #ifdefs look strange, but I don't know how
that's supposed to be handled.

PS: Sorry, I lost the original mail. I don't know how to supply a reference
ID to outlook, so I broke the thread appart

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Philip Nilsson]

On Thu, Jul 24, 2008 at 08:14:40PM +0400, Victor Eremin wrote:
> Supports conversion between most of "unsigned color" argb/xrgb
> surface formats (D3DFMT_A8R8G8B8, D3DFMT_A8R3G3B2, etc), and
> "luminosity" color formats (D3DFMT_L8, etc),
> excluding D3DFMT_A16R16G16B16, D3DFMT_A8P8, D3DFMT_P8 and D3DFMT_A1.
> luminosity to argb/xrgb (and vice versa) conversions are not supported
 
>  dlls/wined3d/surface_base.c|  218 ++-

I think it would be a better idea to put most of the functions in
utils.c, as they are utilities, and may have utility in other places
(for example argb_to_fmt does something similar, although I'm not sure
if more format conversion is neccessary.)

> +inline BYTE getMaskShift(DWORD mask){

Most functions here do not use camelCase.

> +void mask_erase(DWORD dstMask, BYTE pixelSize, 

I'd make all functions static that have no use outside their file to get
nicer warnings (function unused) and smaller binaries.

I wonder how static and inline interacts...

> +/*
> + * Warning: this won't convert all formats
> + */

The FIXMEs should be enough.

>  struct d3dfmt_convertor_desc convertors[] = {
> -{WINED3DFMT_R32F,   WINED3DFMT_R16F,convert_r32f_r16f},
> +{WINED3DFMT_R32F,   WINED3DFMT_R16F,convert_r32f_r16f}
>  };

I think my neckbeard just caught fire!

A lot of tests would be nice (although I guess most of the functionality
is already tested.)

I hope this is somewhat helpful.

Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)

[Victor]

Well, the last attempt (with added big-endian support) was submitted several 
days ago, and again - no feedback, and patch still isn't accepted.

What's wrong with patch now?
---
Victor Eremin


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(4th attempt)

[Alexandre Julliard]

Victor Eremin <[EMAIL PROTECTED]> writes:

> +void mask_copy(DWORD srcMask, BYTE srcPixelSize,
> +DWORD dstMask, BYTE dstPixelSize, 
> +BYTE *src, BYTE* dst, DWORD src_pitch, DWORD dst_pitch, 
> +unsigned int w, unsigned int h){
> +unsigned int x, y;
> +BYTE i;
> +BYTE *dstp, *srcp, *dstmaskp, *srcmaskp, *channelValuePtr;
> +DWORD channelValue;
> +BYTE upShift, downShift;
> +dstmaskp = (BYTE*)&dstMask;
> +srcmaskp = (BYTE*)&srcMask;
> +channelValuePtr = (BYTE*)&channelValue;

You can't access DWORDs as bytes, that will break on big-endian
platforms.

-- 
Alexandre Julliard
[EMAIL PROTECTED]

Re: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Victor]

On Wednesday 23 July 2008 21:48:59 Chris Robinson wrote:
> But extracting the mask offset and size from the actual mask takes a bit of
> time, 
Extracting mask is called once or twice per channel conversion. I.e. in case 
of converting a8r8g8b8->a1r5g5b5 mask should be calculated only 8 times, no 
matter how big surface is. check mask_copy() routine for details. Optimizing 
this is pointless - converter performs much more per pixel operations - 
shifts, bitwise operations, etc, so you won't notice any difference.

> and as it is, the table can't currently set a proper mask for 
> anything over 32 bits (including the 16-bit-per-component unsigned integer
> types).
Yes, this isn't supported. But with current scheme of conversion, adding 
support for 16-bit-per-component surfaces would require operating on 64bit 
numbers or increasing number of per-pixel operations. I think per-pixel 64bit 
shifts on 32bit CPUs will be slower. 

replacing masks with number of bits and shift will need additional work, since 
masks are probably used in other places. There will be also high chance of 
breaking entire table accidentally because of misprint. 

If you don't like current functionality, to my opinion the best approach would 
be to modify it once my patch made it in repository. 

-- 
Victor Eremin ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Chris Robinson]

On Wednesday 23 July 2008 10:29:37 am Victor wrote:
> 0) initial patch used "mask size" + "mask offset", but was rewritten to use
> mask value when Stefan Dösinger requested that. I don't want to rewrite it
> back to use mask size + mask offset.
> 1) mask size and offset can be extracted from mask value.
> 2) using mask instead of "mask size" + "mask offset" requires less function
> arguments and smaller format table, although, yes there is a higher chance
> of producing errors.

But extracting the mask offset and size from the actual mask takes a bit of 
time, and as it is, the table can't currently set a proper mask for anything 
over 32 bits (including the 16-bit-per-component unsigned integer types).

Re: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Victor]

On Wednesday 23 July 2008 21:18:57 Chris Robinson wrote:
> Maybe it would be better if the table was changed to have a bit offset and
> a mask size, instead of the actual mask.
0) initial patch used "mask size" + "mask offset", but was rewritten to use 
mask value when Stefan Dösinger requested that. I don't want to rewrite it 
back to use mask size + mask offset. 
1) mask size and offset can be extracted from mask value.
2) using mask instead of "mask size" + "mask offset" requires less function 
arguments and smaller format table, although, yes there is a higher chance of 
producing errors.

-- 
Victor Eremin ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Chris Robinson]

On Wednesday 23 July 2008 09:10:02 am Stefan Dösinger wrote:
> The patch looks reasonably, just one small thing: There is a count_bits
> function implemented in utils.c, which as far as I can see does the same as
> getMaskSize. Can you check if you can reuse it?

Maybe it would be better if the table was changed to have a bit offset and a 
mask size, instead of the actual mask. All the bits for any given component 
are always continuous, and things would be easier to handle with the offset 
and size (eg. the float formats could get a proper mask; not that they could 
be converted in this way, though). So for a generic converter, you can 
basically get:

outcolor = (((incolor>>infmt->r_offset)&((1r_size / infmt->r_size) << outfmt->r_offset;
outcolor |= (((incolor>>infmt->g_offset)&((1g_size / infmt->g_size) << outfmt->g_offset;
outcolor |= (((incolor>>infmt->b_offset)&((1b_size / infmt->b_size) << outfmt->b_offset;
outcolor |= (((incolor>>infmt->a_offset)&((1a_size / infmt->a_size) << outfmt->a_offset;

For any non-mixed unsigned integer type.

Re: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Victor]

On Wednesday 23 July 2008 20:10:02 Stefan Dösinger wrote:
> Actually, one more idea:
>
> It will be extra-slow, but we could implement a general all-to-all format
> by converting the source format to A32R32G32B32F(float values), and then
> write code to convert this format to all possible destination formats. The
> from->to table lookup could be replaced by code that can combine multiple
> conversions to find a conversion strategy(e.g. R5G6B5->ARGB32F->R8G8B8). I
> am not sure if it is a good idea, but it is worth a consideration
I thought about that, but decided that it'll be too slow (conversion to float 
and back), and there are more than 4 possible channels (luminance, palette, 
depth, stencil, channels for formats like D3DFMT_V8U8), there are compressed 
formats like DXT, and two palette formats that would need additional data for 
conversion. I think it makes sense (later) to implement several different 
generic converters, several special conversion functions (like A8R8G8B8->P8) 
and then use this strategy to combine all these converters into one chain. It 
makes sense to consider which conversions are really used. Stuff like 
R5G6B5->X8R8G8B8 is common, but I don't think anyone would ever need 
D24S8->ARGB32F. Even ARGB32F->P8 is unlikely (although X8R8G8B8->P8 is 
required by some games).

On Wednesday 23 July 2008 20:10:02 Stefan Dösinger wrote:
> The patch looks reasonably, just one small thing: There is a count_bits
> function implemented in utils.c, which as far as I can see does the same as
> getMaskSize. Can you check if you can reuse it?
Done and resubmitted (count_bits was more elegant, by the way). But I hope 
that there is a warranty that unsigned int isn't less than 32 bit on all 
systems where WINE is used (maybe I'm just paranoid).

-- 
Victor Eremin ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

RE: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Stefan Dösinger]

The patch looks reasonably, just one small thing: There is a count_bits 
function implemented in utils.c, which as far as I can see does the same as 
getMaskSize. Can you check if you can reuse it?


> -Original Message-
> From: [EMAIL PROTECTED] [mailto:wine-patches-
> [EMAIL PROTECTED] On Behalf Of Victor Eremin
> Sent: Wednesday, July 23, 2008 10:25 AM
> To: [EMAIL PROTECTED]
> Subject: wined3d: universal surface convertor function for unsigned
> integer color formats(3rd attempt)
> 
> 
> Supports conversion between most of "unsigned color" argb/xrgb surface
> formats (D3DFMT_A8R8G8B8, D3DFMT_A8R3G3B2, etc), and "luminosity" color
> formats (D3DFMT_L8, etc), excluding D3DFMT_A16R16G16B16, D3DFMT_A8P8,
> D3DFMT_P8 and D3DFMT_A1.
> luminosity to argb/xrgb (and vice versa) conversions are not supported
> 
> Removes "Cannot find coverter" FIXMEs from "Ancient Evil" and "Stranded
> 2" games.
> 
> Fixes water glitches in "Stranded 2" game.
> ---
>  dlls/wined3d/surface_base.c|  202
> ++-
>  dlls/wined3d/utils.c   |  126 +-
>  dlls/wined3d/wined3d_private.h |2 +-
>  3 files changed, 260 insertions(+), 70 deletions(-)

RE: wined3d: universal surface convertor function for unsigned integer color formats(3rd attempt)

[Stefan Dösinger]

Actually, one more idea:

It will be extra-slow, but we could implement a general all-to-all format by 
converting the source format to A32R32G32B32F(float values), and then write 
code to convert this format to all possible destination formats. The from->to 
table lookup could be replaced by code that can combine multiple conversions to 
find a conversion strategy(e.g. R5G6B5->ARGB32F->R8G8B8). I am not sure if it 
is a good idea, but it is worth a consideration

RE: wined3d: universal surface convertor function for unsigned integer color formats(2nd attempt)

[Stefan Dösinger]

> It has been a couple of days since I've submitted patch. Patch still
> isn't in git. Will it be accepted? If not, what should be changed to
> make it "acceptable"?
I think the problem was that I forgot to reply to your mail.

I think the idea of using this as a fallback if no special conversion function 
is needed is ok.

Re: wined3d: universal surface convertor function for unsigned integer color formats(2nd attempt)

[Victor]

It has been a couple of days since I've submitted patch. Patch still isn't in 
git. Will it be accepted? If not, what should be changed to make 
it "acceptable"?

-- 
With best regards Victor Eremin ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats(2nd attempt)

[Victor]

On Thursday 17 July 2008 22:33:34 Stefan Dösinger wrote:
> How many different conversions do the two games need? 
The games I've mentioned before use simple things like X8R8G8B8 to R5G6B5 or 
X8R8G8B8 to R8G8B8. By using Google I didn't found much applications with 
this particular FIXME (simcity3k, stranded2 and "army men" game, which 
requires conversion from argb to indexed color). 

I think that I saw "converter not found" messages some time ago (6-12 months 
ago) when I've been trying to run things like Sims1, SimCity 2000 rush hour, 
etc, probably with some other games.

In real DirectX/D3DX conversion code (with similar functionality) might be 
used with loading texture from different surface formats (like when trying to 
load A4R4G4B4 from A16R16G16B16 using D3DXCreateTextureFromFile, etc) and 
when dealing with D3DXFillTexture1D/2D/3D. 

The use of this function might be somehow dependand on hardware. In this:
http://ubuntuforums.org/showthread.php?t=741465
thread user experience game gui corruption, while on my machine without 
converter only water is corrupted, and converter isn't called for gui 
elements at all (I've been setting some channels to zero to check that).

> One of my concerns is 
> that this generic code is way slower than a separate conversion function
> for each pair of color formats.
Ideally, this requires benchmark.
Conversion shouldn't be deadly slow when compared with optimized function.
Besides there is only one optimized function implemented right now.

 An "optimized" replacement for current generic procedure would need up to 
240..380 functions, which will be error-prone, and will need either "macros 
magic" or just a lot of code. It'll be difficult to debug that.

If you are concerned about conversion speed, then conversion function can be 
easily modified in the way that it'll use generic conversion routine only if 
optimized conversion is not available. This way it'll be possible to optimize 
frequently-used conversions, while unusual ones will be still available.

-- 
Victor Eremin ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

RE: wined3d: universal surface convertor function for unsigned integer color formats(2nd attempt)

[Stefan Dösinger]

How many different conversions do the two games need? One of my concerns is 
that this generic code is way slower than a separate conversion function for 
each pair of color formats. On the other hand, one function for each pair gives 
a whole lot more of code, and if we run into the conversion function during 
rendering we can kiss the framerate goodbye anyway


> -Original Message-
> From: [EMAIL PROTECTED] [mailto:wine-patches-
> [EMAIL PROTECTED] On Behalf Of Victor Eremin
> Sent: Thursday, July 17, 2008 11:02 AM
> To: [EMAIL PROTECTED]
> Subject: wined3d: universal surface convertor function for unsigned
> integer color formats(2nd attempt)
> 
> 
> Supports conversion between most of "unsigned color" argb/xrgb surface
> formats (D3DFMT_A8R8G8B8, D3DFMT_A8R3G3B2, etc), and "luminosity" color
> formats (D3DFMT_L8, etc), excluding D3DFMT_A16R16G16B16, D3DFMT_A8P8,
> D3DFMT_P8 and D3DFMT_A1.
> luminosity to argb/xrgb (and vice versa) conversions are not supported
> 
> Removes "Cannot find coverter" FIXMEs from "Ancient Evil" and "Stranded
> 2" games.
> 
> Fixes water glitches in "Stranded 2" game.
> ---
>  dlls/wined3d/surface_base.c|  203
> ++--
>  dlls/wined3d/utils.c   |  126 
>  dlls/wined3d/wined3d_private.h |2 +-
>  3 files changed, 260 insertions(+), 71 deletions(-)

Re: wined3d: universal surface convertor function for unsigned integer color formats

[Victor]

On Monday 14 July 2008 18:21:19 Stefan Dösinger wrote:
> There's a pixelformat table that describes bitmasks and sizes in utils.c,
> you can access it with a function that is next to it. I think you can infer
> the shifting information from this table, if not please add it there
> instead of creating your own table.
I've found this array (StaticPixelFormatDesc formats[]), but I'm not sure if 
adding new fields will be a good idea. Reasons:
1) This will contain redundant and unused information (masks<->shifts. I 
wanted to use masks initially, but decided to use "mask_size" and "shift", 
because it's less prone to errors).  Redundant means higher chance of errors 
and breaking up something somewhere else.
2) Ideally for conversion purposes it should have more channel fields. (at 
least "L" (the only one currently used), but ideally also bitmasks 
for "W", "V", "U", "D", "S" channels). But adding more fields will easily 
turn formats[] table into monstrosity.
3) StaticFormatDescriptor (and my own "SurfFmtDesc") both aren't really what 
I'd like to use with conversion routine. Ideally something like that:
---
struct ChannelDesc{
char name; /*channel neame 'a', 'r', 'g', 'd', etc*/
BYTE bits; 
BYTE shift; 
int defaultBits; /*if 1 then during conversion channel bits in 
destination 
surface must be set to 1, if such channel doesn't exist in source surface.*/
}

struct FormatDesc{
WINED3DFORMAT fmt;
BYTE size;
BYTE numChannels;
struct ChannelDesc channels[8];
}
---
could be perfect/compact descriptor suitable for converting surfaces. This way 
it could be much easier to handle more surface format conversions in simple 
fashion. I.e. given two such descriptors I could put all conversion routine 
in a loop that would handle almost anything, instead of calling same function 
several times for each channel. Such function could be useful in some other 
places.

Is it possible to accept patch without merging two tables, until better 
(or "final") conversion implementation will be written? (like the one which 
handles A8R8G8B8->L8 or A8R8G8B8->P8 conversions) Your arguments about C++ 
comments/spaces, unused x8r8g8b8_to_x5r6g5 are reasonable, but I'm not sure 
that merging current table used by convert_unsigned_pixels() 
with "StaticPixelFormatDesc formats[]" from utils.c will be good idea.

> Also please do not use C++ comments("//"), and watch out regarding tabs and
> spaces
Will do.

> Your patch still adds the x8r8g8b8_to_x5r6g5 function, and I think it is a
> bit ugly to bypass the existing format conversion table entirely and add
> the convert_unsigned_pixels function. I think it would be better to access
> this function using the conversion table. One reason is that we could use
> the table in directx.c in CheckDeviceFormatConversion, so the information
> is all in one place.
Currently default conversion (find_convertor) is not bypassed. 
It is still used if there is no conversion available via 
convert_unsigned_pixels(), which is likely to happen, because 
convert_unsigned_pixels() supports only 20 formats. Also, please note that 
there is only one entry in "convertors" table currently (if you don't count 
x8r8g8b8_to_x5r6g5). 
putting convert_unsigned_pixels in "convertors" table won't be possible, 
because it'll require either writing of 400 wrappers and adding 400 entries 
in converter table (unless I don't know some C-technique for avoiding this). 
This is extremely ugly.
Also, currently it won't be possible to support all conversions using 
convertors table. You won't be able to use current convertors for formats 
with palette (someone already have sent me email asking to implement 
X8R8G8B8->P8 conversion).
I think instead of using only conversion table it will be better to provide 
centrilised functions like "is_conversion_supported(WINED3DFORMAT, 
WINED3DFORMAT)" and "convert_pixels". It might make sense to use different 
routines/tables for different format conversions.

-- 
Виктор Ерёмин ([EMAIL PROTECTED])


signature.asc
Description: This is a digitally signed message part.

Re: wined3d: universal surface convertor function for unsigned integer color formats

[Philip Nilsson]

On Mon, Jul 14, 2008 at 02:51:42AM +0400, Victor Eremin wrote:
> 
> Converter function supports conversion between most of
> "unsigned color" argb/xrgb surface formats (like D3DFMT_A8R8G8B8,
> D3DFMT_A8R3G3B2, and so on), and between "luminosity" color formats
>  (D3DFMT_L8, etc), excluding D3DFMT_A16R16G16B16, D3DFMT_A8P8,
> D3DFMT_P8 and D3DFMT_A1.
> 
> Conversion from rgba to luminosity (and vice versa) is not currently 
> implemented.
> 
> The patch removes "Cannot find coverter" FIXMEs from "Ancient Evil" and
> "Stranded 2" games.
> 
> Patch also fixes water glitches in "Stranded 2" game.

I've attached my pixel conversion function which uses the table in
utils.c.  Hope you find it useful although I haven't really polished it.
/* Counts the number of leading zeros in a mask, returns 0 if the mask is zero. */
static unsigned int count_zeros(unsigned int mask)
{
unsigned int count;
if (!mask)
return 0;
for (count = 0; !(mask & 1); ++count)
{
mask >>= 1;
}
return count;
}

/* Scales a value masked by one mask to another. */
static DWORD convert_channel(DWORD src, DWORD srcmask, DWORD dstmask)
{
src = src & srcmask;
src >>= count_zeros(srcmask);
src *= (dstmask >> count_zeros(dstmask)) / (srcmask >> count_zeros(srcmask));
src <<= count_zeros(dstmask);
return src;
}

static DWORD get_mask(DWORD value, DWORD mask)
{
return (value & mask) >> count_zeros(mask);
}

static DWORD put_mask(DWORD value, DWORD mask)
{
return (value << count_zeros(mask)) & mask;
}

/* MSDN states that the default value is 1 for missing channels; and that
 * D3DFMT_A8 is the exception where the missing channels are 0.
 *
 * I assume 1 means 1 in every bit. */
static DWORD convert_pixel(DWORD srcword, const StaticPixelFormatDesc* srcentry, const StaticPixelFormatDesc* dstentry)
{
DWORD pixel = 0;

if (srcentry->alphaMask && dstentry->alphaMask)
pixel |= convert_channel(srcword, srcentry->alphaMask, dstentry->alphaMask);
else
pixel |= dstentry->alphaMask;

if (srcentry->redMask && dstentry->redMask)
pixel |= convert_channel(srcword, srcentry->redMask, dstentry->redMask);
else
pixel |= dstentry->redMask;

if (srcentry->greenMask && dstentry->greenMask)
pixel |= convert_channel(srcword, srcentry->greenMask, dstentry->greenMask);
else
pixel |= dstentry->greenMask;

if (srcentry->blueMask && dstentry->blueMask)
pixel |= convert_channel(srcword, srcentry->blueMask, dstentry->blueMask);
else
pixel |= dstentry->blueMask;

return pixel;
}

DWORD convert_pixelformat(DWORD pixel, WINED3DFORMAT srcformat, WINED3DFORMAT dstformat)
{
const StaticPixelFormatDesc* srcentry = getFormatDescEntry(srcformat, NULL, NULL);
const StaticPixelFormatDesc* dstentry = getFormatDescEntry(dstformat, NULL, NULL);
if (!srcentry) {
FIXME("Unsupported pixelformat.\n");
return 0;
}
if (!dstentry) {
FIXME("Unsupported pixelformat.\n");
return 0;
}
return convert_pixel(pixel, srcentry, dstentry);
}

static UINT get_pixel(const LPCVOID data, UINT bpp, const RECT* const rect, UINT pitch, UINT x, UINT y)
{
DWORD pixel = 0;
memcpy(&pixel, (char*)data + (rect->top + y) * pitch + (rect->left + x) * bpp, bpp);
return pixel;
}

RE: wined3d: universal surface convertor function for unsigned integer color formats

[Stefan Dösinger]

There's a pixelformat table that describes bitmasks and sizes in utils.c, you 
can access it with a function that is next to it. I think you can infer the 
shifting information from this table, if not please add it there instead of 
creating your own table.

Also please do not use C++ comments("//"), and watch out regarding tabs and 
spaces

Your patch still adds the x8r8g8b8_to_x5r6g5 function, and I think it is a bit 
ugly to bypass the existing format conversion table entirely and add the 
convert_unsigned_pixels function. I think it would be better to access this 
function using the conversion table. One reason is that we could use the table 
in directx.c in CheckDeviceFormatConversion, so the information is all in one 
place.

> -Original Message-
> From: [EMAIL PROTECTED] [mailto:wine-patches-
> [EMAIL PROTECTED] On Behalf Of Victor Eremin
> Sent: Sunday, July 13, 2008 5:52 PM
> To: [EMAIL PROTECTED]
> Subject: wined3d: universal surface convertor function for unsigned
> integer color formats
> 
> 
> Converter function supports conversion between most of "unsigned color"
> argb/xrgb surface formats (like D3DFMT_A8R8G8B8, D3DFMT_A8R3G3B2, and
> so on), and between "luminosity" color formats  (D3DFMT_L8, etc),
> excluding D3DFMT_A16R16G16B16, D3DFMT_A8P8,
> D3DFMT_P8 and D3DFMT_A1.
> 
> Conversion from rgba to luminosity (and vice versa) is not currently
> implemented.
> 
> The patch removes "Cannot find coverter" FIXMEs from "Ancient Evil" and
> "Stranded 2" games.
> 
> Patch also fixes water glitches in "Stranded 2" game.
> ---
>  dlls/wined3d/surface_base.c |  215
> +-
>  1 files changed, 209 insertions(+), 6 deletions(-)