Re: [PATCH 1/9] wined3d: Separate mapping from data writing in surface_cpu_blt_colour_fill.

[Henri Verbeet]

On Sun, 1 May 2022 at 20:39, Stefan Dösinger  wrote:
> +void wined3d_resource_memory_colour_fill(struct wined3d_resource *resource,
> -
>    unsigned int level, const struct wined3d_map_desc *map,
> -
>    const struct wined3d_color *colour, const struct wined3d_box *box)
>
> +{
> - const struct wined3d_format *format = resource->format;
> - unsigned int w, h, d, x, y, z, bpp;
> - struct wined3d_box level_box;
> - uint8_t *dst, *dst2;
> - DWORD c;
> -
> - if (resource->type == WINED3D_RTYPE_BUFFER)
> - {
> -
>    level_box.left = 0; level_box.top = 0; level_box.front = 0;
> -
>    level_box.right = resource->width; level_box.bottom = 1; level_box.back = 1;
> - }
> - else
> - {
> -
>    wined3d_texture_get_level_box(texture_from_resource(resource), level, &level_box);
> - }
> -
> - w = min(box->right, level_box.right) - min(box->left, level_box.right);
> - h = min(box->bottom, level_box.bottom) - min(box->top, level_box.bottom);
> - if (resource->type != WINED3D_RTYPE_TEXTURE_3D)
> -
>    d = 1;
> - else
> -
>    d = min(box->back, level_box.back) - min(box->front, level_box.back);
> -

This (i.e., clipping the supplied box against the resource dimensions)
doesn't belong here; the caller should simply supply the correct box.
Arguably it doesn't belong in surface_cpu_blt_colour_fill() either;
the most appropriate place would be cpu_blitter_clear().
> - dst = (BYTE *)map->data
> -
>        + (box->front * map->slice_pitch)
> -
>        + ((box->top / format->block_height) * map->row_pitch)
> -
>        + ((box->left / format->block_width) * format->block_byte_count);

We might as well cast to "uint8_t *". (And use uint16_t and uint32_t
further below.)
> - c = wined3d_format_convert_from_float(format, colour);
> - bpp = format->byte_count;
> -
> - switch (bpp)
> - {
> -
>    case 1:
> -
>        for (x = 0; x < w; ++x)
> -
>        {
> -
>            dst[x] = c;
> -
>        }
> -
>        break;
> -
> -
>    case 2:
> -
>        for (x = 0; x < w; ++x)
> -
>        {
> -
>            ((WORD *)dst)[x] = c;
> -
>        }
> -
>        break;
> -
> -
>    case 3:
> -
>    {
> -
>        dst2 = dst;
> -
>        for (x = 0; x < w; ++x, dst += 3)
> -
>        {
> -
>            dst2[0] = (c      ) & 0xff;
> -
>            dst2[1] = (c >>  8) & 0xff;
> -
>            dst2[2] = (c >> 16) & 0xff;
> -
>        }
> -
>        break;
> -
>    }

Does that do the right thing? We update "dst", but not "dst2".