Re: [D3D] Added native and emulated S3TC support

[Lionel Ulmer]

I have just a few comments on the patch (yeah, it would not have been fun if
I had commented on wine-d3d first :-) ).

But anyway, great work !
> +static BOOL DDRAW_bind_to_s3tc( void )
> +{
> - const char *s3tcname = SONAME_LIBS3TC;
> -
> - s3tc_handle = wine_dlopen(s3tcname, RTLD_NOW, NULL, 0);
> - if (!s3tc_handle) {
> -
>    printf("Wine cannot find the S3TC software decompression library (%s).\n",s3tcname);

Why did you use 'printf' here instead of WARN or ERR ?

Moreover, with this, we will print an error at DirectX loading when the
users do not have this library installed ... even if the game does never
request any compressed mode textures.

Would it not be better to somehow print the error message when the
application wants to create a DXTN texture (ie somehow remember that we do
not support this feature and keep the error message for later).
> - return FALSE;
> - }
> -
>
> +#define API_FUNCTION(f)  \
> - if((f = wine_dlsym(s3tc_handle, #f, NULL, 0)) == NULL) \
> - { \
> -
>    printf("Can't find symbol %s\n", #f); \

Same printf comment here (but this one can be kept here :-) ).
> -
>    goto sym_not_found; \
> - }
> - API_FUNCTION(fetch_2d_texel_rgba_dxt1);
> - API_FUNCTION(fetch_2d_texel_rgba_dxt3);
> - API_FUNCTION(fetch_2d_texel_rgba_dxt5);
>
> +#undef API_FUNCTION
> -
> - return TRUE;
> -
>
> +sym_not_found:
> - WARN("Wine cannot find functions that are necessary for S3TC decompression\n");
> - wine_dlclose(s3tc_handle, NULL, 0);
> - s3tc_handle = NULL;
> - return FALSE;
>
> +}
> -

(...)
> - if (src_pf->dwFlags & DDPF_FOURCC) {
> - GLenum retVal;
> - int size = surf_ptr->surface_desc.u1.dwLinearSize;
> - int width = surf_ptr->surface_desc.dwWidth;
> - int height = surf_ptr->surface_desc.dwHeight;
> - LPVOID buffer = surf_ptr->surface_desc.lpSurface;
> -
> - switch (src_pf->dwFourCC) {
> -
>    case MAKE_FOURCC('D','X','T','1'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT; break;
> -
>    case MAKE_FOURCC('D','X','T','3'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT3_EXT; break;
> -
>    case MAKE_FOURCC('D','X','T','5'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT; break;
> -
>    default:
> -
> FIXME("FourCC Not supported\n");
> -
> return DD_OK;
> -
> break;
> - }
> - GL_extensions.glCompressedTexImage2D(GL_TEXTURE_2D, current_level, retVal, width, height, 0, size, buffer);

How are we sure that this code will never be called with the
'glCompressedTexImage2D' unset ? I.e. when we are in emulation mode, what
happens here ?

Is this because when we are in emulation mode, we do not advertise support
for DTXN textures, this means that the application can create such textures,
but will be automagically converted to RGB ones at Loading ?

Anyway, I would still prefer a test here to prevent crashes for application
which do not behave well :-)

(...)
> - if (current_surface->surface_desc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) {
> - GLint retVal;
> - int size = current_surface->surface_desc.u1.dwLinearSize;
> - int width_ = current_surface->surface_desc.dwWidth;
> - int height_ = current_surface->surface_desc.dwHeight;
> - LPVOID buffer = current_surface->surface_desc.lpSurface;
> -
> - switch (current_surface->surface_desc.u4.ddpfPixelFormat.dwFourCC) {
> -
>    case MAKE_FOURCC('D','X','T','1'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT; break;
> -
>    case MAKE_FOURCC('D','X','T','3'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT3_EXT; break;
> -
>    case MAKE_FOURCC('D','X','T','5'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT; break;
> -
>    default:
> -
> FIXME("Not supported\n");
> -
> return DD_OK;
> -
> break;
> - }
> - /* GL_extensions.glCompressedTexSubImage2D(GL_TEXTURE_2D, current_level, xoffset, yoffset, width, height, retVal, (unsigned char*)temp_buffer); */
> - GL_extensions.glCompressedTexImage2D(GL_TEXTURE_2D, current_level, retVal, width_, height_, 0, size, buffer);

Same here.
> - }
> -
> - if (strstr(glExtensions, "GL_EXT_texture_compression_s3tc")) {
> -
>    TRACE(" - S3TC compression supported\n");
> -
>    GL_extensions.s3tc_compressed_texture = TRUE;
> -
>    GL_extensions.glCompressedTexImage2D = pglXGetProcAddressARB("glCompressedTexImage2D");
> -
>    GL_extensions.glCompressedTexSubImage2D = pglXGetProcAddressARB("glCompressedTexSubImage2D");
> }

If I were pedantic, you should test for 'GL_ARB_texture_compression' to be
able to get the 'glCompressedTexImage2DARB' and
'glCompressedTexSubImage2DARB' functions (which may be better than using the
non-ARB versions which are, AFAIK, the ones in GL 1.4).

And then to get 'GL_EXT_texture_compression_s3tc' to set
'GL_extensions.s3tc_compressed_texture' to TRUE.

But as 'GL_EXT_texture_compression_s3tc' depends on
'GL_ARB_texture_compression', I will let you do this shortcut :-)
> - /* We do not support for now compressed texture formats... */
> - if (ddsd.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC)
> - /* We support for now only DXT1, DXT3 & DXT5 compressed texture formats... */
> - if ((ddsd.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) &&
> -
>    (ddsd.u4.ddpfPixelFormat.dwFourCC != MAKE_FOURCC('D','X','T','1')) &&
> -
>    (ddsd.u4.ddpfPixelFormat.dwFourCC != MAKE_FOURCC('D','X','T','3')) &&
> -
>    (ddsd.u4.ddpfPixelFormat.dwFourCC != MAKE_FOURCC('D','X','T','5')) )
> {
>        return DDERR_INVALIDPIXELFORMAT;
>    }

What to do here when neither hardware nor emulated support is present ?
Maybe we should print an error (cf previous comment) and return
INVALIDPIXELFORMAT, no ?

Lionel (who goes testing NOFL :-) )

[D3D] Added native and emulated S3TC support

[Mike Hearn]

On Tue, 27 Jul 2004 22:57:53 +0200, Lionel Ulmer wrote:
> I have just a few comments on the patch (yeah, it would not have been fun if
> I had commented on wine-d3d first :-) ).

One comment, it's my understanding that S3TC is patented, is this also
true of the emulation library techniques?

[D3D] Added native and emulated S3TC support

[Lionel Ulmer]

On Tue, Jul 27, 2004 at 11:04:10PM +0100, Mike Hearn wrote:
> On Tue, 27 Jul 2004 22:57:53 +0200, Lionel Ulmer wrote:
> > I have just a few comments on the patch (yeah, it would not have been fun if
> > I had commented on wine-d3d first :-) ).
>
> One comment, it's my understanding that S3TC is patented, is this also
> true of the emulation library techniques?

Why do you think we took the pain to use an external library to do the
decompression instead of having it in the Wine code itself :-) ?

Lionel

[D3D] Added native and emulated S3TC support

[Mike Hearn]

On Wed, 2004-07-28 at 09:32 +0200, Lionel Ulmer wrote:
> Why do you think we took the pain to use an external library to do the
> decompression instead of having it in the Wine code itself :-) ?

Great, so this is another codec-style fiasco where you have to violate
patents with an extra library if you have the "wrong" video card? What
does that mean for distribution?

[D3D] Added native and emulated S3TC support

[Lionel Ulmer]

> Great, so this is another codec-style fiasco where you have to violate
> patents with an extra library if you have the "wrong" video card? What
> does that mean for distribution?

No idea.... And frankly, I do not really care.

Anyway, as we use the library that is (or will) be dynamically loaded by the
DRI code, the packaging issue will be already solved for us for the DRI.

Basically, only people like me with pre-GeForce NVIDIA cards will need to do
the extra steps of actually installing the library, all others will either
have hardware support or have it installed via the DRI package.

Lionel

[D3D] Added native and emulated S3TC support

[Christian Costa]

Lionel Ulmer wrote:
> I have just a few comments on the patch (yeah, it would not have been fun if
> I had commented on wine-d3d first :-) ).

:-)
> But anyway, great work !
> > +static BOOL DDRAW_bind_to_s3tc( void )
> > +{
> > - const char *s3tcname = SONAME_LIBS3TC;
> > -
> > - s3tc_handle = wine_dlopen(s3tcname, RTLD_NOW, NULL, 0);
> > - if (!s3tc_handle) {
> > -
> >    printf("Wine cannot find the S3TC software decompression library (%s).\n",s3tcname);
>
> Why did you use 'printf' here instead of WARN or ERR ?
>
> Moreover, with this, we will print an error at DirectX loading when the
> users do not have this library installed ... even if the game does never
> request any compressed mode textures.

Ooop... Forgot to clean-up these printf.
> Would it not be better to somehow print the error message when the
> application wants to create a DXTN texture (ie somehow remember that we do
> not support this feature and keep the error message for later).

Good idea. I will see...
> > - return FALSE;
> > - }
> > -
> >
> > +#define API_FUNCTION(f)  \
> > - if((f = wine_dlsym(s3tc_handle, #f, NULL, 0)) == NULL) \
> > - { \
> > -
> >    printf("Can't find symbol %s\n", #f); \
>
> Same printf comment here (but this one can be kept here :-) ).

Same as above.
> > -
> >    goto sym_not_found; \
> > - }
> > - API_FUNCTION(fetch_2d_texel_rgba_dxt1);
> > - API_FUNCTION(fetch_2d_texel_rgba_dxt3);
> > - API_FUNCTION(fetch_2d_texel_rgba_dxt5);
> >
> > +#undef API_FUNCTION
> > -
> > - return TRUE;
> > -
> >
> > +sym_not_found:
> > - WARN("Wine cannot find functions that are necessary for S3TC decompression\n");
> > - wine_dlclose(s3tc_handle, NULL, 0);
> > - s3tc_handle = NULL;
> > - return FALSE;
> >
> > +}
> > -
>
> (...)
> > - if (src_pf->dwFlags & DDPF_FOURCC) {
> > - GLenum retVal;
> > - int size = surf_ptr->surface_desc.u1.dwLinearSize;
> > - int width = surf_ptr->surface_desc.dwWidth;
> > - int height = surf_ptr->surface_desc.dwHeight;
> > - LPVOID buffer = surf_ptr->surface_desc.lpSurface;
> > -
> > - switch (src_pf->dwFourCC) {
> > -
> >    case MAKE_FOURCC('D','X','T','1'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT; break;
> > -
> >    case MAKE_FOURCC('D','X','T','3'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT3_EXT; break;
> > -
> >    case MAKE_FOURCC('D','X','T','5'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT; break;
> > -
> >    default:
> > -
> > FIXME("FourCC Not supported\n");
> > -
> > return DD_OK;
> > -
> > break;
> > - }
> > - GL_extensions.glCompressedTexImage2D(GL_TEXTURE_2D, current_level, retVal, width, height, 0, size, buffer);
>
> How are we sure that this code will never be called with the
> 'glCompressedTexImage2D' unset ? I.e. when we are in emulation mode, what
> happens here ?

Humm... You're right. If the app does not check if the device supports 
theses texture formats, this may happen.
> Is this because when we are in emulation mode, we do not advertise support
> for DTXN textures, this means that the application can create such textures,
> but will be automagically converted to RGB ones at Loading ?

If the device does not support these textures formats you can still 
create such textures.
You need to blit from a S3TC texture to a RBG one to do the 
decompression and use the latter with the device.
> Anyway, I would still prefer a test here to prevent crashes for application
> which do not behave well :-)

OK.
> (...)
> > - if (current_surface->surface_desc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) {
> > - GLint retVal;
> > - int size = current_surface->surface_desc.u1.dwLinearSize;
> > - int width_ = current_surface->surface_desc.dwWidth;
> > - int height_ = current_surface->surface_desc.dwHeight;
> > - LPVOID buffer = current_surface->surface_desc.lpSurface;
> > -
> > - switch (current_surface->surface_desc.u4.ddpfPixelFormat.dwFourCC) {
> > -
> >    case MAKE_FOURCC('D','X','T','1'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT; break;
> > -
> >    case MAKE_FOURCC('D','X','T','3'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT3_EXT; break;
> > -
> >    case MAKE_FOURCC('D','X','T','5'): retVal = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT; break;
> > -
> >    default:
> > -
> > FIXME("Not supported\n");
> > -
> > return DD_OK;
> > -
> > break;
> > - }
> > - /* GL_extensions.glCompressedTexSubImage2D(GL_TEXTURE_2D, current_level, xoffset, yoffset, width, height, retVal, (unsigned char*)temp_buffer); */
> > - GL_extensions.glCompressedTexImage2D(GL_TEXTURE_2D, current_level, retVal, width_, height_, 0, size, buffer);
>
> Same here.
> > - }
> > -
> > - if (strstr(glExtensions, "GL_EXT_texture_compression_s3tc")) {
> > -
> >    TRACE(" - S3TC compression supported\n");
> > -
> >    GL_extensions.s3tc_compressed_texture = TRUE;
> > -
> >    GL_extensions.glCompressedTexImage2D = pglXGetProcAddressARB("glCompressedTexImage2D");
> > -
> >    GL_extensions.glCompressedTexSubImage2D = pglXGetProcAddressARB("glCompressedTexSubImage2D");
> > }
>
> If I were pedantic, you should test for 'GL_ARB_texture_compression' to be
> able to get the 'glCompressedTexImage2DARB' and
> 'glCompressedTexSubImage2DARB' functions (which may be better than using the
> non-ARB versions which are, AFAIK, the ones in GL 1.4).
>
> And then to get 'GL_EXT_texture_compression_s3tc' to set
> 'GL_extensions.s3tc_compressed_texture' to TRUE.
>
> But as 'GL_EXT_texture_compression_s3tc' depends on
> 'GL_ARB_texture_compression', I will let you do this shortcut :-)

:-)
> > - /* We do not support for now compressed texture formats... */
> > - if (ddsd.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC)
> > - /* We support for now only DXT1, DXT3 & DXT5 compressed texture formats... */
> > - if ((ddsd.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) &&
> > -
> >    (ddsd.u4.ddpfPixelFormat.dwFourCC != MAKE_FOURCC('D','X','T','1')) &&
> > -
> >    (ddsd.u4.ddpfPixelFormat.dwFourCC != MAKE_FOURCC('D','X','T','3')) &&
> > -
> >    (ddsd.u4.ddpfPixelFormat.dwFourCC != MAKE_FOURCC('D','X','T','5')) )
> > {
> >       return DDERR_INVALIDPIXELFORMAT;
> >   }
>
> What to do here when neither hardware nor emulated support is present ?
> Maybe we should print an error (cf previous comment) and return
> INVALIDPIXELFORMAT, no ?

Yes, we must check if there is any S3TC support otherwise we must return 
an error.
>    Lionel (who goes testing NOFL :-) )

Thanks for the comments.

Bye,
Christian