Media Capture Depth Stream Extensions API: videoKind settings and constraint.

Media Capture Depth Stream Extensions API: videoKind settings and constraint.

This enables selection of depth stream on Linux; both color and
depth device have the same Label and it is not possible to apart
depth from color stream (https://crbug.com/686756).

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackConstraintSet {
    ConstrainDOMString videoKind;
};

and constraining logic applying it to MediaStreamTrack object.

partial dictionary MediaTrackSettings {
    DOMString videoKind;
};

This patch is verified to work with fake capture device and RealSense SR300.

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

BUG=686756, 616098
Review-Url: https://codereview.chromium.org/2664673002
Cr-Commit-Position: refs/heads/master@{#450335}
Committed: https://chromium.googlesource.com/chromium/src/+/28ea8a96d0788781f3ef06053c921d8e740a30ef

[aleksandar.stojiljkovic, 2017-02-06 19:26:51]

Patchset #1 (id:1) has been deleted

[aleksandar.stojiljkovic, 2017-02-06 19:27:20]

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

[commit-bot: I haz the power, 2017-02-06 19:27:46]

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-06 19:28:28]

Description was changed from

==========
Media Capture Depth Stream Extensions API: videoKind settings and constraint.

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackSettings {
    DOMString videoKind;
};

partial dictionary MediaTrackSettings {
    DOMString videoKind;
};

and constaining logic for videoKind.

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: videoKind settings and constraint.

This enables selection of depth stream on Linux; both color and
depth device have the same Label and it is not possible to apart
depth from color stream (https://crbug.com/686756).

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackConstraintSet {
    ConstrainDOMString videoKind;
};

and constraining logic applying it to MediaStreamTrack object.

partial dictionary MediaTrackSettings {
    DOMString videoKind;
};

This patch is verified to work with fake capture device and RealSense SR300.

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

BUG=686756, 616098
==========

[aleksandar.stojiljkovic, 2017-02-06 20:34:55]

Patchset #1 (id:20001) has been deleted

[aleksandar.stojiljkovic, 2017-02-06 20:48:36]

Patchset #1 (id:40001) has been deleted

[aleksandar.stojiljkovic, 2017-02-06 20:49:00]

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

[commit-bot: I haz the power, 2017-02-06 20:49:45]

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, 2017-02-06 22:45:08]

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

[commit-bot: I haz the power, 2017-02-06 22:45:09]

Dry run: This issue passed the CQ dry run.

[aleksandar.stojiljkovic, 2017-02-07 04:32:45]

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

[aleksandar.stojiljkovic, 2017-02-07 04:32:45]

mcasas@chromium.org: PTAL at 
content/renderer/media

guidou@chromium.org: PTAL at 
content/browser
third_party/WebKit/Source/modules/mediastream

kinuko@chromium.org: PTAL at 
third_party/WebKit/Source/platform/
third_party/WebKit/public/platform/
content/common/

[kinuko, 2017-02-07 04:54:24]

> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=kinuko@chromium.org: PTAL
at 
> third_party/WebKit/Source/platform/
> third_party/WebKit/public/platform/
> content/common/

lgtm

[kinuko, 2017-02-07 04:55:49]

some nits

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:43: :
WebString::fromUTF8(kVideoKindColor);
nit: fromASCII() would be enough as they're both ascii

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:51: bool hasVideoKind()
{ return !videoKind.isNull(); }
nit: all of these could be const methods

[Guido Urdaneta, 2017-02-07 10:26:04]

https://codereview.chromium.org/2664673002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:837: if
(request->controls.video.device_id.empty() &&
There is a CL (http://crrev.com/2669243004/) close to landing that performs
spec-compliant device selection based on constraints on the renderer side.
You should leave media_stream_manager unmodified and wait for that CL to land.
That CL depends on a selection algorithm that needs to be patched to be aware of
the new constraint, but that will be easy.

https://codereview.chromium.org/2664673002/diff/60001/content/common/media/me...
File content/common/media/media_stream_options.h (right):

https://codereview.chromium.org/2664673002/diff/60001/content/common/media/me...
content/common/media/media_stream_options.h:26: VIDEO_KIND_NO_CONSTRAINT,
would VIDEO_KIND_NONE be better name?
At this point, it is no longer a constraint but a setting.

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:37: const char
kVideoKindDepth[] = "depth";
This constant is also defined elsewhere and literal values are used in some
other places. Is there a way to make a single constant that is used everywhere?

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:189: if (video_kind_string ==
"color") {
nit: perhaps make the constants that were already defined with these values
public?

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:233: if
(platformSettings.hasVideoKind())
This if could be another && in the previous if

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
(right):

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:35:
[RuntimeEnabled=MediaCaptureDepth] ConstrainDOMString videoKind;
Use RuntimeEnabled=MediaConstraints as with the other constraints.
Also, put it before mandatory, like the other constraints.

[Guido Urdaneta, 2017-02-07 10:45:17]

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

[Guido Urdaneta, 2017-02-07 10:45:56]

+hta

[aleksandar.stojiljkovic, 2017-02-07 11:33:49]

PS #2 : Review #14 & #15 fixes. kinuko@, guidou@, thanks.

Fixed, waiting for CL (http://crrev.com/2669243004/) to rebase.

https://codereview.chromium.org/2664673002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:837: if
(request->controls.video.device_id.empty() &&
On 2017/02/07 10:26:03, Guido Urdaneta wrote:
> There is a CL (http://crrev.com/2669243004/) close to landing that performs
> spec-compliant device selection based on constraints on the renderer side.
> You should leave media_stream_manager unmodified and wait for that CL to land.
> That CL depends on a selection algorithm that needs to be patched to be aware
of
> the new constraint, but that will be easy.

Thanks. Looks good.

https://codereview.chromium.org/2664673002/diff/60001/content/common/media/me...
File content/common/media/media_stream_options.h (right):

https://codereview.chromium.org/2664673002/diff/60001/content/common/media/me...
content/common/media/media_stream_options.h:26: VIDEO_KIND_NO_CONSTRAINT,
On 2017/02/07 10:26:04, Guido Urdaneta wrote:
> would VIDEO_KIND_NONE be better name?
> At this point, it is no longer a constraint but a setting.

Done. I didn't like it's semantics when using it as constraint but your point is
valid.

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:37: const char
kVideoKindDepth[] = "depth";
On 2017/02/07 10:26:04, Guido Urdaneta wrote:
> This constant is also defined elsewhere and literal values are used in some
> other places. Is there a way to make a single constant that is used
everywhere?

Done - media_stream_options.h

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:43: :
WebString::fromUTF8(kVideoKindColor);
On 2017/02/07 04:55:49, kinuko wrote:
> nit: fromASCII() would be enough as they're both ascii

Done.

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:189: if (video_kind_string ==
"color") {
On 2017/02/07 10:26:04, Guido Urdaneta wrote:
> nit: perhaps make the constants that were already defined with these values
> public?

Done.

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:233: if
(platformSettings.hasVideoKind())
On 2017/02/07 10:26:04, Guido Urdaneta wrote:
> This if could be another && in the previous if

Left it deliberately, as more fields (depthNear/Far, focalLenghX/Y) are getting
under it.

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
(right):

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:35:
[RuntimeEnabled=MediaCaptureDepth] ConstrainDOMString videoKind;
On 2017/02/07 10:26:04, Guido Urdaneta wrote:
> Use RuntimeEnabled=MediaConstraints as with the other constraints.
> Also, put it before mandatory, like the other constraints.

Done. [RuntimeEnabled=MediaConstraints,MediaCaptureDepth]

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2664673002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:51: bool hasVideoKind()
{ return !videoKind.isNull(); }
On 2017/02/07 04:55:49, kinuko wrote:
> nit: all of these could be const methods

Done.

[aleksandar.stojiljkovic, 2017-02-09 19:52:56]

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

[commit-bot: I haz the power, 2017-02-09 19:53:43]

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-09 21:39:25]

PS#3: Rebase to http://crrev.com/2669243004/ simplified the patch.
guidou@chromium.org, PTAL. Thanks.

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/2664673002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:37: const char
kVideoKindDepth[] = "depth";
On 2017/02/07 11:33:49, aleksandar.stojiljkovic wrote:
> On 2017/02/07 10:26:04, Guido Urdaneta wrote:
> > This constant is also defined elsewhere and literal values are used in some
> > other places. Is there a way to make a single constant that is used
> everywhere?
> 
> Done - media_stream_options.h

Moved to media_stream_video_track.h as it is used only within
/content/renderer/media.

[commit-bot: I haz the power, 2017-02-09 22:29:18]

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

[commit-bot: I haz the power, 2017-02-09 22:29:19]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-02-10 12:45:21]

https://codereview.chromium.org/2664673002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/2664673002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_track.h:24: // VideoKind enum values.
https://w3c.github.io/mediacapture-depth.
These are used only here and in media_stream_constraints_util_video_source
files.

Since this is more a utility than a MediaStreamVideoTrack-specific thing, I
would prefer that you move this to media_stream_constraints_util_video_source.*

[aleksandar.stojiljkovic, 2017-02-10 15:06:51]

PS#4 : videoKind string constants removed from header.

https://codereview.chromium.org/2664673002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/2664673002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_track.h:24: // VideoKind enum values.
https://w3c.github.io/mediacapture-depth.
On 2017/02/10 12:45:21, Guido Urdaneta wrote:
> These are used only here and in media_stream_constraints_util_video_source
> files.
> 
> Since this is more a utility than a MediaStreamVideoTrack-specific thing, I
> would prefer that you move this to
media_stream_constraints_util_video_source.*
> 

String constants removed. It is not needed to have them in header anymore.
However, left the GetVideoKindForFormat here - it is used in
media_stream_video_track.cc (settings), media_stream_video_source (filter
formats by constraint) and media_stream_constraints_util_video_source.cc.

[Guido Urdaneta, 2017-02-10 15:36:17]

lgtm

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

https://codereview.chromium.org/2664673002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:20: blink::WebString
GetVideoKindForFormat(
nit: I still would prefer the function to be in
media_stream_constraints_util_video_source.cc since this is more an utility than
a Track-specific function.
I particularly dislike the inclusion of media_stream_video_track.h in
media_stream_constraints_util_video_source.cc since the latter should not depend
on anything track related.

[aleksandar.stojiljkovic, 2017-02-10 16:29:23]

PS#5 : GetVideoKindForFormat moved to utility. thanks guidou@.

Done as you suggested. In previous patch I just needed to point out it is used
elsewhere as setting; though it could go to media_stream_video_source.cc before
track, it is better to have it in utility.

I trust that you have reviewed all the code under content.
The owner's approval is in place but let me wait today if mcasas@ has some
comments before committing.

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

https://codereview.chromium.org/2664673002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_track.cc:20: blink::WebString
GetVideoKindForFormat(
On 2017/02/10 15:36:17, Guido Urdaneta wrote:
> nit: I still would prefer the function to be in
> media_stream_constraints_util_video_source.cc since this is more an utility
than
> a Track-specific function.
> I particularly dislike the inclusion of media_stream_video_track.h in
> media_stream_constraints_util_video_source.cc since the latter should not
depend
> on anything track related.

Done.

[Guido Urdaneta, 2017-02-10 16:38:27]

still lgtm

> I trust that you have reviewed all the code under content.
> The owner's approval is in place but let me wait today if mcasas@ has some
> comments before committing.
sgtm

[aleksandar.stojiljkovic, 2017-02-14 07:07:43]

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

[aleksandar.stojiljkovic, 2017-02-14 07:07:44]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2664673002/#ps140001
(title: "GetVideoKindForFormat moved to utility. thanks guidou@.")

[commit-bot: I haz the power, 2017-02-14 07:08:07]

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-14 09:15:57]

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

[commit-bot: I haz the power, 2017-02-14 09:15:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleksandar.stojiljkovic, 2017-02-14 11:28:52]

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

[commit-bot: I haz the power, 2017-02-14 11:29:06]

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-14 13:31:19]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487071732472340,
"parent_rev": "68e6b8117a29dbbaec5a1f38d0d41926ec19fc4c", "commit_rev":
"28ea8a96d0788781f3ef06053c921d8e740a30ef"}

[commit-bot: I haz the power, 2017-02-14 13:32:49]

Description was changed from

==========
Media Capture Depth Stream Extensions API: videoKind settings and constraint.

This enables selection of depth stream on Linux; both color and
depth device have the same Label and it is not possible to apart
depth from color stream (https://crbug.com/686756).

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackConstraintSet {
    ConstrainDOMString videoKind;
};

and constraining logic applying it to MediaStreamTrack object.

partial dictionary MediaTrackSettings {
    DOMString videoKind;
};

This patch is verified to work with fake capture device and RealSense SR300.

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

BUG=686756, 616098
==========

to

==========
Media Capture Depth Stream Extensions API: videoKind settings and constraint.

This enables selection of depth stream on Linux; both color and
depth device have the same Label and it is not possible to apart
depth from color stream (https://crbug.com/686756).

Under experimental MediaCaptureDepth feature, implements [1]:

partial dictionary MediaTrackConstraintSet {
    ConstrainDOMString videoKind;
};

and constraining logic applying it to MediaStreamTrack object.

partial dictionary MediaTrackSettings {
    DOMString videoKind;
};

This patch is verified to work with fake capture device and RealSense SR300.

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

BUG=686756, 616098

Review-Url: https://codereview.chromium.org/2664673002
Cr-Commit-Position: refs/heads/master@{#450335}
Committed:
https://chromium.googlesource.com/chromium/src/+/28ea8a96d0788781f3ef06053c92...
==========

[commit-bot: I haz the power, 2017-02-14 13:32:52]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/28ea8a96d0788781f3ef06053c92...