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

Issue 2648973003: Sort camera device list to use front camera first (Closed)

Created:
3 years, 11 months ago by henryhsu
Modified:
3 years, 11 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Sort camera device list to use front camera first Since javascript doesn't have API to get camera facing, we sort the reported camera device list by facing. Then app can use the front camera by default. BUG=684224, chrome-os-partner:61756 TEST=powerwash device and camera app uses the front camera by default. Review-Url: https://codereview.chromium.org/2648973003 Cr-Commit-Position: refs/heads/master@{#446307} Committed: https://chromium.googlesource.com/chromium/src/+/e9c70e53e5da9b0065ef7f5c6ce78d5b46e49875

Patch Set 1 #

Patch Set 2 : fix issue number #

Total comments: 4

Patch Set 3 : add comments #

Total comments: 2

Patch Set 4 : address comments #

Patch Set 5 : add bug number #

Total comments: 2

Patch Set 6 : address comments #

Total comments: 6

Patch Set 7 : address comments #

Total comments: 4

Patch Set 8 : address comment #

Total comments: 7

Patch Set 9 : address comment #

Total comments: 1

Patch Set 10 : remove #if #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -5 lines) Patch
M media/capture/video/linux/video_capture_device_factory_linux.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M media/capture/video/video_capture_device_descriptor.h View 1 2 3 4 5 6 1 chunk +1 line, -5 lines 0 comments Download
M media/capture/video/video_capture_device_descriptor.cc View 1 2 3 4 5 6 7 8 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 56 (20 generated)
henryhsu
PTAL
3 years, 11 months ago (2017-01-23 06:29:03 UTC) #2
henryhsu
3 years, 11 months ago (2017-01-23 06:30:41 UTC) #4
mtomasz
https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode231 media/capture/video/linux/video_capture_device_factory_linux.cc:231: std::sort(device_descriptors->begin(), device_descriptors->end()); Does it mean that the default camera ...
3 years, 11 months ago (2017-01-23 07:15:22 UTC) #7
henryhsu
https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode231 media/capture/video/linux/video_capture_device_factory_linux.cc:231: std::sort(device_descriptors->begin(), device_descriptors->end()); On 2017/01/23 07:15:22, mtomasz wrote: > Does ...
3 years, 11 months ago (2017-01-23 08:04:38 UTC) #8
mtomasz
On 2017/01/23 08:04:38, henryhsu wrote: > https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc > File media/capture/video/linux/video_capture_device_factory_linux.cc (right): > > https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode231 > ...
3 years, 11 months ago (2017-01-23 08:07:03 UTC) #9
henryhsu
On 2017/01/23 08:07:03, mtomasz wrote: > On 2017/01/23 08:04:38, henryhsu wrote: > > > https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc ...
3 years, 11 months ago (2017-01-23 08:55:36 UTC) #10
mtomasz
Thanks for confirming!
3 years, 11 months ago (2017-01-23 09:16:26 UTC) #12
shenghao
https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode231 media/capture/video/linux/video_capture_device_factory_linux.cc:231: std::sort(device_descriptors->begin(), device_descriptors->end()); May you add comment to document the ...
3 years, 11 months ago (2017-01-23 10:18:21 UTC) #13
henryhsu
https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode231 media/capture/video/linux/video_capture_device_factory_linux.cc:231: std::sort(device_descriptors->begin(), device_descriptors->end()); On 2017/01/23 10:18:21, shenghao wrote: > May ...
3 years, 11 months ago (2017-01-23 11:25:20 UTC) #14
shenghao
On 2017/01/23 11:25:20, henryhsu wrote: > https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc > File media/capture/video/linux/video_capture_device_factory_linux.cc (right): > > https://codereview.chromium.org/2648973003/diff/20001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode231 > ...
3 years, 11 months ago (2017-01-23 11:31:30 UTC) #15
mcasas
The bug BUG=chrome-os-partner:61756 is not publicly accessible, I think, please make a public crbug.com/ or ...
3 years, 11 months ago (2017-01-23 16:46:59 UTC) #17
henryhsu
https://codereview.chromium.org/2648973003/diff/40001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/40001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode233 media/capture/video/linux/video_capture_device_factory_linux.cc:233: // TODO(henryhsu): remove this after JS API completed. On ...
3 years, 11 months ago (2017-01-24 03:14:31 UTC) #19
wuchengli
https://codereview.chromium.org/2648973003/diff/80001/media/capture/video/video_capture_device_descriptor.h File media/capture/video/video_capture_device_descriptor.h (right): https://codereview.chromium.org/2648973003/diff/80001/media/capture/video/video_capture_device_descriptor.h#newcode70 media/capture/video/video_capture_device_descriptor.h:70: // Front camera first We should prefer on-device cameras ...
3 years, 11 months ago (2017-01-24 04:25:09 UTC) #20
henryhsu
https://codereview.chromium.org/2648973003/diff/80001/media/capture/video/video_capture_device_descriptor.h File media/capture/video/video_capture_device_descriptor.h (right): https://codereview.chromium.org/2648973003/diff/80001/media/capture/video/video_capture_device_descriptor.h#newcode70 media/capture/video/video_capture_device_descriptor.h:70: // Front camera first On 2017/01/24 04:25:09, wuchengli wrote: ...
3 years, 11 months ago (2017-01-24 04:55:45 UTC) #21
wuchengli
lgtm. hta@ PTAL
3 years, 11 months ago (2017-01-24 05:47:13 UTC) #22
hta - Chromium
Basic structure seems unobjectionable, but I have C++ comments. https://codereview.chromium.org/2648973003/diff/100001/media/capture/video/video_capture_device_descriptor.h File media/capture/video/video_capture_device_descriptor.h (right): https://codereview.chromium.org/2648973003/diff/100001/media/capture/video/video_capture_device_descriptor.h#newcode78 media/capture/video/video_capture_device_descriptor.h:78: ...
3 years, 11 months ago (2017-01-24 09:46:24 UTC) #23
hta - Chromium
Also added guidou as reviewer, since he's currently rewriting code that (among other things) queries ...
3 years, 11 months ago (2017-01-24 09:47:38 UTC) #26
henryhsu
https://codereview.chromium.org/2648973003/diff/100001/media/capture/video/video_capture_device_descriptor.h File media/capture/video/video_capture_device_descriptor.h (right): https://codereview.chromium.org/2648973003/diff/100001/media/capture/video/video_capture_device_descriptor.h#newcode78 media/capture/video/video_capture_device_descriptor.h:78: } On 2017/01/24 09:46:24, hta - Chromium wrote: > ...
3 years, 11 months ago (2017-01-24 10:26:40 UTC) #27
Guido Urdaneta
https://codereview.chromium.org/2648973003/diff/120001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/120001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode234 media/capture/video/linux/video_capture_device_factory_linux.cc:234: std::sort(device_descriptors->begin(), device_descriptors->end()); Is this sorting relevant only for linux, ...
3 years, 11 months ago (2017-01-24 10:42:08 UTC) #29
henryhsu
https://codereview.chromium.org/2648973003/diff/120001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/120001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode234 media/capture/video/linux/video_capture_device_factory_linux.cc:234: std::sort(device_descriptors->begin(), device_descriptors->end()); On 2017/01/24 10:42:07, Guido Urdaneta wrote: > ...
3 years, 11 months ago (2017-01-25 03:28:18 UTC) #30
henryhsu
On 2017/01/25 03:28:18, henryhsu wrote: > https://codereview.chromium.org/2648973003/diff/120001/media/capture/video/linux/video_capture_device_factory_linux.cc > File media/capture/video/linux/video_capture_device_factory_linux.cc (right): > > https://codereview.chromium.org/2648973003/diff/120001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode234 > ...
3 years, 11 months ago (2017-01-25 10:00:50 UTC) #31
Guido Urdaneta
lgtm, but take a look at the question below and adjust the TODO comment if ...
3 years, 11 months ago (2017-01-25 10:24:48 UTC) #32
Guido Urdaneta
https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/video_capture_device_descriptor.cc File media/capture/video/video_capture_device_descriptor.cc (right): https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/video_capture_device_descriptor.cc#newcode48 media/capture/video/video_capture_device_descriptor.cc:48: int facing_mapping[NUM_MEDIA_VIDEO_FACING_MODE] = {0}; nit: I don't think you ...
3 years, 11 months ago (2017-01-25 10:27:23 UTC) #33
henryhsu
https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode234 media/capture/video/linux/video_capture_device_factory_linux.cc:234: // TODO(henryhsu): remove this after JS API completed (crbug.com/543997). ...
3 years, 11 months ago (2017-01-25 10:38:01 UTC) #34
henryhsu
hta@, PTAL. We need owner approval. Thank you.
3 years, 11 months ago (2017-01-25 10:46:07 UTC) #35
chromium-reviews
Lgtm 25. jan. 2017 11:46 skrev <henryhsu@chromium.org>: > hta@, PTAL. We need owner approval. Thank ...
3 years, 11 months ago (2017-01-25 12:05:45 UTC) #36
mcasas
lgtm with suggestion. https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode233 media/capture/video/linux/video_capture_device_factory_linux.cc:233: // apps can use the front ...
3 years, 11 months ago (2017-01-25 17:35:15 UTC) #37
henryhsu
https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/linux/video_capture_device_factory_linux.cc File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2648973003/diff/140001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode233 media/capture/video/linux/video_capture_device_factory_linux.cc:233: // apps can use the front camera by default. ...
3 years, 11 months ago (2017-01-26 02:22:54 UTC) #38
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/2648973003/160001
3 years, 11 months ago (2017-01-26 02:25:14 UTC) #41
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/220663)
3 years, 11 months ago (2017-01-26 05:09:56 UTC) #43
hta - Chromium
lgtm wouldn't mind seeing this happen on all platforms, permanently. It's within the remit of ...
3 years, 11 months ago (2017-01-26 08:06:02 UTC) #44
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/2648973003/160001
3 years, 11 months ago (2017-01-26 09:19:21 UTC) #46
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/2648973003/180001
3 years, 11 months ago (2017-01-26 10:19:57 UTC) #49
commit-bot: I haz the power
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_asan_rel_ng/builds/300132)
3 years, 11 months ago (2017-01-26 11:21:16 UTC) #51
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/2648973003/180001
3 years, 11 months ago (2017-01-26 11:48:36 UTC) #53
commit-bot: I haz the power
3 years, 11 months ago (2017-01-26 12:36:03 UTC) #56
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e9c70e53e5da9b0065ef7f5c6ce7...

Powered by Google App Engine
This is Rietveld 408576698