Re: [PATCH 3/5] wined3d: Always store the palette index in the alpha component.

[Henri Verbeet]

On 12 May 2014 15:12, Stefan Dösinger  wrote:
> @@ -1458,8 +1457,7 @@ static void surface_download_data(struct wined3d_surface *surface, const struct
>          int src_pitch = 0;
>          int dst_pitch = 0;
> -
>    /* In case of P8 the index is stored in the alpha component if the primary render target uses P8. */
> -
>    if (format->id == WINED3DFMT_P8_UINT && swapchain_is_p8(surface->resource.device->swapchains[0]))
> -
>    if (format->id == WINED3DFMT_P8_UINT)
>    {
>        gl_format = GL_ALPHA;
>        gl_type = GL_UNSIGNED_BYTE;

This branch is pretty pointless, the format / type for
WINED3DFMT_P8_UINT is always GL_ALPHA / GL_UNSIGNED_BYTE. (If
ARB_fragment_program is supported, and the format is supported at all
anyway.)
> @@ -3256,53 +3252,19 @@ static void read_from_framebuffer(struct wined3d_surface *surface, DWORD dst_loc
>      switch (surface->resource.format->id)
>      {
>          case WINED3DFMT_P8_UINT:
> -
>    {
> -
>        if (swapchain_is_p8(context->swapchain))
> -
>        {
> -
>            /* In case of P8 render targets the index is stored in the alpha component */
> -
>            fmt = GL_ALPHA;
> -
>            type = GL_UNSIGNED_BYTE;
> -
>            mem = data.addr;
> -
>            bpp = surface->resource.format->byte_count;
> -
>        }
> -
>        else
> -
>        {
> -
>            /* GL can't return palettized data, so read ARGB pixels into a
> -
>             * separate block of memory and convert them into palettized format
> -
>             * in software. Slow, but if the app means to use palettized render
> -
>             * targets and locks it...
> -
>             *
> -
>             * Use GL_RGB, GL_UNSIGNED_BYTE to read the surface for performance reasons
> -
>             * Don't use GL_BGR as in the WINED3DFMT_R8G8B8 case, instead watch out
> -
>             * for the color channels when palettizing the colors.
> -
>             */
> -
>            fmt = GL_RGB;
> -
>            type = GL_UNSIGNED_BYTE;
> -
>            pitch *= 3;
> -
>            mem = HeapAlloc(GetProcessHeap(), 0, surface->resource.size * 3);
> -
>            if (!mem)
> -
>            {
> -
>                ERR("Out of memory\n");
> -
>                return;
> -
>            }
> -
>            bpp = surface->resource.format->byte_count * 3;
> -
>        }
> -
>    }
> -
>    break;
> -
>        fmt = GL_ALPHA;
> -
>        type = GL_UNSIGNED_BYTE;
> -
>        break;
>
>    default:
> -
>        mem = data.addr;
>        fmt = surface->resource.format->glFormat;
>        type = surface->resource.format->glType;
> -
>        bpp = surface->resource.format->byte_count;
> }

Like above.
>  if (!srcIsUpsideDown)
>  {
> -
>    UINT len;
>    /* glReadPixels returns the image upside down, and there is no way to prevent this.
> -
>        Flip the lines in software */
> -
>    len = surface->resource.width * bpp;
> -
>     * Flip the lines in software.
> -
>     * TODO: Consider flipping the image in an FBO. */

Possibly, but that would be a decision for surface_load_sysmem(), not
read_from_framebuffer(). Note that reading the front buffer isn't
particularly fast on Windows either though.
> -
>    row = HeapAlloc(GetProcessHeap(), 0, pitch);
> -
>        goto error;

This looks unintentional.
> @@ -3088,38 +3088,7 @@ DWORD wined3d_format_convert_from_float(const struct wined3d_surface *surface, c
>      }
>  if (format->id == WINED3DFMT_P8_UINT)
> - {
> -
>    const RGBQUAD *e;
> -
>    BYTE r, g, b, a;
> -
> -
>    if (!surface->palette)
> -
>    {
> -
>        WARN("Surface doesn't have a palette, returning 0.\n");
> -
>        return 0;
> -
>    }
> -
> -
>    r = (BYTE)((color->r * 255.0f) + 0.5f);
> -
>    g = (BYTE)((color->g * 255.0f) + 0.5f);
> -
>    b = (BYTE)((color->b * 255.0f) + 0.5f);
> -
>    a = (BYTE)((color->a * 255.0f) + 0.5f);
> -
> -
>    e = &surface->palette->colors[a];
> -
>    if (e->rgbRed == r && e->rgbGreen == g && e->rgbBlue == b)
> -
>        return a;
> -
> -
>    WARN("Alpha didn't match index, searching full palette.\n");
> -
> -
>    for (i = 0; i < 256; ++i)
> -
>    {
> -
>        e = &surface->palette->colors[i];
> -
>        if (e->rgbRed == r && e->rgbGreen == g && e->rgbBlue == b)
> -
>            return i;
> -
>    }
> -
> -
>    FIXME("Unable to convert color to palette index.\n");
> -
> -
>    return 0;
> - }
> -
>    return (BYTE)((color->a * 255.0f) + 0.5f);

So this could just use the table now, right?