1008310 - Generate a WebGL performance warning when a drawElement call causes a single element array to be consumed as two different index types

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Follow-up from bug 984783.

When drawElements consumes an element array as a certain element type (e.g. UNSIGNED_SHORT) and that element array had previously been consumed as another element type (e.g. UNSIGNED_BYTE) then we should generate a performance warning.

Rationale:
 - There is no good reason for content to do that; lumping different things in the same buffer object is generally done to allow draw-call batching, but since a draw call only uses one specific element type, lumping together multiple element types in an element array doesn't achieve anything.
 - This usage pattern is difficult to optimize for by the WebGLElementArrayCache. See comment in WebGLElementArrayCache.cpp:

 * Another consequence of this constraint is that when updating the trees, we have to update
 * all existing trees. So if trees for types uint8_t, uint16_t and uint32_t have ever been constructed for this buffer,
 * every subsequent update will have to update all trees even if one of the types is never
 * used again. That's inefficient, but content should not put indices of different types in the
 * same element array buffer anyways. Different index types can only be consumed in separate
 * drawElements calls, so nothing particular is to be achieved by lumping them in the same
 * buffer object.

How to implement this:

The WebGLElementArray cache is a good place to return information about that element types a given element array has been interpreted as; but it is not a good place to generate WebGL warnings. There should be some cooperation there, with the WebGL warning actually generated by the WebGLContext::DrawElements() function.

Comment 1

[Walter Litwinczyk [:walter]]

Attachment: webgl_bug_1008310_performance_warning

I wrote a mochi-test as well, but I wasn't sure how to check the JS console output from within the test.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8426592 [details] [diff] [review]
webgl_bug_1008310_performance_warning

Review of attachment 8426592 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLBuffer.h
@@ +48,5 @@
>  
>      bool Validate(GLenum type, uint32_t max_allowed, size_t first, size_t count,
>                    uint32_t* out_upperBound);
>  
> +    bool ElementArrayUsedWithMultipleTypes() const;

Prefix this with 'Is', to make it clear it's a question.

Comment 3

[Walter Litwinczyk [:walter]]

Attachment: webgl_bug_1008310_performance_warning

Comment 4

[Walter Litwinczyk [:walter]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=68e643cbabf9

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f323624f89a7

Please make sure your name is included in your commit information in the future. I added it this time. Also, "BeenUsedWithMultipleTypes" ? Seems like clunky grammar to me?

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #5)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f323624f89a7
> 
> Please make sure your name is included in your commit information in the
> future. I added it this time. Also, "BeenUsedWithMultipleTypes" ? Seems like
> clunky grammar to me?

(Has)BeenUsedWithMultipleTypes, but I sort of agree. WasUsedWithMultipleTypes would also work.

Comment 7

[Walter Litwinczyk [:walter]]

Attachment: webgl_bug_1008310_performance_warning

Changed 'BeenUsed...' -> 'HasBeenUsed...'

Comment 8

[Walter Litwinczyk [:walter]]

Attachment: webgl_bug_1008310_performance_warning

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Walter, I already landed the original patch for this bug. I would suggest at this point that you file a follow-up bug for renaming the function rather than revising the patch here. Makes it much easier to follow the history of things :)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f323624f89a7

Comment 11

[u480271]

This change to NameFrom() breaks ErrorInvalidEnumWithName(). Also, I don't like the stylistic change. Would have appreciated at least an f? 

Also the last two versions of patches don't have r+ or carry r+ comments. Has the renaming of BeenUsed... to HasBeenUsed... been landed?

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #11)
> This change to NameFrom() breaks ErrorInvalidEnumWithName(). Also, I don't
> like the stylistic change. Would have appreciated at least an f? 
Sorry about that. I'll try to pull you in if we hit this again.
I don't love either of the names here. Something more explicit like StringFromEnum would be better, I think, but ultimately I don't know as it matters hugely.
> 
> Also the last two versions of patches don't have r+ or carry r+ comments.
> Has the renaming of BeenUsed... to HasBeenUsed... been landed?

Comment 13

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #11)
> This change to NameFrom() breaks ErrorInvalidEnumWithName(). Also, I don't
> like the stylistic change. Would have appreciated at least an f? 
And what do you mean by 'breaks'? Surely this builds, so how is it broken?

Comment 14

[u480271]

The function no longer returns NULL for an unknown GLenum. This was used to switch to printing the unknown enum as hex value.

I should just bite the bullet and implement a function to turn all known GLenums in WebGL1/WebGL2 into string and assert when an unknown value is seen. I was being lazy and that was why it was internal to WebGLContextValidate.cpp. 

The name of function worked when you read it with the param: NameFrom(format). At least if there's no strong feeling either way, leave it. At a minimum get rid of the redundant WebGLContext:: on calls to EnumName().

Comment 15

[Walter Litwinczyk [:walter]]

Yeah, looking back at this patch there's a few things I would have done differently. I was (and am) still pretty new to this whole process and the code base. 

As for the first question the renaming appears to not have been landed (as I added it after the other patch was committed).

I'll open a new bug and make the modifications:
- Rename to HasBeenUsedWithMultipleTypes
- Remove redundant WebGLContext::
- return NULL again?

Comment 16

[u480271]

Walter: Let's collate all the enums from the IDL files and return "[unknown enum]". 

Actually that might not help if someone passes an integer value through from JS. The intention is to report in the console the value of the enum that is invalid to assist developers in tracking down the cause of the issue.

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> Walter: Let's collate all the enums from the IDL files and return "[unknown
> enum]". 
> 
> Actually that might not help if someone passes an integer value through from
> JS. The intention is to report in the console the value of the enum that is
> invalid to assist developers in tracking down the cause of the issue.

Yeah, I think it's useful to be able to mark unrecognized enums with their hex values.
We could just have this function return an std::string/nsString type.