search

w3c / mediacapture-depth

the Media Capture Depth Stream Extensions specification
https://w3c.github.io/mediacapture-depth/
Other
24 stars 20 forks source link

Fix #137: Define unknown depth map value #139

Closed anssiko closed 7 years ago

fluffy commented 7 years ago

This look right but do you need more? I would assume this might change some of the formulas in the draft

anssiko commented 7 years ago

@huningxin to review if changes are needed.

huningxin commented 7 years ago

Thanks for this PR.

However, IMO, this PR only changes the depth-to-grayscale of "The algorithm to convert the depth map value to grayscale". I suppose we need:

  1. define the 0 is the invalid value of depth value in depth map.
  2. define the invalid depth value case of the depth-to-grayscale conversion (as this PR does).
  3. define the invalid depth value case of the grayscale-to-depth conversion.
anssiko commented 7 years ago

@huningxin:

  1. define the 0 is the invalid value of depth value in depth map.

I attempted to address this in https://github.com/w3c/mediacapture-depth/pull/139/commits/fa43616ed0452062813629e094731f37803725f4.

  1. define the invalid depth value case of the grayscale-to-depth conversion.

Can you clarify what you mean with the grayscale-to-depth conversion? We currently only define depth-to-grayscale for the purpose of rendering depth stream in a video element: https://w3c.github.io/mediacapture-depth/index.src.html#dfn-convert-the-depth-map-value-to-grayscale

I've been away from this spec for some time, so I'm probably missing something obvious :-)

huningxin commented 7 years ago

Can you clarify what you mean with the grayscale-to-depth conversion?

I mean the NOTE section about "The depth measurement d (in meter units) is recovered by ......".

anssiko commented 7 years ago

I mean the NOTE section about "The depth measurement d (in meter units) is recovered by ......".

Given an invalid depth map value d_(16bit) = 0, the grayscale-to-depth normalize step returns 0:

d_(n) = d_(16bit) / 65535 = 0

Substituting d_(n) with 0 in the the conversion step:

d = (d_(n) * (far - near)) + near reduces to d = near

To check the operation is invertible in this special case, we substitute d with near in depth-to-grayscale conversion:

d_(n) = (d - near) / (far - near) reduces to d_(n) = 0

And so does d_(16bit):

d_(16bit) = floor(d_(n) * 65535) = 0

Q.E.D.

What type of additional text you'd prefer us to add to the NOTE section for this special case?

huningxin commented 7 years ago

I think we need tell the case that if the d_(16bit) is 0 (invalid), then the d_(n) is also 0 (the invalid value).

WDYT?

anssiko commented 7 years ago

@huningxin I added https://github.com/w3c/mediacapture-depth/pull/139/commits/8f753dcde55389f50ba2548b0908fab7a1b85f4b to make that case obvious. PTAL. Do you think we have now addressed this issue and can resolve and land?

huningxin commented 7 years ago

Sorry about the confusion, I made a mistake in https://github.com/w3c/mediacapture-depth/pull/139#issuecomment-271296132 by suing d_(n) where it should be d.

So it would be "If the d_(16bit) is 0, then the d is the invalid depth map value."

anssiko commented 7 years ago

@huningxin Updated, does https://github.com/w3c/mediacapture-depth/pull/139/commits/c91cb79eb138a0cc36c42468b610e2ef807ddb63 look better? Moved this into recovery steps proper.

huningxin commented 7 years ago

LGTM. Thanks for the PR!