Re: [PATCH 2/7] wined3d: Move srgb checks away from d3dfmt_get_conv.
Hi, I beleive this patch introduced some kind of regression. While testing Fallout the following message was printed to the screen: trace:d3d_surface:surface_allocate_surface (0x1a25f0) : Creating surface (target 0xde1) level 0, d3d format WINED3DFMT_P8_UINT, internal format 0x80e5, width 1024, height 512, gl format 0x1908, gl type=0x1401 err:d3d_surface:surface_allocate_surface >>>>>>>>>>>>>>>>> GL_INVALID_VALUE (0x501) from glTexImage2D @ surface.c / 2571 Internal format 0x80e5 (GL_COLOR_INDEX8_EXT) is not valid for the format 0x1908 (GL_RGBA). Initially, the surface's format is 0x1900 (GL_COLOR_INDEX) which is compatible with the internal format above. On Thu, Apr 08, 2010 at 10:49:46PM +0200, Roderick Colenbrander wrote:
[...] @@ -1553,7 +1581,7 @@ void surface_prepare_texture(IWineD3DSurfaceImpl *surface, const struct wined3d_
if (surface->Flags & alloc_flag) return;
- d3dfmt_get_conv(surface, TRUE, TRUE, &desc, &convert, srgb); + d3dfmt_get_conv(surface, TRUE, TRUE, &desc, &convert); if(convert != NO_CONVERSION) surface->Flags |= SFLAG_CONVERTED; else surface->Flags &= ~SFLAG_CONVERTED;
@@ -1569,7 +1597,7 @@ void surface_prepare_texture(IWineD3DSurfaceImpl *surface, const struct wined3d_ }
surface_bind_and_dirtify(surface, srgb); - surface_allocate_surface(surface, gl_info, &desc, width, height); + surface_allocate_surface(surface, gl_info, &desc, srgb, width, height); surface->Flags |= alloc_flag; }
However before surface_allocate_surface is called d3dfmt_get_conv which translates the format to GL_RGBA. But the internal format remains in an incompatible value due to a reordering of the instructions which was introduced by the patch. On Thu, Apr 08, 2010 at 10:49:46PM +0200, Roderick Colenbrander wrote:
[...] @@ -2126,18 +2153,6 @@ HRESULT d3dfmt_get_conv(IWineD3DSurfaceImpl *This, BOOL need_alpha_ck, BOOL use_ *desc = *This->resource.format_desc; *convert = NO_CONVERSION;
- /* TODO: the caller should perform this check */ - if(srgb_mode) { - desc->glInternal = glDesc->glGammaInternal; - } - else if (This->resource.usage & WINED3DUSAGE_RENDERTARGET - && surface_is_offscreen((IWineD3DSurface *) This)) - { - desc->glInternal = glDesc->rtInternal; - } else { - desc->glInternal = glDesc->glInternal; - } - /* Ok, now look if we have to do any conversion */ switch(This->resource.format_desc->format) {
Before applying the changes the processing of srgb_mode was done inside d3dfmt_get_conv. Depending on the value of this variable one of glGammaInternal, rtInternal or glInternal was used. But those values were overriden because the conversion was applied afterwards. On Thu, Apr 08, 2010 at 10:49:46PM +0200, Roderick Colenbrander wrote:
[...] @@ -783,11 +797,25 @@ static void surface_upload_data(IWineD3DSurfaceImpl *This, const struct wined3d_ * the correct texture. */ /* Context activation is done by the caller. */ static void surface_allocate_surface(IWineD3DSurfaceImpl *This, const struct wined3d_gl_info *gl_info, - const struct wined3d_format_desc *format_desc, GLsizei width, GLsizei height) + const struct wined3d_format_desc *format_desc, BOOL srgb, GLsizei width, GLsizei height) { BOOL enable_client_storage = FALSE; const BYTE *mem = NULL; - GLenum internal = format_desc->glInternal; + GLenum internal; + + if (srgb) + { + internal = format_desc->glGammaInternal; + } + else if (This->resource.usage & WINED3DUSAGE_RENDERTARGET + && surface_is_offscreen((IWineD3DSurface *)This)) + { + internal = format_desc->rtInternal; + } + else + { + internal = format_desc->glInternal; + }
if (format_desc->heightscale != 1.0f && format_desc->heightscale != 0.0f) height *= format_desc->heightscale;
After the change was applied the result of the conversion done inside d3dfmt_get_conv only takes effect in the else branch of srgb processing. The following code is the conversion involved in this particular test case. format->glFormat = GL_RGBA; format->glInternal = GL_RGBA; format->glType = GL_UNSIGNED_BYTE; format->conv_byte_count = 4; It's located in d3dfmt_get_conv under the WINED3DFMT_P8_UINT case. For some reason only glInternal is updated by the conversion. In any case it didn't matter before the patch as none of the internal values were actually used in case a conversion was applied. Should the three internal values be updated by the conversion now that any of them could be used? Even if it would work I'm not sure how correct would be to do that. Could some wined3d developer review the patch with all this in mind? Thanks!
On 26 December 2011 05:32, Diego Nieto Cid <dnietoc(a)gmail.com> wrote:
trace:d3d_surface:surface_allocate_surface (0x1a25f0) : Creating surface (target 0xde1) level 0, d3d format WINED3DFMT_P8_UINT, internal format 0x80e5, width 1024, height 512, gl format 0x1908, gl type=0x1401 err:d3d_surface:surface_allocate_surface >>>>>>>>>>>>>>>>> GL_INVALID_VALUE (0x501) from glTexImage2D @ surface.c / 2571
Does your hardware really support EXT_paletted_texture? That's somewhat unusual.
It's located in d3dfmt_get_conv under the WINED3DFMT_P8_UINT case. For some reason only glInternal is updated by the conversion. In any case it didn't matter before the patch as none of the internal values were actually used in case a conversion was applied.
Should the three internal values be updated by the conversion now that any of them could be used? Probably, yeah. Not just for WINED3DFMT_P8_UINT, but for all the formats in d3dfmt_get_conv(). I guess something like the following at the end of d3dfmt_get_conv() should work:
if (*convert != NO_CONVERSION) { format->glGammaInternal = format->glInternal; format->rtInternal = format->glInternal; }
Even if it would work I'm not sure how correct would be to do that.
The whole surface conversion code is pretty ugly and fragile, unfortunately.
On Wed, Dec 28, 2011 at 07:46:09AM +0100, Henri Verbeet wrote:
On 26 December 2011 05:32, Diego Nieto Cid <dnietoc(a)gmail.com> wrote:
trace:d3d_surface:surface_allocate_surface (0x1a25f0) : Creating surface (target 0xde1) level 0, d3d format WINED3DFMT_P8_UINT, internal format 0x80e5, width 1024, height 512, gl format 0x1908, gl type=0x1401 err:d3d_surface:surface_allocate_surface >>>>>>>>>>>>>>>>> GL_INVALID_VALUE (0x501) from glTexImage2D @ surface.c / 2571
Does your hardware really support EXT_paletted_texture? That's somewhat unusual.
I've got a rather old NVIDIA video card: 01:00.0 VGA compatible controller: nVidia Corporation NV18 [GeForce4 MX 4000] (rev c1) On Wed, Dec 28, 2011 at 07:46:09AM +0100, Henri Verbeet wrote:
It's located in d3dfmt_get_conv under the WINED3DFMT_P8_UINT case. For some reason only glInternal is updated by the conversion. In any case it didn't matter before the patch as none of the internal values were actually used in case a conversion was applied.
Should the three internal values be updated by the conversion now that any of them could be used? Probably, yeah. Not just for WINED3DFMT_P8_UINT, but for all the formats in d3dfmt_get_conv(). I guess something like the following at the end of d3dfmt_get_conv() should work:
if (*convert != NO_CONVERSION) { format->glGammaInternal = format->glInternal; format->rtInternal = format->glInternal; }
The HEAD checkout is randomly failing due to wine: Unhandled page fault on read access to 0x00000000 at address (nil) (thread 0036), starting debugger... X Error of failed request: GLXBadDrawable Major opcode of failed request: 128 (GLX) Minor opcode of failed request: 5 (X_GLXMakeCurrent) Serial number of failed request: 621 Current serial number in output stream: 621 But I'll try adding that snippet to a checkout of the version installed on my system. (I have no idea how to debug the error above :)
Hi, On Wed, Dec 28, 2011 at 07:46:09AM +0100, Henri Verbeet wrote:
Probably, yeah. Not just for WINED3DFMT_P8_UINT, but for all the formats in d3dfmt_get_conv(). I guess something like the following at the end of d3dfmt_get_conv() should work:
if (*convert != NO_CONVERSION) { format->glGammaInternal = format->glInternal; format->rtInternal = format->glInternal; }
After adding the conditional to wine-1.3.33, Fallout runs without any error. A patch against HEAD is attached. On Wed, Dec 28, 2011 at 09:17:26PM -0300, Diego Nieto Cid wrote:
The HEAD checkout is randomly failing due to
wine: Unhandled page fault on read access to 0x00000000 at address (nil) (thread 0036), starting debugger... X Error of failed request: GLXBadDrawable Major opcode of failed request: 128 (GLX) Minor opcode of failed request: 5 (X_GLXMakeCurrent) Serial number of failed request: 621 Current serial number in output stream: 621
HEAD still shows the error above. I'll try to find what happened through bisection. Any idea what to look for? :)
On Tue, Jan 03, 2012 at 09:48:59PM -0300, Diego Nieto Cid wrote:
HEAD still shows the error above. I'll try to find what happened through bisection.
Oh sorry, I forgot to mention this only happens when I use a virtual desktop.
On 4 January 2012 01:48, Diego Nieto Cid <dnietoc(a)gmail.com> wrote:
After adding the conditional to wine-1.3.33, Fallout runs without any error.
A patch against HEAD is attached.
Looks good to me, please send that to wine-patches, perhaps with a small comment.
On Wed, Dec 28, 2011 at 09:17:26PM -0300, Diego Nieto Cid wrote:
The HEAD checkout is randomly failing due to
wine: Unhandled page fault on read access to 0x00000000 at address (nil) (thread 0036), starting debugger... X Error of failed request: GLXBadDrawable Major opcode of failed request: 128 (GLX) Minor opcode of failed request: 5 (X_GLXMakeCurrent) Serial number of failed request: 621 Current serial number in output stream: 621
HEAD still shows the error above. I'll try to find what happened through bisection.
Any idea what to look for? :)
The message suggests it's a glXMakeCurrent() / wglMakeCurrent() call with a bad Drawable / window, perhaps because the window was already destroyed. It might be something along the lines of http://bugs.winehq.org/show_bug.cgi?id=29236, although that bug turned out to be invalid.
participants (2)
-
Diego Nieto Cid -
Henri Verbeet