[PATCH 3/3] d2d1: Reuse PS constant buffer object.

[Henri Verbeet]

On Tue, 8 Jun 2021 at 10:57, Nikolay Sivov <nsivov at codeweavers.com> wrote:
> -HRESULT d2d_brush_get_ps_cb(struct d2d_brush *brush, struct d2d_brush *opacity_brush,
> -        BOOL outline, BOOL is_arc, struct d2d_device_context *render_target, ID3D10Buffer **ps_cb)
> +HRESULT d2d_brush_init_ps_cb_data(struct d2d_brush *brush, struct d2d_brush *opacity_brush,
> +        BOOL outline, BOOL is_arc, struct d2d_ps_cb *cb_data)
>  {
> -    D3D10_SUBRESOURCE_DATA buffer_data;
> -    struct d2d_ps_cb cb_data = {0};
> -    D3D10_BUFFER_DESC buffer_desc;
> -    HRESULT hr;
> -
> -    cb_data.outline = outline;
> -    cb_data.is_arc = is_arc;
> -    if (!d2d_brush_fill_cb(brush, &cb_data.colour_brush))
> +    cb_data->outline = outline;
> +    cb_data->is_arc = is_arc;
> +    if (!d2d_brush_fill_cb(brush, &cb_data->colour_brush))
>          return E_NOTIMPL;
> -    if (!d2d_brush_fill_cb(opacity_brush, &cb_data.opacity_brush))
> +    if (!d2d_brush_fill_cb(opacity_brush, &cb_data->opacity_brush))
>          return E_NOTIMPL;
>
> -    buffer_desc.ByteWidth = sizeof(cb_data);
> -    buffer_desc.Usage = D3D10_USAGE_DEFAULT;
> -    buffer_desc.BindFlags = D3D10_BIND_CONSTANT_BUFFER;
> -    buffer_desc.CPUAccessFlags = 0;
> -    buffer_desc.MiscFlags = 0;
> -
> -    buffer_data.pSysMem = &cb_data;
> -    buffer_data.SysMemPitch = 0;
> -    buffer_data.SysMemSlicePitch = 0;
> -
> -    if (FAILED(hr = ID3D10Device_CreateBuffer(render_target->d3d_device, &buffer_desc, &buffer_data, ps_cb)))
> -        ERR("Failed to create constant buffer, hr %#x.\n", hr);
> -
> -    return hr;
> +    return S_OK;
>  }
>
Arguably, what's left of the function could just be inlined in
d2d_device_context_update_ps_cb() now.

> @@ -266,6 +266,8 @@ static ULONG STDMETHODCALLTYPE d2d_device_context_inner_Release(IUnknown *iface)
>          ID3D10Buffer_Release(context->vb);
>          ID3D10Buffer_Release(context->ib);
>          ID3D10PixelShader_Release(context->ps);
> +        if (context->ps_cb)
> +            ID3D10Buffer_Release(context->ps_cb);
>          for (i = 0; i < D2D_SHAPE_TYPE_COUNT; ++i)
>          {
>              ID3D10VertexShader_Release(context->shape_resources[i].vs);
If we created "context->ps_cb" in d2d_device_context_init(), this
would become an unconditional release.

> +static HRESULT d2d_device_context_update_ps_cb(struct d2d_device_context *context,
> +        struct d2d_brush *brush, struct d2d_brush *opacity_brush, BOOL outline, BOOL is_arc)
> +{
> +    D3D10_SUBRESOURCE_DATA buffer_data;
> +    struct d2d_ps_cb cb_data = {0};
> +    D3D10_BUFFER_DESC buffer_desc;
> +    void *data;
> +    HRESULT hr;
> +
> +    if (FAILED(hr = d2d_brush_init_ps_cb_data(brush, opacity_brush, outline, is_arc, &cb_data)))
> +    {
> +        WARN("Failed to initialize brush constant buffer data, hr %#x.\n", hr);
> +        return hr;
> +    }
> +
> +    if (context->ps_cb)
> +    {
> +        if (SUCCEEDED(hr = ID3D10Buffer_Map(context->ps_cb, D3D10_MAP_WRITE_DISCARD, 0, &data)))
> +        {
> +            memcpy(data, &cb_data, sizeof(cb_data));
> +            ID3D10Buffer_Unmap(context->ps_cb);
> +        }
> +        else
> +            ERR("Failed to map constant buffer, hr %#x.\n", hr);
> +    }
We don't need the memcpy(). I.e., we can just do the following:

    struct d2d_ps_cb *cb_data;
    [...]
    ID3D10Buffer_Map(..., &cb_data);
    d2d_brush_init_ps_cb_data(..., cb_data);
    ID3D10Buffer_Unmap(...);

> +    else
> +    {
> +        buffer_desc.ByteWidth = sizeof(cb_data);
> +        buffer_desc.Usage = D3D10_USAGE_DYNAMIC;
> +        buffer_desc.BindFlags = D3D10_BIND_CONSTANT_BUFFER;
> +        buffer_desc.CPUAccessFlags = D3D10_CPU_ACCESS_WRITE;
> +        buffer_desc.MiscFlags = 0;
> +
> +        buffer_data.pSysMem = &cb_data;
> +        buffer_data.SysMemPitch = 0;
> +        buffer_data.SysMemSlicePitch = 0;
> +
> +        if (FAILED(hr = ID3D10Device_CreateBuffer(context->d3d_device, &buffer_desc, &buffer_data,
> +                &context->ps_cb)))
> +        {
> +            ERR("Failed to create constant buffer, hr %#x.\n", hr);
> +        }
> +    }
> +
> +    return hr;
> +}
> +
And this could just happen during d2d_device_context_init(). (The
"buffer_data" argument is optional; it can be convenient for
initialising resources without explicitly mapping them, but I don't
think it provides much of an advantage here.)

> @@ -1676,34 +1707,19 @@ static void STDMETHODCALLTYPE d2d_device_context_Clear(ID2D1DeviceContext *iface
>          return;
>      }
>
> -    ps_cb_data.outline = FALSE;
> -    ps_cb_data.colour_brush.type = D2D_BRUSH_TYPE_SOLID;
> -    ps_cb_data.colour_brush.opacity = 1.0f;
> -    c = &ps_cb_data.colour_brush.u.solid.colour;
> +    memset(&brush, 0, sizeof(brush));
> +    brush.type = D2D_BRUSH_TYPE_SOLID;
> +    brush.opacity = 1.0f;
> +    c = &brush.u.solid.color;
>      if (colour)
>          *c = *colour;
>      if (render_target->desc.pixelFormat.alphaMode == D2D1_ALPHA_MODE_IGNORE)
>          c->a = 1.0f;
> -    c->r *= c->a;
> -    c->g *= c->a;
> -    c->b *= c->a;
> -
Constructing a brush like that is a little awkward, I think. To some
extent that's also true of constructing the d2d_ps_cb structure in the
original code, but brush objects are a bit more complex than the
d2d_ps_cb structure. It would seem preferable to either directly map
"ps_cb" here, and generate its contents in the existing way, or to
create a proper brush object using d2d_solid_color_brush_create().