Re: [PATCH 9/9] d2d1: Partially implement ID2D1Bitmap1

[Nikolay Sivov]

On 11/23/2017 02:08 AM, Lucian Poston wrote:
> On Tue, Nov 21, 2017 at 5:52 AM, Nikolay Sivov  wrote:
> > On 21.11.2017 11:41, Lucian Poston wrote:
> > > @@ -69,6 +93,10 @@ static ULONG STDMETHODCALLTYPE d2d_bitmap_Release(ID2D1Bitmap1 *iface)
> >
> > ...
> > > -
> > >        ID2D1ColorContext_Release(bitmap->color_context);
> > > -
> > >    if (bitmap->surface)
> > > -
> > >        IDXGISurface_Release(bitmap->surface);
> >
> > ...
> > > -
> > > - if (This->surface)
> > > - {
> > > -
> > >    IDXGISurface_AddRef(This->surface);
> > > - }
> > > -
> >
> > Is it really possible that bitmap has no surface?
>
> Yes. Even prior to this patch, it could be initialized with a d3d
> device and no surface. This patch series adds a code path to create a
> bitmap with a surface and no device, namely
> CreateBitmapFromDxgiSurface.

I still don't see this. We have d2d_bitmap_create() that always creates 
a texture, and you can query a surface from it.
And we have d2d_bitmap_create_shared(), that is meant to share surface 
passed to it, or a surface used to initialize existing d2d bitmap.
> > > +static D2D1_BITMAP_PROPERTIES1 bitmap_properties_to_properties1(
> > > -
> > >    const D2D1_BITMAP_PROPERTIES *desc)
> > >
> > > +{
> > > - D2D1_BITMAP_PROPERTIES1 d;
> > > - d.bitmapOptions = D2D1_BITMAP_OPTIONS_NONE;
> > > - d.colorContext = NULL;
> > > - if (desc == NULL)
> > > - {
> > > -
> > >    d.pixelFormat.format = DXGI_FORMAT_UNKNOWN;
> > > -
> > >    d.pixelFormat.alphaMode = D2D1_ALPHA_MODE_UNKNOWN;
> > > -
> > >    d.dpiX = 96.0f;
> > > -
> > >    d.dpiY = 96.0f;
> > > - }
> > > - else
> > > - {
> > > -
> > >    d.pixelFormat.format = desc->pixelFormat.format;
> > > -
> > >    d.pixelFormat.alphaMode = desc->pixelFormat.alphaMode;
> > > -
> > >    d.dpiX = desc->dpiX;
> > > -
> > >    d.dpiY = desc->dpiY;
> > > - }
> > > -
> > > - return d;
> > >
> > > +}
> > > -
> >
> > Maybe it's better to switch to PROPERTIES1 internally instead of
> > converting down like that.
>
> We have to convert from PROPERTIES to PROPERTIES1 at some point since
> some input parameters to the older public APIs are PROPERTIES. Are you
> saying the conversion should occur earlier, like immediately in the
> older public APIs?
> > > - if (FAILED(hr = ID3D10Texture2D_QueryInterface(texture, &IID_IDXGISurface, (void **)&surface)))
> > > - {
> > > -
> > >    surface = NULL;
> > > -
> > >    WARN("Texture2D had no underlying DXGISurface");
> > > - }
> > > -
> >
> > When does this fail? I mean it probably makes sense to have a warning,
> > but continue without a surface?
>
> The Texture2D implementation has code that is conditional on whether
> it has an internal surface object, including its QueryInterface
> implementation, so I wrote the bitmap code with that Texture2D
> assumption. If that's an invalid assumption, I can change this.

Texture2D supports it conditionally, yes. But textures are always 
created internally in d2d, in a way that surfaces are supported.
So if QI fails, it should be fatal failure, that's how existing code 
handles that already.