[PATCH 2/7] wined3d: Move srgb checks away from d3dfmt_get_conv.

[Diego Nieto Cid]

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!