[Piglit] [PATCH] astc: avoid deleting a random texture

[Nanley Chery]

On Mon, Nov 23, 2015 at 1:28 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:

> On Mon, Nov 23, 2015 at 4:16 PM, Ian Romanick <idr at freedesktop.org> wrote:
> > On 11/23/2015 12:43 PM, Ilia Mirkin wrote:
> >> On Mon, Nov 23, 2015 at 3:34 PM, Ian Romanick <idr at freedesktop.org>
> wrote:
> >>> On 11/21/2015 04:08 PM, Ilia Mirkin wrote:
> >>>> In the check_error case, decompressed_tex is completely uninitialized
> >>>> and might point to any texture. This can wreak various havoc.
> >>>
>

Thanks for noticing this issue.


> >>> I might suggest a different approach.  In the check_error case, ensure
> >>> that decompress_texture is zero.  Remove the check_error parameter, and
> >>> s/!check_error/decompress_texture ==
> >>> 0/g;s/check_error/decompress_texture != 0/g.
> >>
> >> Is it legal to delete texture 0? Or can texture 0 be a real thing?
> >> Otherwise I can just initialize it to that below.
> >
> > It's similar to free(NULL) in that respect.  From the glDeleteTextures
> > man page:
> >
> >        glDeleteTextures silently ignores 0's and names that do not
> correspond
> >        to existing textures.
>
> Ah that's perfect. I should have checked the man page when I was
> futzing with the test.
>
> >
> >> Separately, how would you feel about making the error color decoding
> >> check failing return a warn instead of a fail? I can't get Adreno A420
> >> to spit it out... haven't tried on blob, perhaps I'm missing some bit
> >> of programming. I just get black with the HDR data instead of the
> >> error color.
> >
> > Hmm... I don't know how this particular test is structured.  When we've
> > had cases like this in the past, we've split the test into subcases.
> > One (or several) subcase checks the things that the driver in question
> > can pass, and one, single subcase checks the thing that the driver
> > fails.
> >
> > If this test is conducive that kind of a split (perhaps by running it
> > twice with different data), that would be my preference.
>
> Right now there are 3 subtests (ldr, hdr, srgb), each of which tests
> loading 3 sets of data (ldr, hdr, and ldr srgb). If there's no
> astc_hdr support, the hdr subtest isn't run, and when loading hdr
> data, it checks for the error color. Instead of 3 subtests, I could
> split this up into 9 subtests for the full cross -- does that sound
> reasonable, and then we'd pass the ldr-data ones and fail the hdr data
> ones.
>
> TBH I'm unsure what the diff between the ldr and hdr subtests is...
> they seem identical except that HDR one skips if you don't hdr. But
> otherwise it's identical to the LDR one from what I can tell. Also
> from the looks of it the srgb test doesn't need the full cross either,
> it only runs against the ldrs data. Urgh, I guess this test needs a
> bit of a redo :( Perhaps we can sucker Nanley into fixing it up?
>

I actually have a patch sitting on the list which redoes some of the logic:
http://lists.freedesktop.org/archives/piglit/2015-October/017743.html

The test previously wasn't splitting up into subtests correctly.

- Nanley
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20151130/2736ca87/attachment.html>