Re: [PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Henri Verbeet]

On 22 May 2015 at 10:11, Stefan Dösinger  wrote:
> @@ -1145,18 +1151,21 @@ static HRESULT texture_init(struct wined3d_texture *texture, const struct wined3
>      /* WINED3DUSAGE_DEPTHSTENCIL needs WINED3DFMT_FLAG_DEPTH or WINED3DFMT_FLAG_STENCIL, not both. */
>  if ((format->flags[WINED3D_GL_RES_TYPE_TEX_2D] & required_flags) == required_flags
> -
>        && (!(desc->usage & WINED3DUSAGE_DEPTHSTENCIL) || (format->flags[WINED3D_GL_RES_TYPE_TEX_2D] & ds_flags))
> -
>        && check_ds_support(format, WINED3D_GL_RES_TYPE_TEX_2D, desc->usage)
>        && (gl_info->supported[WINED3D_GL_NORMALIZED_TEXRECT]
>        || (desc->width == pow2_width && desc->height == pow2_height)))
>    gl_type = WINED3D_GL_RES_TYPE_TEX_2D;
> else if ((format->flags[WINED3D_GL_RES_TYPE_TEX_RECT] & required_flags) == required_flags
> -
>        && (!(desc->usage & WINED3DUSAGE_DEPTHSTENCIL) || (format->flags[WINED3D_GL_RES_TYPE_TEX_RECT] & ds_flags)))
> -
>        && check_ds_support(format, WINED3D_GL_RES_TYPE_TEX_RECT, desc->usage))
>    gl_type = WINED3D_GL_RES_TYPE_TEX_RECT;
> - else if ((format->flags[WINED3D_GL_RES_TYPE_RB] & required_flags) == required_flags
> -
>        && check_ds_support(format, WINED3D_GL_RES_TYPE_RB, desc->usage))
> -
>    gl_type = WINED3D_GL_RES_TYPE_RB;
> else
>        gl_type = WINED3D_GL_RES_TYPE_TEX_2D; /* No error, may be SCRATCH pool or emulated conditional np2. */

This is kind of ok because the existing code is like this, but notice
the similarity to some of the code in resource_init(). In fact, is
there any reason the caller needs to pass "gl_type" to resource_init()
instead of letting it figure it out on its own?
> -static void delete_fbo_attachment(const struct wined3d_gl_info *gl_info)
> +static void delete_fbo_attachment(const struct wined3d_gl_info *gl_info,
> -
>    enum wined3d_gl_resource_type d3d_type, GLuint *object)
>
> {
> - gl_info->fbo_ops.glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
> -
>        GL_TEXTURE_2D, 0, 0);
> - switch (d3d_type)
> - {
> -
>    case WINED3D_GL_RES_TYPE_TEX_1D:
> -
>    case WINED3D_GL_RES_TYPE_TEX_2D:
> -
>    case WINED3D_GL_RES_TYPE_TEX_RECT:
> -
>    case WINED3D_GL_RES_TYPE_TEX_3D:
> -
>    case WINED3D_GL_RES_TYPE_TEX_CUBE:
> -
>        gl_info->gl_ops.gl.p_glDeleteTextures(1, object);
> -
>        break;
> -
> -
>    case WINED3D_GL_RES_TYPE_RB:
> -
>        gl_info->fbo_ops.glDeleteRenderbuffers(1, object);
> -
>        break;
> -
> -
>    case WINED3D_GL_RES_TYPE_BUFFER:
> -
>    case WINED3D_GL_RES_TYPE_COUNT:
> -
>        break;
> - }
>
> }

Why is "object" a pointer?

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-05-25 um 10:27 schrieb Henri Verbeet:
> This is kind of ok because the existing code is like this, but notice
> the similarity to some of the code in resource_init(). In fact, is
> there any reason the caller needs to pass "gl_type" to resource_init()
> instead of letting it figure it out on its own?

