[Mesa-dev] [PATCH] mesa: add GL_UNSIGNED_INT_24_8 to _mesa_pack_depth_span

[Ilia Mirkin]

On Sat, Oct 3, 2015 at 12:06 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Friday, October 02, 2015 05:04:21 PM Ilia Mirkin wrote:
>> On Fri, Oct 2, 2015 at 4:47 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> > On 10/01/2015 12:15 PM, Ilia Mirkin wrote:
>> >> On Thu, Oct 1, 2015 at 3:12 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> >>> I'm just
>> >>> wondering because Mesa doesn't support that extension.  How is this even
>> >>> being hit?
>> >>
>> >> See 81d2fd91a90 (mesa: add NV_read_{depth,stencil,depth_stencil} extensions)
>> >
>> > Okay, that's weird.  I must have had some old branch checked out at the
>> > time.  I did 'grep -r NV_read_depth src/', and it came back empty.  Now
>> > I just get to be irritated that we enabled THREE extensions for which we
>> > have ZERO tests... and least one is clearly completely broken. :(
>> >
>> > I guess now I at least have something concrete to point to then next
>> > time I object to enabling an ES extension that "just" allows some
>> > desktop functionality. ;)
>>
>> I believe Rob tested at least some of it with qapitrace[1], as
>> otherwise there was no way to get access to the data in a
>> renderbuffer, which can be quite useful for debugging. Not all of us
>> have your level of familiarity with how GL works, but how will we
>> learn without making some mistakes? :)
>>
>> No matter how many tests we might have, they'll always leave
>> *something* out. The fact that there are no tests at all for this ext
>> isn't great, of course. But there are also no functional tests for
>> {ARB,AMD}_conservative_depth and probably a number of others.
>>
>>   -ilia
>
> I think we can all agree that enabling a feature with 0 tests is bad
> practice.  Just because people have done it before doesn't make it wise.
> Or in a positive spin: our commitment to Piglit is one of the reasons
> we have such high quality drivers.  It's worth it, even if it can be a
> bunch of unglamorous and annoying work.
>
> I'm OK with not adding comprehensive tests for ES extensions that port
> over GL functionality, but at least touch testing the feature is
> valuable.
>
> Also...GL_ARB_conservative_depth is a bit different.  It provides a hint
> which the driver can use to speed up some operations - but there's no
> new feature, or observable behavior.  Drivers can completely ignore it
> and still have a valid, correct implementation of the extension.  So
> it's a bit tricky to test (but we could at least look for breakage...)

Ah yes, but drivers that don't ignore it can definitely mess it up. A
proper test would do a layout(depth) + modify depth in a permitted
fashion and make sure that it still works. I have a pending
implementation for nvc0 that's waiting on such tests to make sure that
I didn't get it backwards... with depth I can never tell what's up and
what's down :)

>
> In contrast, GL_NV_read_depth_stencil turns a GL call that used to be an
> error into a functioning read of depth data, which seems straightforward
> to test.

For the record, I largely agree with everything you say about the
value/importance of having tests. There's a non-trivial problem with
the same person writing the tests and implementation though -- if I
think an implementation should work a particular way, I'm going to
write a test that conforms to that line of thought. This is a large
part of the reason I tend to stay away from test writing for
extensions I don't fully understand -- it's dangerous for me to write
tests if I don't fully grasp all the functionality. For example take
the ARB_conservative_depth thing -- I could write tests that check the
depth stuff in the way I think it should work, but I'm not
sufficiently confident in my understanding of that stuff. The only
reason I was able to write tests for EXT_polygon_offset_clamp was that
I was 100% confident of my impl and just jiggered the test until it
passed.