[Mesa-dev] [PATCH] vbo: Ignore invalid element ranges if fixing 'end' breaks 'start'.

[Roland Scheidegger]

FWIW, I think there are some more errors (wrt base index) in that function:
http://lists.freedesktop.org/archives/mesa-dev/2012-January/018306.html

(I agree though there's a difference if the bounds would point to
vertices outside the array or the bounds are so bogus that no index can
be in the specified range, which is all we can do for detecting indices
outside the range without scanning the indices.)

Roland

Am 04.02.2012 14:03, schrieb Jose Fonseca:
> Hi Kenneth,
> 
> I agree with the behavior change for compatability sake, but I think there's a distinction to be made here in the warnings passed to the user.
> 
> 
> The spec says the app supplied [start, end] range are supposed to be conservative hints of the ranges of index value in the index buffer, i.e, all values in the index buffer should be inside [start, end]. In particular http://www.opengl.org/sdk/docs/man/xhtml/glDrawRangeElements.xml says: "There is no requirement that all vertices in the range start end be referenced."
> 
> 
> Passing an end value bigger than the maximum representable index is perfectly fine and conformant behavior as the above rule is preserved. For example, an app developer may set the end to 0xfffffff when they don't have that value readily available.  Mesa will emit a warning but it is being pedantic, because it really should handle glDrawRangeElements(..., 0, 0xfffffff) _exactly_ the same way as glDrawElements(...).
> 
> 
> Passing start value that is above the vertex arrays size is clearly _not_ conformant behavior.
> 
> 
> IMO, over-generous `end` index, and totally bogus `start` are in totally different scales of bad. The former should be worded as a performance hint, or even removed, while the latter should be clearly marked as an error.
> 
> 
> Jose
> 
> 
> ----- Original Message -----
>> Certain applications, such as Regnum Online, appear to pass invalid
>> start/end values to glDrawRangeElements.  In particular, the 'end'
>> index
>> sometimes exceeds the maximum array element.
>>
>> In this case, we issued a warning and attempted to "fix" the end
>> value
>> by clamping it to the last array element.  Then we re-checked whether
>> end < start (which would normally result in an API error), and if so,
>> dropped the call on the floor.
>>
>> This seems foolish: if 'end' is invalid, why should we trust 'start'?
>> The application has already proven that it can't track these things.
>>
>> This patch takes a more conservative approach:
>> 1. Force 'end' to be the maximum array element.
>> 2. If end < start, assume both bounds were rubbish and discard them,
>>    treating the call as an ordinary glDrawElements().
>>
>> This should allow more broken applications to work, while preserving
>> the
>> useful "This should probably be fixed in the application" warning.
>>
>> NOTE: This is a candidate for release branches.
>>
>> Cc: Mateusz Kaduk <mateusz.kaduk at gmail.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45214
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44701
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=41152
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=40361
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28138
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/mesa/vbo/vbo_exec_array.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> Originally, I thought we must be doing something wrong, as I saw this
>> message in some Unigine demos, Regnum Online, and even an oglconform
>> test case.
>>
>> It looks like the oglconform test case is indeed specifying an 'end'
>> value way larger than the buffer size, which is wrong (though the GL
>> isn't required to raise an error.)
>>
>> I've also gone through the _MaxElement calculations and convinced
>> myself that they're correct.  Regnum is also passing us a range
>> that's _completely_ outside of the buffer---both start and end are
>> past _MaxElement.  A small miscalculation wouldn't account for this.
>>
>> I also did some googling and found a bunch of bug reports about this
>> where application authors were told about this warning and did appear
>> to find and fix bugs in their application.
>>
>> So, at the end of the day, I think these apps are broken, but since
>> we're already working around them, we may as well make them work.
>>
>> I'd appreciate review since this is my first foray into VBO land.
>> The ~0's match what vbo_exec_DrawElements uses, so I feel pretty
>> safe.
>>
>> Tested on Sandybridge.  No regressions in Piglit's
>> quick-driver.tests,
>> the Khronos ES 2.0 test suite, or the vao tests in Intel's
>> oglconform.
>>
>> diff --git a/src/mesa/vbo/vbo_exec_array.c
>> b/src/mesa/vbo/vbo_exec_array.c
>> index d6b4d61..8a3be1b 100644
>> --- a/src/mesa/vbo/vbo_exec_array.c
>> +++ b/src/mesa/vbo/vbo_exec_array.c
>> @@ -937,7 +937,12 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum
>> mode,
>>        end = ctx->Array.ArrayObj->_MaxElement - 1;
>>  
>>        if (end < start) {
>> -         return;
>> +	 /* We found that 'end' was too large, and corrected it to be the
>> +	  * maximum value.  Now it's less than 'start', which probably
>> means
>> +	  * that 'start' was invalid too.  Just ignore the range entirely
>> and
>> +	  * treat this as a call to glDrawElements.
>> +	  */
>> +	 start = end = ~0;
>>        }
>>     }
>>  
>> --
>> 1.7.7.6
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev