WebGL section initial content. Updated the examples. by astojilj · Pull Request #155 · w3c/mediacapture-depth

astojilj · 2017-03-03T08:29:15Z

Renoves the splitting depth to red and green component.
@anssiko @huningxin PTAL

anssiko · 2017-03-03T08:48:58Z

depthTexture is undefined, can we define its creation for completeness?

astojilj · 2017-03-03T09:05:25Z

depthTexture is undefined, can we define its creation for completeness?
Fixed in updated PR.

anssiko · 2017-03-03T10:08:17Z

@kenchris please review.

kenchris · 2017-03-03T10:49:38Z

          <div class="note">
            This section is currently work in progress, and subject to change.
          </div>
+          <p>


I would do

There are several use-cases which are a good fit to be, at least partially, implemented on the GPU, such as motion recognition, pattern recognition, background removal, as well as 3D point cloud.

This section explains which APIs can be used for some of these mentioned use-cases; ...

kenchris · 2017-03-03T10:50:21Z

            containing a <a>depth stream track</a> may be uploaded to a WebGL
-            texture of format <code>RGB</code> and type
-            <code>UNSIGNED_BYTE</code>. [[WEBGL]]
+            texture of format <code>RGBA</code> or <code>RED</code> and type


See also sounds a bit weird to me. Maybe just remove also?

kenchris · 2017-03-03T10:50:47Z

+            "height: 2.225em; vertical-align: -0.778em;" class=
+            "mjx-vsize"></span></span></span></span></span></span></span>
+          </p>
+          <h3>


kenchris · 2017-03-03T10:51:23Z

+          <h3>
+            Read the data from WebGL texture
+          </h3>
+          <p>


Here we list some of the possible approaches. (s/:/. and remove space before ,)

in addition to what?

thanks. should have remove it. Now the content is only:
Here we list some of the possible approaches.

kenchris · 2017-03-03T10:52:44Z

+            We list here some of the possible approaches:In addition to , there
+            are two ways available for asynchronous access:
+          </p>
+          <ul>


You said async above and now talk about sync...

Do you mean: In addition to asynchronous access, there are ....

removed the mention of async above.

kenchris · 2017-03-03T10:53:09Z

+          </p>
+          <ul>
+            <li>Synchronous readPixels requires the least amount of code and it
+            is available om WebGL1. See the <a>readPixels from float</a>


for further details?

kenchris · 2017-03-03T10:53:48Z

+            is available om WebGL1. See the <a>readPixels from float</a>
+            example for details.
+            </li>
+            <li>Asynchronous readPixels using pixel buffer objects should


the -> any blocking?

waiting for the readPixels call to finish?

Replaced by: Asynchronous readPixels using pixel buffer objects to avoid blocking the readPixels call.

kenchris · 2017-03-03T10:54:10Z

+            <li>Asynchronous readPixels using pixel buffer objects should
+            remove the blocking while waiting for the readPixels call.
+            </li>
+            <li>Transform feedback with GetBufferSubData(Async) provides


spellign of synchronous

kenchris · 2017-03-03T10:54:25Z

+            remove the blocking while waiting for the readPixels call.
+            </li>
+            <li>Transform feedback with GetBufferSubData(Async) provides
+            synchronos and asynchronous read access to depth and color texture


in the vertex shader

kenchris · 2017-03-03T10:56:18Z

+            </li>
+          </ul>
+          <p></p>
+          <p class="note">


a bit hard to read.

The "readPixel from float" example above, while being simplistic, suffers from some performance issues, due to the fact that ... This can be mitigated by...

I wanted to say: blocking is not bad, it is only 3-6 ms on my development laptops.

But web vr and such use cases will be on entirely different hardware, so not sure that we can make such a statement

kenchris · 2017-03-03T10:56:52Z

-// This code sets up a video element from a depth stream, uploads it to a WebGL
-// texture, and samples that texture in the fragment shader, reconstructing the
-// 16-bit depth values from the red and green channels.
+// This code sets up a video element from a depth stream, uploads it to a WebGL2


holding -> containing?

kenchris · 2017-03-03T10:57:34Z

@@ -1256,31 +1344,78 @@ <h3>
  // handle gUM error here
 });



pretty useless comment, if people dont know that, they cannot understand any example here :) it is basic knowledge of webgl. Comments should make sense, or dont have them

ok, removed

kenchris · 2017-03-03T10:58:00Z

+      <p>
+        This example extends <a>upload to float texture</a> example.
+      </p>
+      <pre class="example">


attach the texture we upload the depth - sounds weird

Changed my mind about: "the texture we upload the depth video to" -> "the texture with depth data". Doesn't have the depth data when created.

Now:
// This code creates the texture to which we will upload the depth video frame.
// Then, it sets up a named framebuffer, attach the texture as color attachment

kenchris · 2017-03-03T10:58:08Z

+        This example extends <a>upload to float texture</a> example.
+      </p>
+      <pre class="example">
+// This code sets up a named framebuffer, attach the texture we upload the depth


AT rendering time

thanks. all replaced by:
// This code creates the texture to which we will upload the depth video frame.
// Then, it sets up a named framebuffer, attach the texture as color attachment
// and, after uploading the depth video to the texture, reads the texture
// content to Float32Array.

kenchris · 2017-03-03T10:58:27Z

+// This code sets up a named framebuffer, attach the texture we upload the depth
+// video to as color attachment and, in rendering time, reads the texture
+// content  to Float32Array.
+


why the ... at the beginning, what purpose do they serve?

Don't have a clue. It was there before.
I am often comfortable accepting existing code approach and replicating it without understanding what is it for.

kenchris · 2017-03-03T10:59:32Z

+   gl.RED,
+   gl.FLOAT,
+   depthVideo);
+


const should work instead of var here... var is not recommended in JS now. const doesnt make the object const, only the reference which cannot be reassigned.

OK. It is better to pre-allocate the buffer outside rendering loop, and we can do it only after texImage2D and gl.getParameter(gl.IMPLEMENTATION_COLOR_READ_FORMAT) call.
That's why it is var declared somewhere else but initialized here.
It complicates the example so I will use const and put the comment.

astojilj · 2017-03-03T12:33:57Z

@kenchris
Thanks. Updated. PTAL.

kenchris · 2017-03-03T13:32:08Z

+            This section explains which APIs can be used for some of these
+            mentioned use-cases; the concrete usage examples are provided in
+            the <a href=
+            "https://www.w3.org/wiki/Media_Capture_Depth_Stream_Extension#Examples">


section below?

copying approach from other places, prefer not to have bellow together with link.

kenchris · 2017-03-03T13:32:22Z

+            "https://www.w3.org/wiki/Media_Capture_Depth_Stream_Extension#Examples">
+            Examples</a> section.
+          </p>
+          <h3>


a video frame, or video frames?

video frames, since there is a mention of loop. thanks.

kenchris · 2017-03-03T13:33:04Z

            A <a>video</a> element whose source is a <a>MediaStream</a> object
            containing a <a>depth stream track</a> may be uploaded to a WebGL
-            texture of format <code>RGB</code> and type
-            <code>UNSIGNED_BYTE</code>. [[WEBGL]]


Not BLUE? If not, explain why. if yes, then change the wording

There is no BLUE texture in webgl.

kenchris · 2017-03-03T13:33:58Z

+            "height: 2.225em; vertical-align: -0.778em;" class=
+            "mjx-vsize"></span></span></span></span></span></span></span>
+          </p>
+          <h3>


I am wondering whether people say "of a texture" - so probably check with a native speaker

from a texture memory, from a texture or read back the texture data is used. keeping the existing.

kenchris · 2017-03-03T13:34:57Z

+          <p>
+            Here we list some of the possible approaches.
+          </p>
+          <ul>


Synchronous calls to readPixels, which requires...

Synchronous readPixels usage. It is the same call as async, just different setup.

kenchris · 2017-03-03T13:36:01Z

+            Here we list some of the possible approaches.
+          </p>
+          <ul>
+            <li>Synchronous readPixels requires the least amount of code and it


om? works with WebGL 1.0 (that is the spec name :))

readPixels should be with right?

kenchris · 2017-03-03T13:37:06Z

+            </li>
+            <li>Asynchronous readPixels using pixel buffer objects to avoid
+            blocking the readPixels call.
+            </li>


I am not sure it is clear what "transform feedback" is, or whether it is one concept... can we add link or at least make it cursive or so

Links to khronos specifications added for both Transform feedback with GetBufferSubDatasync
Don't have a clue how to add informative reference to the bottom of the page [WEBGL2], @anssiko how to do that? Thanks

kenchris · 2017-03-03T13:38:28Z

 );
 </pre>
      <h3>
-        WebGL Fragment Shader based post-processing


Spec is called WebGL 2.0

kenchris · 2017-03-03T13:39:07Z

-// This code sets up a video element from a depth stream, uploads it to a WebGL
-// texture, and samples that texture in the fragment shader, reconstructing the
-// 16-bit depth values from the red and green channels.
+// This code sets up a video element from a depth stream, uploads it to a WebGL2


How is this depth map exposed, Uint16Array?

removed the comment.
depth map is 16 bit. It is not exposed.
The texture is float.

kenchris · 2017-03-03T13:40:13Z

 });

-// ... later, in the rendering loop ...
+var gl = canvas.getContext("webgl2");


Is that alwasy available in WebGL 2.0? if so I would write

Activate the standard WebGL 2.0 extension for using single component R32F texture format

kenchris · 2017-03-03T13:40:58Z

+var gl = canvas.getContext("webgl2");
+// Activate the extension to use single component R32F texture format.
+gl.getExtension('EXT_color_buffer_float');
+


Could be have this be in another box in the spec, so it is super clear that this is a separate part of the code

It is better if handled in your PR #154.

kenchris · 2017-03-03T13:41:13Z

-  }
-&lt;/script&gt;
 </pre>
+      <h3>


Replaced WebGL2 by WebGL.

kenchris · 2017-03-03T13:41:41Z

+      <p>
+        This example extends <a>upload to float texture</a> example.
+      </p>
+      <pre class="example">


This should not be in the code comments... check my former patch to see how it can be done as part of the example box

moved out to
. You are adding in PR #154 so it is better to handle it there for all. Thanks

kenchris · 2017-03-03T13:42:24Z

+// Then, it sets up a named framebuffer, attach the texture as color attachment
+// and, after uploading the depth video to the texture, reads the texture
+// content to Float32Array.
+


Sometimes it is better to define functions with descriptive names instead of comments. It also makes the code nicely scoped and easy to read

Please feel free to handle it in your PR #154. My goal with this patch is to replace the original example code - since it is not supported and need to be removed ASAP. Cosmetic changes will need to be handled in follow up.

kenchris · 2017-03-03T13:42:40Z

+// and, after uploading the depth video to the texture, reads the texture
+// content to Float32Array.
+
+// Initialize texture and framebuffer for reading back the texture...


kenchris · 2017-03-03T13:42:50Z

+gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE);
+gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST);
+gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.NEAREST);
+


let - it is supposed to be used in different scope from creation. feel free to refactor in your patch

kenchris · 2017-03-03T13:44:07Z

+   gl.RED,
+   gl.FLOAT,
+   depthVideo);
+


only once? I dont think the comment is clear

replaced by:
if (!buffer) {
buffer =
new Float32Array(depthVideo.videoWidth * depthVideo.videoHeight);
}

astojilj · 2017-03-04T07:10:17Z

PR updated.
@kenchris Thanks. I have incorporated the text changes and some of the suggested example code refactoring.
I think it is more productive if you continue improving the examples in PR #154 - here I covered just the high priority work: replace unsupported example code.

Open:
Links to khronos specifications added for both Transform feedback with GetBufferSubDatasync
Don't have a clue how to add informative reference to the bottom of the page [WEBGL2], @anssiko how to do that? Thanks

kenchris · 2017-03-06T12:22:52Z

Can you rebase this so it can get merged?

astojilj · 2017-03-06T12:44:56Z

Rebased. Thanks.

anssiko · 2017-03-06T16:10:59Z

@astojilj You can add your Khronos informative references to localBiblio, see https://github.com/w3c/mediacapture-depth/blob/gh-pages/index.src.html#L80

To make a reference informative in the prose, use [[WEBGL2]] i.e. without "!". [[!WEBGL2]] would make it normative.

Renoves the splitting depth to red and green component.

astojilj · 2017-03-06T18:36:48Z

@anssiko
Added [WEBGL2] and [WEBGL-GET-BUFFER-SUB-DATA-ASYNC].
This PR is updated and no open issues to merge it.

I will start adding [WEBGL2] and [WEBGL-GET-BUFFER-SUB-DATA-ASYNC] to SPECREF
as the usage of localBiblio is discouraged and submit another PR once ready.

anssiko · 2017-03-07T21:25:13Z

Thanks!

astojilj force-pushed the rg88cleanup branch from ca06ff9 to 8a4e509 Compare March 3, 2017 09:03

astojilj force-pushed the rg88cleanup branch from 8a4e509 to a365811 Compare March 3, 2017 09:27

kenchris reviewed Mar 3, 2017

View reviewed changes

astojilj force-pushed the rg88cleanup branch from a365811 to b66a489 Compare March 3, 2017 12:33

kenchris reviewed Mar 3, 2017

View reviewed changes

astojilj force-pushed the rg88cleanup branch from b66a489 to b279e45 Compare March 4, 2017 06:46

astojilj force-pushed the rg88cleanup branch from b279e45 to 96be1a8 Compare March 6, 2017 12:44

astojilj force-pushed the rg88cleanup branch from 96be1a8 to 60b04a3 Compare March 6, 2017 18:31

WebGL section initial content. Updated the examples.

d80f57e

Renoves the splitting depth to red and green component.

astojilj force-pushed the rg88cleanup branch from 60b04a3 to d80f57e Compare March 6, 2017 18:34

anssiko merged commit 36ff7ac into w3c:gh-pages Mar 7, 2017

Conversation

astojilj commented Mar 3, 2017

Uh oh!

anssiko commented Mar 3, 2017

Uh oh!

astojilj commented Mar 3, 2017

Uh oh!

anssiko commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astojilj Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astojilj Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

astojilj Mar 3, 2017 •

edited

Loading

astojilj Mar 3, 2017 •

edited

Loading

astojilj commented Mar 3, 2017 •

edited

Loading