1048693 - WebGL2 - Experiment with MapBufferRange

Status: NEW

(Core :: Graphics: CanvasWebGL, enhancement, P5)

Description

[u480271]

Currently, MapBufferRange, et al are not in the spec for WebGL2
because Chrome can't implement those functions efficiently. We think
there's an opportunity for us to create a WebGL2 extension that allows
direct writing to map buffers via ArrayBuffer and ArrayBufferViews.

Comment 1

[u480271]

Attachment: WebGL2 - Implement MapBufferRange

This is an experiment in implementing MapBufferRanges. This code should not be landed!

MapBufferRange maps wraps the result of glMapBufferRange with a
special ArrayBuffer. calling glUnmapBuffer neuters the ArrayBuffer.

As roc pointed out, this design doesn't work if the ArrayBuffer is
passed to a web worker. If we decide to move forward with supporting
these functions, then we would have to work out how to handle the
lifetime of the mapped pointer so as to not allow writes after
unmap. Perhaps a new object is returned that "unmap" is called on,
instead of relying on unmapBuffer?

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8467532 [details] [diff] [review]
WebGL2 - Implement MapBufferRange

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

::: dom/bindings/Bindings.conf
@@ -1557,5 @@
>      'nativeType': 'mozilla::WebGL2Context',
>      'headerFile': 'WebGL2Context.h',
> -    'resultNotAddRefed': [ 'canvas', 'getContextAttributes', 'getExtension',
> -                           'getAttachedShaders' ],
> -    'implicitJSContext': [ 'getSupportedExtensions' ],

Not sure why this is changed.

