917616 - WebGL fails to drawElements without any attrib arrays

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Technically, there's nothing stopping someone from disabling all active attribs (making them all non-arrays) but then using drawElements to do uselessly indexed drawing.
It's not that useful, but it's within spec.

At least, I can't find where in the spec this is forbidden.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Give drawElements a way to get a maxElem back from ArrBuff->Validate, and use that for fakeblack

Comment 2

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

Comment on attachment 806355 [details] [diff] [review]
patch: Give drawElements a way to get a maxElem back from ArrBuff->Validate, and use that for fakeblack

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

Important: update TestWebGLElementArrayCache to check that the returned maxValue is indeed an upper bound.

::: content/canvas/src/WebGLElementArrayCache.cpp
@@ +508,5 @@
>  template<typename T>
> +bool WebGLElementArrayCache::Validate(uint32_t maxAllowed,
> +                                      size_t firstElement,
> +                                      size_t countElements,
> +                                      uint32_t* maxValue) 

Whitespace.

@@ +514,5 @@
> +  uint32_t dummy = 0;
> +  if (!maxValue) {
> +    maxValue = &dummy;
> +  }
> +  

Look, you're using _conservative_ estimates and you're adding _white_ space. Are you trying to appeal to a particular category of voters?

::: content/canvas/src/WebGLElementArrayCache.h
@@ +32,5 @@
>  public:
>    bool BufferData(const void* ptr, size_t byteSize);
>    void BufferSubData(size_t pos, const void* ptr, size_t updateByteSize);
>  
> +  // Returns a conservative maxValue (if non-null) on returning true.

Please clarify what conservative means here.

I am continuing this review with the assumption that conservative means that the value returned is >= the actual maximum i.e. is an upper bound. If that is true, "conservative maximum value" is better rephrased as "upper bound".

It also seems that when the function returns false, the value of that out-param is undefined. That's worth saying explicitly.

@@ +35,5 @@
>  
> +  // Returns a conservative maxValue (if non-null) on returning true.
> +  bool Validate(GLenum type, uint32_t maxAllowed,
> +                size_t first, size_t count,
> +                uint32_t* maxValue = nullptr);

Is any caller left using Validate without passing a maxValue?

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 806355 [details] [diff] [review]
> patch: Give drawElements a way to get a maxElem back from ArrBuff->Validate,
> and use that for fakeblack
> 
> Review of attachment 806355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Important: update TestWebGLElementArrayCache to check that the returned
> maxValue is indeed an upper bound.
Good point.
> 
> ::: content/canvas/src/WebGLElementArrayCache.cpp
> @@ +508,5 @@
> >  template<typename T>
> > +bool WebGLElementArrayCache::Validate(uint32_t maxAllowed,
> > +                                      size_t firstElement,
> > +                                      size_t countElements,
> > +                                      uint32_t* maxValue) 
> 
> Whitespace.
Ugh.
> 
> @@ +514,5 @@
> > +  uint32_t dummy = 0;
> > +  if (!maxValue) {
> > +    maxValue = &dummy;
> > +  }
> > +  
> 
> Look, you're using _conservative_ estimates and you're adding _white_ space.
> Are you trying to appeal to a particular category of voters?
You found me out!
> 
> ::: content/canvas/src/WebGLElementArrayCache.h
> @@ +32,5 @@
> >  public:
> >    bool BufferData(const void* ptr, size_t byteSize);
> >    void BufferSubData(size_t pos, const void* ptr, size_t updateByteSize);
> >  
> > +  // Returns a conservative maxValue (if non-null) on returning true.
> 
> Please clarify what conservative means here.
> 
> I am continuing this review with the assumption that conservative means that
> the value returned is >= the actual maximum i.e. is an upper bound. If that
> is true, "conservative maximum value" is better rephrased as "upper bound".
Alright.
> 
> It also seems that when the function returns false, the value of that
> out-param is undefined. That's worth saying explicitly.
It's not undefined, it's just not changed. I'll add a comment for this, but this is standard practice for failures, since I can't really return anything useful.
> 
> @@ +35,5 @@
> >  
> > +  // Returns a conservative maxValue (if non-null) on returning true.
> > +  bool Validate(GLenum type, uint32_t maxAllowed,
> > +                size_t first, size_t count,
> > +                uint32_t* maxValue = nullptr);
> 
> Is any caller left using Validate without passing a maxValue?
I think I saw compiler errors before adding this, yes.

Comment 4

[Kelsey Gilbert [:jgilbert]]

This was fixed elsewhere.