Bug 126941 - [WebGL2] Sampler objects
Summary: [WebGL2] Sampler objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
: 206388 (view as bug list)
Depends on:
Blocks: 126404
  Show dependency treegraph
 
Reported: 2014-01-13 15:45 PST by Dean Jackson
Modified: 2020-01-22 13:48 PST (History)
12 users (show)

See Also:


Attachments
Patch (37.61 KB, patch)
2020-01-16 17:59 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (37.64 KB, patch)
2020-01-21 11:04 PST, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-01-13 15:45:04 PST
/* 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);
Comment 1 Dean Jackson 2014-01-13 15:45:29 PST
<rdar://problem/15002402>
Comment 2 Justin Fan 2020-01-16 17:22:58 PST
*** Bug 206388 has been marked as a duplicate of this bug. ***
Comment 3 Justin Fan 2020-01-16 17:59:32 PST
Created attachment 387999 [details]
Patch
Comment 4 Dean Jackson 2020-01-21 10:52:33 PST
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?
Comment 5 Myles C. Maxfield 2020-01-21 10:59:03 PST
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.
Comment 6 Justin Fan 2020-01-21 11:04:45 PST
Created attachment 388324 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2020-01-21 11:48:11 PST
Comment on attachment 388324 [details]
Patch for landing

Clearing flags on attachment: 388324

Committed r254869: <https://trac.webkit.org/changeset/254869>
Comment 8 WebKit Commit Bot 2020-01-21 11:48:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2020-01-21 11:49:22 PST
<rdar://problem/58767659>
Comment 10 Justin Fan 2020-01-21 14:43:19 PST
<rdar://problem/15002402>
Comment 12 Jon Lee 2020-01-22 12:14:29 PST
(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.
Comment 13 Justin Fan 2020-01-22 13:48:29 PST
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.