::: dom/canvas/WebGL2ContextBuffers.cpp
@@ +29,5 @@
> +{
> +    retval.set(nullptr);
> +    WebGLRefPtr<WebGLBuffer>* bufferSlot = GetBufferSlotByTarget(target, "mapBufferRange");
> +    if (!bufferSlot)
> +        return;

INVALID_ENUM?

@@ +33,5 @@
> +        return;
> +    WebGLBuffer* buffer = *bufferSlot;
> +
> +    MakeContextCurrent();
> +    void* data = gl->fMapBufferRange(target, offset, length, access);

Spec-as-written, we don't need to check ourselves if offset+length is shorter than the buffer size. I believe it should be very cheap to do so, and so probably worth it?

Also, shouldn't we check if the buffer is already mapped?

@@ +48,5 @@
> +    if (!bufferSlot)
> +        return nullptr;
> +
> +    WebGLBuffer* buffer = *bufferSlot;
> +    buffer->ForgetMapping(cx);

Shouldn't we check if the buffer is mapped at all?

::: gfx/gl/GLContext.cpp
@@ +1086,5 @@
>                  ClearSymbols(coreSymbols);
>              }
>          }
>  
> +        if (IsSupported(GLFeature::map_buffer_range)) {

Split this GLContext code off into another patch, so we can just land it.

Comment 3

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8467532 [details] [diff] [review]
> WebGL2 - Implement MapBufferRange
> 
> Review of attachment 8467532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ -1557,5 @@
> >      'nativeType': 'mozilla::WebGL2Context',
> >      'headerFile': 'WebGL2Context.h',
> > -    'resultNotAddRefed': [ 'canvas', 'getContextAttributes', 'getExtension',
> > -                           'getAttachedShaders' ],
> > -    'implicitJSContext': [ 'getSupportedExtensions' ],
> 
> Not sure why this is changed.

I need to before to WebIDL expert, but this functions are defined on the WebGLContext that WebGL2Context implements. I guess I assumed that it didn't need to be added on the derived class too. Do we need to ask bz?

Comment 4

[u480271]

Attachment: Patch 1 - WebGL2 - MapBufferRange symbols.

Comment 5

[u480271]

Attachment: Patch 2 - WebGL2 - Implement Experimental MapBufferRange.

Comment 6

[u480271]

Split patches apart as jgilbert requested.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8469780 [details] [diff] [review]
Patch 2 - WebGL2 - Implement Experimental MapBufferRange.

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

This needs review for the JS stuff.

::: dom/canvas/WebGL2ContextBuffers.cpp
@@ +29,5 @@
> +{
> +    retval.set(nullptr);
> +    WebGLRefPtr<WebGLBuffer>* bufferSlot = GetBufferSlotByTarget(target, "mapBufferRange");
> +    if (!bufferSlot)
> +        return;

Shouldn't this be INVALID_ENUM?

@@ +45,5 @@
> +WebGL2Context::UnmapBuffer(JSContext* cx, GLenum target)
> +{
> +    WebGLRefPtr<WebGLBuffer>* bufferSlot = GetBufferSlotByTarget(target, "unmapBufferRange");
> +    if (!bufferSlot)
> +        return nullptr;

Shouldn't this be INVALID_ENUM?
`false` not `nullptr`.

@@ +48,5 @@
> +    if (!bufferSlot)
> +        return nullptr;
> +
> +    WebGLBuffer* buffer = *bufferSlot;
> +    buffer->ForgetMapping(cx);

Shouldn't we check if it's mapped, first?

@@ +58,5 @@
> +void
> +WebGL2Context::FlushMappedBufferRange(GLenum target, GLintptr offset, GLsizeiptr length)
> +{
> +    MakeContextCurrent();
> +    gl->fFlushMappedBufferRange(target, offset, length);

It seems to me that we should be doing validation here:
* Is the target valid?
* Is offset non-negative?
* Is offset+length past the end of the buffer?
* Is the buffer range currently mapped?

::: dom/canvas/WebGLBuffer.cpp
@@ +99,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(WebGLBuffer)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(WebGLBuffer)
> +    tmp->mMappedArrayBuffer = nullptr;

Someone who knows better needs to review this.
Or we need a comment here explaining what's going on.

Comment 8

[u480271]

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/e00274c3f800
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/bab79974d14c

Comment 9

[u480271]

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/069d7461eed4
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/8966e5e9175f

Comment 10

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/037d0c1640ae

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/037d0c1640ae

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8469780 [details] [diff] [review]
Patch 2 - WebGL2 - Implement Experimental MapBufferRange.

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

::: dom/canvas/WebGL2ContextBuffers.cpp
@@ +24,5 @@
>  }
> +
> +void
> +WebGL2Context::MapBufferRange(JSContext* cx, GLenum target, GLintptr offset,
> +                              GLsizeiptr length, GLbitfield access, JS::MutableHandleObject retval)

I assume this is a moderately prototypal patch, but https://www.khronos.org/opengles/sdk/docs/man3/html/glMapBufferRange.xhtml suggests to me that |access| requires some sort of sanity validation to avoid the "the result is undefined and system errors (possibly including program termination) may occur" possibilities mentioned, as well as all the "may not be used" restrictions.

@@ +37,5 @@
> +    void* data = gl->fMapBufferRange(target, offset, length, access);
> +    if (!data)
> +        return;
> +
> +    retval.set(buffer->CreateMappingFor(cx, data, length));

If this isn't created, shouldn't you perform some sort of manual, immediate unmapBuffer so as not to leak the memory?

::: dom/webidl/WebGL2RenderingContext.webidl
@@ +535,3 @@
>      /* TODO:
>       *  - Consider adding getBufferSubData replacing MapBufferRange, etc.
>       *  - Figure out what to do abouT glMapBufferRange, glFlushMappedBufferRange, glUnmapBuffer

I assume that the addition of these methods means this comment should be removed?  If not, at least fix "abouT".

@@ +541,5 @@
>       *  - Deliberately not exposing glGetProgramBinary, glProgramBinary, glProgramParameteri
>       */
> +    ArrayBuffer? mapBufferRange(GLenum target, GLintptr offset, GLsizeiptr length, GLbitfield access);
> +    boolean unmapBuffer(GLenum target);
> +    void flushMappedBufferRange(GLenum target, GLintptr offset, GLsizeiptr length);

This is not *necessarily* a worrisome API, but the way you've implemented it here, I'm pretty certain it is.  What do you think should happen here?  What do you *want* to happen here?

   var canvas = document.createElement("canvas");
   var gl = canvas.getContext("webgl");
   var ab = gl.mapBufferRange(...); // assume this actually returns an ArrayBuffer, not null

   window.postMessage([ab], "*", [ab]); // neuters ab

   gl.flushMappedBufferRange(...); // with ... corresponding to before
   gl.unmapBuffer(...); // still corresponding

This interface *could* be implemented in such a way that the neutering operation implicitly performed by the postMessage line, causes the flushMappedBufferRange/unmapBuffer calls to be no-ops.  But it doesn't appear to be implemented that way.  All this patch does is take random memory, stuff it into an ArrayBuffer, assume it lives forever, then unmap it when the WebGL call indicating doneness happens.  And because the ArrayBuffer in question is returned to script, it's impossible to prevent that neutering operation from happening.

If the goal here is that you can create an ArrayBuffer, then arbitrarily write into it in JS with occasional explicit flushes using flushMappedBufferRange, *and* those flushes make a full copy of the buffer (or at least the selected range of it -- I didn't look super-closely to figure out exactly the API presented, mostly only looked at the ArrayBuffer side of things), then you can probably get away with roughly what you have -- *but* you need a check in flushMappedBufferRange that the buffer isn't neutered.  And again in unmapBuffer, if the operations there aren't idempotent as appears to be the case.

::: js/src/vm/ArrayBufferObject.cpp
@@ +713,5 @@
> +    JS_ASSERT(obj->getClass() == &class_);
> +    JS_ASSERT(!gc::IsInsideNursery(obj));
> +
> +    obj->initialize(nbytes, contents, DoesntOwnData);
> +    obj->setIsMappedArrayBuffer();

Off the top of my head, I'm not certain whether these particular calls are enough to create an ArrayBuffer backed by particular memory, where said memory *will not be touched* on GC of the ArrayBuffer or on neutering/transferring of the ArrayBuffer.  Given the other issues I've mentioned, I'm not going to investigate too deeply.

Comment 13

[Jeff Walden [:Waldo]]

Oh, apologies for taking so long to get to this.  :-(  Too many reviews these days, atop actually fixing bugs and implementing features of my own...

Comment 14

[Jeff Walden [:Waldo]]

sfink: we herd u liek ArrayBuffers!!cos(0)!!

The current claim is that this is not a super-high priority thing, so no need to panic yet.  But while a conversation just now is still fresh in mind, I'm going to braindump some.

Once this becomes a Thing Needed, after discussion with jgilbert, I think we might want to do this as a custom kind of ArrayBufferObject::BufferContents.  The contents are ultimately owned by the graphics card, and may be referred to by a single ArrayBuffer at a time.  At some point it seems like we'd want the ability to pass these mapped memory bits to workers or so, so it's probably not feasible to just have WebGL hold onto a reference to the ArrayBuffer for neutering purposes.

If an unmapBuffer(...) happens that causes an ArrayBuffer to become invalid, then *somehow* the WebGL implementation can work backwards from the BufferContents encapsulating that memory, to the views, and the ArrayBuffer that encapsulates the memory.  Then it can neuter them all.  Not sure how that should work, exactly, as all our tracking code currently depends upon an ArrayBufferObject* being the owner.

If the user deliberately neuters the ArrayBuffer returned by these calls, then that would simply disconnect the ArrayBuffer from the BufferContents owned by the graphics code, and then WebGL could as a spec matter cut off the mapping created here in response to that.  This could be eager, or it could be a lazy thing happening when unmapBuffer(...) or flushMappedBufferRange(...) happens.

An additional consideration to all of this.  While WebGL can expose a read/write ArrayBuffer here, exposing it that way requires a full copy for each flushMappedBufferRange(...) -- not what people want.  Really they want either a read-only buffer, or a write-only (!) buffer.  At one time (apparently not now) ES6 actually had a concept of read-only views, corresponding to frozen (?) typed arrays.  So in theory that could map to a concept already imagined to be standards-trackable.  I don't believe it had anything like write-only views.  So this will definitely require some path-breaking in terms of implementation, and probably in terms of spec language, to make work.

Assuming any of this happens, this way, at least.

Comment 15

[Ethan Lin[:ethlin]]

Is there any work need to be done?

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Ethan Lin[:ethlin] from comment #15)
> Is there any work need to be done?

Let's leave this for now.

Comment 17

[u480271]

I'm no longer working on WebGL

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jgilbert, maybe it's time to close this bug?

Comment 19

[Kelsey Gilbert [:jgilbert]]

Let's close this for now. We might return to it later.