Re: wined3d: universal surface convertor function for unsigned integer color formats(5th attempt)
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
Victor ErV2005@rambler.ru 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.
On Monday 28 July 2008 19:09:03 you wrote:
Victor ErV2005@rambler.ru 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.
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
Victor ErV2005@rambler.ru 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.
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
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).
Victor ErV2005@rambler.ru 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.
- 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.
- 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.
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).
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.
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
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.
-
Alexandre Julliard -
Michael Karcher -
Stefan Dosinger -
Victor