<div dir="ltr">On 20 November 2013 11:13, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 11/19/2013 11:02 PM, Paul Berry wrote:<br>
> Previously we were using the code path for validating<br>
> glFramebufferTextureLayer(). But glFramebufferTexture() allows<br>
> additional texture types.<br>
><br>
> Fixes piglit tests:<br>
> - spec/!OpenGL 3.2/layered-rendering/gl-layer-cube-map<br>
> - spec/!OpenGL 3.2/layered-rendering/framebuffertexture<br>
><br>
> Cc: "10.0" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>><br>
> ---<br>
> src/mesa/main/fbobject.c | 50 ++++++++++++++++++++++++++++++++++++++----------<br>
> 1 file changed, 40 insertions(+), 10 deletions(-)<br>
><br>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c<br>
> index 9dd7161..2feb4c3 100644<br>
> --- a/src/mesa/main/fbobject.c<br>
> +++ b/src/mesa/main/fbobject.c<br>
> @@ -2334,16 +2334,46 @@ framebuffer_texture(struct gl_context *ctx, const char *caller, GLenum target,<br>
<br>
</div>As an aside, it's confusing, but correct, that layered is set to true by<br>
a caller without Layer in the name.<br></blockquote><div><br></div><div>Yeah, that's unfortunate. I'm open to suggestions about how to make the code less confusing.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im"><br>
> texObj = _mesa_lookup_texture(ctx, texture);<br>
> if (texObj != NULL) {<br>
> if (textarget == 0) {<br>
> - /* If textarget == 0 it means we're being called by<br>
> - * glFramebufferTextureLayer() and textarget is not used.<br>
> - * The only legal texture types for that function are 3D and<br>
> - * 1D/2D arrays textures.<br>
> - */<br>
<br>
</div>So this comment was just wrong? What does textarget == 0 mean?<br></blockquote><div><br></div><div>The comment was correct before support for layered framebuffers was added. Now, it's only correct if layered is false. That's why I moved it (with a slight amount of rewording) to the else block below.<br>
<br>As to what textarget == 0 means: the way this function works is that framebuffer_texture() is called by all the various glFramebufferTexture*() functions. textarget == 0 means that it was called from a function that doesn't take a textarget parameter (glFramebufferTextureLayer() or glFramebufferTexture()).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="h5"><br>
> - err = (texObj->Target != GL_TEXTURE_3D) &&<br>
> - (texObj->Target != GL_TEXTURE_1D_ARRAY_EXT) &&<br>
> - (texObj->Target != GL_TEXTURE_2D_ARRAY_EXT) &&<br>
> - (texObj->Target != GL_TEXTURE_CUBE_MAP_ARRAY) &&<br>
> - (texObj->Target != GL_TEXTURE_2D_MULTISAMPLE_ARRAY);<br>
> + if (layered) {<br>
> + /* We're being called by glFramebufferTexture() and textarget<br>
> + * is not used.<br>
> + */<br>
> + switch (texObj->Target) {<br>
> + case GL_TEXTURE_3D:<br>
> + case GL_TEXTURE_1D_ARRAY_EXT:<br>
> + case GL_TEXTURE_2D_ARRAY_EXT:<br>
> + case GL_TEXTURE_CUBE_MAP:<br>
> + case GL_TEXTURE_CUBE_MAP_ARRAY:<br>
> + case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:<br>
> + err = false;<br>
> + break;<br>
> + case GL_TEXTURE_1D:<br>
> + case GL_TEXTURE_2D:<br>
> + case GL_TEXTURE_RECTANGLE:<br>
> + case GL_TEXTURE_2D_MULTISAMPLE:<br>
> + /* These texture types are valid to pass to<br>
> + * glFramebufferTexture(), but since they aren't layered, it<br>
> + * is equivalent to calling glFramebufferTexture{1D,2D}().<br>
> + */<br>
> + err = false;<br>
<br>
</div></div>If this is changed to<br>
<br>
err = !layered;<br>
<br>
then...<br>
<div class="im"><br>
> + layered = false;<br>
> + textarget = texObj->Target;<br>
> + break;<br>
> + default:<br>
> + err = true;<br>
> + break;<br>
> + }<br>
> + } else {<br>
> + /* We're being called by glFramebufferTextureLayer() and<br>
> + * textarget is not used. The only legal texture types for<br>
> + * that function are 3D and 1D/2D arrays textures.<br>
> + */<br>
> + err = (texObj->Target != GL_TEXTURE_3D) &&<br>
> + (texObj->Target != GL_TEXTURE_1D_ARRAY_EXT) &&<br>
> + (texObj->Target != GL_TEXTURE_2D_ARRAY_EXT) &&<br>
> + (texObj->Target != GL_TEXTURE_CUBE_MAP_ARRAY) &&<br>
> + (texObj->Target != GL_TEXTURE_2D_MULTISAMPLE_ARRAY);<br>
<br>
</div>I think this could be handled by the switch-statement above.<br>
<br>
If that happens, I'd also recommend dropping _EXT from some of the case<br>
labels.<br></blockquote><div><br></div><div>Yeah, but it gets a little ugly beccause GL_TEXTURE_CUBE_MAP is accepted by glFramebufferTexture() but not glFramebufferTextureLayer(). I'm also not thrilled about the fact that it mixes together the error checking for two functions in the same switch statement, so it's a little harder to follow what's going on.<br>
<br>Here's what the switch statement looks like if I coalesce it together, like you suggest:<br><br> /* If textarget == 0 it means we're either being called by<br> * glFramebufferTextureLayer() (layered == false) or<br>
* glFramebufferTexture() (layered == true) and textarget is not<br> * used.<br> */<br> switch (texObj->Target) {<br> case GL_TEXTURE_3D:<br> case GL_TEXTURE_1D_ARRAY:<br>
case GL_TEXTURE_2D_ARRAY:<br> case GL_TEXTURE_CUBE_MAP_ARRAY:<br> case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:<br> /* These texture types are accepted by both<br> * glFramebufferTexture() and glFramebufferTextureLayer().<br>
*/<br> err = false;<br> break;<br> case GL_TEXTURE_CUBE_MAP:<br> /* GL_TEXTURE_CUBE_MAP is accepted by glFramebufferTexture()<br> * but not glFramebufferTextureLayer().<br>
*/<br> err = !layered;<br> break;<br> case GL_TEXTURE_1D:<br> case GL_TEXTURE_2D:<br> case GL_TEXTURE_RECTANGLE:<br> case GL_TEXTURE_2D_MULTISAMPLE:<br>
/* These texture types are accepted by glFramebufferTexture(),<br> * but since they aren't layered, it is equivalent to calling<br> * glFramebufferTexture{1D,2D}(). They aren't accepted by<br>
* glFramebufferTextureLayer().<br> */<br> err = !layered;<br> layered = false;<br> textarget = texObj->Target;<br> break;<br> default:<br>
err = true;<br> break;<br> }<br><br></div><div>If you think it's better this way, I guess I could be talked into changing it, but IMHO it's not a clear improvement.<br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><div class="h5"><br>
> + }<br>
> }<br>
> else {<br>
> /* Make sure textarget is consistent with the texture's type */<br>
><br>
<br>
</div></div></blockquote></div><br></div></div>