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?

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

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-05-12 16:44, schrieb 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.)

Not exactly. We load P8 GL textures for the front buffer when
ARB_fragment_program is unsupported (and thus the format in theory is
unsupported). So we might end up reading back the texture even though
the format table doesn't contain data for P8.

Currently we never read back P8 data due to the ddraw front buffer and
because we never load a GL P8 texture for offscreen surfaces (unless
this changed at some point and I missed it). This doesn't make testing
this easier. (And that's how the goto error thing didn't fail during
testing.)

I'll see if I can make the P8 line in the GL format table
unconditional, that would make things easier. I'd prefer not to detour
too much into a P8 GL handling cleanup though.
> > -
> >    return (BYTE)((color->a * 255.0f) + 0.5f);
>
> So this could just use the table now, right?

Right.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTceKHAAoJEN0/YqbEcdMwMLcP/1uDNi+e4e3wenb7mcmxjs3t
J7R7w/8ojIT6hLrSghP3d1QaECp+bQxTBVpUfuZ6lSu4sMEPb/kYu50+okFyjlIT
dJNkJvNEwyCrSyx7pq6nY8nQRB9YzaqgOExfMEix9X+z1NqzB9JSSInSZu/Cqy3l
Wg9oKJwSWSSbfoKzXXKKR/1TAVvE/ReZ2iJaefl+BfH3oSDm1X+s8TYfsqlD4HfM
1IoOB2iuhkyykLUdNmKe8tbzPw5Ql/xVdGeQBc7r4Rgv+O1Rz/qHClTxqapAYbZe
n5YKPYSPTiaNjl9yBXAQjm72XlJXwNFMUxO7j+XnWr8EkAmwiLRptiy1sR89vFO4
HGpOX/3U58mRTRKZCAmQIzTiuJFXudLosK2Od+J9ZfzPUhIf0vPAly6evtgDxK/V
kV3SGtla/cWS77GVPTWpoiSdY7Vfgzkjn/Ww3QXQ5rg3FBfqCNKbYrPSKU+SG000
Q+u/ul3/ro8lGpFofGRymfTGC4xgyUH5Zl9DM08DNh8JoNcsJCNVyvr06xa6RHV3
IJfjooqdjVGUhCVWbyISNSPnEBWjdRv8U2nge9SHRrNPshBjyRJq6PY8cawbzkAN
4/4JjI4pCcrjh0dKHiiW9iI7PfF6rjZ2k9H611eZJIObzsT1+mnTFdTHa3sfDYGg
Ni7LabYC0LThIyZLG4TQ
=bPJW
-----END PGP SIGNATURE-----

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

[Henri Verbeet]

On 13 May 2014 11:14, Stefan Dösinger  wrote:
> Not exactly. We load P8 GL textures for the front buffer when
> ARB_fragment_program is unsupported (and thus the format in theory is
> unsupported). So we might end up reading back the texture even though
> the format table doesn't contain data for P8.

So which internal format would be used to create that texture?

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

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-05-13 12:28, schrieb Henri Verbeet:> On 13 May 2014 11:14,
Stefan Dösinger  wrote:
> > Not exactly. We load P8 GL textures for the front buffer when 
> > ARB_fragment_program is unsupported (and thus the format in
> > theory is unsupported). So we might end up reading back the
> > texture even though the format table doesn't contain data for
> > P8.
>
> So which internal format would be used to create that texture?

internalFormat = GL_RGBA, format = GL_RGBA, type = GL_UNSIGNED_BYTE.
This is set in d3dfmt_get_conv.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTcgElAAoJEN0/YqbEcdMwN74P/AmodlWZ+AyfCPSEWmOFM1OY
XWuhZC/x250E/NpnaNlMMld/GD1RuhCLENLlklhEqMXtVtC6ppDW1X/5tQaWwkl8
T3p/FGAn0WJ/a/pt2NexxrFZ6KkbLJLDeikyOHgsNOwORxvrQtztZwq5vG0cbxro
h/902Ivwq4C5llieu15xFfg8IzrHUaKelD9l7T4rWJTnnom8FbA5iLD6cg+M9x4R
8CeCV7AbJBJ7PblQ5mRRggzNXtwMfBKP4rnv8MFak6IJ3M7WyDmw5ls10GW+2RMH
LzUYD6hc245CL+AylXJ7k75W1Fbaf1A463dbccsHvgKZLf8i/9e8QRYnr1/25jm+
k3iMerQmurMKJ6lhH5vBoMKe1iaMhBFFXzI+FWqrTyfFZuNIu3e4lk0ll95CRE/3
ADVQDO1o8ckwi96vdbSniGUw9losejhEaoNEXRoVmxAZMTPZQfR8h27M3Wq90foN
sPD7dhaP9WOtsXuYqK0nfnyWhiD4qliWAs3BzByeJF2ypljv4KTHWMixbcJYWMrh
/ftMHAdbjhJEORKioYRppXBxMxrj9l39iWOSpyBsfWMaQP5ya2B3Rv0vF3A0WlBB
hIJ56ZfMNQJxrXbsoSLatKCiBe+dOTYIhZDd4cZxiI/FEiOnIUzSrfQ7BHv1Jvs9
RglVQMOoRdSjs6TeKZJ0
=dCUx
-----END PGP SIGNATURE-----