The idea is to keep the switch between the different 2D types in the 2D
texture init parts of the code instead of the generic code.
> > -static void delete_fbo_attachment(const struct wined3d_gl_info *gl_info)
> > +static void delete_fbo_attachment(const struct wined3d_gl_info *gl_info,
>
> Why is "object" a pointer?

Symmetry with create_and_bind_fbo_attachment(). But yeah, it's not
really necessary, I'll change this.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVYuDJAAoJEN0/YqbEcdMwJO4QAJS9Kb6d/0Wq5ioRpOeK0DI/
pLD7J9/jzRkH1DYNQzdhyH9LtAj4ugIx5XjjzR1tIsVXz8spjSDDMGDVF0QCEa5a
wK0fckdV31QdFG81fdLgKsR+U/+ZehFgy4s/lBZugDq+foGpfsbQwLMb/nDzG9AS
SEoWVOQYKEfOY3bdpsJxTx/vjK9d6Dt/KVGAlJFEPT5Xm64k74BkBW8MDfmN//Oe
+HDdHnaxTi4seDbtcK2NLyHGU+YPaHt1YgK0HtLZW1uJ0IlvrV2O2XrSEmbGQUpu
7EkyNaYvZFJKzB+0EjGZrBIN2M38C0mLn8b9stE/FBAkt42e0YTGbXyo8/DENci6
xkn+ZMH4mtg2IaW9wxO2Xu92YOId3X5tyIYSE2zA+5/hXtCGCxOPdRzjVCO5TvV3
RHUg28pt8RPikHTnglV5NzsL8wuA/yaPgLPj0uGbLCz4t/1OApWnGWX7L5P9v5BT
SuW6H9iJHWmXarbobgpHqmmqKA2+sPhmUjHCVJW4OM/WmLUGgRSZ6a0sRrbcXY8P
8bbtWNkhNq2JhaAcVMNJ0PgAvQfEvxSrRQyxAJQgj9Sf87/+KXYy0eV8pwYs8i2A
IdBDW0wGW5y+40+08WHAa+k+IC43nMaGT77NhHm+OHLJecEkm671cDi5cQRLX20z
RqDmOWRybXghDocnS8WH
=70T8
-----END PGP SIGNATURE-----

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Henri Verbeet]

On 25 May 2015 at 10:43, Stefan Dösinger  wrote:
> Am 2015-05-25 um 10:27 schrieb Henri Verbeet:
> > This is kind of ok because the existing code is like this, but notice
> > the similarity to some of the code in resource_init(). In fact, is
> > there any reason the caller needs to pass "gl_type" to resource_init()
> > instead of letting it figure it out on its own?
>
> The idea is to keep the switch between the different 2D types in the 2D
> texture init parts of the code instead of the generic code.

I guess, but it doesn't really seem worth it if that means duplicating
much of the generic usage/format validation code.

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-05-25 um 10:50 schrieb Henri Verbeet:
> I guess, but it doesn't really seem worth it if that means
> duplicating much of the generic usage/format validation code.

I tried to move the code to resource_init and the non power of 2
handling is a big problem. resource_init doesn't care about power of 2
and non power of 2 sizes, and I don't think it should. It would need
to know that though to switch between 2D and RECT.

Adding a flag for "this resource type supports NP2 textures" isn't
really an option either. If we have neither TEXTURE_NP2 nor
TEXTURE_RECTANGLE (some GL ES systems according to Matteo) we want to
use 2D textures with padding.

The other relatively minor problem is handling subresources. The
solution here is to never set or read the resource type on
subresources, which we already do, and don't do capability validation
on subresources. Right now we read format flags from subresources though
.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVZFDRAAoJEN0/YqbEcdMwZWoP/jhFewDCNegghTsvWiiWmwvV
sHTn+tVduHCUqnZDYoytEYmQbAKC6nZCvAe+29j8OW/mP9/AraKc0edscrVha7ya
uH0lgWWSITSSNQwg7VZMnIqq7zmrrO8BwSDLfNZILTtFzAZf2q72qknkkyiN62ui
oVi1NjsHrBOBed7AyqVj+7JpilZ6DB1q7zrJ48p/k2sOEWJ8zcoqLcAlBXe51XQ8
sxXX4+zOmkNzg8n/x0bXAqZbgWHKRIzGbyI15iSY5cf849syNYKhJqVsL2hN0BZJ
53l+jdWVQYXNs+/jwIIOsStt7obvkcNeQj7hqZQGdPGjDxhhDStwxMRnFjZQmrcF
RRTWKVAyXRk9I+SybHIn9Q9sR/7qN1GRv962xi856vf/F6SuLw5NE7+tZ8t2/UoD
XlQkeDVNr/bcGeieFnjUlXZDwkv+EhEe1s7/tr+cEbrAxE+ze5Etj8/b+wTFgjP5
tc5M7SLZWQC8GrKGoOotR/CPPdHB0bXoZ4keYvmtUCKE9Vp/Qd7y3pLoZV9N9ZF7
r7NsjH2uOun4m1PaQ/EnsfxWvp8BbRKTsa34XYEWN6yaVVAjd9DvS4B/nZP73MDo
bK4TQvpQq21GMWprAnBAi+mwcdNrjbkEPJm1EXPeHpgacJ9aQJG7gjAcuKc6+8oI
k4ptRVihfkc98lnlQd4N
=NsZR
-----END PGP SIGNATURE-----

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Henri Verbeet]

On 26 May 2015 at 12:54, Stefan Dösinger  wrote:
> I tried to move the code to resource_init and the non power of 2
> handling is a big problem. resource_init doesn't care about power of 2
> and non power of 2 sizes, and I don't think it should. It would need
> to know that though to switch between 2D and RECT.

What is the issue exactly?

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-05-26 um 13:03 schrieb Henri Verbeet:
> On 26 May 2015 at 12:54, Stefan Dösinger
>  wrote:
> > I tried to move the code to resource_init and the non power of 2 
> > handling is a big problem. resource_init doesn't care about power
> > of 2 and non power of 2 sizes, and I don't think it should. It
> > would need to know that though to switch between 2D and RECT.
>
> What is the issue exactly?

