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 <stefan(a)codeweavers.com> 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 <stefan(a)codeweavers.com>
> 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 <stefandoesinger(a)gmail.com> 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 <stefandoesinger(a)gmail.com> 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-----