[WebGL2] Sampler objects

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

/* Sampler Objects */
  WebGLSampler? createSampler();
  void deleteSampler(WebGLSampler? sampler);
  [WebGLHandlesContextLoss] GLboolean isSampler(WebGLSampler? sampler);
  void bindSampler(GLuint unit, WebGLSampler? sampler);
  void samplerParameteri(WebGLSampler? sampler, GLenum pname, GLint param);
  void samplerParameteriv(WebGLSampler? sampler, GLenum pname, Int32Array param);
  void samplerParameteriv(WebGLSampler? sampler, GLenum pname, sequence<GLint> param);
  void samplerParameterf(WebGLSampler? sampler, GLenum pname, GLfloat param);
  void samplerParameterfv(WebGLSampler? sampler, GLenum pname, Float32Array param);
  void samplerParameterfv(WebGLSampler? sampler, GLenum pname, sequence<GLfloat> param);

[Dean Jackson]

<rdar://problem/15002402>

[Justin Fan]

*** Bug 206388 has been marked as a duplicate of this bug. ***

[Justin Fan]

Created attachment 387999 [details]
Patch

[Dean Jackson]

Comment on attachment 387999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387999&action=review

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1182
> +        for (auto& samplerSlot : m_boundSamplers) {
> +            if (samplerSlot == sampler)
> +                samplerSlot = nullptr;

Rather than relying on the fact you got a ref out of the loop, I think doing this via an index would be more obvious. e.g. 

for (int i = 0; i < m_boundSamplers.length(); i++) {
  if (sampler == m_boundSamplers[i])
    m_boundSamplers[i] = nullptr;
}

.. even though it is more verbose and old-school. But that's just my opinion. I guess this is the only object that can be bound in multiple places.

I wouldn't be surprised if everyone else disagrees with me here.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1206
> +

Maybe check if it is already bound to that unit and return early?

[Myles C. Maxfield]

Comment on attachment 387999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387999&action=review

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1182
>> +                samplerSlot = nullptr;
> 
> Rather than relying on the fact you got a ref out of the loop, I think doing this via an index would be more obvious. e.g. 
> 
> for (int i = 0; i < m_boundSamplers.length(); i++) {
>   if (sampler == m_boundSamplers[i])
>     m_boundSamplers[i] = nullptr;
> }
> 
> .. even though it is more verbose and old-school. But that's just my opinion. I guess this is the only object that can be bound in multiple places.
> 
> I wouldn't be surprised if everyone else disagrees with me here.

I disagree. for(auto& is a legit pattern.

[Justin Fan]

Created attachment 388324 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 388324 [details]
Patch for landing

Clearing flags on attachment: 388324

Committed r254869: <https://trac.webkit.org/changeset/254869>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58767659>

[Justin Fan]

<rdar://problem/15002402>

[Jonathan Bedard]

Not 100% confident this is the commit to blame, but it seems like the most reasonable one in the range:

https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=webgl%2F2.0.0%2Fconformance2%2Fmisc%2Fexpando-loss-2.html&test=webgl%2F2.0.0%2Fconformance2%2Fsamplers%2Fsampler-drawing-test.html&test=webgl%2F2.0.0%2Fconformance2%2Fsamplers%2Fsamplers.html&test=webgl%2F2.0.0%2Fconformance2%2Ftextures%2Fmisc%2Factive-3d-texture-bug.html

[Jon Lee]

(In reply to Jonathan Bedard from comment #11)
> Not 100% confident this is the commit to blame, but it seems like the most
> reasonable one in the range:
> 
> https://results.webkit.org/?suite=layout-tests&suite=layout-
> tests&suite=layout-tests&suite=layout-tests&test=webgl%2F2.0.
> 0%2Fconformance2%2Fmisc%2Fexpando-loss-2.html&test=webgl%2F2.0.
> 0%2Fconformance2%2Fsamplers%2Fsampler-drawing-test.html&test=webgl%2F2.0.
> 0%2Fconformance2%2Fsamplers%2Fsamplers.html&test=webgl%2F2.0.
> 0%2Fconformance2%2Ftextures%2Fmisc%2Factive-3d-texture-bug.html

At minimum it looks like we should rebaseline here. Some of them got further along. But the rest likely needs further investigation.

[Justin Fan]

The *sampler*.html tests have passing expectations that are not relevant for the WebGL bot, since they are skipped on the normal bots. They will be unskipped, and should pass, when the ANGLE backend is switched back on.

The other two tests are making it further in parsing due to the functions I implemented. We could rebase them, but it makes more sense to rebase all of 2.0.0 when ANGLE is back.