1200011 - Gather information about WebGL resource, like WebGLTexture or WebGLBuffer

Status: RESOLVED INVALID

(DevTools Graveyard :: Canvas Debugger, enhancement, P3)

Description

[Ying Ruei Liang [:KK]]

Since there's already an actor for WebGL, shader and shader program, this feature will not be hard to achieve. I've hack some code to retrieve the URL source of uploaded image and it works well.

The goal is to get information, like [0]WebGL inspector, from WebGL front. Then figure out if it's possible to reupload texture dynamically without refresh the page or other useful features.

[0] https://github.com/benvanik/WebGL-Inspector

Comment 1

[Ying Ruei Liang [:KK]]

Attachment: firstTry

Here's my modification that creating TextureActor for devtools' need. Basically, I follow the structure which ProgramActor does so it's not too mess.
I only wrapped part of WebGL function calls on this stage and need some feedbacks before I wrap them all. Thanks.

Comment 2

[Victor Porof [:vporof][:vp]]

I'll take a look at this asap.

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8655260 [details] [diff] [review]
firstTry

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

Good start. See comments below.

::: toolkit/devtools/server/actors/webgl.js
@@ +23,5 @@
> +  // buffer
> +  src: "string",
> +  // The buffer used for creating texture, could be null if texture was generated
> +  // from Image html element
> +  buffer: "array:number",

I suggest renaming these as srcURL and srcBuffer, if you really want to maintain type consistency. This way it's clear that sometimes one is used instead of the other for a texture source.

Additionally, texImage2D can receive an ImageData object which isn't an Image nor a WebGLBuffer. Which one of these properties should we use in that case? What's the `buffer` property's expected internal format here? Packed rgba8? Should explain all of this in the comment.

@@ +27,5 @@
> +  buffer: "array:number",
> +  // The original image dimensions, in the form below:
> +  //   width: xxx
> +  //   height: xxx
> +  size: "json",

Nit: should just have width and height here as separate numbers.

@@ +43,5 @@
> +   *
> +   * @param DebuggerServerConnection conn
> +   *        The server connection.
> +   * @param uint32 texID
> +   *        The argument used when client call texImage2D.

s/client call/calling

@@ +45,5 @@
> +   *        The server connection.
> +   * @param uint32 texID
> +   *        The argument used when client call texImage2D.
> +   * @param WebGLProxy proxy
> +   *        The proxy methods for the WebGL context owning this shader.

The `cache` param doesn't have documentation here.

I only see `texID` used in this actor? Why the need for `linkedCache` and `linkedProxy`?

