Benoit Jacob [:bjacob] (mostly away) Reporter
	Description • 10 years ago

WebGL2 adds invalidateFramebuffer and invalidateSubFramebuffer. They should work exactly like OpenGL ES 3.0.3's glInvalidateFramebuffer and glInvalidateSubFramebuffer, respectively.

This bug is only about implementing invalidateFramebuffer which we have a short-term need for; implementing also invalidateSubFramebuffer would be a bonus not a requirement.

The main steps are:

1) General OpenGL part: Edit code around gfx/gl/GLContext.* to add the glInvalidateFramebuffer entry point, to be exposed as GLContext::fInvalidateFramebuffer, if the GL version is high enough (for GL ES it should be >= 3.0, not sure for desktop; this website is the quickest way to check: http://docs.gl )

Generally OpenGL features are exposed if either the OpenGL core version is high enough, or some combination of OpenGL extensions are supported. We abstract that as "GLFeatures" - see in the gfx/gl directory. Here too you'll probably want to add a new GLFeature for this, which you can name however you want, say invalidate_framebuffer. See in gfx/gl/GLContextFeatures.cpp.

For a model to follow, see how the map_buffer_range feature is declared, how its OpenGL requirements are described in GLContextFeatures.cpp, and how it is initialized in GLContext.cpp in GLContext::InitWithPrefix().

2) WebGL part:

2a) In WebGLContext::InitWebGL2(), add your new invalidate_framebuffer feature to the list of required features.

2b) Edit code in WebGL2ContextFramebuffers.cpp to implement WebGL2Context::InvalidateFramebuffer, using the GLContext::fInvalidateFramebuffer that you'll have implemented in step 1.

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 2 • 10 years ago

Comment on attachment 8499944 [details] [diff] [review]
bug1076456-invalidate-framebuffer.patch

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

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +33,5 @@
>  }
>  
>  void
>  WebGL2Context::InvalidateFramebuffer(GLenum target, const dom::Sequence<GLenum>& attachments)
>  {

This needs to do the typical IsContextLost check and early return. On a lost context, the 'gl' pointer is null, so we would crash below.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 10 years ago

Address comment #2

	Kelsey Gilbert [:jgilbert]
	Comment 4 • 10 years ago

Comment on attachment 8500321 [details] [diff] [review]
bug1076456-invalidate-framebuffer-v2.patch

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

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +37,5 @@
>  {
> +    if (IsContextLost())
> +        return;
> +    MakeContextCurrent();
> +    gl->fInvalidateFramebuffer(target, attachments.Length(), attachments.Elements());

To be clear, being this thin of a shim is fine for the prototype stage. Eventually we'll have to be doing full validation, but we can punt on that.

However, we do need to handle the different behavior for the default framebuffer as opposed to bound framebuffers.

::: dom/canvas/test/webgl-mochitest/test_webgl2_invalidate_framebuffer.html
@@ +14,5 @@
> +WebGLUtil.withWebGL2('c', function (gl) {
> +  gl.enable(gl.DEPTH_TEST);
> +  gl.viewportWidth = 64;
> +  gl.viewportHeight = 64;
> +  gl.invalidateFramebuffer(gl.FRAMEBUFFER, [gl.DEPTH_ATTACHMENT]);

This is invalid. "If the default framebuffer is bound, then attachments may contain GL_COLOR, identifying the color buffer; GL_DEPTH, identifying the depth buffer; and/or GL_STENCIL, identifying the stencil buffer. "

::: gfx/gl/GLContext.h
@@ +890,5 @@
>  
>          raw_fBindFramebuffer(target, framebuffer);
>      }
>  
> +    void fInvalidateFramebuffer(GLenum target, GLsizei numAttachments, const GLenum *attachments) {

* to left, against the type. [1]

@@ +897,5 @@
> +        mSymbols.fInvalidateFramebuffer(target, numAttachments, attachments);
> +        AFTER_GL_CALL;
> +    }
> +
> +    void fInvalidateSubFramebuffer(GLenum target, GLsizei numAttachments, const GLenum *attachments, GLint x, GLint y, GLsizei width, GLsizei height) {

[1]

::: gfx/gl/GLContextSymbols.h
@@ +333,5 @@
>      PFNGLISVERTEXARRAY fIsVertexArray;
>      typedef void (GLAPIENTRY * PFNGLRENDERBUFFERSTORAGE) (GLenum target, GLenum internalFormat, GLsizei width, GLsizei height);
>      PFNGLRENDERBUFFERSTORAGE fRenderbufferStorage;
>  
> +    typedef void (GLAPIENTRY * PFNINVALIDATEFRAMEBUFFER) (GLenum target, GLsizei numAttachments, const GLenum *attachments);

[1]

@@ +335,5 @@
>      PFNGLRENDERBUFFERSTORAGE fRenderbufferStorage;
>  
> +    typedef void (GLAPIENTRY * PFNINVALIDATEFRAMEBUFFER) (GLenum target, GLsizei numAttachments, const GLenum *attachments);
> +    PFNINVALIDATEFRAMEBUFFER fInvalidateFramebuffer;
> +    typedef void (GLAPIENTRY * PFNINVALIDATESUBFRAMEBUFFER) (GLenum target, GLsizei numAttachments, const GLenum *attachments, GLint x, GLint y, GLsizei width, GLsizei height);

[1]

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 5 • 10 years ago

Address review comments.

	Kelsey Gilbert [:jgilbert]
	Comment 6 • 10 years ago

Comment on attachment 8501517 [details] [diff] [review]
bug1076456-invalidate-framebuffer-v3.patch

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

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +34,5 @@
>  
> +// Map attachments intended for the default buffer, to attachments for a non-
> +// default buffer.
> +static void
> +TranslateDefaultAttachments(dom::Sequence<GLenum>& out, const dom::Sequence<GLenum>& in)

Foo(const dom::Sequence<GLenum>& in, nsTArray<GLenum>* out)

Out-vars at the end, and they should be pointers to make it clear from the call-site that they're out-vars.

::: dom/canvas/test/webgl-mochitest/test_webgl2_invalidate_framebuffer.html
@@ +12,5 @@
> +<script>
> +
> +WebGLUtil.withWebGL2('c', function (gl) {
> +  gl.viewportWidth = 64;
> +  gl.viewportHeight = 64;

These don't have any function, so remove them.

::: dom/canvas/test/webgl-mochitest/webgl-util.js
@@ +57,5 @@
>  
>      return gl;
>    }
>  
> +  function withWebGL2(canvasId, callback, finish) {

`finish` should probably be `postCallback` or `onFinished`.

::: gfx/gl/GLScreenBuffer.h
@@ +275,5 @@
>      void BindDrawFB_Internal(GLuint fb);
>      void BindReadFB_Internal(GLuint fb);
> +
> +    bool IsDrawFramebufferDefault();
> +    bool IsReadFramebufferDefault();

These should both be const.

	u480271
	Comment 7 • 10 years ago

I believe Walter implemented all the functions around framebuffers and renderbuffers. This is why I was asking Jeff if we had all his patches. I can look in my email to see if he sent me diffs.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/33aa799d4fe2

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/33aa799d4fe2

	Florian Scholz (Open Web Docs)
	Comment 10 • 8 years ago

https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/invalidateFramebuffer