Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(84)

Issue 2609863004: Pass camera facing to WebKit (Closed)

Created:
3 years, 11 months ago by shenghao
Modified:
3 years, 11 months ago
CC:
avayvod+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org, Srirama, xjz+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Pass camera facing to WebKit We want to expose camera facing information in JS API. First step is to pass it to MediaStreamVideoTrack::getSettings(). BUG=webrtc:2481, crbug:543997 TEST=Log the camera facing value in VideoCaptureImplManager Review-Url: https://codereview.chromium.org/2609863004 Cr-Commit-Position: refs/heads/master@{#445663} Committed: https://chromium.googlesource.com/chromium/src/+/5466f424bef9d4997ff78783f9bb2ec4acbbabcc

Patch Set 1 : modify settings names #

Total comments: 28

Patch Set 2 : address review comments #

Total comments: 4

Patch Set 3 : use =default syntax #

Total comments: 4

Patch Set 4 : address review comment #

Total comments: 16

Patch Set 5 : fix trybot errors #

Total comments: 26

Patch Set 6 : Rename to video_facing.h #

Patch Set 7 : fix trybot errors #

Patch Set 8 : try to fix trybot errors #

Total comments: 1

Patch Set 9 : rebase #

Patch Set 10 : Reverted media_device.* #

Total comments: 4

