Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2609863004:
Pass camera facing to WebKit (Closed)
|
Created:
3 years, 11 months ago by shenghao Modified:
3 years, 11 months ago Reviewers:
jochen (gone - plz use gerrit), nasko, Guido Urdaneta, chcunningham, mcasas, wuchengli, miu, hta - Chromium, tommi (sloooow) - chröme 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. |
DescriptionPass 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 #Dependent Patchsets: Messages
Total messages: 141 (75 generated)
Description was changed from ========== 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=b/31580933 TEST=Log the camera facing value in VideoCaptureImplManager ========== to ========== 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=b/31580933 TEST=Log the camera facing value in VideoCaptureImplManager ==========
shenghao@chromium.org changed reviewers: + jochen@chromium.org, tommi@chromium.org, wuchengli@chromium.org
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 to file an OWP launch bug on crbug.com - please reference this bug for the CL description
Description was changed from ========== 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=b/31580933 TEST=Log the camera facing value in VideoCaptureImplManager ========== to ========== 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=b:31580933 TEST=Log the camera facing value in VideoCaptureImplManager ==========
Description was changed from ========== 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=b:31580933 TEST=Log the camera facing value in VideoCaptureImplManager ========== to ========== 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, b:31580933 TEST=Log the camera facing value in VideoCaptureImplManager ==========
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) > > as part of the process, you will have to file an OWP launch bug on http://crbug.com - > please reference this bug for the CL description Actually this is part of the effort in webrtc:2481. It will launch with webrtc:2481 and I think webrtc:2481 already has sent out intent to implement. I shouldn't need a separate intent to implement, should I?
thanks for linking the bug! I couldn't find the intent to implement, maybe I searched for the wrong keyword in bit.ly/blinkintents. could you please point me at it?
On 2017/01/05 07:25:04, jochen wrote: > thanks for linking the bug! > > I couldn't find the intent to implement, maybe I searched for the wrong keyword > in bit.ly/blinkintents. could you please point me at it? It's at row 687 in bit.ly/blinkintents
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 use different names / values than the spec? The spec has facingMode (user/environment/left/right), but you use cameraFacing (front/back)?
On 2017/01/05 09:01:18, jochen wrote: > 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 use different names / values than the spec? > > The spec has facingMode (user/environment/left/right), but you use cameraFacing > (front/back)? I changed them to match the spec.
i'm still a bit confused why we have videofacingmode and camerafacing instead of just one of them? https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:52: #include "media/capture/video/linux/camera_facing_chromeos.h" we shouldn't unconditionally include platform specific headers https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:72: base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = why is that a singleton can't we create a new instance when we need one? also, platform specific code should either go into platform specific files, or needs to be #ifdef'd https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... content/common/media/media_devices.h:41: std::string model_id; why not a pair of ints then?
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Drive by https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... 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/... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_track.cc:310: media::CameraFacing facing = source_->GetCameraFacing(); nit: const https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_track.cc:321: DVLOG(1) << "Unrecongnized CameraFacing value" << facing; s/Unrecongnized/Unrecognized/ s/DVLOG(1)/DLOG(ERROR)/ ? https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... media/capture/video_capture_types.h:31: enum CameraFacing { This should be an enum class, right? https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... File media/capture/video_capturer_source.h (right): https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... media/capture/video_capturer_source.h:118: virtual CameraFacing GetCameraFacing() { return CameraFacing::FRONT; } This should be s/FRONT/DEFAULT/ or perhaps /UNKNOWN/, right? Because we don't know at this point where is the camera pointing to, and it could be movable like a USB one. What about making this method const? https://codereview.chromium.org/2609863004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right): https://codereview.chromium.org/2609863004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMediaStreamTrack.h:55: VideoFacingMode facingMode = VideoFacingMode::User; WebMediaStreamTrack applies to non-video tracks (currently only audio), so there should be some kind of FacingMode "none" or "unknown", but there isn't such thing in the spec [1], so please make a Spec bug for it and link it here as TODO. Perhaps also remove the "Video" from it, since we could have microphones orientations, right? [1] https://www.w3.org/TR/mediacapture-streams/#def-constraint-facingMode
miu@chromium.org changed reviewers: + miu@chromium.org
https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... 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/... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_track.cc:320: default: Don't include default clauses: Let the compiler force developers making future changes to the enum to have to also change this code. https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... media/capture/video_capture_types.h:34: DEFAULT = FRONT, This is not correct for the screen capture devices (IIUC what "front means"). Also, could you add comments here to des ribe these? E.g., "FRONT means a person's right will be on the left in the video."
https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... 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() would be more appropriate here. Why? If using push_back(), I will need to create a temporary MediaDeviceInfo object and incur one extra copy. https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_devices_manager.cc:389: } On 2017/01/05 20:26:37, mcasas wrote: > No {} for one-line bodies. Done. https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:52: #include "media/capture/video/linux/camera_facing_chromeos.h" On 2017/01/05 12:17:59, jochen wrote: > we shouldn't unconditionally include platform specific headers Done. https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:72: base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = On 2017/01/05 12:17:59, jochen wrote: > why is that a singleton can't we create a new instance when we need one? > It's a lazy instance so it only be constructed when first used. We don't want to create one instance for each use because constructing CameraFacingChromeOS involving reading some data from file, which is slow. > also, platform specific code should either go into platform specific files, or > needs to be #ifdef'd done https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... content/common/media/media_devices.h:41: std::string model_id; On 2017/01/05 12:17:59, jochen wrote: > why not a pair of ints then? We use the formatted "vid:pid" string as identifier in camera config files. When we get model_id from VideoCaptureDeviceDescriptor, it's already in this format, so converting it to 2 ints and back seems redundant. Plus that separated vid or pid are not useful anywhere in the system. https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_track.cc:310: media::CameraFacing facing = source_->GetCameraFacing(); On 2017/01/05 20:26:37, mcasas wrote: > nit: const Done. https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_track.cc:320: default: On 2017/01/05 21:55:33, miu wrote: > Don't include default clauses: Let the compiler force developers making future > changes to the enum to have to also change this code. Done. https://codereview.chromium.org/2609863004/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_track.cc:321: DVLOG(1) << "Unrecongnized CameraFacing value" << facing; On 2017/01/05 20:26:37, mcasas wrote: > s/Unrecongnized/Unrecognized/ > > s/DVLOG(1)/DLOG(ERROR)/ ? Deleted https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... media/capture/video_capture_types.h:31: enum CameraFacing { On 2017/01/05 20:26:37, mcasas wrote: > This should be an enum class, right? Done. https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... media/capture/video_capture_types.h:34: DEFAULT = FRONT, On 2017/01/05 21:55:33, miu wrote: > This is not correct for the screen capture devices (IIUC what "front means"). > > Also, could you add comments here to des ribe these? E.g., "FRONT means a > person's right will be on the left in the video." Done. https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... File media/capture/video_capturer_source.h (right): https://codereview.chromium.org/2609863004/diff/60001/media/capture/video_cap... media/capture/video_capturer_source.h:118: virtual CameraFacing GetCameraFacing() { return CameraFacing::FRONT; } On 2017/01/05 20:26:37, mcasas wrote: > This should be s/FRONT/DEFAULT/ or perhaps > /UNKNOWN/, right? Because we don't know at > this point where is the camera pointing to, and > it could be movable like a USB one. > > What about making this method const? Done. https://codereview.chromium.org/2609863004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right): https://codereview.chromium.org/2609863004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMediaStreamTrack.h:55: VideoFacingMode facingMode = VideoFacingMode::User; On 2017/01/05 20:26:37, mcasas wrote: > WebMediaStreamTrack applies to non-video tracks > (currently only audio), so there should be some > kind of FacingMode "none" or "unknown", but there > isn't such thing in the spec [1], so please make a > Spec bug for it and link it here as TODO. > > Perhaps also remove the "Video" from it, since > we could have microphones orientations, right? > > > [1] https://www.w3.org/TR/mediacapture-streams/#def-constraint-facingMode Done.
On 2017/01/05 12:17:59, jochen wrote: > i'm still a bit confused why we have videofacingmode and camerafacing instead of > just one of them? Because it's kind of weird to include third_party/WebKit/public/platform/WebMediaStreamTrack.h everywhere when camera-facing enum is used. I would like to restrict the use of enum in WebMediaStreamTrack.h to the layer where we expose the information to public API. > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > File content/browser/renderer_host/media/media_stream_manager.cc (right): > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > content/browser/renderer_host/media/media_stream_manager.cc:52: #include > "media/capture/video/linux/camera_facing_chromeos.h" > we shouldn't unconditionally include platform specific headers > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > content/browser/renderer_host/media/media_stream_manager.cc:72: > base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = > why is that a singleton can't we create a new instance when we need one? > > also, platform specific code should either go into platform specific files, or > needs to be #ifdef'd > > https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... > content/common/media/media_devices.h:41: std::string model_id; > why not a pair of ints then?
lgtm https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... content/common/media/media_devices.h:41: std::string model_id; On 2017/01/06 09:43:01, shenghao wrote: > On 2017/01/05 12:17:59, jochen wrote: > > why not a pair of ints then? > > We use the formatted "vid:pid" string as identifier in camera config files. When > we get model_id from VideoCaptureDeviceDescriptor, it's already in this format, > so converting it to 2 ints and back seems redundant. Plus that separated vid or > pid are not useful anywhere in the system. Another reason is we'll have non-USB cameras soon. Those cameras do not have vid and pid. https://codereview.chromium.org/2609863004/diff/80001/media/capture/video_cap... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/80001/media/capture/video_cap... media/capture/video_capture_types.h:33: // FRONT means a person's right will be on the left in the video. This is not always true. Apps can choose not to mirror the video. To be consistent, we can follow mediacapture spec. FRONT means "The camera is facing toward the user (a self-view camera)." https://www.w3.org/TR/mediacapture-streams/#idl-def-VideoFacingModeEnum.user https://codereview.chromium.org/2609863004/diff/80001/media/capture/video_cap... media/capture/video_capture_types.h:34: FRONT, Also document BACK. // The source is facing away from the user (viewing the environment).
On 2017/01/06 at 09:49:26, shenghao wrote: > On 2017/01/05 12:17:59, jochen wrote: > > i'm still a bit confused why we have videofacingmode and camerafacing instead of > > just one of them? > > Because it's kind of weird to include third_party/WebKit/public/platform/WebMediaStreamTrack.h everywhere when camera-facing enum is used. > I would like to restrict the use of enum in WebMediaStreamTrack.h to the layer where we expose the information to public API. but that doesn't mean that the internal enum needs to have a different name and different values.. > > > > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > > File content/browser/renderer_host/media/media_stream_manager.cc (right): > > > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > > content/browser/renderer_host/media/media_stream_manager.cc:52: #include > > "media/capture/video/linux/camera_facing_chromeos.h" > > we shouldn't unconditionally include platform specific headers > > > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > > content/browser/renderer_host/media/media_stream_manager.cc:72: > > base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = > > why is that a singleton can't we create a new instance when we need one? > > > > also, platform specific code should either go into platform specific files, or > > needs to be #ifdef'd > > > > https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... > > File content/common/media/media_devices.h (right): > > > > https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... > > content/common/media/media_devices.h:41: std::string model_id; > > why not a pair of ints then?
On 2017/01/09 at 08:29:17, jochen wrote: > On 2017/01/06 at 09:49:26, shenghao wrote: > > On 2017/01/05 12:17:59, jochen wrote: > > > i'm still a bit confused why we have videofacingmode and camerafacing instead of > > > just one of them? > > > > Because it's kind of weird to include third_party/WebKit/public/platform/WebMediaStreamTrack.h everywhere when camera-facing enum is used. > > I would like to restrict the use of enum in WebMediaStreamTrack.h to the layer where we expose the information to public API. > > but that doesn't mean that the internal enum needs to have a different name and different values.. also, for chromium code, it's ok to include a blink header everywhere: you'd introduce a new header that just defines an enum class, and that header can be included everywhere > > > > > > > > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > > > File content/browser/renderer_host/media/media_stream_manager.cc (right): > > > > > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > > > content/browser/renderer_host/media/media_stream_manager.cc:52: #include > > > "media/capture/video/linux/camera_facing_chromeos.h" > > > we shouldn't unconditionally include platform specific headers > > > > > > https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... > > > content/browser/renderer_host/media/media_stream_manager.cc:72: > > > base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = > > > why is that a singleton can't we create a new instance when we need one? > > > > > > also, platform specific code should either go into platform specific files, or > > > needs to be #ifdef'd > > > > > > https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... > > > File content/common/media/media_devices.h (right): > > > > > > https://codereview.chromium.org/2609863004/diff/60001/content/common/media/me... > > > content/common/media/media_devices.h:41: std::string model_id; > > > why not a pair of ints then?
On Mon, Jan 9, 2017 at 4:29 PM, <jochen@chromium.org> wrote: > On 2017/01/06 at 09:49:26, shenghao wrote: > > On 2017/01/05 12:17:59, jochen wrote: > > > i'm still a bit confused why we have videofacingmode and camerafacing > instead of > > > just one of them? > > > > Because it's kind of weird to include > third_party/WebKit/public/platform/WebMediaStreamTrack.h everywhere when > camera-facing enum is used. > > I would like to restrict the use of enum in WebMediaStreamTrack.h to the > layer > where we expose the information to public API. > > but that doesn't mean that the internal enum needs to have a different > name and > different values.. > Sheng-hao. We should either follow WebKit or Android naming/values. Android uses FRONT, BACK, and EXTERNAL. Android doesn't have DEFAULT and its FRONT value is 0. https://developer.android.com/reference/android/hardware/camera2/CameraCharac... > > > > > > > > > > > https://codereview.chromium.org/2609863004/diff/60001/ > 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/60001/ > content/browser/renderer_host/media/media_stream_manager.cc#newcode52 > > > content/browser/renderer_host/media/media_stream_manager.cc:52: > #include > > > "media/capture/video/linux/camera_facing_chromeos.h" > > > we shouldn't unconditionally include platform specific headers > > > > > > > https://codereview.chromium.org/2609863004/diff/60001/ > content/browser/renderer_host/media/media_stream_manager.cc#newcode72 > > > content/browser/renderer_host/media/media_stream_manager.cc:72: > > > base::LazyInstance<media::CameraFacingChromeOS>::Leaky > g_camera_facing_ = > > > why is that a singleton can't we create a new instance when we need > one? > > > > > > also, platform specific code should either go into platform specific > files, > or > > > needs to be #ifdef'd > > > > > > > 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; > > > why not a pair of ints then? > > > > https://codereview.chromium.org/2609863004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, Jan 9, 2017 at 4:29 PM, <jochen@chromium.org> wrote: > On 2017/01/06 at 09:49:26, shenghao wrote: > > On 2017/01/05 12:17:59, jochen wrote: > > > i'm still a bit confused why we have videofacingmode and camerafacing > instead of > > > just one of them? > > > > Because it's kind of weird to include > third_party/WebKit/public/platform/WebMediaStreamTrack.h everywhere when > camera-facing enum is used. > > I would like to restrict the use of enum in WebMediaStreamTrack.h to the > layer > where we expose the information to public API. > > but that doesn't mean that the internal enum needs to have a different > name and > different values.. > Sheng-hao. We should either follow WebKit or Android naming/values. Android uses FRONT, BACK, and EXTERNAL. Android doesn't have DEFAULT and its FRONT value is 0. https://developer.android.com/reference/android/hardware/camera2/CameraCharac... > > > > > > > > > > > https://codereview.chromium.org/2609863004/diff/60001/ > 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/60001/ > content/browser/renderer_host/media/media_stream_manager.cc#newcode52 > > > content/browser/renderer_host/media/media_stream_manager.cc:52: > #include > > > "media/capture/video/linux/camera_facing_chromeos.h" > > > we shouldn't unconditionally include platform specific headers > > > > > > > https://codereview.chromium.org/2609863004/diff/60001/ > content/browser/renderer_host/media/media_stream_manager.cc#newcode72 > > > content/browser/renderer_host/media/media_stream_manager.cc:72: > > > base::LazyInstance<media::CameraFacingChromeOS>::Leaky > g_camera_facing_ = > > > why is that a singleton can't we create a new instance when we need > one? > > > > > > also, platform specific code should either go into platform specific > files, > or > > > needs to be #ifdef'd > > > > > > > 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; > > > why not a pair of ints then? > > > > https://codereview.chromium.org/2609863004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I changed the fields in the enum to be the same as Webkit. I can't include files in the Webkit directory -- "git cl upload" checks prohibit that. https://codereview.chromium.org/2609863004/diff/80001/media/capture/video_cap... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/80001/media/capture/video_cap... media/capture/video_capture_types.h:33: // FRONT means a person's right will be on the left in the video. On 2017/01/09 08:28:00, wuchengli wrote: > This is not always true. Apps can choose not to mirror the video. To be > consistent, we can follow mediacapture spec. FRONT means "The camera is facing > toward the user (a self-view camera)." > > https://www.w3.org/TR/mediacapture-streams/#idl-def-VideoFacingModeEnum.user Done. https://codereview.chromium.org/2609863004/diff/80001/media/capture/video_cap... media/capture/video_capture_types.h:34: FRONT, On 2017/01/09 08:28:00, wuchengli wrote: > Also document BACK. > > // The source is facing away from the user (viewing the environment). Done.
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
what's the error you get when you upload?
Comments on PS8: https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); On 2017/01/06 09:43:01, shenghao wrote: > On 2017/01/05 21:55:33, miu wrote: > > nit: push_back() would be more appropriate here. > > Why? If using push_back(), I will need to create a temporary MediaDeviceInfo > object and incur one extra copy. push_back() takes a const-ref argument (or an rvalue). You are giving it a const-ref type. std::vector will copy-construct in-place. There is no extra copy. ;-) emplace_back() takes arguments to a constructor of the class. So, C++ will take your const-ref and invoke the copy-constructor in-place. Same thing as push_back, but the compiler may have to do more work to parse/analyze the varargs and choose a constructor. http://en.cppreference.com/w/cpp/container/vector/emplace_back http://en.cppreference.com/w/cpp/container/vector/push_back Like I said, a nit point, but since your intention is to insert a copy of an existing object (as opposed to constructing a different object and inserting it), push_back() is slightly more appropriate. https://codereview.chromium.org/2609863004/diff/140001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/140001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:237: GetVideoFacing(info.device_id, info.model_id)); This returns the wrong value if content::IsScreenCaptureMediaType(stream_type) returns true. NONE will map to USER in LocalVideoCapturerSource::GetVideoFacing(), which is wrong for the screen capture devices. https://codereview.chromium.org/2609863004/diff/140001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/140001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:360: default: Again, please do not use default clauses when processing enums. (See my comments from last week.)
On 2017/01/10 14:18:33, jochen wrote:
> what's the error you get when you upload?
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
media/capture/video_capture_types.h
Illegal include:
"third_party/WebKit/public/platform/WebMediaStreamTrack.h"
Because of no rule applying.
https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... 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 09:43:01, shenghao wrote: > > On 2017/01/05 21:55:33, miu wrote: > > > nit: push_back() would be more appropriate here. > > > > Why? If using push_back(), I will need to create a temporary MediaDeviceInfo > > object and incur one extra copy. > > push_back() takes a const-ref argument (or an rvalue). You are giving it a > const-ref type. std::vector will copy-construct in-place. There is no extra > copy. ;-) > > emplace_back() takes arguments to a constructor of the class. So, C++ will take > your const-ref and invoke the copy-constructor in-place. Same thing as > push_back, but the compiler may have to do more work to parse/analyze the > varargs and choose a constructor. > > http://en.cppreference.com/w/cpp/container/vector/emplace_back > http://en.cppreference.com/w/cpp/container/vector/push_back > > Like I said, a nit point, but since your intention is to insert a copy of an > existing object (as opposed to constructing a different object and inserting > it), push_back() is slightly more appropriate. Actually, |snapshot| is a vector of MediaDeviceInfo and |descriptor| is a VideoCaptureDeviceDescriptor, so I am creating the MediaDeviceInfo object through emplace_back(). ;) https://codereview.chromium.org/2609863004/diff/140001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/140001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:237: GetVideoFacing(info.device_id, info.model_id)); On 2017/01/10 22:14:23, miu wrote: > This returns the wrong value if content::IsScreenCaptureMediaType(stream_type) > returns true. NONE will map to USER in > LocalVideoCapturerSource::GetVideoFacing(), which is wrong for the screen > capture devices. Done. https://codereview.chromium.org/2609863004/diff/140001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/140001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:360: default: On 2017/01/10 22:14:23, miu wrote: > Again, please do not use default clauses when processing enums. (See my comments > from last week.) Done.
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
after digging around a bit, it seems like there's a project underway to move capture to mojo. In that case, blink will be able to access video_capture_types.mojom-shared.h directly. I'd be fine with temporarily allowing this already now by putting a rule in public/platform/DEPS mcasas, wdyt? https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right): https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaStreamTrack.h:45: // TODO(hta): b/34118695. Consider to rename it to FacingMode. as a rule of thumb, please never refer to buganizer in chromium sources
lgtm https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_manager.cc (right): https://codereview.chromium.org/2609863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/media_devices_manager.cc:388: snapshot.emplace_back(descriptor); On 2017/01/11 12:00:53, shenghao wrote: > On 2017/01/10 22:14:23, miu wrote: > > On 2017/01/06 09:43:01, shenghao wrote: > > > On 2017/01/05 21:55:33, miu wrote: > > > > nit: push_back() would be more appropriate here. > > > > > > Why? If using push_back(), I will need to create a temporary MediaDeviceInfo > > > object and incur one extra copy. > > > > push_back() takes a const-ref argument (or an rvalue). You are giving it a > > const-ref type. std::vector will copy-construct in-place. There is no extra > > copy. ;-) > > > > emplace_back() takes arguments to a constructor of the class. So, C++ will > take > > your const-ref and invoke the copy-constructor in-place. Same thing as > > push_back, but the compiler may have to do more work to parse/analyze the > > varargs and choose a constructor. > > > > http://en.cppreference.com/w/cpp/container/vector/emplace_back > > http://en.cppreference.com/w/cpp/container/vector/push_back > > > > Like I said, a nit point, but since your intention is to insert a copy of an > > existing object (as opposed to constructing a different object and inserting > > it), push_back() is slightly more appropriate. > > Actually, |snapshot| is a vector of MediaDeviceInfo and |descriptor| is a > VideoCaptureDeviceDescriptor, so I am creating the MediaDeviceInfo object > through emplace_back(). ;) > > OIC! Okay then, sounds good to me. :)
Looking good, just still concerned about the way to make sure AudioTracks and VideoTracks that are not webcam-originated (i.e. tab/screen/window/ canvas/etc) do not get a video facing mode pegged by mistake. jochen@ regarding the types, yeah, we can homogenise them further when services::video_capture is fully landed. *IMPORTANT*: I can only see webrtc: and a b/ bugs: --> This won't do, since adding all this code will need an intent-to-ship sent to blink-dev@ (there has been an intent-to-implement sent to that list too, right?). Also you'll need an OWP Chromium bug and possibly an entry in chromestatus.com http://www.chromium.org/blink/launching-features https://codereview.chromium.org/2609863004/diff/160001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:73: base::LazyInstance<media::CameraFacingChromeOS>::Leaky g_camera_facing_ = s/g_camera_facing_/g_camera_facing/ (no trailing underscore if not a member). https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:355: switch (facing_) { I'd add here if (is_content_capture_) return media::VideoFacingMode::NONE; https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... so tab/desktop/window capture will not have a Facing Mode (assuming this is what we want). https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_source.h (right): https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_source.h:94: } In line with my other comments, return here a new enum entry media::VideoFacingMode::NONE; https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_track.cc:328: } And here, if we get a media::VideoFacingMode::None, don't fill in |facing|. Which should do the trick assuming it has been changed to base::Optional<> as mentioned elsewhere. https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... media/capture/video_capture_types.h:39: RIGHT, I'd add here NONE/UNKNOWN or similar. https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... File media/capture/video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... media/capture/video_capturer_source.cc:17: return VideoFacingMode::USER; Here I would return VideoFacingMode::NONE; https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right): https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaStreamTrack.h:56: VideoFacingMode facingMode = VideoFacingMode::User; I made a comment that's possibly lost in the review: > WebMediaStreamTrack applies to non-video tracks > (currently only audio), so there should be some >kind of FacingMode "none" or "unknown". I don't like that, as-is, AudioTracks, or even VideoTracks coming from a Screen/Tab/Desktop/<canvas>/<video> can also have a "facingMode" :-S I propose making this field [1] a base::Optional<VideoFacingMode> so that, if it's not applicable, we can return an empty string to the JS user. base::Optional is perfect for this case [2]: "A very common use case is for classes and structures that have an object not always available, because it is early initialized or because the underlying data structure doesn't require it." [1] https://cs.chromium.org/chromium/src/base/optional.h?q=base::optional&sq=pack... [2] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
> Looking good, just still concerned about the way > to make sure AudioTracks and VideoTracks that > are not webcam-originated (i.e. tab/screen/window/ > canvas/etc) do not get a video facing mode pegged > by mistake. > > jochen@ regarding the types, yeah, we can homogenise > them further when services::video_capture is fully landed. > Ok. I will keep the enums as is for now. > *IMPORTANT*: I can only see webrtc: and a b/ bugs: > --> This won't do, since adding all this code will need > an intent-to-ship sent to blink-dev@ (there has been > an intent-to-implement sent to that list too, right?). > Also you'll need an OWP Chromium bug and possibly > an entry in http://chromestatus.com > http://www.chromium.org/blink/launching-features Intent to ship has been sent out by hta@: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/s-NMzjSfVQs This CL is part of his effort. hta@ will handle the launching process of the whole thing.
https://codereview.chromium.org/2609863004/diff/160001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/browser/render... 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: > s/g_camera_facing_/g_camera_facing/ (no trailing underscore > if not a member). Done. https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:355: switch (facing_) { On 2017/01/11 22:24:58, mcasas wrote: > I'd add here > if (is_content_capture_) > return media::VideoFacingMode::NONE; > > > https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... > > so tab/desktop/window capture will not have a Facing > Mode (assuming this is what we want). > Done. https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_source.h (right): https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_source.h:94: } On 2017/01/11 22:24:58, mcasas wrote: > In line with my other comments, return here a new enum entry > media::VideoFacingMode::NONE; > Done. https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/160001/content/renderer/media... content/renderer/media/media_stream_video_track.cc:328: } On 2017/01/11 22:24:58, mcasas wrote: > And here, if we get a media::VideoFacingMode::None, don't > fill in |facing|. Which should do the trick assuming it has been > changed to base::Optional<> as mentioned elsewhere. Done. https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... File media/capture/video_capture_types.h (right): https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... media/capture/video_capture_types.h:39: RIGHT, On 2017/01/11 22:24:59, mcasas wrote: > I'd add here NONE/UNKNOWN or similar. Done. https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... File media/capture/video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/160001/media/capture/video_ca... media/capture/video_capturer_source.cc:17: return VideoFacingMode::USER; On 2017/01/11 22:24:59, mcasas wrote: > Here I would return VideoFacingMode::NONE; Done. https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right): https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaStreamTrack.h:45: // TODO(hta): b/34118695. Consider to rename it to FacingMode. On 2017/01/11 13:23:27, jochen wrote: > as a rule of thumb, please never refer to buganizer in chromium sources Done. https://codereview.chromium.org/2609863004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaStreamTrack.h:56: VideoFacingMode facingMode = VideoFacingMode::User; On 2017/01/11 22:24:59, mcasas wrote: > I made a comment that's possibly lost in the review: > > > WebMediaStreamTrack applies to non-video tracks > > (currently only audio), so there should be some > >kind of FacingMode "none" or "unknown". > > I don't like that, as-is, AudioTracks, or even VideoTracks > coming from a Screen/Tab/Desktop/<canvas>/<video> > can also have a "facingMode" :-S > > I propose making this field [1] a base::Optional<VideoFacingMode> > so that, if it's not applicable, we can return an empty string to > the JS user. base::Optional is perfect for this case [2]: > > "A very common use case is for classes and structures > that have an object not always available, because it is > early initialized or because the underlying data > structure doesn't require it." > > [1] > https://cs.chromium.org/chromium/src/base/optional.h?q=base::optional&sq=pack... > [2] > https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When... Done.
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:200001) has been deleted
On 2017/01/12 at 05:55:24, shenghao wrote: > > Looking good, just still concerned about the way > > to make sure AudioTracks and VideoTracks that > > are not webcam-originated (i.e. tab/screen/window/ > > canvas/etc) do not get a video facing mode pegged > > by mistake. > > > > jochen@ regarding the types, yeah, we can homogenise > > them further when services::video_capture is fully landed. > > > Ok. I will keep the enums as is for now. can you just include the mojo header and use that enum? > > > > *IMPORTANT*: I can only see webrtc: and a b/ bugs: > > --> This won't do, since adding all this code will need > > an intent-to-ship sent to blink-dev@ (there has been > > an intent-to-implement sent to that list too, right?). > > Also you'll need an OWP Chromium bug and possibly > > an entry in http://chromestatus.com > > http://www.chromium.org/blink/launching-features > > Intent to ship has been sent out by hta@: > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/s-NMzjSfVQs > This CL is part of his effort. hta@ will handle the launching process of the whole thing. can you remove the buganizer reference, and replace it with the OWP tracking bug?
Description was changed from ========== 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, b:31580933 TEST=Log the camera facing value in VideoCaptureImplManager ========== to ========== 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 TEST=Log the camera facing value in VideoCaptureImplManager ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
shenghao@chromium.org changed reviewers: + nasko@chromium.org
Patchset #6 (id:220001) has been deleted
Patchset #5 (id:180001) has been deleted
On 2017/01/12 08:43:27, jochen wrote: > On 2017/01/12 at 05:55:24, shenghao wrote: > > > Looking good, just still concerned about the way > > > to make sure AudioTracks and VideoTracks that > > > are not webcam-originated (i.e. tab/screen/window/ > > > canvas/etc) do not get a video facing mode pegged > > > by mistake. > > > > > > jochen@ regarding the types, yeah, we can homogenise > > > them further when services::video_capture is fully landed. > > > > > Ok. I will keep the enums as is for now. > > can you just include the mojo header and use that enum? > Done. > > > > > > > *IMPORTANT*: I can only see webrtc: and a b/ bugs: > > > --> This won't do, since adding all this code will need > > > an intent-to-ship sent to blink-dev@ (there has been > > > an intent-to-implement sent to that list too, right?). > > > Also you'll need an OWP Chromium bug and possibly > > > an entry in http://chromestatus.com > > > http://www.chromium.org/blink/launching-features > > > > Intent to ship has been sent out by hta@: > > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/s-NMzjSfVQs > > This CL is part of his effort. hta@ will handle the launching process of the > whole thing. > > can you remove the buganizer reference, and replace it with the OWP tracking > bug? Deleted the b/ bug. hta@ hasn't filed a OWP bug, but that is not required for submitted this CL, right?
Description was changed from ========== 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 TEST=Log the camera facing value in VideoCaptureImplManager ========== to ========== 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 ==========
On 2017/01/12 11:40:15, jochen wrote: > he filed https://bugs.chromium.org/p/chromium/issues/detail?id=543997 Updated
much better, thx! https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:240: base::ThreadRestrictions::SetIOAllowed(true); why is this required? how long will the operation block? Should this be done on a background thread? https://codereview.chromium.org/2609863004/diff/240001/content/common/media/m... File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/240001/content/common/media/m... content/common/media/media_devices.h:29: MediaDeviceInfo(const MediaDeviceInfo& other); why do you need the copy ctor here? https://codereview.chromium.org/2609863004/diff/240001/content/common/media/m... content/common/media/media_devices.h:33: const std::string& group_id); this should also take the model_id https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:230: const VideoFacingMode facing_; why not use media::mojom::VideoFacingMode ? https://codereview.chromium.org/2609863004/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/DEPS (right): https://codereview.chromium.org/2609863004/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/DEPS:16: "+media/capture/mojo/video_capture_types.mojom-shared.h", this should go above the comment, and you should add a similar comment about capture referencing crbug.com/584797
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hta@chromium.org changed reviewers: + hta@chromium.org
Please talk to me. I've been doing the same thing in https://codereview.chromium.org/2590193002/ which is a *lot* shorter. The problem I have is that the only device that exposes the information is on Android, but I don't have an Android platform to test on.
Is there a design document for this change? I see a very large amount of change, without a clear picture of what it's supposed to be doing, and most of it seems to be avoidable. And at the moment, there are no unit tests. So this is not landable. My recommendation: - Help land https://codereview.chromium.org/2590193002/ (currently waiting on esprehn), so that you have the Web API installed - Go back to storing the information in device_info.device.video_facing as it was before. Remove the whole use of a mojom enum; at this stage, it just seems to complicate things. - Add the LEFT and RIGHT variants to VideoFacingMode - Make unit tests to check that video_facing is set correctly in your various scenarios - Profit. https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:75: LAZY_INSTANCE_INITIALIZER; If this is a singleton, does it make the assumption that a ChromeOS device has only one camera? If not, how is a ChromeOS device with two cameras represented? Remember that ChromeOS devices have USB ports; cameras attached there will have to have facing = "none". https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:232: return VideoFacingMode::MEDIA_VIDEO_FACING_ENVIRONMENT; How does this choice make sense? I'd have thought not saying anything (ie none) would be more correct. https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:262: facing_(device_info.device.video_facing), Why are you making a copy of this information? https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.h (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.h:37: media::mojom::VideoFacingMode GetVideoFacing() override; Why are you introducing a new mojom enum instead of using the VideoFacingMode from media_stream_request.h? https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_track.cc:310: settings.facingMode = source_->GetVideoFacing(); Why aren't you picking this out of source_->device_info().device.video_facing? https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... File media/capture/mojo/video_capture_types.mojom (right): https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... media/capture/mojo/video_capture_types.mojom:29: // TODO(hta): crbug:680396. Consider to rename it to FacingMode. The convention for TODO is TODO(name of person who entered the todo). https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... media/capture/mojo/video_capture_types.mojom:40: }; I don't understand what value this Mojom definition adds over the naked enum VideoFacingMode. You seem to be using it as an enum everywhere.
https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... File media/capture/mojo/video_capture_types.mojom (right): https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... media/capture/mojo/video_capture_types.mojom:40: }; On 2017/01/12 at 19:36:41, hta - Chromium wrote: > I don't understand what value this Mojom definition adds over the naked enum VideoFacingMode. You seem to be using it as an enum everywhere. we should remove the other enum, and just use the mojom version everywhere (so we don't have different enums in media and content and blink for the exact same thing)
On 2017/01/13 08:47:09, jochen wrote: > https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... > File media/capture/mojo/video_capture_types.mojom (right): > > https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... > media/capture/mojo/video_capture_types.mojom:40: }; > On 2017/01/12 at 19:36:41, hta - Chromium wrote: > > I don't understand what value this Mojom definition adds over the naked enum > VideoFacingMode. You seem to be using it as an enum everywhere. > > we should remove the other enum, and just use the mojom version everywhere (so > we don't have different enums in media and content and blink for the exact same > thing) Why is a mojom enum better than a straightforward enum, given that this interface is not using mojo at the moment? I notice that the use of mojom ends up having "media::mojom::" as a prefix everywhere - this adds visual noise; is there an approved way to avoid it?
https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:75: LAZY_INSTANCE_INITIALIZER; On 2017/01/12 19:36:41, hta - Chromium wrote: > If this is a singleton, does it make the assumption that a ChromeOS device has > only one camera? > No. > If not, how is a ChromeOS device with two cameras represented? > Remember that ChromeOS devices have USB ports; cameras attached there will have > to have facing = "none". g_camera_facing is a helper class that takes in model_id and/or usb_id and tell you what facing the device is. Facing information for each device is stored in MediaStreamDevice. If one chromebook has 2 cameras, then there will be 2 MediaStreamDevice. I rename g_camera_facing to g_camera_facing_helper to make it more clear. For externally plugged in cameras, g_camera_facing_helper.Get().GetCameraFacing() would return NONE. https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:232: return VideoFacingMode::MEDIA_VIDEO_FACING_ENVIRONMENT; On 2017/01/12 19:36:41, hta - Chromium wrote: > How does this choice make sense? > > I'd have thought not saying anything (ie none) would be more correct. done https://codereview.chromium.org/2609863004/diff/240001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:240: base::ThreadRestrictions::SetIOAllowed(true); On 2017/01/12 11:54:41, jochen wrote: > why is this required? how long will the operation block? Should this be done on > a background thread? Because we need to access camera config file when g_camera_facing is created. Now I moved it to when VideoCaptureDeviceDescriptor is created, so no need to access IO here anymore. https://codereview.chromium.org/2609863004/diff/240001/content/common/media/m... File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/240001/content/common/media/m... content/common/media/media_devices.h:29: MediaDeviceInfo(const MediaDeviceInfo& other); On 2017/01/12 11:54:42, jochen wrote: > why do you need the copy ctor here? Required by chromium trybot checks. A complex class needs to have out-of-line copy ctor https://codereview.chromium.org/2609863004/diff/240001/content/common/media/m... content/common/media/media_devices.h:33: const std::string& group_id); On 2017/01/12 11:54:42, jochen wrote: > this should also take the model_id Not every media device has model_id. Ex. Audio devices using hdmi or bluetooth do not have it. https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:230: const VideoFacingMode facing_; On 2017/01/12 11:54:42, jochen wrote: > why not use media::mojom::VideoFacingMode ? deleted https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.cc:262: facing_(device_info.device.video_facing), On 2017/01/12 19:36:41, hta - Chromium wrote: > Why are you making a copy of this information? deleted https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_capturer_source.h (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_capturer_source.h:37: media::mojom::VideoFacingMode GetVideoFacing() override; On 2017/01/12 19:36:41, hta - Chromium wrote: > Why are you introducing a new mojom enum instead of using the VideoFacingMode > from media_stream_request.h? Deleted https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/2609863004/diff/240001/content/renderer/media... content/renderer/media/media_stream_video_track.cc:310: settings.facingMode = source_->GetVideoFacing(); On 2017/01/12 19:36:41, hta - Chromium wrote: > Why aren't you picking this out of source_->device_info().device.video_facing? Deleted. https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... File media/capture/mojo/video_capture_types.mojom (right): https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... media/capture/mojo/video_capture_types.mojom:29: // TODO(hta): crbug:680396. Consider to rename it to FacingMode. On 2017/01/12 19:36:41, hta - Chromium wrote: > The convention for TODO is TODO(name of person who entered the todo). Deleted https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... media/capture/mojo/video_capture_types.mojom:40: }; On 2017/01/12 19:36:41, hta - Chromium wrote: > I don't understand what value this Mojom definition adds over the naked enum > VideoFacingMode. You seem to be using it as an enum everywhere. Deleted https://codereview.chromium.org/2609863004/diff/240001/media/capture/mojo/vid... media/capture/mojo/video_capture_types.mojom:40: }; On 2017/01/13 08:47:09, jochen wrote: > On 2017/01/12 at 19:36:41, hta - Chromium wrote: > > I don't understand what value this Mojom definition adds over the naked enum > VideoFacingMode. You seem to be using it as an enum everywhere. > > we should remove the other enum, and just use the mojom version everywhere (so > we don't have different enums in media and content and blink for the exact same > thing) After looking at hta@'s CL (https://codereview.chromium.org/2590193002/), I don't need to modify renderer and Webkit anymore, and therefore no enum needs to be added. https://codereview.chromium.org/2609863004/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/DEPS (right): https://codereview.chromium.org/2609863004/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/DEPS:16: "+media/capture/mojo/video_capture_types.mojom-shared.h", On 2017/01/12 11:54:42, jochen wrote: > this should go above the comment, and you should add a similar comment about > capture referencing crbug.com/584797 Deleted
I will wait for https://codereview.chromium.org/2590193002/ to be landed before submitting this CL. Please review the current version. I will write unit tests after reviewers agree with the current design.
I'll defer to hta / mcasas on the design
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This looks good to me now, apart from the linking issue, which I think you can solve by moving the video facing mode enum into a file called "media/base/video_parameters.h", parallel to the existing audio_parameters file. then let media_stream_request include this file, and other uses of the facing mode enum can include just that file. Hope the API owners are comfortable with that solution.
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shenghao@chromium.org changed reviewers: + chcunningham@chromium.org
On 2017/01/17 12:25:48, hta - Chromium wrote: > This looks good to me now, apart from the linking issue, which I think you can > solve by moving the video facing mode enum into a file called > "media/base/video_parameters.h", parallel to the existing audio_parameters file. > > then let media_stream_request include this file, and other uses of the facing > mode enum can include just that file. > > Hope the API owners are comfortable with that solution. I moved the enum to media/base/video_parameters.h. chcunningham@, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/17 13:16:59, shenghao wrote: > On 2017/01/17 12:25:48, hta - Chromium wrote: > > This looks good to me now, apart from the linking issue, which I think you can > > solve by moving the video facing mode enum into a file called > > "media/base/video_parameters.h", parallel to the existing audio_parameters > file. > > > > then let media_stream_request include this file, and other uses of the facing > > mode enum can include just that file. > > > > Hope the API owners are comfortable with that solution. > > I moved the enum to media/base/video_parameters.h. > chcunningham@, please take a look. Can you rename to media/base/video_facing.h ? I think defining this enum is more analogous to media/base/video_rotation.h (name with narrow scope), whereas audio_parameters.h is for a broad set of system audio params.
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shenghao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by shenghao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wuchengli@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2609863004/#ps340001 (title: "try to fix trybot errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
lgtm
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
On 2017/01/19 02:41:20, commit-bot: I haz the power wrote: > 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_presub...) jochen@, need your LGTM for content/common/media/media_stream_messages.h content/public/common/media_stream_request.cc content/public/common/media_stream_request.h
guidou@chromium.org changed reviewers: + guidou@chromium.org
https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... File content/common/media/media_devices.h (right): https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... content/common/media/media_devices.h:43: media::VideoFacingMode facing; Don't add the facing field here. To properly support the new features in the MediaStream spec, including getSettings, we are introducing different IPCs and structures. Also, remove any other changes in the CL that depend on changes to MediaDeviceInfo. The rest of the CL looks fine to me.
On 2017/01/19 10:29:41, Guido Urdaneta wrote: > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... > content/common/media/media_devices.h:43: media::VideoFacingMode facing; > Don't add the facing field here. > To properly support the new features in the MediaStream spec, including > getSettings, we are introducing different IPCs and structures. > Also, remove any other changes in the CL that depend on changes to > MediaDeviceInfo. > The rest of the CL looks fine to me. Do you have design doc or CLs? What's your timeline look like? We planned to launch this in m57. It's kind of late and we want to get it out sooner than later. If I remove |facing| in media::VideoFacingMode, 60% of this CL will disappear. Only remaining logic would be populating |facing| in video_capture_device_descriptor.cc.
On 2017/01/19 10:29:41, Guido Urdaneta wrote: > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... > content/common/media/media_devices.h:43: media::VideoFacingMode facing; > Don't add the facing field here. > To properly support the new features in the MediaStream spec, including > getSettings, we are introducing different IPCs and structures. > Also, remove any other changes in the CL that depend on changes to > MediaDeviceInfo. > The rest of the CL looks fine to me. Before this CL, facing mode is part of MediaStreamDevice only. After this CL, it is part of MediaDeviceInfo, which allows the media stream manager to return it. The MediaDeviceInfo will not be used to pass capabilities once Guido's changes land (there will be new, Mojo-based structures), so the facing mode in MediaDeviceInfo will become useless at that time. I think it's OK to land this change now, and take the field out again when the Mojo-based capability enumeration lands.
On 2017/01/19 10:29:41, Guido Urdaneta wrote: > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... > File content/common/media/media_devices.h (right): > > https://codereview.chromium.org/2609863004/diff/340001/content/common/media/m... > content/common/media/media_devices.h:43: media::VideoFacingMode facing; > Don't add the facing field here. > To properly support the new features in the MediaStream spec, including > getSettings, we are introducing different IPCs and structures. > Also, remove any other changes in the CL that depend on changes to > MediaDeviceInfo. > The rest of the CL looks fine to me. Before this CL, facing mode is part of MediaStreamDevice only. After this CL, it is part of MediaDeviceInfo, which allows the media stream manager to return it. The MediaDeviceInfo will not be used to pass capabilities once Guido's changes land (there will be new, Mojo-based structures), so the facing mode in MediaDeviceInfo will become useless at that time. I think it's OK to land this change now, and take the field out again when the Mojo-based capability enumeration lands.
lgtm
The CQ bit was checked by shenghao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
> Before this CL, facing mode is part of MediaStreamDevice only. > After this CL, it is part of MediaDeviceInfo, which allows the media stream > manager to return it. > > The MediaDeviceInfo will not be used to pass capabilities once Guido's changes > land (there will be new, Mojo-based structures), so the facing mode in > MediaDeviceInfo will become useless at that time. > > I think it's OK to land this change now, and take the field out again when the > Mojo-based capability enumeration lands.
On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > Before this CL, facing mode is part of MediaStreamDevice only. > > After this CL, it is part of MediaDeviceInfo, which allows the media stream > > manager to return it. > > > > The MediaDeviceInfo will not be used to pass capabilities once Guido's changes > > land (there will be new, Mojo-based structures), so the facing mode in > > MediaDeviceInfo will become useless at that time. > > > > I think it's OK to land this change now, and take the field out again when the > > Mojo-based capability enumeration lands. Sounds reasonble, but note that you have to add the new field to media_devices_param_traits.h too.
On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > Before this CL, facing mode is part of MediaStreamDevice only. > > After this CL, it is part of MediaDeviceInfo, which allows the media stream > > manager to return it. > > > > The MediaDeviceInfo will not be used to pass capabilities once Guido's changes > > land (there will be new, Mojo-based structures), so the facing mode in > > MediaDeviceInfo will become useless at that time. > > > > I think it's OK to land this change now, and take the field out again when the > > Mojo-based capability enumeration lands. Sounds reasonable, but note that you have to add the new field to media_devices_param_traits.h too.
On 2017/01/20 09:30:25, Guido Urdaneta wrote: > On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > > Before this CL, facing mode is part of MediaStreamDevice only. > > > After this CL, it is part of MediaDeviceInfo, which allows the media stream > > > manager to return it. > > > > > > The MediaDeviceInfo will not be used to pass capabilities once Guido's > changes > > > land (there will be new, Mojo-based structures), so the facing mode in > > > MediaDeviceInfo will become useless at that time. > > > > > > I think it's OK to land this change now, and take the field out again when > the > > > Mojo-based capability enumeration lands. > > Sounds reasonable, but note that you have to add the new field to > media_devices_param_traits.h too. Can't find ways to modify media_devices_param_traits.h. I reverted media_device.*
On 2017/01/23 13:06:10, shenghao wrote: > On 2017/01/20 09:30:25, Guido Urdaneta wrote: > > On 2017/01/20 09:29:14, Guido Urdaneta wrote: > > > > Before this CL, facing mode is part of MediaStreamDevice only. > > > > After this CL, it is part of MediaDeviceInfo, which allows the media > stream > > > > manager to return it. > > > > > > > > The MediaDeviceInfo will not be used to pass capabilities once Guido's > > changes > > > > land (there will be new, Mojo-based structures), so the facing mode in > > > > MediaDeviceInfo will become useless at that time. > > > > > > > > I think it's OK to land this change now, and take the field out again when > > the > > > > Mojo-based capability enumeration lands. > > > > Sounds reasonable, but note that you have to add the new field to > > media_devices_param_traits.h too. > > Can't find ways to modify media_devices_param_traits.h. I reverted > media_device.* nasko@, we need your LGTM to submit. It requires IPC security owner's approval.
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_facin... media/base/video_facing.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2609863004/diff/380001/media/base/video_facin... media/base/video_facing.h:18: } nit: Add "// namespace media" and an empty line between the end of enum and the end of the namespace.
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_facin... media/base/video_facing.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/23 18:39:22, nasko wrote: > 2017 Done. https://codereview.chromium.org/2609863004/diff/380001/media/base/video_facin... media/base/video_facing.h:18: } On 2017/01/23 18:39:22, nasko wrote: > nit: Add "// namespace media" and an empty line between the end of enum and the > end of the namespace. Done.
The CQ bit was checked by shenghao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, chcunningham@chromium.org, wuchengli@chromium.org, miu@chromium.org, hta@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2609863004/#ps400001 (title: "change comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1485231843607120,
"parent_rev": "3deaebb8edf5fb2461d962efbfa6673f5669dacc", "commit_rev":
"5466f424bef9d4997ff78783f9bb2ec4acbbabcc"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/5466f424bef9d4997ff78783f9bb... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:400001) as https://chromium.googlesource.com/chromium/src/+/5466f424bef9d4997ff78783f9bb... |
