[PATCH v4] d2d1: Implement ID2D1DeviceContext::CreateImageBrush().

[Dmitry Timoshkov]

Henri Verbeet <hverbeet at gmail.com> wrote:

> On Wed, 11 May 2022 at 16:22, Dmitry Timoshkov <dmitry at baikal.ru> wrote:
> > +HRESULT d2d_image_brush_create(ID2D1Factory *factory, ID2D1Image *image,
> > +        const D2D1_IMAGE_BRUSH_PROPERTIES *image_brush_desc, const D2D1_BRUSH_PROPERTIES *brush_desc,
> > +        struct d2d_brush **brush)
> > +{
> > +    if (!(*brush = heap_alloc_zero(sizeof(**brush))))
> > +        return E_OUTOFMEMORY;
> > +
> > +    d2d_brush_init(*brush, factory, D2D_BRUSH_TYPE_BITMAP,
> > +            brush_desc, (ID2D1BrushVtbl *)&d2d_image_brush_vtbl);
> 
> I think you intended to use D2D_BRUSH_TYPE_IMAGE above.

Good catch, thanks!

> > +    if (((*brush)->u.image.image = image))
> > +        ID2D1Image_AddRef((*brush)->u.image.image);
> > +    if (image_brush_desc)
> > +    {
> > +        (*brush)->u.image.extend_mode_x = image_brush_desc->extendModeX;
> > +        (*brush)->u.image.extend_mode_y = image_brush_desc->extendModeY;
> > +        (*brush)->u.image.interpolation_mode = image_brush_desc->interpolationMode;
> > +    }
> 
> This ignores "image_brush_desc->sourceRectangle". That's fine to some
> extent, but might be easy to overlook when implementing
> d2d_image_brush_SetSourceRectangle()/d2d_image_brush_GetSourceRectangle()
> later.

Also good point, is that fine to just add a FIXME, or it makes sense to also
save it in the bitmap?

> > +static void d2d_brush_bind_image(struct d2d_brush *brush, struct d2d_device_context *context,
> > +        unsigned int brush_idx)
> > +{
> > +    ID3D11SamplerState **sampler_state;
> > +    ID3D11DeviceContext *d3d_context;
> > +    ID2D1Bitmap *bitmap;
> > +    struct d2d_bitmap *src_impl;
> > +    HRESULT hr;
> > +
> > +    hr = ID2D1Image_QueryInterface(brush->u.image.image, &IID_ID2D1Bitmap, (void **)&bitmap);
> > +    if (FAILED(hr))
> > +    {
> > +        FIXME("ID2D1Image doesn't support ID2D1Bitmap interface.\n");
> > +        return;
> > +    }
> > +
> > +    src_impl = unsafe_impl_from_ID2D1Bitmap(bitmap);
> > +
> > +    ID3D11Device1_GetImmediateContext(context->d3d_device, &d3d_context);
> > +    ID3D11DeviceContext_PSSetShaderResources(d3d_context, brush_idx, 1, &src_impl->srv);
> > +
> > +    sampler_state = &context->sampler_states
> > +            [brush->u.image.interpolation_mode % D2D_SAMPLER_INTERPOLATION_MODE_COUNT]
> > +            [brush->u.image.extend_mode_x % D2D_SAMPLER_EXTEND_MODE_COUNT]
> > +            [brush->u.image.extend_mode_y % D2D_SAMPLER_EXTEND_MODE_COUNT];
> > +
> > +    if (!*sampler_state)
> > +    {
> > +        D3D11_SAMPLER_DESC sampler_desc;
> > +
> > +        if (brush->u.image.interpolation_mode == D2D1_INTERPOLATION_MODE_NEAREST_NEIGHBOR)
> > +            sampler_desc.Filter = D3D11_FILTER_MIN_MAG_MIP_POINT;
> > +        else
> > +            sampler_desc.Filter = D3D11_FILTER_MIN_MAG_MIP_LINEAR;
> > +        sampler_desc.AddressU = texture_address_mode_from_extend_mode(brush->u.image.extend_mode_x);
> > +        sampler_desc.AddressV = texture_address_mode_from_extend_mode(brush->u.image.extend_mode_y);
> > +        sampler_desc.AddressW = D3D11_TEXTURE_ADDRESS_CLAMP;
> > +        sampler_desc.MipLODBias = 0.0f;
> > +        sampler_desc.MaxAnisotropy = 0;
> > +        sampler_desc.ComparisonFunc = D3D11_COMPARISON_NEVER;
> > +        sampler_desc.BorderColor[0] = 0.0f;
> > +        sampler_desc.BorderColor[1] = 0.0f;
> > +        sampler_desc.BorderColor[2] = 0.0f;
> > +        sampler_desc.BorderColor[3] = 0.0f;
> > +        sampler_desc.MinLOD = 0.0f;
> > +        sampler_desc.MaxLOD = 0.0f;
> > +
> > +        if (FAILED(hr = ID3D11Device1_CreateSamplerState(context->d3d_device, &sampler_desc, sampler_state)))
> > +            ERR("Failed to create sampler state, hr %#lx.\n", hr);
> > +    }
> > +
> > +    ID3D11DeviceContext_PSSetSamplers(d3d_context, brush_idx, 1, sampler_state);
> > +    ID3D11DeviceContext_Release(d3d_context);
> > +    ID2D1Bitmap_Release(bitmap);
> > +}
> 
> This doesn't seem wrong, although we could probably introduce a common
> helper for this and d2d_brush_bind_bitmap(). At the same time though,
> it's not going to do much as long as the pixel shader (i.e., ps_code[]
> in d2d_device_context_init()) doesn't handle image brushes. (And in
> it's current form, we never get here because we set the brush type to
> D2D_BRUSH_TYPE_BITMAP in d2d_image_brush_create().) It might be best
> to just go with the FIXME in d2d_brush_bind_resources(), unless we
> need the brush to actually work.

Thanks for the review and suggestions, I guess that adding a FIXME could be
a reasonable approach at this point.

-- 
Dmitry.