Bug 126941

Summary: [WebGL2] Sampler objects
Product: WebKit Reporter: Dean Jackson <dino>
Component: WebGLAssignee: Justin Fan <justin_fan>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, jbedard, jonlee, justin_fan, kondapallykalyan, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 126404    
Attachments:
Description Flags
Patch
none
Patch for landing none

Dean Jackson
Reported 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);
Attachments
Patch (37.61 KB, patch)
2020-01-16 17:59 PST, Justin Fan 		no flags
DetailsFormatted DiffDiff
Patch for landing (37.64 KB, patch)
2020-01-21 11:04 PST, Justin Fan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dean Jackson
Comment 1 2014-01-13 15:45:29 PST
<rdar://problem/15002402>
Justin Fan
Comment 2 2020-01-16 17:22:58 PST
*** Bug 206388 has been marked as a duplicate of this bug. ***
Justin Fan
Comment 3 2020-01-16 17:59:32 PST
Created attachment 387999 [details] Patch
Dean Jackson
Comment 4 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?
Myles C. Maxfield
Comment 5 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.
Justin Fan
Comment 6 2020-01-21 11:04:45 PST
Created attachment 388324 [details] Patch for landing
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2020-01-21 11:48:13 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2020-01-21 11:49:22 PST
<rdar://problem/58767659>
Justin Fan
Comment 10 2020-01-21 14:43:19 PST
<rdar://problem/15002402>
Jon Lee
Comment 12 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.
Justin Fan
Comment 13 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.
Note You need to log in before you can comment on or make changes to this bug.