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