d3dx9: Implement converting and copying surface data in D3DXLoadSurfaceFromMemory (review please)

Henri Verbeet hverbeet at gmail.com
Wed Aug 26 10:25:10 CDT 2009


2009/8/26 Tony Wasserka <tony.wasserka at freenet.de>:
> I hope the code is understandable in the other parts, my few main
> questions are:
> Do you agree that the architecture is the best approach or can you think
> of a better one? would using macros be legal here since it could really
> help reduce code duplication when implementing the other filters?
> Is the code itself okay? Suggestions about coding style or other things
> are welcome.
> Is there any optimization I missed? - I spend a somewhat large amount of
> time getting the right balance between wide format support and good
> performance, but maybe things can be improved a bit more. Right now my
> implementation is about 10 % slower than the native one.
>
I think the main thing is that you really don't want branches inside
the inner loop. You can handle the cases where a component is unused
by building a mask before entering the loop and just or'ing with that
mask inside the loop. Perhaps to compiler is smart enough to handle
that one, but I don't think you want to rely on it. As for the shifts,
in the large majority of cases you will only need two shifts. The only
time you need more than that is when the destination component is more
than two times the size of the source component, so I don't think you
want to bother with that in the common function.

I notice you store the already shifted masks in the table. You'll
probably find that that's impractical for formats where the total bit
count is larger than 32.

> +typedef struct _StaticPixelFormatDesc {
> +    D3DFORMAT format;
> +    BYTE abits, rbits, gbits, bbits;
> +    BYTE ashift, rshift, gshift, bshift;
> +    DWORD amask, rmask, gmask, bmask;
> +    UINT bytes_per_pixel;
> +    StaticFormatType type;
> +} StaticPixelFormatDesc;
Looks like you took this from wined3d. I think the typedef and naming
are unfortunate. Note that you can calculate the mask and bit count
from each other.

> +void copy_simple_data(LPBYTE  pSrc, UINT  SrcPitch, POINT  SrcSize, StaticPixelFormatDesc  SrcFormat,
> +                      LPBYTE pDest, UINT DestPitch, POINT DestSize, StaticPixelFormatDesc DestFormat,
> +                      DWORD dwFilter)
The function should be static. "pSrc", "SrcFormat", and "DestFormat"
should be const. I'm not a fan of hiding that something is a pointer
behind a type like LPBYTE and using Hungarian notation. "BYTE *src"
seems simpler and clearer. Same goes for pretending control statements
are function calls. Caps should be reserved for enum elements and
macros.

> +void get_format_info(D3DFORMAT format, StaticPixelFormatDesc *desc)
> +{
> +    int i = 0;
> +    while(formats[i].format != format && formats[i].format != D3DFMT_UNKNOWN) i++;
> +
> +    memcpy(desc, &formats[i].format, sizeof(StaticPixelFormatDesc));
> +}
As long as you don't modify "desc", the memcpy is redundant. Just
return a const pointer to the original data.



More information about the wine-devel mailing list