Re: [PATCH] d3d11.idl: Add ID3D11VideoContext interface (try 2)

[Józef Kucia]

On Wed, Jan 13, 2016 at 8:08 AM, Andrey Turkin  wrote:
> UINT Usage:1;

It would preferably be written as "UINT Usage : 1;". This applies to
other bit fields.
> - void VideoProcessorSetOutputConstriction(
> -
>    [in] ID3D11VideoProcessor *pVideoProcessor,
> -
>    BOOL Enable,
> -
>    SIZE Size);
> - void VideoProcessorSetOutputStereoMode(
> -
>    [in] ID3D11VideoProcessor *pVideoProcessor,
> -
>    BOOL Enable);

The Enable and Size parameters should be annotated as input parameters.

Ideally, the method parameters would be renamed and written in
lower_case_with_underscores.

[PATCH] d3d11.idl: Add ID3D11VideoContext interface (try 2)

[Andrey Turkin]

2016-01-15 3:35 GMT+03:00 Józef Kucia :
> It would preferably be written as "UINT Usage : 1;". This applies to
> other bit fields.

Make sense, will do

The Enable and Size parameters should be annotated as input parameters.
>

Will do
> Ideally, the method parameters would be renamed and written in
> lower_case_with_underscores.

Existing interfaces in that file don't use this naming scheme.

[PATCH] d3d11.idl: Add ID3D11VideoContext interface (try 2)

[Henri Verbeet]

On 15 January 2016 at 09:30, Andrey Turkin  wrote:
> 2016-01-15 3:35 GMT+03:00 Józef Kucia :
> > It would preferably be written as "UINT Usage : 1;". This applies to
> > other bit fields.
>
> Make sense, will do

The main thing I'd personally be worried about would be the
portability of bit fields. I think that in case of
D3D11_VIDEO_PROCESSOR_COLOR_SPACE the layout ends up being the same
between MSVC and gcc on x86, but I'm not sure that's something we can
depend on, or if that also holds on different compilers or e.g.
64-bit.

[PATCH] d3d11.idl: Add ID3D11VideoContext interface (try 2)

[Andrey Turkin]

I thought about that too. In the end, I don't think I can really do
anything to make them more portable while also keeping compatibility with
DX SDK. We are a compilers' mercy here; at least (recent) GCC targeting
Windows will use MSVC-compatible bitfield layout.
Anyway, there's already some bitfield use in WinSDK headers in Wine - same
concern applies there.

2016-01-15 12:44 GMT+03:00 Henri Verbeet :
> On 15 January 2016 at 09:30, Andrey Turkin 
> wrote:
> > 2016-01-15 3:35 GMT+03:00 Józef Kucia :
> > > It would preferably be written as "UINT Usage : 1;". This applies to
> > > other bit fields.
> >
> > Make sense, will do
>
> The main thing I'd personally be worried about would be the
> portability of bit fields. I think that in case of
> D3D11_VIDEO_PROCESSOR_COLOR_SPACE the layout ends up being the same
> between MSVC and gcc on x86, but I'm not sure that's something we can
> depend on, or if that also holds on different compilers or e.g.
> 64-bit.

[PATCH] d3d11.idl: Add ID3D11VideoContext interface (try 2)

[Jacek Caban]

On 01/15/16 10:44, Henri Verbeet wrote:
> On 15 January 2016 at 09:30, Andrey Turkin  wrote:
> > 2016-01-15 3:35 GMT+03:00 Józef Kucia :
> > > It would preferably be written as "UINT Usage : 1;". This applies to
> > > other bit fields.
> >
> > Make sense, will do
>
> The main thing I'd personally be worried about would be the
> portability of bit fields. I think that in case of
> D3D11_VIDEO_PROCESSOR_COLOR_SPACE the layout ends up being the same
> between MSVC and gcc on x86, but I'm not sure that's something we can
> depend on, or if that also holds on different compilers or e.g.
> 64-bit.

I don't think this should block the patch. We already use bit fields in
Wine. Even if we ever find a problem with it (which is currently not the
case AFAIK) and we'd need something special for Wine, we should still
have a variant of declaration that is source compatible with PSDK
(except probably ifdefed).

Jacek

[PATCH] d3d11.idl: Add ID3D11VideoContext interface (try 2)

[Henri Verbeet]

On 15 January 2016 at 14:19, Jacek Caban  wrote:
> I don't think this should block the patch. We already use bit fields in
> Wine. Even if we ever find a problem with it (which is currently not the
> case AFAIK) and we'd need something special for Wine, we should still
> have a variant of declaration that is source compatible with PSDK
> (except probably ifdefed).

Probably.

At first sight I see some minor style issues like the brace placement
after "union" in D3D11_VIDEO_COLOR, D3D11_OMAC_SIZE should probably be
a constant, and I'm really not a fan of the parameter naming
convention in ID3D11VideoContext either, but ultimately nothing that
would necessarily block the patch. I didn't review the patch closely
though, and it's not assigned to me.