Patch Set 11 : change comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -48 lines) Patch
M content/common/media/media_stream_messages.h View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M content/public/common/media_stream_request.h View 1 2 3 4 5 4 chunks +7 lines, -10 lines 0 comments Download
M content/public/common/media_stream_request.cc View 1 2 3 4 5 2 chunks +14 lines, -7 lines 0 comments Download
M content/renderer/media/media_stream_video_track.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -3 lines 0 comments Download
M content/renderer/media/mock_media_stream_dispatcher.cc View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
A media/base/video_facing.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +21 lines, -0 lines 0 comments Download
M media/capture/video/linux/camera_facing_chromeos.h View 1 2 3 4 5 4 chunks +7 lines, -8 lines 0 comments Download
M media/capture/video/linux/camera_facing_chromeos.cc View 1 2 3 4 5 3 chunks +9 lines, -3 lines 0 comments Download
M media/capture/video/linux/camera_facing_chromeos_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M media/capture/video/linux/video_capture_device_chromeos.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/capture/video/linux/video_capture_device_chromeos.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M media/capture/video/linux/video_capture_device_factory_linux.cc View 1 2 3 4 5 2 chunks +18 lines, -0 lines 0 comments Download
M media/capture/video/video_capture_device_descriptor.h View 1 2 3 4 5 3 chunks +11 lines, -6 lines 0 comments Download
M media/capture/video/video_capture_device_descriptor.cc View 1 2 3 4 5 6 7 3 chunks +6 lines, -2 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 141 (75 generated)
shenghao
3 years, 11 months ago (2017-01-04 12:15:05 UTC) #3
jochen (gone - plz use gerrit)
please send an intent to implement (http://www.chromium.org/blink#TOC-Launch-process:-launching-a-new-feature) as part of the process, you will have ...
3 years, 11 months ago (2017-01-04 12:17:05 UTC) #4
shenghao
On 2017/01/04 12:17:05, jochen wrote: > please send an intent to implement > (http://www.chromium.org/blink#TOC-Launch-process:-launching-a-new-feature) > ...
3 years, 11 months ago (2017-01-05 03:25:06 UTC) #7
jochen (gone - plz use gerrit)
thanks for linking the bug! I couldn't find the intent to implement, maybe I searched ...
3 years, 11 months ago (2017-01-05 07:25:04 UTC) #8
shenghao
On 2017/01/05 07:25:04, jochen wrote: > thanks for linking the bug! > > I couldn't ...
3 years, 11 months ago (2017-01-05 08:19:47 UTC) #9
jochen (gone - plz use gerrit)
ok, thanks, with that, I found the intent to ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/s-NMzjSfVQs I wonder why you ...
3 years, 11 months ago (2017-01-05 09:01:18 UTC) #10
shenghao
On 2017/01/05 09:01:18, jochen wrote: > ok, thanks, with that, I found the intent to ...
3 years, 11 months ago (2017-01-05 09:47:51 UTC) #11
jochen (gone - plz use gerrit)
i'm still a bit confused why we have videofacingmode and camerafacing instead of just one ...
3 years, 11 months ago (2017-01-05 12:17:59 UTC) #12
mcasas
Drive by https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc#newcode389 content/browser/renderer_host/media/media_devices_manager.cc:389: } No {} for one-line bodies. https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/media_stream_video_track.cc ...
3 years, 11 months ago (2017-01-05 20:26:37 UTC) #14
miu
https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc#newcode388 content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); nit: push_back() would be more appropriate here. https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/media_stream_video_track.cc ...
3 years, 11 months ago (2017-01-05 21:55:33 UTC) #16
shenghao
https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc#newcode388 content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); On 2017/01/05 21:55:33, miu wrote: > nit: push_back() ...
3 years, 11 months ago (2017-01-06 09:43:02 UTC) #17
shenghao
On 2017/01/05 12:17:59, jochen wrote: > i'm still a bit confused why we have videofacingmode ...
3 years, 11 months ago (2017-01-06 09:49:26 UTC) #18
wuchengli
lgtm https://codereview.chromium.org/2609863004/diff/60001/content/common/media/media_devices.h File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/60001/content/common/media/media_devices.h#newcode41 content/common/media/media_devices.h:41: std::string model_id; On 2017/01/06 09:43:01, shenghao wrote: > ...
3 years, 11 months ago (2017-01-09 08:28:00 UTC) #19
jochen (gone - plz use gerrit)
On 2017/01/06 at 09:49:26, shenghao wrote: > On 2017/01/05 12:17:59, jochen wrote: > > i'm ...
3 years, 11 months ago (2017-01-09 08:29:17 UTC) #20
jochen (gone - plz use gerrit)
On 2017/01/09 at 08:29:17, jochen wrote: > On 2017/01/06 at 09:49:26, shenghao wrote: > > ...
3 years, 11 months ago (2017-01-09 08:30:16 UTC) #21
wuchengli
On Mon, Jan 9, 2017 at 4:29 PM, <jochen@chromium.org> wrote: > On 2017/01/06 at 09:49:26, ...
3 years, 11 months ago (2017-01-09 08:44:49 UTC) #22
wuchengli
On Mon, Jan 9, 2017 at 4:29 PM, <jochen@chromium.org> wrote: > On 2017/01/06 at 09:49:26, ...
3 years, 11 months ago (2017-01-09 08:44:50 UTC) #23
shenghao
I changed the fields in the enum to be the same as Webkit. I can't ...
3 years, 11 months ago (2017-01-10 12:16:35 UTC) #24
jochen (gone - plz use gerrit)
what's the error you get when you upload?
3 years, 11 months ago (2017-01-10 14:18:33 UTC) #31
miu
Comments on PS8: https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc#newcode388 content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); On 2017/01/06 09:43:01, shenghao wrote: ...
3 years, 11 months ago (2017-01-10 22:14:23 UTC) #32
shenghao
On 2017/01/10 14:18:33, jochen wrote: > what's the error you get when you upload? ** ...
3 years, 11 months ago (2017-01-11 11:31:07 UTC) #33
shenghao
https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc#newcode388 content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); On 2017/01/10 22:14:23, miu wrote: > On 2017/01/06 ...
3 years, 11 months ago (2017-01-11 12:00:53 UTC) #34
jochen (gone - plz use gerrit)
after digging around a bit, it seems like there's a project underway to move capture ...
3 years, 11 months ago (2017-01-11 13:23:27 UTC) #39
miu
lgtm https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/renderer_host/media/media_devices_manager.cc#newcode388 content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); On 2017/01/11 12:00:53, shenghao wrote: > On ...
3 years, 11 months ago (2017-01-11 21:08:44 UTC) #40
mcasas
Looking good, just still concerned about the way to make sure AudioTracks and VideoTracks that ...
3 years, 11 months ago (2017-01-11 22:24:59 UTC) #41
shenghao
> Looking good, just still concerned about the way > to make sure AudioTracks and ...
3 years, 11 months ago (2017-01-12 05:55:24 UTC) #47
shenghao
https://codereview.chromium.org/2609863004/diff/160001/content/browser/renderer_host/media/media_stream_manager.cc File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/browser/renderer_host/media/media_stream_manager.cc#newcode73 content/browser/renderer_host/media/media_stream_manager.cc:73: base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = On 2017/01/11 22:24:58, mcasas wrote: > ...
3 years, 11 months ago (2017-01-12 08:10:49 UTC) #48
jochen (gone - plz use gerrit)
On 2017/01/12 at 05:55:24, shenghao wrote: > > Looking good, just still concerned about the ...
3 years, 11 months ago (2017-01-12 08:43:27 UTC) #56
shenghao
On 2017/01/12 08:43:27, jochen wrote: > On 2017/01/12 at 05:55:24, shenghao wrote: > > > ...
3 years, 11 months ago (2017-01-12 11:38:52 UTC) #63
shenghao
3 years, 11 months ago (2017-01-12 11:39:04 UTC) #64
jochen (gone - plz use gerrit)
he filed https://bugs.chromium.org/p/chromium/issues/detail?id=543997
3 years, 11 months ago (2017-01-12 11:40:15 UTC) #65
shenghao
On 2017/01/12 11:40:15, jochen wrote: > he filed https://bugs.chromium.org/p/chromium/issues/detail?id=543997 Updated
3 years, 11 months ago (2017-01-12 11:47:01 UTC) #67
jochen (gone - plz use gerrit)
much better, thx! https://codereview.chromium.org/2609863004/diff/240001/content/browser/renderer_host/media/media_stream_manager.cc File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/browser/renderer_host/media/media_stream_manager.cc#newcode240 content/browser/renderer_host/media/media_stream_manager.cc:240: base::ThreadRestrictions::SetIOAllowed(true); why is this required? how ...
3 years, 11 months ago (2017-01-12 11:54:42 UTC) #68
hta - Chromium
Please talk to me. I've been doing the same thing in https://codereview.chromium.org/2590193002/ which is a ...
3 years, 11 months ago (2017-01-12 18:27:56 UTC) #74
hta - Chromium
Is there a design document for this change? I see a very large amount of ...
3 years, 11 months ago (2017-01-12 19:36:41 UTC) #75
jochen (gone - plz use gerrit)
https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/video_capture_types.mojom File media/capture/mojo/video_capture_types.mojom (right): https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/video_capture_types.mojom#newcode40 media/capture/mojo/video_capture_types.mojom:40: }; On 2017/01/12 at 19:36:41, hta - Chromium wrote: ...
3 years, 11 months ago (2017-01-13 08:47:09 UTC) #76
hta - Chromium
On 2017/01/13 08:47:09, jochen wrote: > https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/video_capture_types.mojom > File media/capture/mojo/video_capture_types.mojom (right): > > https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/video_capture_types.mojom#newcode40 > ...
3 years, 11 months ago (2017-01-13 10:27:15 UTC) #77
shenghao
https://codereview.chromium.org/2609863004/diff/240001/content/browser/renderer_host/media/media_stream_manager.cc File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/browser/renderer_host/media/media_stream_manager.cc#newcode75 content/browser/renderer_host/media/media_stream_manager.cc:75: LAZY_INSTANCE_INITIALIZER; On 2017/01/12 19:36:41, hta - Chromium wrote: > ...
3 years, 11 months ago (2017-01-13 11:09:35 UTC) #78
shenghao
I will wait for https://codereview.chromium.org/2590193002/ to be landed before submitting this CL. Please review the ...
3 years, 11 months ago (2017-01-13 11:13:05 UTC) #79
jochen (gone - plz use gerrit)
I'll defer to hta / mcasas on the design
3 years, 11 months ago (2017-01-13 14:24:14 UTC) #80
hta - Chromium
This looks good to me now, apart from the linking issue, which I think you ...
3 years, 11 months ago (2017-01-17 12:25:48 UTC) #85
shenghao
On 2017/01/17 12:25:48, hta - Chromium wrote: > This looks good to me now, apart ...
3 years, 11 months ago (2017-01-17 13:16:59 UTC) #89
chcunningham
On 2017/01/17 13:16:59, shenghao wrote: > On 2017/01/17 12:25:48, hta - Chromium wrote: > > ...
3 years, 11 months ago (2017-01-17 19:12:39 UTC) #92
hta - Chromium
lgtm
3 years, 11 months ago (2017-01-18 15:05:17 UTC) #107
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2609863004/340001
3 years, 11 months ago (2017-01-19 01:56:50 UTC) #110
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/344911)
3 years, 11 months ago (2017-01-19 02:05:57 UTC) #112
chcunningham
lgtm
3 years, 11 months ago (2017-01-19 02:33:30 UTC) #113
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2609863004/340001
3 years, 11 months ago (2017-01-19 02:33:59 UTC) #115
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/344936)
3 years, 11 months ago (2017-01-19 02:41:20 UTC) #117
shenghao
On 2017/01/19 02:41:20, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 11 months ago (2017-01-19 03:51:35 UTC) #118
Guido Urdaneta
https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h#newcode43 content/common/media/media_devices.h:43: media::VideoFacingMode facing; Don't add the facing field here. To ...
3 years, 11 months ago (2017-01-19 10:29:41 UTC) #120
shenghao
On 2017/01/19 10:29:41, Guido Urdaneta wrote: > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h#newcode43 ...
3 years, 11 months ago (2017-01-19 11:01:25 UTC) #121
hta - Chromium
On 2017/01/19 10:29:41, Guido Urdaneta wrote: > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h#newcode43 ...
3 years, 11 months ago (2017-01-19 11:36:43 UTC) #122
hta - Chromium
On 2017/01/19 10:29:41, Guido Urdaneta wrote: > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/media_devices.h#newcode43 ...
3 years, 11 months ago (2017-01-19 11:36:48 UTC) #123
jochen (gone - plz use gerrit)
lgtm
3 years, 11 months ago (2017-01-20 07:39:56 UTC) #124
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2609863004/340001
3 years, 11 months ago (2017-01-20 08:42:46 UTC) #126
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/346278)
3 years, 11 months ago (2017-01-20 08:49:34 UTC) #128
Guido Urdaneta
> Before this CL, facing mode is part of MediaStreamDevice only. > After this CL, ...
3 years, 11 months ago (2017-01-20 09:29:14 UTC) #129
Guido Urdaneta
On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > Before this CL, facing mode is part ...
3 years, 11 months ago (2017-01-20 09:30:18 UTC) #130
Guido Urdaneta
On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > Before this CL, facing mode is part ...
3 years, 11 months ago (2017-01-20 09:30:25 UTC) #131
shenghao
On 2017/01/20 09:30:25, Guido Urdaneta wrote: > On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > ...
3 years, 11 months ago (2017-01-23 13:06:10 UTC) #132
shenghao
On 2017/01/23 13:06:10, shenghao wrote: > On 2017/01/20 09:30:25, Guido Urdaneta wrote: > > On ...
3 years, 11 months ago (2017-01-23 13:09:31 UTC) #133
nasko
IPC LGTM with a couple of nits. https://codereview.chromium.org/2609863004/diff/380001/media/base/video_facing.h File media/base/video_facing.h (right): https://codereview.chromium.org/2609863004/diff/380001/media/base/video_facing.h#newcode1 media/base/video_facing.h:1: // Copyright ...
3 years, 11 months ago (2017-01-23 18:39:23 UTC) #134
shenghao
https://codereview.chromium.org/2609863004/diff/380001/media/base/video_facing.h File media/base/video_facing.h (right): https://codereview.chromium.org/2609863004/diff/380001/media/base/video_facing.h#newcode1 media/base/video_facing.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. ...
3 years, 11 months ago (2017-01-24 04:23:47 UTC) #135
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2609863004/400001
3 years, 11 months ago (2017-01-24 04:24:29 UTC) #138
commit-bot: I haz the power
3 years, 11 months ago (2017-01-24 06:08:26 UTC) #141
Message was sent while issue was closed.
Committed patchset #11 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/5466f424bef9d4997ff78783f9bb...

Powered by Google App Engine
This is Rietveld 408576698