Re: [Mesa-dev] [PATCH 3/7] mesa: implement glBindTextures()
On 01/07/2014 12:05 AM, Fredrik Höglund wrote: > On Friday 03 January 2014, Marek Olšák wrote: >> On Fri, Jan 3, 2014 at 2:04 PM, Marek Olšák wrote: >>> On Fri, Jan 3, 2014 at 1:27 AM, Maxence Le Doré >>> wrote: --- src/mesa/main/texobj.c | 52 ++ src/mesa/main/texobj.h | 3 +++ 2 files changed, 55 insertions(+) diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c index bddbc50..66e2fb0 100644 --- a/src/mesa/main/texobj.c +++ b/src/mesa/main/texobj.c @@ -1686,4 +1686,56 @@ _mesa_InvalidateTexImage(GLuint texture, GLint level) return; } +/** ARB_multi_bind / OpenGL 4.4 */ + +void GLAPIENTRY +_mesa_BindTextures(GLuint first, GLsizei count, const GLuint *textures) +{ + GET_CURRENT_CONTEXT(ctx); + struct GLuint currentTexUnit = 0; + int i = 0; + + currentTexUnit = ctx->Texture.CurrentUnit; + + if(first + count > ctx->Const.MaxCombinedTextureImageUnits) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glBindTextures(first+count)"); + return; + } + + for(i = 0 ; i < count ; i++) { + GLuint texture; + struct gl_texture_object *texObj; + GLenum texTarget; + int j = 0; + + if(textures == NULL) +texture = 0; + else +texture = textures[i]; + + _mesa_ActiveTexture(GL_TEXTURE0 + first + i); + if(texture != 0) { +texObj = _mesa_lookup_texture(ctx, texture); +if(texObj) { + texTarget = texObj->Target; + _mesa_BindTexture(texTarget, texture); +} +else + _mesa_error(ctx, GL_INVALID_OPERATION, + "glBindTextures(textures[%i])", i); >>> >>> This error is set too late. It should be done before changing textures. >> >> Note that you make the same mistake in the other patches too. Also >> please double-check that none of the _mesa_ functions generate errors. > > This is actually not the case with the ARB_multi_bind functions: > > (11) Typically, OpenGL specifies that if an error is generated by a > command, that command has no effect. This is somewhat unfortunate > for multi-bind commands, because it would require a first pass to > scan the entire list of bound objects for errors and then a second > pass to actually perform the bindings. Should we have different > error semantics? > > RESOLVED: Yes. In this specification, when the parameters for one of > the binding points are invalid, that binding point is not > updated and an error will be generated. However, other binding points > in the same command will be updated if their parameters are valid and no > other error occurs. The code should reference this spec text. Otherwise someone will come along later and try to "fix" it. > The code is still wrong for a different reason though; when a texture has > has never been bound, it doesn't have a target. That case needs to be > handled correctly. > > Fredrik > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] mesa: implement glBindTextures()
Thanks Fredrik, I was nearly sure this was ok from what I read of the specs. And you're also right again : I totally forget a case where a texture has never been bound ! 2014/1/7 Maxence Le Doré : > Thanks Fredrik, > I was nearly sure this was ok from what I read of the specs. And > you're also right again : I totally forget a case where a texture has > never been bound ! > > 2014/1/7 Fredrik Höglund : >> On Friday 03 January 2014, Marek Olšák wrote: >>> On Fri, Jan 3, 2014 at 2:04 PM, Marek Olšák wrote: >>> > On Fri, Jan 3, 2014 at 1:27 AM, Maxence Le Doré >>> > wrote: >>> >> --- >>> >> src/mesa/main/texobj.c | 52 >>> >> ++ >>> >> src/mesa/main/texobj.h | 3 +++ >>> >> 2 files changed, 55 insertions(+) >>> >> >>> >> diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c >>> >> index bddbc50..66e2fb0 100644 >>> >> --- a/src/mesa/main/texobj.c >>> >> +++ b/src/mesa/main/texobj.c >>> >> @@ -1686,4 +1686,56 @@ _mesa_InvalidateTexImage(GLuint texture, GLint >>> >> level) >>> >> return; >>> >> } >>> >> >>> >> +/** ARB_multi_bind / OpenGL 4.4 */ >>> >> + >>> >> +void GLAPIENTRY >>> >> +_mesa_BindTextures(GLuint first, GLsizei count, const GLuint *textures) >>> >> +{ >>> >> + GET_CURRENT_CONTEXT(ctx); >>> >> + struct GLuint currentTexUnit = 0; >>> >> + int i = 0; >>> >> + >>> >> + currentTexUnit = ctx->Texture.CurrentUnit; >>> >> + >>> >> + if(first + count > ctx->Const.MaxCombinedTextureImageUnits) { >>> >> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> >> "glBindTextures(first+count)"); >>> >> + return; >>> >> + } >>> >> + >>> >> + for(i = 0 ; i < count ; i++) { >>> >> + GLuint texture; >>> >> + struct gl_texture_object *texObj; >>> >> + GLenum texTarget; >>> >> + int j = 0; >>> >> + >>> >> + if(textures == NULL) >>> >> +texture = 0; >>> >> + else >>> >> +texture = textures[i]; >>> >> + >>> >> + _mesa_ActiveTexture(GL_TEXTURE0 + first + i); >>> >> + if(texture != 0) { >>> >> +texObj = _mesa_lookup_texture(ctx, texture); >>> >> +if(texObj) { >>> >> + texTarget = texObj->Target; >>> >> + _mesa_BindTexture(texTarget, texture); >>> >> +} >>> >> +else >>> >> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> >> + "glBindTextures(textures[%i])", i); >>> > >>> > This error is set too late. It should be done before changing textures. >>> >>> Note that you make the same mistake in the other patches too. Also >>> please double-check that none of the _mesa_ functions generate errors. >> >> This is actually not the case with the ARB_multi_bind functions: >> >> (11) Typically, OpenGL specifies that if an error is generated by a >> command, that command has no effect. This is somewhat unfortunate >> for multi-bind commands, because it would require a first pass to >> scan the entire list of bound objects for errors and then a second >> pass to actually perform the bindings. Should we have different >> error semantics? >> >> RESOLVED: Yes. In this specification, when the parameters for one of >> the binding points are invalid, that binding point is not >> updated and an error will be generated. However, other binding points >> in the same command will be updated if their parameters are valid and >> no >> other error occurs. >> >> The code is still wrong for a different reason though; when a texture has >> has never been bound, it doesn't have a target. That case needs to be >> handled correctly. >> >> Fredrik >> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] mesa: implement glBindTextures()
On Friday 03 January 2014, Marek Olšák wrote: > On Fri, Jan 3, 2014 at 2:04 PM, Marek Olšák wrote: > > On Fri, Jan 3, 2014 at 1:27 AM, Maxence Le Doré > > wrote: > >> --- > >> src/mesa/main/texobj.c | 52 > >> ++ > >> src/mesa/main/texobj.h | 3 +++ > >> 2 files changed, 55 insertions(+) > >> > >> diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c > >> index bddbc50..66e2fb0 100644 > >> --- a/src/mesa/main/texobj.c > >> +++ b/src/mesa/main/texobj.c > >> @@ -1686,4 +1686,56 @@ _mesa_InvalidateTexImage(GLuint texture, GLint > >> level) > >> return; > >> } > >> > >> +/** ARB_multi_bind / OpenGL 4.4 */ > >> + > >> +void GLAPIENTRY > >> +_mesa_BindTextures(GLuint first, GLsizei count, const GLuint *textures) > >> +{ > >> + GET_CURRENT_CONTEXT(ctx); > >> + struct GLuint currentTexUnit = 0; > >> + int i = 0; > >> + > >> + currentTexUnit = ctx->Texture.CurrentUnit; > >> + > >> + if(first + count > ctx->Const.MaxCombinedTextureImageUnits) { > >> + _mesa_error(ctx, GL_INVALID_OPERATION, > >> "glBindTextures(first+count)"); > >> + return; > >> + } > >> + > >> + for(i = 0 ; i < count ; i++) { > >> + GLuint texture; > >> + struct gl_texture_object *texObj; > >> + GLenum texTarget; > >> + int j = 0; > >> + > >> + if(textures == NULL) > >> +texture = 0; > >> + else > >> +texture = textures[i]; > >> + > >> + _mesa_ActiveTexture(GL_TEXTURE0 + first + i); > >> + if(texture != 0) { > >> +texObj = _mesa_lookup_texture(ctx, texture); > >> +if(texObj) { > >> + texTarget = texObj->Target; > >> + _mesa_BindTexture(texTarget, texture); > >> +} > >> +else > >> + _mesa_error(ctx, GL_INVALID_OPERATION, > >> + "glBindTextures(textures[%i])", i); > > > > This error is set too late. It should be done before changing textures. > > Note that you make the same mistake in the other patches too. Also > please double-check that none of the _mesa_ functions generate errors. This is actually not the case with the ARB_multi_bind functions: (11) Typically, OpenGL specifies that if an error is generated by a command, that command has no effect. This is somewhat unfortunate for multi-bind commands, because it would require a first pass to scan the entire list of bound objects for errors and then a second pass to actually perform the bindings. Should we have different error semantics? RESOLVED: Yes. In this specification, when the parameters for one of the binding points are invalid, that binding point is not updated and an error will be generated. However, other binding points in the same command will be updated if their parameters are valid and no other error occurs. The code is still wrong for a different reason though; when a texture has has never been bound, it doesn't have a target. That case needs to be handled correctly. Fredrik ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] mesa: implement glBindTextures()
On Fri, Jan 3, 2014 at 2:04 PM, Marek Olšák wrote: > On Fri, Jan 3, 2014 at 1:27 AM, Maxence Le Doré > wrote: >> --- >> src/mesa/main/texobj.c | 52 >> ++ >> src/mesa/main/texobj.h | 3 +++ >> 2 files changed, 55 insertions(+) >> >> diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c >> index bddbc50..66e2fb0 100644 >> --- a/src/mesa/main/texobj.c >> +++ b/src/mesa/main/texobj.c >> @@ -1686,4 +1686,56 @@ _mesa_InvalidateTexImage(GLuint texture, GLint level) >> return; >> } >> >> +/** ARB_multi_bind / OpenGL 4.4 */ >> + >> +void GLAPIENTRY >> +_mesa_BindTextures(GLuint first, GLsizei count, const GLuint *textures) >> +{ >> + GET_CURRENT_CONTEXT(ctx); >> + struct GLuint currentTexUnit = 0; >> + int i = 0; >> + >> + currentTexUnit = ctx->Texture.CurrentUnit; >> + >> + if(first + count > ctx->Const.MaxCombinedTextureImageUnits) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, "glBindTextures(first+count)"); >> + return; >> + } >> + >> + for(i = 0 ; i < count ; i++) { >> + GLuint texture; >> + struct gl_texture_object *texObj; >> + GLenum texTarget; >> + int j = 0; >> + >> + if(textures == NULL) >> +texture = 0; >> + else >> +texture = textures[i]; >> + >> + _mesa_ActiveTexture(GL_TEXTURE0 + first + i); >> + if(texture != 0) { >> +texObj = _mesa_lookup_texture(ctx, texture); >> +if(texObj) { >> + texTarget = texObj->Target; >> + _mesa_BindTexture(texTarget, texture); >> +} >> +else >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> + "glBindTextures(textures[%i])", i); > > This error is set too late. It should be done before changing textures. Note that you make the same mistake in the other patches too. Also please double-check that none of the _mesa_ functions generate errors. Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] mesa: implement glBindTextures()
On Fri, Jan 3, 2014 at 1:27 AM, Maxence Le Doré wrote: > --- > src/mesa/main/texobj.c | 52 > ++ > src/mesa/main/texobj.h | 3 +++ > 2 files changed, 55 insertions(+) > > diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c > index bddbc50..66e2fb0 100644 > --- a/src/mesa/main/texobj.c > +++ b/src/mesa/main/texobj.c > @@ -1686,4 +1686,56 @@ _mesa_InvalidateTexImage(GLuint texture, GLint level) > return; > } > > +/** ARB_multi_bind / OpenGL 4.4 */ > + > +void GLAPIENTRY > +_mesa_BindTextures(GLuint first, GLsizei count, const GLuint *textures) > +{ > + GET_CURRENT_CONTEXT(ctx); > + struct GLuint currentTexUnit = 0; > + int i = 0; > + > + currentTexUnit = ctx->Texture.CurrentUnit; > + > + if(first + count > ctx->Const.MaxCombinedTextureImageUnits) { > + _mesa_error(ctx, GL_INVALID_OPERATION, "glBindTextures(first+count)"); > + return; > + } > + > + for(i = 0 ; i < count ; i++) { > + GLuint texture; > + struct gl_texture_object *texObj; > + GLenum texTarget; > + int j = 0; > + > + if(textures == NULL) > +texture = 0; > + else > +texture = textures[i]; > + > + _mesa_ActiveTexture(GL_TEXTURE0 + first + i); > + if(texture != 0) { > +texObj = _mesa_lookup_texture(ctx, texture); > +if(texObj) { > + texTarget = texObj->Target; > + _mesa_BindTexture(texTarget, texture); > +} > +else > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glBindTextures(textures[%i])", i); This error is set too late. It should be done before changing textures. Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev