Closed Bug 1048720 Opened 10 years ago Closed 9 years ago

WebGL2 - Implement WebGLSampler

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: u480271, Assigned: u480271)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(18 files, 1 obsolete file)

11.37 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
38 bytes, text/x-review-board-request
jgilbert
: review+
Details
Implement functions and types to support WebGLSampler object.
Query GL functions for Samplers.
Attachment #8493656 - Flags: review?(bjacob)
Keywords: leave-open
Comment on attachment 8493656 [details] [diff] [review]
WebGL2 - GL symbols for Samplers.

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

I'll just grab this review, since it's simple.
Attachment #8493656 - Flags: review?(bjacob) → review+
Attachment #8521085 - Flags: review?(jgilbert)
Attachment #8521085 - Flags: review?(dvander)
/r/457 - Bug 1048720 - [WebGL2] Implement WebGL2Sampler. r=jgiblert
/r/459 - Bug 1048741 - [WebGL2] Partner code doesn't specify all mipmaps for compressed textures allocated via glTexStorage. r=jgilbert
/r/461 - Bug 1097413 - Symbolic constants kGLESVersion2 and kGLESVersion3. r=jgilbert
/r/463 - Bug 1097411 - Extend error reporting. r=jgilbert
/r/465 - Bug 1097416 - [WebGL2] enabled if all GLES 3 level features are available. r=jgilbert
/r/467 - Bug 1048745 - [WebGL2] Extend WebGLUniformInfo with GLES 3 types. r=jgilbert
/r/469 - Bug 1048743 - [WebGL2] No support for binary shaders. r=jgilbert
/r/471 - Bug 1048747 - [WebGL2] Query active uniform blocks. r=jgilbert
/r/473 - Bug 1048743 - [WebGL2] Split shader compilation into ANGLE and Bypass options. r=jgilbert
/r/475 - Bug 1048724 - [WebGL2] Transform Feedback. r=jgilbert
/r/477 - Bug 1048719 - [WebGL2] Query objects. r=jgilbert
/r/479 - Bug 1048745 - [WebGL2] Integer vertex attributes. r=jgilbert
/r/481 - Bug 1048747 - [WebGL2] Implement uniform block/buffer. r=jgilbert
/r/483 - Bug 1048747 - [WebGL2] Implement GetTexParameter for all valid WebGL2 pnames. r=jgilbert
/r/485 - Bug 1048731 - [WebGL2] Implement new buffer binding targets. r=jgilbert
/r/487 - Bug 1048745 - [WebGL2] Extend UniformInfo with WebGL2/GLES3 types. r=jgilbert
/r/489 - Bug 1048741 - [WebGL2] texParameter: TEXTURE_COMPARE_MODE and TEXTURE_COMPARE_FUNC support. r=jgilbert

Pull down these commits:

hg pull review -r 417423811da0284fd6f8866d40cc04c636905c55
https://reviewboard.mozilla.org/r/457/#review211

::: dom/canvas/WebGL2ContextSamplers.cpp
(Diff revision 1)
> +    // TODO(djg): If active handle unbinding

NS_WARNING, maybe?

::: dom/canvas/WebGLContextUnchecked.h
(Diff revision 1)
> +namespace gl { class GLContext; }

Use newlines.

::: dom/canvas/WebGLContextUnchecked.h
(Diff revision 1)
> +    explicit WebGLContextUnchecked(gl::GLContext* gl);

I think this is a good move. Right now, we keep the same WebGLContext around even though `gl` might change due to context loss. It would be easier to make sure context loss recovery works well to have `gl` elsewhere. (as you have here)

::: dom/canvas/WebGLContextUnchecked.cpp
(Diff revision 1)
> +WebGLContextUnchecked::BindSampler(WebGLSampler* sampler, GLuint unit)

It seems like the order should be (unit, sampler), since that's what GL has.

::: dom/canvas/WebGLContextValidate.cpp
(Diff revision 1)
> +                  pname == LOCAL_GL_TEXTURE_COMPARE_FUNC);

Use a switch.

::: dom/canvas/WebGLContextUnchecked.cpp
(Diff revision 1)
> +#include "GLContext.h"

Alphabetize includes other than the include for the parent header file.

::: dom/canvas/WebGL2ContextSamplers.cpp
(Diff revision 1)
> +        GLint value;
> +        WebGLContextUnchecked::GetSamplerParameteriv(sampler, pname, &value);

If GetSamplerParameteriv is infallible, then have it return GLint instead of having an out-param. If it's not infallible, we need to initialize value to something reasonable.

::: dom/canvas/WebGL2ContextSamplers.cpp
(Diff revision 1)
> +    return (ValidateObjectAllowDeleted("isSampler", sampler) &&
> +            !sampler->IsDeleted() &&
> +            sampler->HasEverBeenBound());

I would rather you do this in parts, rather than as a big concatenanted line.

::: dom/canvas/WebGLContext.h
(Diff revision 1)
> +class WebGLIntOrFloat {
> +    enum {
> +        Int,
> +        Float
> +    } mType;
> +    union {
> +        GLint i;
> +        GLfloat f;
> +    } mValue;
> +
> +public:
> +    explicit WebGLIntOrFloat(GLint i) : mType(Int) { mValue.i = i; }
> +    explicit WebGLIntOrFloat(GLfloat f) : mType(Float) { mValue.f = f; }
> +
> +    GLint AsInt() const { return (mType == Int) ? mValue.i : NS_lroundf(mValue.f); }
> +    GLfloat AsFloat() const { return (mType == Float) ? mValue.f : GLfloat(mValue.i); }
> +};
> +

This is nearly dangerously clever.

::: dom/canvas/WebGLContext.h
(Diff revision 1)
> +    // TODO(djg): Does this need a rethink? Should it be WebGL2Context?
> +    LinkedList<WebGLSampler> mSamplers;

I think it probably should.

::: dom/canvas/WebGLContextValidate.cpp
(Diff revision 1)
> +    bool valid = (pname == LOCAL_GL_TEXTURE_MIN_FILTER ||
> +                  pname == LOCAL_GL_TEXTURE_MAG_FILTER ||
> +                  pname == LOCAL_GL_TEXTURE_WRAP_S ||
> +                  pname == LOCAL_GL_TEXTURE_WRAP_T ||
> +                  pname == LOCAL_GL_TEXTURE_WRAP_R ||
> +                  pname == LOCAL_GL_TEXTURE_MIN_LOD ||
> +                  pname == LOCAL_GL_TEXTURE_MAX_LOD ||
> +                  pname == LOCAL_GL_TEXTURE_COMPARE_MODE ||
> +                  pname == LOCAL_GL_TEXTURE_COMPARE_FUNC);
> +    if (!valid)
> +        ErrorInvalidEnum("%s: invalid pname.", info);

Use a switch.

::: dom/canvas/WebGLContextValidate.cpp
(Diff revision 1)
> +        bool valid = (p == LOCAL_GL_NEAREST ||
> +                      p == LOCAL_GL_LINEAR ||
> +                      p == LOCAL_GL_NEAREST_MIPMAP_NEAREST ||
> +                      p == LOCAL_GL_NEAREST_MIPMAP_LINEAR ||
> +                      p == LOCAL_GL_LINEAR_MIPMAP_NEAREST ||
> +                      p == LOCAL_GL_LINEAR_MIPMAP_LINEAR);
> +        if (!valid)
> +            ErrorInvalidEnum("%s: invalid param", info);

Use a switch.

::: dom/canvas/WebGLContextValidate.cpp
(Diff revision 1)
> +        bool valid = (p == LOCAL_GL_LEQUAL ||
> +                      p == LOCAL_GL_GEQUAL ||
> +                      p == LOCAL_GL_LESS ||
> +                      p == LOCAL_GL_GREATER ||
> +                      p == LOCAL_GL_EQUAL ||
> +                      p == LOCAL_GL_NOTEQUAL ||
> +                      p == LOCAL_GL_ALWAYS ||
> +                      p == LOCAL_GL_NEVER);

Use a switch.

::: dom/canvas/WebGLContextValidate.cpp
(Diff revision 1)
> +    }

I think the format for switches should be like:
switch (foo) {
case bar: {
        int x;
        break;
    }
}

Otherwise we can get:
switch (foo) {
case bar: {
    int x;
    break;
}
}

::: dom/canvas/WebGLSampler.cpp
(Diff revision 1)
> -    MOZ_CRASH("Not Implemented.");
> +    mContext->MakeContextCurrent();
> +    mContext->gl->fGenSamplers(1, &mGLName);

Really MakeCurrent and GenSamplers should be pulled out into a helper function, so that mGLName can be const.
https://reviewboard.mozilla.org/r/457/#review213

> NS_WARNING, maybe?

I checked and we don't track bound samplers at the moment. I think I can remove the comment.

> If GetSamplerParameteriv is infallible, then have it return GLint instead of having an out-param. If it's not infallible, we need to initialize value to something reasonable.

GetSamplerParameteriv is a very thin wrapper over GL/GL ES. I think it would be appropriate to have it return value.

> This is nearly dangerously clever.

It's not clever, but dodgy discriminated union in C++. Rust/ML enum would be so much nicer.

Also, the GL spec is a bit fuzzy around these conversions. I found some language about it in Sec. 2.3.1, "Data Conversion For State-Setting Commands", es_spec_3.0.4.pdf, pg 14

> I think it probably should.

The issue then becomes how to order calls that clean up mSamplers, eg. WebGLContext::DestroyResourcesAndContext().

I've been erring on adding tracking like this into WebGLContext to make it a little easier and I would like to leave it here.

> Really MakeCurrent and GenSamplers should be pulled out into a helper function, so that mGLName can be const.

I've implemented this in another patch. You made the same comment previous on Bug 1002281.
Attachment #8521085 - Flags: review?(jgilbert) → review+
Attachment #8521085 - Attachment is obsolete: true
Attachment #8618256 - Flags: review+
Attachment #8618257 - Flags: review+
Attachment #8618258 - Flags: review+
Attachment #8618259 - Flags: review+
Attachment #8618260 - Flags: review+
Attachment #8618261 - Flags: review+
Attachment #8618262 - Flags: review+
Attachment #8618263 - Flags: review+
Attachment #8618264 - Flags: review+
Attachment #8618265 - Flags: review+
Attachment #8618266 - Flags: review+
Attachment #8618267 - Flags: review+
Attachment #8618268 - Flags: review+
Attachment #8618269 - Flags: review+
Attachment #8618270 - Flags: review+
Attachment #8618271 - Flags: review+
Attachment #8618272 - Flags: review+
What's all this stuff it thinks I requested?!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1285086
You need to log in before you can comment on or make changes to this bug.