Media Capture Depth Stream Extensions API: focal length and depth range.

Media Capture Depth Stream Extensions API: focal length and depth range.

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackSettings {
    double depthNear;
    double depthFar;
    double focalLengthX;
    double focalLengthY;
};

This patch adds the support for it to fake capture device only.

[1]
https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary

BUG=616098
Review-Url: https://codereview.chromium.org/2606983002
Cr-Commit-Position: refs/heads/master@{#448567}
Committed: https://chromium.googlesource.com/chromium/src/+/9e1e3f73fc41ca1f8a5d42b3a418c286c2e39fac

[aleksandar.stojiljkovic, 2016-12-28 21:33:21]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 21:33:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-28 21:42:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 21:42:24]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[aleksandar.stojiljkovic, 2016-12-28 21:49:07]

Patchset #1 (id:1) has been deleted

[aleksandar.stojiljkovic, 2016-12-28 21:49:41]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 21:49:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-28 22:22:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 22:22:08]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[aleksandar.stojiljkovic, 2016-12-28 23:29:11]

Patchset #2 (id:40001) has been deleted

[aleksandar.stojiljkovic, 2016-12-29 08:18:19]

Patchset #2 (id:60001) has been deleted

[aleksandar.stojiljkovic, 2016-12-29 08:18:30]

Patchset #1 (id:20001) has been deleted

[aleksandar.stojiljkovic, 2016-12-29 09:20:21]

Patchset #1 (id:80001) has been deleted

[aleksandar.stojiljkovic, 2016-12-29 11:02:12]

Description was changed from

==========
Media Capture Depth Stream Extensions API: focal length and depth range.

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackSettings {
    double near;
    double far;
    double focalLengthX;
    double focalLengthY;
};

This patch adds the support for it to fake capture device only.

[1]
https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary

BUG=616098
==========

to

==========
Media Capture Depth Stream Extensions API: focal length and depth range.

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackSettings {
    double depthNear;
    double depthFar;
    double focalLengthX;
    double focalLengthY;
};

This patch adds the support for it to fake capture device only.

[1]
https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary

BUG=616098
==========

[aleksandar.stojiljkovic, 2016-12-29 11:02:33]

Patchset #1 (id:100001) has been deleted

[aleksandar.stojiljkovic, 2016-12-29 11:38:58]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 11:39:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleksandar.stojiljkovic, 2016-12-29 13:22:35]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ kinuko@chromium.org, mcasas@chromium.org, tommi@chromium.org

[aleksandar.stojiljkovic, 2016-12-29 13:22:37]

