On 08/18/2015 11:11 AM, Timothy Arceri wrote:
On Tue, 2015-08-18 at 10:16 +0300, Tapani Pälli wrote:On 08/18/2015 12:30 AM, Timothy Arceri wrote:On Mon, 2015-08-17 at 08:12 +0300, Tapani Pälli wrote:On 08/14/2015 04:01 PM, Timothy Arceri wrote:On Fri, 2015-08-14 at 08:55 +0300, Tapani Pälli wrote:On 08/13/2015 11:54 AM, Timothy Arceri wrote:I've sent a couple of follow-up patches I notice when reviewing this. On Thu, 2015-08-13 at 09:30 +0300, Tapani Pälli wrote:v2: code cleanup Signed-off-by: Tapani Pälli <tapani.pa...@intel.com> --- src/mesa/main/teximage.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c index d35dc12..add7438 100644 --- a/src/mesa/main/teximage.c +++ b/src/mesa/main/teximage.c @@ -5782,6 +5782,18 @@ _mesa_TexImage3DMultisample(GLenum target, GLsizei samples, "glTexImage3DMultisample"); } +static bool +valid_texstorage_ms_parameters(GLsizei width, GLsizei height, GLsizei depth, + GLsizei samples, unsigned dims) +{ + GET_CURRENT_CONTEXT(ctx); + + if (_mesa_tex_storage_invalid_dim(width, height, depth) || samples < 1)Rather than do the samples < 1 check here you should move it to _mesa_texture_image_multisample as the spec says "An INVALID_VALUE error is generated ifsamplesis zero." for glTexImage*Multisample too. With that change you could change all the calls below to something like if (_mesa_tex_storage_invalid_dim(width, height, depth)) { _mesa_error(ctx, GL_INVALID_VALUE, "glTexStorage2DMultisample()" return; }Would it be OK if we do possible unification for checking samples parameter after these patches? My goal here is to do check for glTexStorage*Multisample only. I don't feel comfortable changing checks for other functions at the same time, that should deserve it's own commit so it's easier to bisect later.There is no reason to split the change into two patches. The correct change is to update _mesa_texture_image_multisample (which I will rename texture_image_multisample once your patch lands) to do the samples check. The only other user is image*Multisample and the spec clearly states that this is also an error when 0. That spec says it undefined what happens when this is negative but we already throw an error via _mesa_check_sample_count().OK, how about land your patch first so that we can test for regressions. If nothing regresses after your changes then I can safely land mine.The changes are pushed to master :)OK nice, I've updated my patch to not check samples (only dimensions). Do you want me to send it separately or are you ok giving r-b with these changes?Sorry just one more nit, it would be nice to print width, height, depth as part of the error message: _mesa_error(ctx, GL_INVALID_VALUE, "glTexStorage%uDMultisample(width=%d,height=%d,depth=%d)", dims, width, height, depth);
Makes sense, thanks!
With that feel free to add my r-b.You don't have to touch _mesa_check_sample_count() that change should be in another patch which I already sent [1] feel free not to review it :) Doing it the way your suggesting is code churn for no gain, it just means your code needs to be deleted and replaced with the change I'm suggesting later anyway, so why not just fix it right the first time.Yes, maybe I'm too pessimistic that nothing would break. It's just that typically something does and I've been really hard trying to not break ES 3.0 conformance (which includes ES 2.0 tests too) while making ES 3.1 changes. We are very close to being ES 3.0 conformant (just one test failing ATM).Nice work. I can see where your coming from but if we are going to break things now is the time to do it while people like yourself are keeping a close eye on things :)Also feel free to keep your valid_texstorage_ms_parameters() function I was just suggesting an alternative to having another function call. [1] http://lists.freedesktop.org/archives/mesa-dev/2015-August/0915 88.html{ + _mesa_error(ctx, GL_INVALID_VALUE, "glTexStorage%uDMultisample()", dims); + return false; + } + return true; +} void GLAPIENTRY _mesa_TexStorage2DMultisample(GLenum target, GLsizei samples, @@ -5795,6 +5807,9 @@ _mesa_TexStorage2DMultisample(GLenum target, GLsizei samples, if (!texObj) return; + if (!valid_texstorage_ms_parameters(width, height, 1, samples, 2)) + return; + _mesa_texture_image_multisample(ctx, 2, texObj, target, samples, internalformat, width, height, 1, fixedsamplelocations, GL_TRUE, @@ -5814,6 +5829,9 @@ _mesa_TexStorage3DMultisample(GLenum target, GLsizei samples, if (!texObj) return; + if (!valid_texstorage_ms_parameters(width, height, depth, samples, 3)) + return; + _mesa_texture_image_multisample(ctx, 3, texObj, target, samples, internalformat, width, height, depth, fixedsamplelocations, GL_TRUE, @@ -5834,6 +5852,9 @@ _mesa_TextureStorage2DMultisample(GLuint texture, GLsizei samples, if (!texObj) return; + if (!valid_texstorage_ms_parameters(width, height, 1, samples, 2)) + return; + _mesa_texture_image_multisample(ctx, 2, texObj, texObj ->Target, samples, internalformat, width, height, 1, fixedsamplelocations, GL_TRUE, @@ -5855,6 +5876,9 @@ _mesa_TextureStorage3DMultisample(GLuint texture, GLsizei samples, if (!texObj) return; + if (!valid_texstorage_ms_parameters(width, height, depth, samples, 3)) + return; + _mesa_texture_image_multisample(ctx, 3, texObj, texObj ->Target, samples, internalformat, width, height, depth, fixedsamplelocations, GL_TRUE,
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
