https://bugs.freedesktop.org/show_bug.cgi?id=91314
--- Comment #4 from Iago Toral <ito...@igalia.com> ---
(In reply to Ilia Mirkin from comment #2)
> So from the backtrace on IRC we see:
>
> #5 0xb6ab9a64 in init_teximage_fields_ms (ctx=<optimized out>,
> img=0x204c7420, width=<optimized out>, height=0, depth=0, border=0,
> internalFormat=0, format=MESA_FORMAT_NONE, numSamples=0,
> fixedSampleLocations=1 '\001') at main/teximage.c:1320
> #6 0xb6ab9d4a in _mesa_init_teximage_fields (ctx=0x2055bed0,
> img=0x204c7420, width=0, height=0, depth=0, border=0, internalFormat=0,
> format=MESA_FORMAT_NONE) at main/teximage.c:1420
> #7 0xb6abee31 in _mesa_texture_image_multisample (ctx=0x2055bed0, dims=2,
> texObj=0x204a55d8, target=37120, samples=5, internalformat=34836,
> width=32, height=32, depth=1, fixedsamplelocations=0 '\000', immutable=0
> '\000', func=0xb6ec6f81 "glTexImage2DMultisample")
> at main/teximage.c:5703
> #8 0xb6abeecc in _mesa_TexImage2DMultisample (target=37120, samples=5,
> internalformat=34836, width=32, height=32,
> fixedsamplelocations=0 '\000') at main/teximage.c:5731
>
> So the culprit is:
>
> if (width > 0 && height > 0 && depth > 0) {
> if (!ctx->Driver.AllocTextureStorage(ctx, texObj, 1,
> width, height, depth)) {
> /* tidy up the texture image state. strictly speaking,
> * we're allowed to just leave this in whatever state we
> * like, but being tidy is good.
> */
> _mesa_init_teximage_fields(ctx, texImage,
> 0, 0, 0, 0, GL_NONE, MESA_FORMAT_NONE);
> }
> }
>
> The texture storage allocation fails because nv50 doesn't support GL_RGBA32F
> with > 4 samples.
>
> Of course the sample count check above (samplesOK) passes because in st/mesa
> format choosing, GL_RGBA32F will happily fall back to RGBA16F, which is
> perfectly allowed to have 8 samples on nv50.
>
> However st_AllocTextureStorage is blissfully unaware of such a restriction,
> and fails when it can't create a RGBA32F MS8 texture.
>
> So the bug is 2-fold:
>
> (a) st_AllocateTextureStorage should use st_choose_format (or equivalent) so
> that it deals with any sample count limitations
>
> (b) _mesa_init_teximage_fields with MESA_FORMAT_NONE appears to trigger the
> assert no matter what -- this should be handled better. Iago, should we just
> let MESA_FORMAT_NONE through the assert, as was done before? (By the way,
> GLenum, which is the type of baseFormat, appears to be unsigned, so a check
> against -1 is a bit dangerous.)
Well, if we are going to be calling this with MESA_FORMAT_NONE then it does not
make sense to have an assertion to check that _mesa_base_tex_format() on that
does not return -1 of course. That said, maybe this code is abusing
_mesa_init_teximage_fields() a bit too much?
I am not sure about the purpose of this call in the code above but the comment
seems to suggest that it is not necessary anyway and it is just part of a bail
out process, so in this particular case maybe it would make more sense to just
remove the call to _mesa_init_teximage_fields since we are calling it in a way
that is not expected only to reset texImage, and instead, use something else
to reset that variable (maybe memset with zeroes would suffice here?) or not do
anything at all if it is not necessary like the comment suggests.
Personally, I'd only remove the assert if we expect that
_mesa_base_tex_format() returning -1 is possible in normal scenarios, but
looking at other uses of that function even in that same file it does not look
like that is the case since I see plenty of errors being generated in such
situations, even stating that such situations should never happen, which backs
the idea that the assertion is legit.
What do you think?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