@@ +369,5 @@
> +  getTexImage2Ds: method(function() {
> +    // TODO Find out if we need these two line
> +    //let id = ContentObserver.GetInnerWindowID(this.tabActor.window);
> +    //return this._programActorsCache.filter(e => e.ownerWindow == id);
> +    return this._texImage2DActorCache;

Absolutely. Even in case of content navigation, the same webgl actor keeps being used. You don't want to mix up texture actors from one page with another.

@@ +744,5 @@
> +   * @param WebGLProxy proxy
> +   *        The proxy methods for the WebGL context initiating this call.
> +   */
> +  texImage2D: function(glArgs, glResult, cache, proxy) {
> +    let texID = cache.currentBindedTexture;

Should handle the case when texID is null. It might happen that texImage2D is called even when no texture is actually bound.

s/currentBindedTexture/currentBoundTexture/

Furthermore, we should absolutely *not* work directly with content xray wrapped objects here, let alone setting properties on them, like you do (e.g. texId.foo = bar). Follow the pattern used by programs. Have an `addProgram` method that creates a wrapped object around the content object, on which additional properties may be set.

@@ +749,5 @@
> +    let source = (glArgs.length === 9) ? glArgs[8] : glArgs[5];
> +    let width = (glArgs.length === 9) ? glArgs[3] : source.naturalWidth || source.width;
> +    let height = (glArgs.length === 9) ? glArgs[4] : source.naturalHeight || source.height;
> +
> +    /* XXX Place this here for now */

?

@@ +763,5 @@
> +    // filter out that case and don't emit event.
> +    emit(this, "texImage2D-registered", texID, cache, proxy);
> +  },
> +
> +  bindTexture: function(glArgs, glResult, cache, proxy) {

Documentation for all methods please.

@@ +774,5 @@
> +    cache.currentBindedTexture = texID;
> +  },
> +
> +  texParameter_: function(glArgs, glResult, cache, proxy) {
> +    // TODO Check if type of binded texture is the same as glArgs[0]

s/binded/bound here and everywhere else.

@@ +1118,5 @@
>        uniforms: new Map() // keys are WebGLUniformLocations (objects)
>      });
>    },
>  
> +  setGLTextureType: function(texType, texID) {

No need for this method. You're not calling an observed context function.

@@ +1122,5 @@
> +  setGLTextureType: function(texType, texID) {
> +    texID.type = texType;
> +  },
> +
> +  setGLTextureParameter: function(texID, parameter, value) {

No need for this method.

@@ +1130,5 @@
> +
> +    texID.parameters[parameter] = value;
> +  },
> +
> +  setGLTextureDimension: function(texID, width, height) {

No need for this method.

@@ +1135,5 @@
> +    texID.width = width;
> +    texID.height = height;
> +  },
> +
> +  setGLTextureSource: function(texID, source) {

No need for this method.

@@ +1492,5 @@
>      return error;
>    },
>  
>    /**
> +   * Changes a shader's source code and relinks the respective program.

This comment is outdated.

@@ +1503,5 @@
> +   *        The new shader source code.
> +   * @return object
> +   *         An object containing the compilation and linking status.
> +   */
> +  _texImage2D: function() {

Why is this method here?

Comment 4

[yliang]

Attachment: Create TextureActor that store basic texture info

Except improving and fixing previous problems, I also
- store complete properties of WebGLTexture to texture actor
- store partial properties of WebGLTexture's mipmap to texture actor
- emit event while `deleteTexture` is required

Now the method I use for retrieving pixel data of texture is by creating a framebuffer, draw texture on it then read its pixels out. So I don't need to track every texSubImage request for texture modification. But this method could not retrieve pixel data of mipmaps due to the limitation of WebGL [0].

[0] https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf#nameddest=section-4.4.3

Comment 5

[Ying Ruei Liang [:KK]]

Attachment: Create BufferActor that store buffer info

Here's the actor for WebGLBuffer.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8662191 [details] [diff] [review]
Create TextureActor that store basic texture info

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

::: toolkit/devtools/server/actors/webgl.js
@@ +33,5 @@
> +  // Name of srcBuffer's format, could be Uint8Array, float32Array, etc.
> +  srcBufferFormat: "string",
> +
> +  // Uint8Array that store current pixel data this texture present.
> +  pixels: "array:number",

Might be ClampedUint8Array, but I don't think we care so much.

Here's a thought: would it make sense to send this over the protocol as a Uint32Array, for compactness? This way we avoid serializing 4 times more numbers.

@@ +35,5 @@
> +
> +  // Uint8Array that store current pixel data this texture present.
> +  pixels: "array:number",
> +
> +  // GL texture target, could be gl.TEXTURE_2D or gl.TEXTURE_CUBE_MAPs.

Nit: Update this comment to say 'target type', not just 'target'. The target itself would be a buffer id number.

@@ +38,5 @@
> +
> +  // GL texture target, could be gl.TEXTURE_2D or gl.TEXTURE_CUBE_MAPs.
> +  target: "number",
> +
> +  // The dimensions of this texture, in pixel.

Nit: 'pixels'

@@ +121,5 @@
> +      mipmapsToReturn[mipmap.level] = {
> +        srcURL: mipmap.srcImage ? mipmap.srcImage.src : null,
> +        srcBuffer: mipmap.srcBuffer,
> +        srcBufferFormat: mipmap.srcBufferFormat,
> +        pixels: [], // TODO: How to get this?

Can't afaik. Might be wrong.

@@ +432,5 @@
>    }),
>  
>    /**
> +   * Gets an array of cached texture actors for the current tab actor's window.
> +   * This is useful for dealing with bfcache, when no new programs are linked.

The "when no new programs are linked" part doesn't make sense for this particular case. We're dealing with textures.

@@ +569,5 @@
> +   */
> +  _onTextureRemoved: function(...args) {
> +    let [textureObject] = args;
> +    let textureActorToRemove = this._textureActorsCache.filter(actor =>
> +      actor.textureObject == textureObject);

Need a test for this especially, but everything else too.

@@ +573,5 @@
> +      actor.textureObject == textureObject);
> +
> +    // Remove that textureActor from cache
> +    this._textureActorsCache = this._textureActorsCache.filter(actor =>
> +      actor.textureObject != textureObject);

Use `removeFromArray` defined in this file instead, to avoid creating a new array.

@@ +850,5 @@
> +
> +    // No current bound texture
> +    if (!textureObject) {
> +      return;
> +    }

If this function is being called with bad arguments, in other words – if the context throws an error because of the current state or the arguments themselves, we shouldn't register a new texture here. Need to handle and test this scenario as well.

@@ +865,5 @@
> +    }
> +
> +    cache.setTextureProp(proxy, textureObject, source, target, width, height, format, type);
> +
> +    // Don't emit event if this texture has been tracked

