[PATCH v3] d2d1: Make ID2D1Device::CreateImageBrush() accept only bitmap as a source image.

[Nikolay Sivov]

On 6/3/22 15:40, Dmitry Timoshkov wrote:
> Nikolay Sivov <nsivov at codeweavers.com> wrote:
>
>> On 5/26/22 17:36, Dmitry Timoshkov wrote:
>>> Nikolay Sivov <nsivov at codeweavers.com> wrote:
>>>
>>>> On 5/26/22 17:15, Dmitry Timoshkov wrote:
>>>>> Nikolay Sivov <nsivov at codeweavers.com> wrote:
>>>>>
>>>>>> On 5/26/22 14:34, Dmitry Timoshkov wrote:
>>>>>>> Nikolay Sivov <nsivov at codeweavers.com> wrote:
>>>>>>>
>>>>>>>> On 5/25/22 18:58, Dmitry Timoshkov wrote:
>>>>>>>>> Dmitry Timoshkov <dmitry at baikal.ru> wrote:
>>>>>>>>>
>>>>>>>>>> ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource.
>>>>>>>>>> That means that ID2D1Device::CreateImageBrush() can't be really passed anything
>>>>>>>>>> but a ID2D1Bitmap* represented as a ID2D1Image*.
>>>>>>>>>>
>>>>>>>>>> I've added QueryInterface+FIXME just in case, probably it could be dropped.
>>>>>>>>>>
>>>>>>>>>> v2: Fix test crashes with image == NULL.
>>>>>>>>>> v3: Add a QueryInterface() check to SetImage().
>>>>>>>>> Is there anything that could be improved in this patch to make it acceptable?
>>>>>>>>> Probably I should add once again, that this patch allows a bitmap work as an
>>>>>>>>> image brush (unlike current broken state) and actually makes work the app that
>>>>>>>>> have here.
>>>>>>>>>
>>>>>>>> Same thing I mention last time - it's backwards. Bitmap brush could
>>>>>>>> probably be implemented as image brush, but not the other way. I don't
>>>>>>>> think you'll need any shader changes for that.
>>>>>>> Do you mean something like the attached patch, or do I miss something?
>>>>>>> With the attached patch (on top of just sent ::Map()/::Unmap() patches)
>>>>>>> I get areas filled with black in the application that I'm working on,
>>>>>>> unlike with the proposed patch where I get correctly filled areas with
>>>>>>> a proper brush.
>>>>>>>
>>>>>> I suspect black color is from this in sample_brush():
>>>>>>
>>>>>> return float4(0.0, 0.0, 0.0, brush.opacity);
>>>>>>
>>>>>> Which means you didn't set brush type correctly in the constant buffer.
>>>>> I also think that's the source of the problem. However that's the shader
>>>>> code, and you mentioned above that I don't need them to make image brush
>>>>> work. Or did I misunderstand something? Do you have a suggestion for that?
>>>>>
>>>> Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly"
>>>> to bitmap brush type.
>>> Thank you very much, setting cb->type to TYPE_BITMAP in the TYPE_IMAGE case
>>> has fixed painting in my application. Do you think that the patch I attached
>>> in my previous reply with this fix is an acceptable solution, or there are
>>> other things you'd like to see addressed?
>>>
>> I think we should avoid duplication of bind_bitmap() function, since
>> it's exactly the same. Probably passing image brush description there,
>> which could be filled from bitmap brush description, expect for source
>> rectangle.
> Thanks for the helpful pointers. Does the attached patch match your suggestions?
> The patch works prety well with the application that I have here.
>
Yes, along those lines.

> +static void d2d_brush_bind_bitmap(struct d2d_brush *brush, struct 
> d2d_device_context *context,
> +        unsigned int brush_idx)
> +{
> +    D2D1_IMAGE_BRUSH_PROPERTIES image_brush_desc;
> +
> +    image_brush_desc.sourceRectangle.left = 0.0f;
> +    image_brush_desc.sourceRectangle.top = 0.0f;
> +    image_brush_desc.sourceRectangle.right = 
> brush->u.bitmap.bitmap->pixel_size.width;
> +    image_brush_desc.sourceRectangle.bottom = 
> brush->u.bitmap.bitmap->pixel_size.height;
This needs a test, so we don't assume coordinate system here. We have 
tests for filling with 4x4 bitmap brush, it will be a matter of creating 
image brush with same bitmap, and rectangle as (0,0-4,4) vs (0,0-1,1). 
If that shows no difference it means this is using normalized 
coordinates, and we should initialize it here as (0,0-1,1).

> @@ -1043,7 +1043,7 @@ static void STDMETHODCALLTYPE 
> d2d_device_context_FillGeometry(ID2D1DeviceContext
>      if (FAILED(context->error.code))
>          return;
>
> -    if (opacity_brush && brush_impl->type != D2D_BRUSH_TYPE_BITMAP)
> +    if (opacity_brush && !(brush_impl->type == D2D_BRUSH_TYPE_BITMAP 
> || brush_impl->type == D2D_BRUSH_TYPE_IMAGE))
>      {
>          d2d_device_context_set_error(context, 
> D2DERR_INCOMPATIBLE_BRUSH_TYPES);
>          return;
Same here, we have a test for this case already, that needs to be 
extended to verify this change.