KhronosGroup / OpenGL-API

OpenGL and OpenGL ES API Issue Tracker
34 stars 5 forks source link

OpenGL 4.6: Multi sample texture always incomplete with integer internal format. #45

Closed illiaSkif closed 4 years ago

illiaSkif commented 5 years ago

I investigate this issue(https://bugs.freedesktop.org/show_bug.cgi?id=109057) in mesa3d. When i try to figure out how to fix it i faced with self-contradictory points in OpenGL 4.6 specification. I can create texture by

void glTextureStorage2DMultisample( GLuint texture,
    GLsizei samples,
    GLenum internalformat,
    GLsizei width,
    GLsizei height,
    GLboolean fixedsamplelocations);

and internalformat can be integer(GL_R8I). After creating I can't render from it because it always incomplete.

OpenGL 4.6 spec:

Using the preceding definitions, a texture is complete unless any of the following conditions hold true: Any of – The internal format of the texture is integer and either the magnification filter is not NEAREST, or the minification filter is neither NEAREST nor NEAREST_MIPMAP_NEAREST.

And we can’t make it complete because we can’t change texture parameters:

OpenGL 4.6 spec:

An INVALID_ENUM error is generated if the effective target is either TEXTURE_2D_MULTISAMPLE or TEXTURE_2D_MULTISAMPLE_ARRAY, and pname is any sampler state from table

Default parameters are:

OpenGL 4.6 spec:

In the initial state, the value assigned to TEXTURE_MIN_FILTER is NEAREST_MIPMAP_LINEAR (except for rectangle textures, where the initial value is LINEAR ), and the value for TEXTURE_MAG_FILTER is LINEAR

Also ARB_texture_multisample spec:

Multisample textures support only NEAREST filtering.

Can you clarify what specification is more important and clarify what to do with integer Multi sample texture?

ianromanick commented 5 years ago

After some old Khronos internal bugzilla archaeology (circa 2014), this should be covered by section 11.1.3.3 (Multisample Texel Fetches). Basically, multisample textures ignore the sampler state, so the texture completeness problem is irrelevant.

That applies to rendering from it. I just noticed that the OP was talking about rendering to the texture, and I haven't through through that yet.

pdaniell-nv commented 5 years ago

Thanks for digging that up Ian. The filter state doesn't apply when rendering to the texture so it shouldn't affect whether it's complete or not for rendering. In section 9.4.1 Framebuffer Attachment Completeness there is no mention of the sample state so perhaps we're fine.

illiaSkif commented 5 years ago

I'm sorry I was wrong. The problem shows up when we read from int MS texture. Thanks so much for the investigation. The sequence of actions on which the problem appears is:

After that, I get an array filled with zeros, and not with the values that I wrote to the texture.

pdaniell-nv commented 5 years ago

@illiaSkif It's not clear to me if there is still a spec issue. Perhaps you're just hitting an implementation bug?

illiaSkif commented 5 years ago

I think no, because the specification contains contradictions. We can create multisample texture with integer internal format. (specification allows it) We have possibility to draw from this texture (it is prohibited nowhere), but I can’t do it because this texture always incomplete this is due to this texture always does not meet this condition of completeness:

Using the preceding definitions, a texture is complete unless any of the following conditions hold true: Any of – The internal format of the texture is integer and either the magnification filter is not NEAREST, or the minification filter is neither NEAREST nor NEAREST_MIPMAP_NEAREST

It happens because by default TEXTURE_MIN_FILTERis NEAREST_MIPMAP_LINEAR and TEXTURE_MAG_FILTER is LINEAR (I can’t find any exceptions or clarifications for multisample texture) and we can’t change it in user code because it is prohibited by specification.

I think we have two possibility to resolve this problem.

  1. Add MS int texture to the exception list and don’t require from it that condition of completeness.
  2. Change the initial state of TEXTURE_MIN_FILTER and TEXTURE_MAG_FILTER to NEAREST as it said in ARB_texture_multisample spec, but this part did not migrate to specification.
pdaniell-nv commented 5 years ago

Putting an exception for the initial values of TEXTURE_MIN_FILTER and TEXTURE_MAG_FILTER to be NEAREST for multisample textures makes sense to me. It would be a fairly easy spec change to section "8.22 Texture State and Proxy State" to include it in the exceptions, like there already is for rectangle textures.

sergiiromantsov commented 5 years ago

Hello, Piers Daniell. Nice to hear it. Could we treat and implement it as a final decision? And when we would expect a specification update?

вт, 26 лют. 2019 о 23:30 Piers Daniell notifications@github.com пише:

Putting an exception for the initial values of TEXTURE_MIN_FILTER and TEXTURE_MAG_FILTER to be NEAREST for multisample textures makes sense to me. It would be a fairly easy spec change to section "8.22 Texture State and Proxy State" to include it in the exceptions, like there already is for rectangle textures.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-API/issues/45#issuecomment-467621763, or mute the thread https://github.com/notifications/unsubscribe-auth/AIOpQ9KdN9AM69oQONwGjXvwJNiAKWTsks5vRaeEgaJpZM4aezgo .

pdaniell-nv commented 5 years ago

Unfortunately we didn't get to this issue during our OpenGL meeting today. I don't expect there to be an objection from the working group for this and you're safe to update your implementation and make progress. An updated spec will probably happen mid 2019.

ianromanick commented 5 years ago

Here is all of the spec and bug archaeology that I have performed this morning...

Originally the GL_ARB_texture_multisample spec and section 2.11.12 (Shader Execution) of the OpenGL 4.2 spec said:

Multisample textures support only NEAREST filtering.

I guess the expectation is that after calling glTexImage2DMultisample an application would have to change the filter mode to GL_NEAREST. That's a bit broken because section 8.10 (Texture Parameters) of the OpenGL 4.3 spec (the next version) adds:

An INVALID_ENUM error is generated if target is either TEXTURE_2D_MULTISAMPLE or TEXTURE_2D_MULTISAMPLE_ARRAY , and pname is any sampler state from table 23.18.

In OpenGL 4.5, this language changed a bit. Section 11.1.3.3 (Multisample Texel Fetches) of the OpenGL 4.6 spec says:

Multisample textures are not filtered when samples are fetched, and filter state is ignored.

According to the spec release notes, this change was in response to internal bug 12255. That bug references an internal OpenGL ES bug 12171 for literally this exact same issue. :scream: Looking back at 12171, I believe the intention was that "filter state is ignored" was intended to apply to completeness checks as well. Setting the default GL_TEXTURE_MIN_FILTER state to GL_NEAREST was specifically suggested in that bug by @oddhack and rejected by @dgkoch (in comment 3).

If any change is necessary in the spec, I would suggest changing this bullet in section 8.17 (Texture Completeness):

to read

I would suggest making this clarifying change to both OpenGL 4.6 and OpenGL ES 3.2.

gnl21 commented 5 years ago

I agree with the principle of what @ianromanick is saying, although I find the idea of ignoring the filter state to be a bit confusing (is an ignored filter state functionally the same as NEAREST, or NEAREST_MIPMAP_LINEAR, etc?). Perhaps we should just add the phrase "Multisample textures are never considered to require mipmaps"?

pdaniell-nv commented 5 years ago

Assigned to @oddhack to update the OpenGL and OpenGL ES specs as suggested above. Namely:

Changing this bullet in section 8.17 (Texture Completeness):

to read:

illiaSkif commented 5 years ago

The proposed update of the specification doesn't fix the issue I described. In my case the cause of texture incompleteness is a combination of:

The internal format of the texture is integer and either the magnification filter is not NEAREST, or the minification filter is neither NEAREST nor NEAREST_MIPMAP_NEAREST.

If I correctly understand your reasoning this part of the spec should be changed to:

The internal format of the texture is integer and either the magnification filter is not NEAREST, or the minification filter is neither NEAREST nor NEAREST_MIPMAP_NEAREST. For multisample textures, this condition is ignored.

oddhack commented 4 years ago

@pdaniell-nv I've made this change for the queued-up spec updates, but in light of @illiaSkif's most recent comment, it may not close this issue - can the WG revisit?

pdaniell-nv commented 4 years ago

I think the queued-up spec update looks okay. @gnl21 can you check the matches your suggestion? Maybe my proposal puts it in the wrong place.

My intent was to state that multisample textures don't have mipmaps so the filters would always just be NEAREST.

gnl21 commented 4 years ago

I've had a look at the update and I think it does match my suggestion. If I were to make the suggestion again then I'd probably prefer to say:

The minification filter requires a mipmap (is neither NEAREST nor LINEAR), the texture is not a multisample texture and it is not mipmap complete.

because I think it reads a bit more easily, but what we have is also fine.

I agree with @illiaSkif that this only partially addresses the issue, and we should add a similar clause to the following bullet point:

  • The internal format of the texture is integer [...] and the texture is not multisample, and either the magnification filter is not ...
pdaniell-nv commented 4 years ago

Discussed in the OpenGL/ES meeting today. We agreed @gnl21's proposed change is an improvement. @oddhack could you update the pending spec updates with this suggestion?

oddhack commented 4 years ago

I'm unclear on what @gnl21 proposes here. Is the "and the texture is not multisample" clause supposed to be added following the "Any of" bullet list? Is that the only change? N.b. if we're going to adopt "and the texture is not multisample" clause here, I'd also do it in the minification filter bullet, above, as @gnl21 suggests. Otherwise the two adjacent clauses are annoyingly inconsistent phrasing-wise.

gnl21 commented 4 years ago

Yeah, sorry, I wasn't very clear. I was suggesting that we add the clause following the "Any of" sub-bulleted list. I think that what we want for that bullet overall is: (Any of sub-bullets 1,2,3) && not multisample && (either mag or min filter is incorrect). Does that make sense?

oddhack commented 4 years ago

Yes, thanks. Will do, and we can publish a spec update shortly thereafter.

oddhack commented 4 years ago

Fixed in the 2019-10-22 spec updates.