Hmm, why not? Maybe emit a different event here? How would the frontend know to update itself when this happens otherwise?

@@ +964,5 @@
> +
> +    // Don't emit event if this texture has been tracked
> +    if (!isTracked) {
> +      emit(this, "texture-uploaded", textureObject, cache, proxy);
> +    }

Lots of code duplication between `texImage2D`, `compressedTexImage2D` and `copyTexImage2D`. Maybe we can consolidate these three methods?

@@ +1343,5 @@
> +   *        The texture for which the properties to be cached.
> +   */
> +  addTexture: function(texture) {
> +    this._textures.set(texture, {
> +      srcImage: null,

There's a few properties you haven't initialized, like srcBuffer and srcBufferFormat. Either init all of them or none at all unless you have to.

@@ +1391,5 @@
> +    let data = null;
> +    let dataFormat = null;
> +
> +    // TODO: Need better way to identify object type
> +    if (source && source.src) { // HTMLImageElement

source instanceof HTMLImageElement
source instanceof ImageData
source.buffer && source.buffer instanceof ArrayBuffer

@@ +1454,5 @@
> +    } else if (source && source.data) { // ImageData
> +      data = source.data;
> +    } else { // TypedArray
> +      data = source;
> +    }

See above.

@@ +1844,5 @@
> +  _getBoundTexture: function(target) {
> +    let gl = this._gl;
> +
> +    switch (target) {
> +    case gl.TEXTURE_2D:

Nit: add tab before each case.

@@ +2011,5 @@
> +
> +  let constructorName = array.constructor.toString();
> +  if (!constructorName) {
> +    return null;
> +  }

Much better idea to case/switch instanceof here instead of mucking with constructor names.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8664089 [details] [diff] [review]
Create BufferActor that store buffer info

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

::: toolkit/devtools/server/actors/webgl.js
@@ +21,5 @@
> +types.addDictType("bufferInfo", {
> +  // The array used by this buffer.
> +  data: "array:number",
> +
> +  // The GL buffer target, could be gl.ARRAY_BUFFER or gl.ELEMENT_ARRAY_BUFFER.

Again, "target type" here, not just "target".

@@ +499,5 @@
> +  _onBufferRemoved: function(...args) {
> +    let [bufferObject] = args;
> +    let bufferActorToRemove = this._bufferActorsCache[this._bufferActorsCache.indexOf(bufferObject)];
> +
> +    // Remove bufferActor from cache

Useless comment.

@@ +501,5 @@
> +    let bufferActorToRemove = this._bufferActorsCache[this._bufferActorsCache.indexOf(bufferObject)];
> +
> +    // Remove bufferActor from cache
> +    this._bufferActorsCache = this._bufferActorsCache.filter(actor =>
> +      actor.bufferObject != bufferObject);

Use `removeFromArray`.

@@ +806,5 @@
> +
> +    // Don't emit event if this buffer has been tracked
> +    if (!isTracked) {
> +      emit(this, "buffer-uploaded", bufferObject, cache, proxy);
> +    }

See question from previous review.

@@ +834,5 @@
> +    // No bound buffer
> +    if (!bufferObject) {
> +      return;
> +    }
> +

Need to also handle the case when the buffer isn't tracked here.

@@ +1176,5 @@
> +   */
> +  addBuffer: function(buffer, target, data, usage) {
> +    this._buffers.set(buffer, {
> +      target: target,
> +      data: Cu.cloneInto(data, {}), // Deep copy

Object.create(null) instead of {} please.

Comment 8

[Ying Ruei Liang [:KK]]

Attachment: BufferActor and related tests

I've though about the "removed" event and decide don't use it. The usage is limited since WebGL delete unused object automatically and that can't be caught in any way. The solution I use is making a function that could check if the WebGLBuffer still exists by requesting gl.getBufferParameter(target, gl.BUFFER_SIZE). If the buffer no longer exists, it return null, otherwise its size in byte.

I think the ability to know if the resource still alive is necessary, but we can still discus the best way to achieve this.

BTW, I'll rename these test cases after it's ready to be reviewed.

Comment 9

[Ying Ruei Liang [:KK]]

I also have some design problem.

Take textureActor for instance, currently I decide to only record the first source for creating texture, the other changes wouldn't be recorded and here comes some problems.

I found that the implementation of WebGL-inspector is to capture the "calls history with arguments" but the changes of data. It's clever but I'm not sure if this fit our need.

Any idea or comment?

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8674799 [details] [diff] [review]
BufferActor and related tests

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

Very nice and clean patch! Kudos!

::: toolkit/devtools/server/actors/webgl.js
@@ +25,5 @@
> +
> +  // The WebGLBuffer target type, could be gl.ARRAY_BUFFER or gl.ELEMENT_ARRAY_BUFFER.
> +  target: "number",
> +
> +  // The WebGLBuffer usage type, for example gl.STATIC_DRAE.

typo: DRAW

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Ying Ruei Liang [:KK] from comment #9)
> I also have some design problem.
> 
> Take textureActor for instance, currently I decide to only record the first
> source for creating texture, the other changes wouldn't be recorded and here
> comes some problems.
> 
> I found that the implementation of WebGL-inspector is to capture the "calls
> history with arguments" but the changes of data. It's clever but I'm not
> sure if this fit our need.
> 
> Any idea or comment?

I'm not sure I understand the design problem here. Could you elaborate a bit more?

Comment 12

[Ying Ruei Liang [:KK]]

In other word, will "save all WebGL calls about resources used in one frame" needed to reach our goal?

For instance, a WebGL buffer could be bound and applied bufferSubData in certain frame. My current idea is to save all these calls with arguments which relate to this buffer in this frame, so we could provide a snapshot about what happened to this buffer in certain frame.

Comment 13

[Ying Ruei Liang [:KK]]

Hi, victor.

I've struggled with finding a way to pass properties of an object from actor to front for a while. Now I have a rough idea about what I could do but since I'm not deeply familiar with codebase of devtools, I think I need some technical advice in case I break something by accident.

The problem I want to solve here is similar to Bug 978957: I should use ObejctActor to pass arguments of function calls, so the client could get more useful information. Since we should provide some functions and variables to use ObjectActor, I have two ideas for this:

1. Maintain another actor pool and other requirements inside WebGLActor, or
2. Use those which owned by WebConsolePanel

I think solution 1 would work, except we might duplicate some actors.
Solution 2 would solve the duplication, but I'm not sure if it's allowed to do interaction between tabs like that, and have no idea if this solution would work. 

What do you think?

PS. I found that we use regular array rather than ActorPool to store actors inside WebGLActor, is this meant to be?

Comment 14

[Victor Porof [:vporof][:vp]]

Putting a hold on this until we figure things out; let's wait and see what the plan is for game tools moving forward.

Comment 15

[BMO Automation]

The code behind DevTools:Canvas Debugger has gone away. Closing this bug as INVALID