mcasas@chromium.org: Please review changes in 
content/browser/renderer_host/media/video_capture_manager.*
media/capture/video/*
content/renderer/media/media_stream_video_track.cc

kinuko@chromium.org: Please review changes in 
third_party/WebKit/Source/platform/*
third_party/WebKit/public/*

tommi@chromium.org: Please review changes in 
third_party/WebKit/Source/modules/mediastream/*
content/browser/*

Note: Current version of spec names fields as "near" and "far".
I have just submitted pull request [1] to the spec renaming near/far to
depthNear/depthFar.
The reason is that generated Blink modules and bindings code cannot compile on
Windows as
near and far are reserved keywords in MS compiler. It might be possible to undef
them in
C++ headers but it seems more appropriate to rename them for the reasons
explained in [2].
This patch won't get submitted until the renaming in spec is done.
However, the patch here is ready for review. 

[2]
https://github.com/w3c/mediacapture-depth/pull/148

[commit-bot: I haz the power, 2016-12-29 14:32:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 14:32:44]

Dry run: This issue passed the CQ dry run.

[aleksandar.stojiljkovic, 2017-01-02 11:38:14]

On 2016/12/29 13:22:37, aleksandar.stojiljkovic wrote:
> mailto:mcasas@chromium.org: Please review changes in 
> content/browser/renderer_host/media/video_capture_manager.*
> media/capture/video/*
> content/renderer/media/media_stream_video_track.cc
> 
> mailto:kinuko@chromium.org: Please review changes in 
> third_party/WebKit/Source/platform/*
> third_party/WebKit/public/*
> 
> mailto:tommi@chromium.org: Please review changes in 
> third_party/WebKit/Source/modules/mediastream/*
> content/browser/*
> 
> Note: Current version of spec names fields as "near" and "far".
> I have just submitted pull request [1] to the spec renaming near/far to
> depthNear/depthFar.
The spec pull request got merged.
PTAL.

[mcasas, 2017-01-05 01:09:32]

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1746: }
No {} for one-line bodies.

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1748: if
(stream_type == MEDIA_DEVICE_VIDEO_CAPTURE) {
if (stream_type != MEDIA_DEVICE_VIDEO_CAPTURE)
  return devices;
// the rest...

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:385: // Creates
MediaStreamDevices for |devices_info| of |stream_type|. For video
s/devices_info/device_infos/

https://codereview.chromium.org/2606983002/diff/120001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/2606983002/diff/120001/content/common/media/m...
content/common/media/media_stream_messages.h:61:
IPC_STRUCT_TRAITS_MEMBER(device.camera_calibration.valid)
o_0  all this communication between browser
and renderer hasn't been migrated to mojo yet?

https://codereview.chromium.org/2606983002/diff/120001/content/public/common/...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/2606983002/diff/120001/content/public/common/...
content/public/common/media_stream_request.h:181: };
content/public/ can pull data types out of media/, so
you can just reuse the CameraCalibration from there,
right?  No need to redefine it here.

https://codereview.chromium.org/2606983002/diff/120001/content/public/common/...
content/public/common/media_stream_request.h:184: CameraCalibration
camera_calibration;
If this field is not always present we probably want to use
base::Optional (see [1]).

[1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md

https://codereview.chromium.org/2606983002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2606983002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:297: if (source_) {
Not your fault but what about:

  // TODO(hta): Extract the real value.
  settings.deviceId = blink::WebString("video device ID");
  if (!source_)
    return;

  // The rest, including your new stuff.

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/fa...
media/capture/video/fake_video_capture_device_factory.cc:90: if
(device_descriptors->size() > kDepthDeviceIndex) {
  if (device_descriptors < kDepthDeviceIndex)
    return;

which should cover the vast majority of uses.

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:101: friend class
base::RefCountedThreadSafe<CameraCalibration>;
I fail to see why should CameraCalibration be ref counted, 
since it's just a POD.

[aleksandar.stojiljkovic, 2017-01-09 18:47:01]

Patch Set 2 : #23 fix - base::Optional and nits, thanks mcasas@.

base::Optional required change in IPC handling of CameraCalibration struct
(media_stream_messages.h and media/base/ipc/media_param_traits.*).

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1746: }
On 2017/01/05 01:09:32, mcasas wrote:
> No {} for one-line bodies.

Done.

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1748: if
(stream_type == MEDIA_DEVICE_VIDEO_CAPTURE) {
On 2017/01/05 01:09:32, mcasas wrote:
> if (stream_type != MEDIA_DEVICE_VIDEO_CAPTURE)
>   return devices;
> // the rest...

Done.

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2606983002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:385: // Creates
MediaStreamDevices for |devices_info| of |stream_type|. For video
On 2017/01/05 01:09:32, mcasas wrote:
> s/devices_info/device_infos/

Done.

https://codereview.chromium.org/2606983002/diff/120001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/2606983002/diff/120001/content/common/media/m...
content/common/media/media_stream_messages.h:61:
IPC_STRUCT_TRAITS_MEMBER(device.camera_calibration.valid)
On 2017/01/05 01:09:32, mcasas wrote:
> o_0  all this communication between browser
> and renderer hasn't been migrated to mojo yet?

Apparently not. I couldn't find a task similar to Issue 584797 (Mojify Video
Capture) for this in crbug.com - don't know if someone is already working on it.

https://codereview.chromium.org/2606983002/diff/120001/content/public/common/...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/2606983002/diff/120001/content/public/common/...
content/public/common/media_stream_request.h:181: };
On 2017/01/05 01:09:32, mcasas wrote:
> content/public/ can pull data types out of media/, so
> you can just reuse the CameraCalibration from there,
> right?  No need to redefine it here.

Done.
I saw related TODO(ajm) at L.126 but assumed there is a need to keep it this
way...
It is now aliased to content::CameraCalibration (used in content code only) as
it is shorter.

https://codereview.chromium.org/2606983002/diff/120001/content/public/common/...
content/public/common/media_stream_request.h:184: CameraCalibration
camera_calibration;
On 2017/01/05 01:09:32, mcasas wrote:
> If this field is not always present we probably want to use
> base::Optional (see [1]).
> 
> [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md

Done. Thanks.

https://codereview.chromium.org/2606983002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2606983002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:297: if (source_) {
On 2017/01/05 01:09:32, mcasas wrote:
> Not your fault but what about:
> 
>   // TODO(hta): Extract the real value.
>   settings.deviceId = blink::WebString("video device ID");
>   if (!source_)
>     return;
> 
>   // The rest, including your new stuff.

Done.

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/fa...
media/capture/video/fake_video_capture_device_factory.cc:90: if
(device_descriptors->size() > kDepthDeviceIndex) {
On 2017/01/05 01:09:32, mcasas wrote:
>   if (device_descriptors < kDepthDeviceIndex)
>     return;
> 
> which should cover the vast majority of uses.

Done.

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2606983002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:101: friend class
base::RefCountedThreadSafe<CameraCalibration>;
On 2017/01/05 01:09:32, mcasas wrote:
> I fail to see why should CameraCalibration be ref counted, 
> since it's just a POD.

Done.
Didn't know better. As I learned about it here; base::Optional looks like a
proper way.

[kinuko, 2017-01-10 14:43:24]

https://codereview.chromium.org/2606983002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2606983002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:300: return;
why don't we have this before line 298?

https://codereview.chromium.org/2606983002/diff/140001/media/base/ipc/media_p...
File media/base/ipc/media_param_traits.cc (right):

https://codereview.chromium.org/2606983002/diff/140001/media/base/ipc/media_p...
media/base/ipc/media_param_traits.cc:159: GetParamSize(s, p.depth_far);
Wouldn't just add IPC_STRUCT_TRAITS for CameraCalibration work?

[aleksandar.stojiljkovic, 2017-01-11 08:58:46]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ nasko@chromium.org

[aleksandar.stojiljkovic, 2017-01-11 08:58:46]

Patch Set 3 : review #25 fix. Thanks kinuko@.

+ nasko@, PTAL at:
    content/common/media/media_stream_messages.h
    media/base/ipc/media_param_traits_macros.h
Thanks.

https://codereview.chromium.org/2606983002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2606983002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:300: return;
On 2017/01/10 14:43:24, kinuko wrote:
> why don't we have this before line 298?
Good point.
That change would be a functionality change orthogonal to this patch - usually I
try to avoid doing those in the same patch. Let me address it in follow up
patch; could also add a fix for the non-const reference function parameter in
L.296:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

https://codereview.chromium.org/2606983002/diff/140001/media/base/ipc/media_p...
File media/base/ipc/media_param_traits.cc (right):

https://codereview.chromium.org/2606983002/diff/140001/media/base/ipc/media_p...
media/base/ipc/media_param_traits.cc:159: GetParamSize(s, p.depth_far);
On 2017/01/10 14:43:24, kinuko wrote:
> Wouldn't just add IPC_STRUCT_TRAITS for CameraCalibration work?

Done.

[aleksandar.stojiljkovic, 2017-01-11 09:32:18]

https://codereview.chromium.org/2606983002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2606983002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:300: return;
On 2017/01/11 08:58:46, aleksandar.stojiljkovic wrote:
> On 2017/01/10 14:43:24, kinuko wrote:
> > why don't we have this before line 298?
> Good point.
> That change would be a functionality change orthogonal to this patch - usually
I
> try to avoid doing those in the same patch. Let me address it in follow up
> patch; could also add a fix for the non-const reference function parameter in
> L.296:
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Please ignore a note about non-const reference. As defined by Blink, it is fine.

[nasko, 2017-01-11 18:41:41]

https://codereview.chromium.org/2606983002/diff/160001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/2606983002/diff/160001/content/common/media/m...
content/common/media/media_stream_messages.h:57:
IPC_STRUCT_TRAITS_MEMBER(device.camera_calibration)
This should be before session_id, as it is still part of the device field.

https://codereview.chromium.org/2606983002/diff/160001/media/base/ipc/media_p...
File media/base/ipc/media_param_traits_macros.h (right):

https://codereview.chromium.org/2606983002/diff/160001/media/base/ipc/media_p...
media/base/ipc/media_param_traits_macros.h:100:
IPC_STRUCT_TRAITS_MEMBER(focal_length_x)
nit: Please indent by two spaces to match the style above.

[aleksandar.stojiljkovic, 2017-01-11 23:06:22]

PS#4: Rebase. #29 fix. Thanks nasko@.

https://codereview.chromium.org/2606983002/diff/160001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/2606983002/diff/160001/content/common/media/m...
content/common/media/media_stream_messages.h:57:
IPC_STRUCT_TRAITS_MEMBER(device.camera_calibration)
On 2017/01/11 18:41:41, nasko wrote:
> This should be before session_id, as it is still part of the device field.

Done.

https://codereview.chromium.org/2606983002/diff/160001/media/base/ipc/media_p...
File media/base/ipc/media_param_traits_macros.h (right):

https://codereview.chromium.org/2606983002/diff/160001/media/base/ipc/media_p...
media/base/ipc/media_param_traits_macros.h:100:
IPC_STRUCT_TRAITS_MEMBER(focal_length_x)
On 2017/01/11 18:41:41, nasko wrote:
> nit: Please indent by two spaces to match the style above.

Done.

[mcasas, 2017-01-11 23:18:20]

looking good, couple of comments.

https://codereview.chromium.org/2606983002/diff/160001/content/browser/render...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2606983002/diff/160001/content/browser/render...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:473:
// Video device on index 1 is depth video capture device.
Because of the '<=' in FakeVCDFactory, this comment is 
right but the comparisons below (e.g l.487) are incongruent.  
Correct there and here.

https://codereview.chromium.org/2606983002/diff/160001/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2606983002/diff/160001/media/capture/video/fa...
media/capture/video/fake_video_capture_device_factory.cc:90: if
(device_descriptors->size() <= kDepthDeviceIndex)
s/<=/!=/  ?
IIRC only the 1st device (numbering from 0) can be a
depth-enabled device, all others should be "normal".

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:231: if
(RuntimeEnabledFeatures::mediaCaptureDepthEnabled()) {
I guess this also only makes sense if:
  m_component->source()->type() == MediaStreamSource::TypeVideo

Which is a similar problem to what we see in [1], (check out 
WebMediaStreamTrack.h).  

I believe that the current implementation is not fully up to Spec, 
which is regrettable. The Spec instructs the use of the Constrainable 
Pattern for these occasions: i.e. using the CP we should not return 
any near/far/... in the MediaTrackSupportedConstraints dictionary 
if the said constraints are not supported, and any associated 
MediaTrackCapabilities readings would not have any said
near/far/... in that case, etc.

As things stand right now, I'm afraid the next logical step for you 
would diverge from [2] ... but that'll be for another CL.

[1] https://codereview.chromium.org/2609863004/
[2] https://www.w3.org/TR/mediacapture-depth/#mediatracksettings-dictionary

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:58: double depthFar =
0.0;
What happened to |focalLength|?

https://www.w3.org/TR/mediacapture-depth/#dom-mediatracksettings-focallength

[aleksandar.stojiljkovic, 2017-01-12 14:13:59]

PS#5 : Add comments related to #31. Thanks mcasas@.

https://codereview.chromium.org/2606983002/diff/160001/content/browser/render...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2606983002/diff/160001/content/browser/render...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:473:
// Video device on index 1 is depth video capture device.
On 2017/01/11 23:18:20, mcasas wrote:
> Because of the '<=' in FakeVCDFactory, this comment is 
> right but the comparisons below (e.g l.487) are incongruent.  
> Correct there and here.

I added more comments to this method, explaining 2 physical devices, only one
video stream open and the expected results. Hopefully it clarifies the code.
In short, GenerateStreamAndWaitForResult -> MediaStreamManager::GenerateStream
can generate only one video stream and we use controls.video.source_id (L.481)
to specify that the stream is depth video.
This is in line with other tests with multiple video streams opened - we can
only open one by one using the media stream manager pipeline.

https://codereview.chromium.org/2606983002/diff/160001/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2606983002/diff/160001/media/capture/video/fa...
media/capture/video/fake_video_capture_device_factory.cc:90: if
(device_descriptors->size() <= kDepthDeviceIndex)
On 2017/01/11 23:18:20, mcasas wrote:
> s/<=/!=/  ?
> IIRC only the 1st device (numbering from 0) can be a
> depth-enabled device, all others should be "normal".

The code is correct here - it is just a bounds check before lookup based on
index in L.93. The comment is not helping so it got modified. It could be also
written like:
if (device_descriptors->size() > kDepthDeviceIndex) {
  VideoCaptureDeviceDescriptor& depth_device(
      (*device_descriptors)[kDepthDeviceIndex]);
  depth_device.camera_calibration.emplace();
  ...
}
it might look clearer but early return seems better as it covers majority of
cases.

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:231: if
(RuntimeEnabledFeatures::mediaCaptureDepthEnabled()) {
On 2017/01/11 23:18:20, mcasas wrote:
> I guess this also only makes sense if:
>   m_component->source()->type() == MediaStreamSource::TypeVideo
> 
> Which is a similar problem to what we see in [1], (check out 
> WebMediaStreamTrack.h).  
> 
> I believe that the current implementation is not fully up to Spec, 
> which is regrettable. The Spec instructs the use of the Constrainable 
> Pattern for these occasions: i.e. using the CP we should not return 
> any near/far/... in the MediaTrackSupportedConstraints dictionary 
> if the said constraints are not supported, and any associated 
> MediaTrackCapabilities readings would not have any said
> near/far/... in that case, etc.

MediaTrackSupportedConstraints / I understand you meant
MediaStreamConstraints.video [1].
Yes, that's right. Haven't added them to WebMediaConstraints.h [2] either.

[1]
https://w3c.github.io/mediacapture-main/archives/20160513/getusermedia.html#n...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebMe...

> 
> As things stand right now, I'm afraid the next logical step for you 
> would diverge from [2] ... but that'll be for another CL.
> 
> [1] https://codereview.chromium.org/2609863004/
> [2] https://www.w3.org/TR/mediacapture-depth/#mediatracksettings-dictionary

I could add them here or defer all IDL changes to the final intrgration patch
but I assume it is better like this to add full API pieces while it is under
flag.
Added TODO for now:
    // TODO(aleksandar.stojiljkovic): Add MediaTrackConstraintSet support for
    // https://www.w3.org/TR/mediacapture-depth/#mediatracksettings-dictionary
    // See also https://crbug.com/616098 and WebMediaConstraints.h.
Thanks.

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:58: double depthFar =
0.0;
On 2017/01/11 23:18:20, mcasas wrote:
> What happened to |focalLength|?
> 
> https://www.w3.org/TR/mediacapture-depth/#dom-mediatracksettings-focallength

Replaced [focalLength, horizontalFieldOfView and verticalFieldOfView] by
[focalLengthX and focalLengthY] it Editor's draft:
https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary

The reasoning behing it is that focalLenth is underdefined and fovs are
calculated from near/far and focal lengths. Details:
https://github.com/w3c/mediacapture-depth/issues/135#issuecomment-260118438

[aleksandar.stojiljkovic, 2017-01-20 22:18:49]

On 2017/01/12 14:13:59, aleksandar.stojiljkovic wrote:
> PS#5 : Add comments related to #31. Thanks mcasas@.
> 
>
https://codereview.chromium.org/2606983002/diff/160001/content/browser/render...
> File
> content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2606983002/diff/160001/content/browser/render...
>
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:473:
> // Video device on index 1 is depth video capture device.
> On 2017/01/11 23:18:20, mcasas wrote:
> > Because of the '<=' in FakeVCDFactory, this comment is 
> > right but the comparisons below (e.g l.487) are incongruent.  
> > Correct there and here.
> 
> I added more comments to this method, explaining 2 physical devices, only one
> video stream open and the expected results. Hopefully it clarifies the code.
> In short, GenerateStreamAndWaitForResult -> MediaStreamManager::GenerateStream
> can generate only one video stream and we use controls.video.source_id (L.481)
> to specify that the stream is depth video.
> This is in line with other tests with multiple video streams opened - we can
> only open one by one using the media stream manager pipeline.
> 
>
https://codereview.chromium.org/2606983002/diff/160001/media/capture/video/fa...
> File media/capture/video/fake_video_capture_device_factory.cc (right):
> 
>
https://codereview.chromium.org/2606983002/diff/160001/media/capture/video/fa...
> media/capture/video/fake_video_capture_device_factory.cc:90: if
> (device_descriptors->size() <= kDepthDeviceIndex)
> On 2017/01/11 23:18:20, mcasas wrote:
> > s/<=/!=/  ?
> > IIRC only the 1st device (numbering from 0) can be a
> > depth-enabled device, all others should be "normal".
> 
> The code is correct here - it is just a bounds check before lookup based on
> index in L.93. The comment is not helping so it got modified. It could be also
> written like:
> if (device_descriptors->size() > kDepthDeviceIndex) {
>   VideoCaptureDeviceDescriptor& depth_device(
>       (*device_descriptors)[kDepthDeviceIndex]);
>   depth_device.camera_calibration.emplace();
>   ...
> }
> it might look clearer but early return seems better as it covers majority of
> cases.
> 
>
https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
(right):
> 
>
https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:231: if
> (RuntimeEnabledFeatures::mediaCaptureDepthEnabled()) {
> On 2017/01/11 23:18:20, mcasas wrote:
> > I guess this also only makes sense if:
> >   m_component->source()->type() == MediaStreamSource::TypeVideo
> > 
> > Which is a similar problem to what we see in [1], (check out 
> > WebMediaStreamTrack.h).  
> > 
> > I believe that the current implementation is not fully up to Spec, 
> > which is regrettable. The Spec instructs the use of the Constrainable 
> > Pattern for these occasions: i.e. using the CP we should not return 
> > any near/far/... in the MediaTrackSupportedConstraints dictionary 
> > if the said constraints are not supported, and any associated 
> > MediaTrackCapabilities readings would not have any said
> > near/far/... in that case, etc.
> 
> MediaTrackSupportedConstraints / I understand you meant
> MediaStreamConstraints.video [1].
> Yes, that's right. Haven't added them to WebMediaConstraints.h [2] either.
> 
> [1]
>
https://w3c.github.io/mediacapture-main/archives/20160513/getusermedia.html#n...
> [2]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebMe...
> 
> > 
> > As things stand right now, I'm afraid the next logical step for you 
> > would diverge from [2] ... but that'll be for another CL.
> > 
> > [1] https://codereview.chromium.org/2609863004/
> > [2] https://www.w3.org/TR/mediacapture-depth/#mediatracksettings-dictionary
> 
> I could add them here or defer all IDL changes to the final intrgration patch
> but I assume it is better like this to add full API pieces while it is under
> flag.
> Added TODO for now:
>     // TODO(aleksandar.stojiljkovic): Add MediaTrackConstraintSet support for
>     // https://www.w3.org/TR/mediacapture-depth/#mediatracksettings-dictionary
>     // See also https://crbug.com/616098 and WebMediaConstraints.h.
> Thanks.

mcasas@,
are you fine with this - to add the constraints in follow up patch?
Otherwise, I'll just add them here.
The rest of issues seems resolved to me.
Thanks.

> 
>
https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/pub...
> File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):
> 
>
https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/WebMediaStreamTrack.h:58: double depthFar =
> 0.0;
> On 2017/01/11 23:18:20, mcasas wrote:
> > What happened to |focalLength|?
> > 
> > https://www.w3.org/TR/mediacapture-depth/#dom-mediatracksettings-focallength
> 
> Replaced [focalLength, horizontalFieldOfView and verticalFieldOfView] by
> [focalLengthX and focalLengthY] it Editor's draft:
> https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary
> 
> The reasoning behing it is that focalLenth is underdefined and fovs are
> calculated from near/far and focal lengths. Details:
> https://github.com/w3c/mediacapture-depth/issues/135#issuecomment-260118438

[aleksandar.stojiljkovic, 2017-01-27 21:13:05]

Patchset #6 (id:220001) has been deleted

[aleksandar.stojiljkovic, 2017-01-29 16:36:37]

Patchset #6 (id:240001) has been deleted

[aleksandar.stojiljkovic, 2017-01-29 18:45:34]

PS#6: Added to MediaTrackSupportedConstraints and MediaTrackConstraintSet.
Thanks mcasas@.

PTAL.

The issues seems resolved now.
Plan to add depth related constraining logic in follow up patch.

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2606983002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:231: if
(RuntimeEnabledFeatures::mediaCaptureDepthEnabled()) {
On 2017/01/11 23:18:20, mcasas wrote:
> I guess this also only makes sense if:
>   m_component->source()->type() == MediaStreamSource::TypeVideo
> 
> Which is a similar problem to what we see in [1], (check out 
> WebMediaStreamTrack.h).  
> 
> I believe that the current implementation is not fully up to Spec, 
> which is regrettable. The Spec instructs the use of the Constrainable 
> Pattern for these occasions: i.e. using the CP we should not return 
> any near/far/... in the MediaTrackSupportedConstraints dictionary 
> if the said constraints are not supported, and any associated 
> MediaTrackCapabilities readings would not have any said
> near/far/... in that case, etc.
> 
> As things stand right now, I'm afraid the next logical step for you 
> would diverge from [2] ... but that'll be for another CL.
> 
> [1] https://codereview.chromium.org/2609863004/
> [2] https://www.w3.org/TR/mediacapture-depth/#mediatracksettings-dictionary

Done.
PS#6:
- added near, far and focal length to MediaTrackSupportedConstraints and
MediaTrackConstraintSet.
- GetBothStreamsAndCheckForFeaturesPresence test that video and depth stream
tracks contain only relevant constraints and settings.

[aleksandar.stojiljkovic, 2017-01-30 13:30:21]

Patchset #6 (id:260001) has been deleted

[mcasas, 2017-01-30 15:10:15]

mcasas@chromium.org changed reviewers:
+ guidou@chromium.org

[mcasas, 2017-01-30 15:10:15]

lgtm

guidou@ can you take a look as well at the
MediaStream-lands? Thanks.

https://codereview.chromium.org/2606983002/diff/280001/content/test/data/medi...
File content/test/data/media/getusermedia-depth-capture.html (right):

https://codereview.chromium.org/2606983002/diff/280001/content/test/data/medi...
content/test/data/media/getusermedia-depth-capture.html:77: // tracks have them
in constraints and settings.
Although not 100% enforced, try to keep this file to 80 columns.

[tommi (sloooow) - chröme, 2017-01-30 15:15:26]

-myself as I think Guido will cover owner review as well.

[tommi (sloooow) - chröme, 2017-01-30 15:15:49]

tommi@chromium.org changed reviewers:
- tommi@chromium.org

[Guido Urdaneta, 2017-02-01 12:38:48]

https://codereview.chromium.org/2606983002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/MediaTrackSupportedConstraints.idl
(right):

https://codereview.chromium.org/2606983002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaTrackSupportedConstraints.idl:29:
boolean depthNear = true;
Shouldn't these be behind the MediaCaptureDepth flag?

[aleksandar.stojiljkovic, 2017-02-02 12:43:41]

PS#7: RuntimeEnabled=MediaCaptureDepth to MediaTrackSupportedConstraints.idl.
Thanks guidou@.

https://codereview.chromium.org/2606983002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/MediaTrackSupportedConstraints.idl
(right):

https://codereview.chromium.org/2606983002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/MediaTrackSupportedConstraints.idl:29:
boolean depthNear = true;
On 2017/02/01 12:38:48, Guido Urdaneta wrote:
> Shouldn't these be behind the MediaCaptureDepth flag?

Done.

[Guido Urdaneta, 2017-02-02 12:50:48]

lgtm

[aleksandar.stojiljkovic, 2017-02-02 13:04:44]

kinuko@, PTAL at:
third_party/WebKit/Source/platform/*
third_party/WebKit/public/*
content/public/common/media_stream_request.h
Thanks.

[kinuko, 2017-02-03 00:07:08]

On 2017/02/02 13:04:44, aleksandar.stojiljkovic wrote:
> kinuko@, PTAL at:
> third_party/WebKit/Source/platform/*
> third_party/WebKit/public/*
> content/public/common/media_stream_request.h
> Thanks.

lgtm

[kinuko, 2017-02-03 00:20:01]

https://codereview.chromium.org/2606983002/diff/300001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2606983002/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:51: bool hasDepthFar()
{ return depthFar >= 0.0; }
nit: no need to be done in this CL but it looks all these could be const methods

[nasko, 2017-02-03 05:30:13]

IPC LGTM

[aleksandar.stojiljkovic, 2017-02-03 12:28:32]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 12:28:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleksandar.stojiljkovic, 2017-02-03 12:33:45]

PS#9: move CameraCalibration traits from media/base/ipc to media/capture/ipc.

kinuko@, nasko@, thanks. After rebase, I have moved the traits to proper place
according to mcasas@, change https://codereview.chromium.org/2539543002/

https://codereview.chromium.org/2606983002/diff/300001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2606983002/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:51: bool hasDepthFar()
{ return depthFar >= 0.0; }
On 2017/02/03 00:20:01, kinuko wrote:
> nit: no need to be done in this CL but it looks all these could be const
methods

Thanks. Let me cover it in follow up patch as it is related to existing code
too.

[commit-bot: I haz the power, 2017-02-03 14:28:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 14:28:30]

Dry run: This issue passed the CQ dry run.

[aleksandar.stojiljkovic, 2017-02-07 04:56:08]

a nit before submitting.

https://codereview.chromium.org/2606983002/diff/280001/content/test/data/medi...
File content/test/data/media/getusermedia-depth-capture.html (right):

https://codereview.chromium.org/2606983002/diff/280001/content/test/data/medi...
content/test/data/media/getusermedia-depth-capture.html:77: // tracks have them
in constraints and settings.
On 2017/01/30 15:10:15, mcasas wrote:
> Although not 100% enforced, try to keep this file to 80 columns.

Done.

[aleksandar.stojiljkovic, 2017-02-07 04:56:57]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2017-02-07 04:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
kinuko@chromium.org, guidou@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2606983002/#ps360001
(title: "80-char line fix in test html.")

[commit-bot: I haz the power, 2017-02-07 04:57:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 07:44:25]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1486443417549460,
"parent_rev": "059255004a9e29dab9936f0527da655a148ce459", "commit_rev":
"9e1e3f73fc41ca1f8a5d42b3a418c286c2e39fac"}

[commit-bot: I haz the power, 2017-02-07 07:45:30]

Description was changed from

==========
Media Capture Depth Stream Extensions API: focal length and depth range.

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackSettings {
    double depthNear;
    double depthFar;
    double focalLengthX;
    double focalLengthY;
};

This patch adds the support for it to fake capture device only.

[1]
https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary

BUG=616098
==========

to

==========
Media Capture Depth Stream Extensions API: focal length and depth range.

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackSettings {
    double depthNear;
    double depthFar;
    double focalLengthX;
    double focalLengthY;
};

This patch adds the support for it to fake capture device only.

[1]
https://w3c.github.io/mediacapture-depth/#mediatracksettings-dictionary

BUG=616098

Review-Url: https://codereview.chromium.org/2606983002
Cr-Commit-Position: refs/heads/master@{#448567}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e1e3f73fc41ca1f8a5d42b3a418...
==========

[commit-bot: I haz the power, 2017-02-07 07:45:32]

Committed patchset #10 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/9e1e3f73fc41ca1f8a5d42b3a418...