I'd have to move or duplicate the power of two size calculation
(texture.c lines 1092 - 1123) to resource_init, and I don't think the
generic resource code should have to deal with this functionality that
is very specific to 2D textures. Passing a "this is a np2 texture"
flag to resource_init is a possible option, but it would still take
quite a bit of 2D texture specific code along with it (in particular
the switch between true NP2, faked WINED3D_GL_NORMALIZED_TEXRECT NP2,
ARB_texture_rectangle and 2D texture emulated NP2).

I'm not saying we can't do it that way. I just don't think it is
nicer. I'll see if I can extract the flag checks into a separate
function that texture_init and resource_init can call.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVZFoKAAoJEN0/YqbEcdMwk8cP/2yLi7Ol7FJU/YFsjA50IAGG
0WpmXBNw93UQLU8DK63+l4+0y+ExqpvO27ACkmMotwAVJT8WhNIK8wxWQ2uUiirD
jXCLq1dq70o4X7xm5odjY0x2k6zqDcbeptdQ0j9U0cy8GxedmII8wKEH7FZEOtsW
2v/6BCP7z300nfDGu3Gpv9i9M0KZ0OyS2D8CiPgxl1k4TU2diDEcTHwCPEAPTOpB
73a9aWJfn27nGAYJcGlpe14lNxBuHTzP8E+QSk2Jiv3lySPWi3WonArmjTRnexkW
vgoR5ZCn+7SYo99qMJI2TM2gvdDKwbfckriSnJLGNiZjgi2OWRwXN1TVCWn6W0gu
kZQ/cjrEfgkxb87vpQViM/+mwoKcdLL4PDJyHWIx5cBkjkyCbaGsGpzI5MTlvYa4
SNs83DRdGlAt5GQo7tAORRYBbUPXfVI1zCtwYwfVwOWmPQ10WppnKy4zuH+TOad4
H07sRm+xhYWBqw5TB11z3j0Yp/HSfnvYeks66ZdHe8iVL0YuzSONfy3gVX2DQqoK
xY8tERj9IkRwUcpyepV5nO57tk32VRiUv3FWqyx7g8xWJTd17gF2fXZPSpAhTbcX
778iXhtmPFm/6wedHcr7a2fECM3ZaaKSaX+Ch9jhKadvxSIx57TjmlntWv7jt8OE
BzPGbwd56AVgu0uJmi47
=lNvw
-----END PGP SIGNATURE-----

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Henri Verbeet]

On 26 May 2015 at 13:33, Stefan Dösinger  wrote:
> I'd have to move or duplicate the power of two size calculation
> (texture.c lines 1092 - 1123) to resource_init, and I don't think the
> generic resource code should have to deal with this functionality that
> is very specific to 2D textures. Passing a "this is a np2 texture"
> flag to resource_init is a possible option, but it would still take
> quite a bit of 2D texture specific code along with it (in particular
> the switch between true NP2, faked WINED3D_GL_NORMALIZED_TEXRECT NP2,
> ARB_texture_rectangle and 2D texture emulated NP2).

You don't really need pow2_width/pow2_height itself though, you just
need to determine if the resource dimensions are powers of two, which
is much easier. (I.e., "!(desc->width & desc->width - 1)", etc.)

[PATCH 3/8] wined3d: Keep track of renderbuffer capabilities.

[Matteo Bruni]

2015-05-26 12:54 GMT+02:00 Stefan Dösinger :
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 2015-05-25 um 10:50 schrieb Henri Verbeet:
> > I guess, but it doesn't really seem worth it if that means
> > duplicating much of the generic usage/format validation code.
>
> I tried to move the code to resource_init and the non power of 2
> handling is a big problem. resource_init doesn't care about power of 2
> and non power of 2 sizes, and I don't think it should. It would need
> to know that though to switch between 2D and RECT.
>
> Adding a flag for "this resource type supports NP2 textures" isn't
> really an option either. If we have neither TEXTURE_NP2 nor
> TEXTURE_RECTANGLE (some GL ES systems according to Matteo) we want to
> use 2D textures with padding.

Just to clarify on this point, even though it's quite tangential to
the discussion: basic GL ES 2 has pretty much the same limitations as
a D3D device with D3DPTEXTURECAPS_NONPOW2CONDITIONAL: NP2 textures are
supported as long as you use CLAMP_TO_EDGE texcoord wrapping and don't
make use of mipmapping in minification filtering.
In theory D3D applications should check the cap bit and avoid that
kind of usage, except often times they don't. The texture handling
stuff related with WINED3D_GL_NORMALIZED_TEXRECT and
WINED3D_TEXTURE_COND_NP2 mostly does the right thing here and yeah, it
would be nice if that stays around. BTW, rectangle textures are not
supported in OpenGL ES.