Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2508803002:
Rotate frames correctly for back camera (Closed)
|
Created:
4 years, 1 month ago by shenghao Modified:
4 years ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRotate frames correctly for back camera
In 90 and 270 degree tablet mode, back camera used to show upside-down
frames. We need to rotate the frames correctly for back cameras.
BUG=56762
TEST=Verify that back camera shows correct preview in all rotations.
Committed: https://crrev.com/fd4e73efe59269463f10743694597074ef2ceefb
Cr-Commit-Position: refs/heads/master@{#436594}
Patch Set 1 #Patch Set 2 : remove unused header #
Total comments: 18
Patch Set 3 : calculate rotation in video_capture_device_linux #
Total comments: 13
Patch Set 4 : add some comments #
Total comments: 25
Patch Set 5 : rewrite camera_characteristics #Patch Set 6 : Add comment #
Total comments: 45
Patch Set 7 : Use usb_path to get lens_facing #
Total comments: 32
Patch Set 8 : move from linux to chromeos #
Total comments: 19
Patch Set 9 : Move parsing back to constructor #
Total comments: 23
Patch Set 10 : misc review comments #
Total comments: 28
Patch Set 11 : change camera_characteristics to camera_facing_chromeos #
Total comments: 11
Patch Set 12 : add unit test #
Total comments: 14
Patch Set 13 : try bot #Patch Set 14 : use test/data include runtime data #Patch Set 15 : add copy in BUILD.gn #
Total comments: 2
Patch Set 16 : create temp file in code #Patch Set 17 : remove copy target #Messages
Total messages: 82 (36 generated)
Description was changed from ========== Rotate frames for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ========== to ========== Rotate frames for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ==========
shenghao@chromium.org changed reviewers: + henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
wuchengli@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@ OWNER review. Thanks.
camera_characteristics.h/.cc are copied from ChromeOS.
https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:13: Document where we can find the definition of lens_facing and sensor_orientation. Is it camera_metadata.h? https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:15: std::string device_path; Give an example of device_path. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:23: uint32_t frames_to_skip_after_streamon; Document frames_to_skip_after_streamon. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:139: bool VideoCaptureDeviceLinux::GetCameraFacing( This is just two lines of code. We don't need a function for this. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:142: return characteristics.GetCameraFacing(descriptor.model_id) == 1; Need a constant for 0 and 1. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.h:65: bool is_back_camera_; I think we need three states -- back facing, front facing, and unknown (or external). https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:142: rotation = (360 - rotation) % 360; Can we calculate the right rotation in VideoCaptureDeviceLinux and pass it to VideoCaptureDeviceClient::OnIncomingCapturedData?
https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:13: On 2016/11/16 12:15:19, wuchengli wrote: > Document where we can find the definition of lens_facing and sensor_orientation. > Is it camera_metadata.h? Done. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:15: std::string device_path; On 2016/11/16 12:15:19, wuchengli wrote: > Give an example of device_path. Done. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:23: uint32_t frames_to_skip_after_streamon; On 2016/11/16 12:15:19, wuchengli wrote: > Document frames_to_skip_after_streamon. Done. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:139: bool VideoCaptureDeviceLinux::GetCameraFacing( On 2016/11/16 12:15:19, wuchengli wrote: > This is just two lines of code. We don't need a function for this. But I need it to be a function so that I can initialize is_back_camera_ in the initializer list. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:142: return characteristics.GetCameraFacing(descriptor.model_id) == 1; On 2016/11/16 12:15:19, wuchengli wrote: > Need a constant for 0 and 1. Done. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.h:65: bool is_back_camera_; On 2016/11/16 12:15:19, wuchengli wrote: > I think we need three states -- back facing, front facing, and unknown (or > external). But what we really cares is whether it is a back camera. For external and front camera, we don't need to adjust the rotation value. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:142: rotation = (360 - rotation) % 360; On 2016/11/16 12:15:19, wuchengli wrote: > Can we calculate the right rotation in VideoCaptureDeviceLinux and pass it to > VideoCaptureDeviceClient::OnIncomingCapturedData? Done.
https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:139: bool VideoCaptureDeviceLinux::GetCameraFacing( On 2016/11/17 08:41:44, shenghao wrote: > On 2016/11/16 12:15:19, wuchengli wrote: > > This is just two lines of code. We don't need a function for this. > > But I need it to be a function so that I can initialize is_back_camera_ in the > initializer list. Why not initialize it in the constructor? https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.h:65: bool is_back_camera_; On 2016/11/17 08:41:44, shenghao wrote: > On 2016/11/16 12:15:19, wuchengli wrote: > > I think we need three states -- back facing, front facing, and unknown (or > > external). > > But what we really cares is whether it is a back camera. For external and front > camera, we don't need to adjust the rotation value. OK. Let's document it then. // True if the camera is world-facing. False if the camera is user-facing, external, or unknown. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:16: static const int LENS_FACING_UNKNOWN = -1; There's no LENS_FACING_UNKNOWN. This has to be consistent with ARC camera service. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:120: // Ex: when screen rotation is 90, we have to rotate the frame by 270 This example doesn't explain anything. This example is just a trivial rephrase of the previous line. We want to know why from the comments. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.h:54: bool GetCameraFacing(const VideoCaptureDeviceDescriptor& device_descriptor); s/GetCameraFacing/IsCameraBackFacing/ https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:136: revert this
https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:139: bool VideoCaptureDeviceLinux::GetCameraFacing( On 2016/11/17 09:26:04, wuchengli wrote: > On 2016/11/17 08:41:44, shenghao wrote: > > On 2016/11/16 12:15:19, wuchengli wrote: > > > This is just two lines of code. We don't need a function for this. > > > > But I need it to be a function so that I can initialize is_back_camera_ in the > > initializer list. > > Why not initialize it in the constructor? Done. https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.h:65: bool is_back_camera_; On 2016/11/17 09:26:04, wuchengli wrote: > On 2016/11/17 08:41:44, shenghao wrote: > > On 2016/11/16 12:15:19, wuchengli wrote: > > > I think we need three states -- back facing, front facing, and unknown (or > > > external). > > > > But what we really cares is whether it is a back camera. For external and > front > > camera, we don't need to adjust the rotation value. > OK. Let's document it then. > // True if the camera is world-facing. False if the camera is user-facing, > external, or unknown. Done. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:16: static const int LENS_FACING_UNKNOWN = -1; On 2016/11/17 09:26:04, wuchengli wrote: > There's no LENS_FACING_UNKNOWN. This has to be consistent with ARC camera > service. Done. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.h:54: bool GetCameraFacing(const VideoCaptureDeviceDescriptor& device_descriptor); On 2016/11/17 09:26:04, wuchengli wrote: > s/GetCameraFacing/IsCameraBackFacing/ deleted https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:136: On 2016/11/17 09:26:04, wuchengli wrote: > revert this Done.
kcwu@chromium.org changed reviewers: + kcwu@chromium.org
I just quickly skim. General comments: 1. Please use C++ string instead of C string. Chrome has many string operation helpers in base/. It will be less error-prone use those higher level functions. 2. Please describe the syntax somewhere. Maybe in .cc header of .h header. 3. I don't know if any. Search chrome's existing code see how chrome do config parsing. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:73: LOG(ERROR) << __func__ << ": Invalid device: " << device; If parse failed, you will push_back a default value. Why do so? https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:94: char* sub_key = strtok(key, "."); Don't use strtok() because it keep internal state and thus it is not thread safe. strtok_r() is preferred over strtok(). But the better solution probably use chrome's string functions like base/strings/string_split.h or base/strings/string_tokenizer.h They are slightly higher level string operations and less likely misuse. https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:110: } what if camera_id < device_infos_.size() ? I'd prefer if (camera_id == device_infos_.size()) { ... } else { catch all else } https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:167: sprintf(vid_pid, "%s:%s", device_info.usb_vid.c_str(), I am wondering will this buffer overflow? https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:256: if (tmp_value != 0.0) { compare float value using "!=" ? https://codereview.chromium.org/2508803002/diff/40001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:260: LOG(ERROR) << __func__ << ": Invalid " << characteristic_name << ": " I didn't check the detail. But reject 0 for this general named function "AddFloatValue" sounds strange.
https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:167: sprintf(vid_pid, "%s:%s", device_info.usb_vid.c_str(), Now I'm sure it will overflow buffer. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:221: device_infos_[camera_id].lens_info_available_focal_lengths.clear(); Can it assign directly? device_infos_[camera_id].lens_info_available_focal_lengths = kDefaultCharacteristics.lens_info_available_focal_lengths; https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:16: static const int LENS_FACING_FRONT = 0; enum ? https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:23: lens_facing; // Direction the camera faces relative to device screen. You put some comment at the same line and some not. Please be consistent. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:26: int32_t sensor_orientation; what is expected range? could it be negative? https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:41: // CameraCharacteristics reads the file /etc/camera/camera_characteristics.conf oh, I found the comments here. but you only describe the assumptions, not syntax.
https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:63: LOG(ERROR) << __func__ << ": Can't open file " s/LOG/DLOG/ here and everywhere else. Also s/DVLOG/VLOG/ https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:83: while (fgets(buffer, sizeof(buffer), file)) { Use base::File [1] instead of these direct methods, because they are all inherently unsafe( also, you're missing HANDLE_EINTRs, right?). Also, don't use direct string methods, but base::StringPiece and relatives. I suggest you take a look at e.g. Y4mFileParser [2] or its neighbour MjpegFileParser and rewrite all this code to follow those patterns. While you do so, you can discard all unnecessary parsing. [1] https://cs.chromium.org/chromium/src/base/files/file.h?sq=package:chromium&dr... [2] https://cs.chromium.org/chromium/src/media/capture/video/file_video_capture_d... https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c), here and in the .cc file. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:36: float vertical_view_angle_4_3; This structure has a number of fields, but IIUC we only need a few for the purpose at hand: please remove all the not strictly necessary code. Also see my comments further on.
trybot failed.
FAILED: obj/media/capture/capture/video_capture_device_linux.o
/b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
-MMD -MF obj/media/capture/capture/video_capture_device_linux.o.d
-DCAPTURE_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS
-DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT
-DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1
-DUSE_X11=1 -DNO_TCMALLOC -DMEMORY_TOOL_REPLACES_ALLOCATOR
-DMEMORY_SANITIZER_INITIAL_SIZE -DMEMORY_SANITIZER -DENABLE_WEBRTC=1
-DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD
-DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
-DCR_CLANG_REVISION=284979-2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG
-DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DMEDIA_IMPLEMENTATION
-DGL_GLEXT_PROTOTYPES -DUSE_GLX -DUSE_EGL -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DSK_IGNORE_DW_GRAY_FIX
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1 -I../.. -Igen
-I../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
-I../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I../../third_party/khronos -I../../gpu -I../../third_party/ced/src
-I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
-I../../third_party/libwebm/source -I../../third_party/opus/src/include
-I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c
-I../../third_party/skia/include/config -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/images
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu
-I../../third_party/skia/src/sksl -I../../third_party/libyuv/include
-fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables
-fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
-fcolor-diagnostics -fdebug-prefix-map=/b/c/b/linux_chromium_msan_rel_ng/src=.
-pthread -m64 -march=x86-64 -Wall -Werror -Wextra
-Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing
-Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-undefined-var-template
-Wno-nonportable-include-path -Wno-address-of-packed-member -O2 -fno-ident
-fdata-sections -ffunction-sections -fno-omit-frame-pointer -g1
--sysroot=../../build/linux/debian_wheezy_amd64-sysroot -gline-tables-only
-gcolumn-info -fno-omit-frame-pointer -fsanitize=memory
-fsanitize-memory-track-origins=2
-fsanitize-blacklist=../../tools/msan/blacklist.txt -fvisibility=hidden -Xclang
-load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang
-add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs
-Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wno-unused-function
-fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -fno-rtti
-nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include
-isystem../../buildtools/third_party/libc++abi/trunk/include -fno-exceptions -c
../../media/capture/video/linux/video_capture_device_linux.cc -o
obj/media/capture/capture/video_capture_device_linux.o
In file included from
../../media/capture/video/linux/video_capture_device_linux.cc:14:
../../media/capture/video/linux/camera_characteristics.h:15:1: error:
[chromium-style] Complex class/struct needs an explicit out-of-line constructor.
struct DeviceInfo {
^
../../media/capture/video/linux/camera_characteristics.h:15:1: error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.
2 errors generated.
Description was changed from ========== Rotate frames for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ========== to ========== Rotate frames correctly for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ==========
Remember to run git cl format. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:121: if (is_back_camera_) { Add a comment to explain we assume an external camera is facing the users. If it's not, users can manually rotate the camera. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:124: // clockwise. Use ascii art to explain the image of front facing and back facing will be opposite after rotation. Example: // // Original frame New frame // ----------------------- ----------------------- // | | | ******** | // | * | | * **** | // | * * | ==> | * *** | // | ***** | | * *** | // | * * | | * **** | // | | | ******** | // ----------------------- ----------------------- //
https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:63: LOG(ERROR) << __func__ << ": Can't open file " On 2016/11/17 17:53:07, mcasas wrote: > s/LOG/DLOG/ here and everywhere else. > Also s/DVLOG/VLOG/ Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:83: while (fgets(buffer, sizeof(buffer), file)) { On 2016/11/17 17:53:07, mcasas wrote: > Use base::File [1] instead of these direct methods, > because they are all inherently unsafe( also, you're > missing HANDLE_EINTRs, right?). > > Also, don't use direct string methods, but > base::StringPiece and relatives. > > I suggest you take a look at e.g. Y4mFileParser [2] > or its neighbour MjpegFileParser and rewrite all > this code to follow those patterns. While you do > so, you can discard all unnecessary parsing. > > > [1] > https://cs.chromium.org/chromium/src/base/files/file.h?sq=package:chromium&dr... > [2] > https://cs.chromium.org/chromium/src/media/capture/video/file_video_capture_d... Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:167: sprintf(vid_pid, "%s:%s", device_info.usb_vid.c_str(), On 2016/11/17 15:02:18, kcwu wrote: > Now I'm sure it will overflow buffer. Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.cc:221: device_infos_[camera_id].lens_info_available_focal_lengths.clear(); On 2016/11/17 15:02:18, kcwu wrote: > Can it assign directly? > device_infos_[camera_id].lens_info_available_focal_lengths = > kDefaultCharacteristics.lens_info_available_focal_lengths; Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/17 17:53:07, mcasas wrote: > No (c), here and in the .cc file. done https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:16: static const int LENS_FACING_FRONT = 0; On 2016/11/17 15:02:18, kcwu wrote: > enum ? I think it's better to use int here to match the values in config file. Using enum needs another layer of conversion. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:23: lens_facing; // Direction the camera faces relative to device screen. On 2016/11/17 15:02:18, kcwu wrote: > You put some comment at the same line and some not. Please be consistent. Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:26: int32_t sensor_orientation; On 2016/11/17 15:02:18, kcwu wrote: > what is expected range? could it be negative? Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:36: float vertical_view_angle_4_3; On 2016/11/17 17:53:07, mcasas wrote: > This structure has a number of fields, but IIUC > we only need a few for the purpose at hand: > please remove all the not strictly necessary code. > Also see my comments further on. Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:41: // CameraCharacteristics reads the file /etc/camera/camera_characteristics.conf On 2016/11/17 15:02:18, kcwu wrote: > oh, I found the comments here. but you only describe the assumptions, not > syntax. Isn't the example below enough to help readers understand the syntax? https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:121: if (is_back_camera_) { On 2016/11/18 05:08:50, wuchengli wrote: > Add a comment to explain we assume an external camera is facing the users. If > it's not, users can manually rotate the camera. Done. https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:124: // clockwise. On 2016/11/18 05:08:50, wuchengli wrote: > Use ascii art to explain the image of front facing and back facing will be > opposite after rotation. > > Example: > // > // Original frame New frame > // ----------------------- ----------------------- > // | | | ******** | > // | * | | * **** | > // | * * | ==> | * *** | > // | ***** | | * *** | > // | * * | | * **** | > // | | | ******** | > // ----------------------- ----------------------- > // Done.
https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/60001/media/capture/video/lin... media/capture/video/linux/camera_characteristics.h:41: // CameraCharacteristics reads the file /etc/camera/camera_characteristics.conf On 2016/11/21 06:24:53, shenghao wrote: > On 2016/11/17 15:02:18, kcwu wrote: > > oh, I found the comments here. but you only describe the assumptions, not > > syntax. > > Isn't the example below enough to help readers understand the syntax? For example, 1. do you allow space before and after "=", trailing spaces ? 2. Are "cameraX" and "moduleX" names are required? Can they be free text? I have this question because you didn't check their value now. 3. You need to explain A.key=value is applied to A.*.key. This is not trivial to know from the syntax. btw, do you still need 256 limit since you use c++ string now? https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:48: base::StringPiece(content), base::StringPiece("\n"), IIRC, StringPiece will do casting implicitly, so you can just write base::SplitStringPiece(content, "\n"); https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:65: base::StringPiece key = key_value[0]; const auto& key = key_value[0]; const auto& value = key_value[1]; https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:67: std::vector<base::StringPiece> sub_keys = base::SplitStringPiece( the value of sub_keys[0] is ignored? https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:70: if (sub_keys.size() >= 2 && sub_keys[1].compare(kLensFacing) == 0) { 1. why ">=" instead of "==" ? Do you intent to ignore sub_keys[2:] silently? 2. Could it use "==" instead of compare() ? https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:71: current_lens_facing = strtol(value.data(), NULL, 10); base::StringToUint or the like? https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:74: if (sub_keys.size() >= 3 && sub_keys[2].compare(kUsbVidPid) == 0) { ditto https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:78: info.lens_facing = current_lens_facing; You need to reset current_lens_facing when parsing different model. For example, camera0.lens_facing=1 camera0.module0.usb_vid_pic=foo:1 camera1.module0.usb_vid_pic=foo:2 foo:2 will have the same current_lens_facing carrying from foo:1. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:81: } what about else? DLOG(ERROR) ? https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:16: struct DeviceInfo { s/DeviceInfo/CameraDeviceInfo/ DeviceInfo seems too general to media namespace. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:29: // the config file: Please relax these rules as possible as you can. Otherwise they are easily to break. And for required rules, please give enough error log if they don't meet.
https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:29: // the config file: On 2016/11/21 07:59:24, kcwu wrote: > Please relax these rules as possible as you can. Otherwise they are easily to > break. > And for required rules, please give enough error log if they don't meet. Why relaxing the rules? We don't need the flexibility.
On 2016/11/21 08:56:03, wuchengli wrote: > https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... > File media/capture/video/linux/camera_characteristics.h (right): > > https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... > media/capture/video/linux/camera_characteristics.h:29: // the config file: > On 2016/11/21 07:59:24, kcwu wrote: > > Please relax these rules as possible as you can. Otherwise they are easily to > > break. > > And for required rules, please give enough error log if they don't meet. > Why relaxing the rules? We don't need the flexibility. It's okay if we can detect and produce error log. Otherwise the mistake may be hard to know. For example, current code didn't check if the input lines are not in order. This may result in unexpected behavior but no warnings are given.
On 2016/11/21 09:01:40, kcwu wrote: > On 2016/11/21 08:56:03, wuchengli wrote: > > > https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... > > File media/capture/video/linux/camera_characteristics.h (right): > > > > > https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... > > media/capture/video/linux/camera_characteristics.h:29: // the config file: > > On 2016/11/21 07:59:24, kcwu wrote: > > > Please relax these rules as possible as you can. Otherwise they are easily > to > > > break. > > > And for required rules, please give enough error log if they don't meet. > > Why relaxing the rules? We don't need the flexibility. > > It's okay if we can detect and produce error log. Otherwise the mistake may be > hard to know. Agree. Sheng-hao. Please detect the syntax error and print error logs. > > For example, current code didn't check if the input lines are not in order. This > may result in unexpected behavior but no warnings are given.
Here are the comments so far. I'll continue to review camera_characteristics.cc. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:21: static const struct DeviceInfo kDefaultCharacteristics = { not used? https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:42: const base::FilePath path(kCameraCharacteristicsConfigFile); We should not read the file every time a camera is opened. Make CameraCharacteristics a LazyInstance and do GetDeviceInfoFromFile in the constructor. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:16: struct DeviceInfo { On 2016/11/21 07:59:25, kcwu wrote: > s/DeviceInfo/CameraDeviceInfo/ > DeviceInfo seems too general to media namespace. +1 https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:54: // Get camera facing by specifying vid and pid. |model_id| should be formatted s/vid/USB vid/ https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:122: // rotate the camera manually by theirselves. s/theirselves/themselves/ https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:129: // | ***** | Great diagram! Can you make the A of the first one bigger to match the sizes of the other two?
https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:45: DLOG(ERROR) << "Config file bigger than buffer size."; If the file doesn't exist, ReadFileToString also returns false. We should return here. if (!base::ReadFileToString(path, &content)) { DVLOG(1) << "ReadFileToString fails"; return; } https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:49: base::WhitespaceHandling::TRIM_WHITESPACE, Check if TRIM_WHITESPACE will remove "\r" if the config file uses \r\n as newline. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:54: for (base::StringPiece line : lines) { // Ignore the comment that starts with "#". https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:62: DLOG(ERROR) << "Invalid line in config file: " << line; LOG(ERROR) because this is an actual error. We need to fix it when it happens. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:73: } else if (sub_keys.size() >= 3 && sub_keys[2].compare(kUsbVidPid) == 0) { https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:81: } On 2016/11/21 07:59:24, kcwu wrote: > what about else? DLOG(ERROR) ? No need to log. We ignore keys like lens_info_available_focal_lengths. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:34: // 5. lens_facing should be put before any modules for a camera As discussed offline, let's remove all these constraints. Use two hashmaps to store the camera info. The first hashmap maps from camera id to lens facing and usb bus/port numbers. The second hashmap maps from vid/pid to camera id. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:42: CameraCharacteristics characteristics; base::LazyInstance<CameraCharacteristics> g_camera_characteristics_ = LAZY_INSTANCE_INITIALIZER; is_back_camera_ = g_camera_characteristics_.Get().GetCameraFacing(device_descriptor.model_id);
https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:21: static const struct DeviceInfo kDefaultCharacteristics = { On 2016/11/22 03:47:06, wuchengli wrote: > not used? Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:42: const base::FilePath path(kCameraCharacteristicsConfigFile); On 2016/11/22 03:47:06, wuchengli wrote: > We should not read the file every time a camera is opened. Make > CameraCharacteristics a LazyInstance and do GetDeviceInfoFromFile in the > constructor. Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:45: DLOG(ERROR) << "Config file bigger than buffer size."; On 2016/11/22 07:59:33, wuchengli wrote: > If the file doesn't exist, ReadFileToString also returns false. We should return > here. > > if (!base::ReadFileToString(path, &content)) { > DVLOG(1) << "ReadFileToString fails"; > return; > } > Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:48: base::StringPiece(content), base::StringPiece("\n"), On 2016/11/21 07:59:24, kcwu wrote: > IIRC, StringPiece will do casting implicitly, so you can just write > base::SplitStringPiece(content, "\n"); Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:49: base::WhitespaceHandling::TRIM_WHITESPACE, On 2016/11/22 07:59:34, wuchengli wrote: > Check if TRIM_WHITESPACE will remove "\r" if the config file uses \r\n as > newline. Yes. https://cs.chromium.org/chromium/src/base/strings/string_util_constants.cc?gs... https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:54: for (base::StringPiece line : lines) { On 2016/11/22 07:59:34, wuchengli wrote: > // Ignore the comment that starts with "#". Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:62: DLOG(ERROR) << "Invalid line in config file: " << line; On 2016/11/22 07:59:33, wuchengli wrote: > LOG(ERROR) because this is an actual error. We need to fix it when it happens. Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:65: base::StringPiece key = key_value[0]; On 2016/11/21 07:59:24, kcwu wrote: > const auto& key = key_value[0]; > const auto& value = key_value[1]; Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:67: std::vector<base::StringPiece> sub_keys = base::SplitStringPiece( On 2016/11/21 07:59:24, kcwu wrote: > the value of sub_keys[0] is ignored? deleted https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:71: current_lens_facing = strtol(value.data(), NULL, 10); On 2016/11/21 07:59:24, kcwu wrote: > base::StringToUint or the like? Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:73: } On 2016/11/22 07:59:33, wuchengli wrote: > else if (sub_keys.size() >= 3 && sub_keys[2].compare(kUsbVidPid) == 0) { Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:74: if (sub_keys.size() >= 3 && sub_keys[2].compare(kUsbVidPid) == 0) { On 2016/11/21 07:59:24, kcwu wrote: > ditto Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:78: info.lens_facing = current_lens_facing; On 2016/11/21 07:59:24, kcwu wrote: > You need to reset current_lens_facing when parsing different model. For example, > > camera0.lens_facing=1 > camera0.module0.usb_vid_pic=foo:1 > camera1.module0.usb_vid_pic=foo:2 > > foo:2 will have the same current_lens_facing carrying from foo:1. Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:81: } On 2016/11/22 07:59:33, wuchengli wrote: > On 2016/11/21 07:59:24, kcwu wrote: > > what about else? DLOG(ERROR) ? > No need to log. We ignore keys like lens_info_available_focal_lengths. Acknowledged. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:16: struct DeviceInfo { On 2016/11/21 07:59:25, kcwu wrote: > s/DeviceInfo/CameraDeviceInfo/ > DeviceInfo seems too general to media namespace. Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:34: // 5. lens_facing should be put before any modules for a camera On 2016/11/22 07:59:34, wuchengli wrote: > As discussed offline, let's remove all these constraints. Use two hashmaps to > store the camera info. The first hashmap maps from camera id to lens facing and > usb bus/port numbers. The second hashmap maps from vid/pid to camera id. Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:54: // Get camera facing by specifying vid and pid. |model_id| should be formatted On 2016/11/22 03:47:06, wuchengli wrote: > s/vid/USB vid/ Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:42: CameraCharacteristics characteristics; On 2016/11/22 07:59:34, wuchengli wrote: > base::LazyInstance<CameraCharacteristics> g_camera_characteristics_ = > LAZY_INSTANCE_INITIALIZER; > > > > is_back_camera_ = > g_camera_characteristics_.Get().GetCameraFacing(device_descriptor.model_id); Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:122: // rotate the camera manually by theirselves. On 2016/11/22 03:47:06, wuchengli wrote: > s/theirselves/themselves/ Done. https://codereview.chromium.org/2508803002/diff/100001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:129: // | ***** | On 2016/11/22 03:47:06, wuchengli wrote: > Great diagram! Can you make the A of the first one bigger to match the sizes of > the other two? Done.
https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:27: if (!base::ReadFileToString(path, &content_)) { Don't have non-trivial code in ctor. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:66: DCHECK_EQ(0, device_id.compare(0, device_dir.length(), device_dir)); DCHECK(base::StartsWith(device_id, device_dir, base::CompareCase::SENSITIVE)); if device_id is StringPiece, it has stars_with(). https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:68: device_id.substr(device_dir.length(), device_id.length()); device_id.substr(device_dir.length()); the second argument could be skip. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:74: if (readlink(symlink.c_str(), usb_full, kUsbIdFullSize) == -1) { base::ReadSymbolicLink() then we can get rid of kUsbIdFullSize. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:80: base::StringPiece usb_part = usb_full; If usb_full is base::FilePath, it has BaseName() https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:88: return usb_id_pieces[0].as_string(); Check usb_id_pieces.size(), otherwise access usb_id_pieces[0] will crash. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:101: if (content_.empty()) { No need to keep |content_| in memory after parse. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:110: if (line.starts_with(base::StringPiece("#"))) { line.starts_with("#") IIUC, the good of StringPiece is that it will cast automatically. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:139: int camera_id = 0; sub_keys[1] is not used? https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:150: int camera_id = 0; sub_keys[1] is not used? https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:75: int GetCameraFacing(const std::string& model_id, If device_id is matched first, please put it as the first function parameter. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:81: std::map<base::StringPiece, int> usb_id_to_camera_id_; use std::string instead. Then we don't need to keep content_ after initialize. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:84: std::string GetUsbId(const std::string& device_id); Methods should put before member variables. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:45: is_back_camera_ = Do you intent to have this in normal linux? If not, please make it conditional compile for only chromeos. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:46: camera_characteristics_.Get().GetCameraFacing( per coding style, don't do non-trivial thing in ctor.
https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:27: if (!base::ReadFileToString(path, &content_)) { On 2016/11/24 10:16:08, kcwu wrote: > Don't have non-trivial code in ctor. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:66: DCHECK_EQ(0, device_id.compare(0, device_dir.length(), device_dir)); On 2016/11/24 10:16:08, kcwu wrote: > DCHECK(base::StartsWith(device_id, device_dir, base::CompareCase::SENSITIVE)); > > if device_id is StringPiece, it has stars_with(). Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:68: device_id.substr(device_dir.length(), device_id.length()); On 2016/11/24 10:16:08, kcwu wrote: > device_id.substr(device_dir.length()); > the second argument could be skip. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:74: if (readlink(symlink.c_str(), usb_full, kUsbIdFullSize) == -1) { On 2016/11/24 10:16:08, kcwu wrote: > base::ReadSymbolicLink() then we can get rid of kUsbIdFullSize. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:80: base::StringPiece usb_part = usb_full; On 2016/11/24 10:16:08, kcwu wrote: > If usb_full is base::FilePath, it has BaseName() Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:88: return usb_id_pieces[0].as_string(); On 2016/11/24 10:16:08, kcwu wrote: > Check usb_id_pieces.size(), otherwise access usb_id_pieces[0] will crash. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:101: if (content_.empty()) { On 2016/11/24 10:16:08, kcwu wrote: > No need to keep |content_| in memory after parse. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:110: if (line.starts_with(base::StringPiece("#"))) { On 2016/11/24 10:16:08, kcwu wrote: > line.starts_with("#") > IIUC, the good of StringPiece is that it will cast automatically. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:139: int camera_id = 0; On 2016/11/24 10:16:08, kcwu wrote: > sub_keys[1] is not used? Yeah, the module number is not needed for our case https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:150: int camera_id = 0; On 2016/11/24 10:16:08, kcwu wrote: > sub_keys[1] is not used? Yeah, the module number is not needed for our case https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:75: int GetCameraFacing(const std::string& model_id, On 2016/11/24 10:16:08, kcwu wrote: > If device_id is matched first, please put it as the first function parameter. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:81: std::map<base::StringPiece, int> usb_id_to_camera_id_; On 2016/11/24 10:16:08, kcwu wrote: > use std::string instead. > Then we don't need to keep content_ after initialize. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:84: std::string GetUsbId(const std::string& device_id); On 2016/11/24 10:16:08, kcwu wrote: > Methods should put before member variables. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:45: is_back_camera_ = On 2016/11/24 10:16:08, kcwu wrote: > Do you intent to have this in normal linux? If not, please make it conditional > compile for only chromeos. Done. https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:46: camera_characteristics_.Get().GetCameraFacing( On 2016/11/24 10:16:08, kcwu wrote: > per coding style, don't do non-trivial thing in ctor. Done.
https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:139: int camera_id = 0; On 2016/11/24 14:13:24, shenghao wrote: > On 2016/11/24 10:16:08, kcwu wrote: > > sub_keys[1] is not used? > > Yeah, the module number is not needed for our case What if the line is camera0.helloworld.usb_vid_pid=0123:4567 ? https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:48: LOG(ERROR) << "Can't find camera ID in config file: " << camera_id; "Can't find facing data in config file for camera: " maybe you have better sentence. My point is, it is confusing if there are other data for this camera in config file, it just lacks facing. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:131: if (!GetCameraId(sub_keys[0], &camera_id)) { Please refactor to avoid duplicated code. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:26: uint32_t lens_facing; I still prefer enum, though. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:114: camera_characteristics_.Get().InitializeDeviceInfo(); Will VideoCaptureDeviceChromeOS be created more than once? If so, you will call CameraCharacteistics's init more than once. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:115: lens_facing_ = camera_characteristics_.Get().GetCameraFacing( If there are two cameras starting capture at the same time, will two VideoCaptureDeviceChromeOS() call camera_characteristics_.Get().GetCameraFacing() at the same time? https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:141: // screen_rotation = 90, this is what front camera sees 270 ? https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:154: v4l2_thread_.task_runner()->PostTask( How about call VideoCaptureDeviceLinux::SetRotation(rotation); then VideoCaptureDeviceLinux don't need to expose many members to children and don't have duplicated code. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.h:38: base::LazyInstance<CameraCharacteristics> camera_characteristics_; This should be static or global, otherwise no point to use LazyInstance<>. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:14: #include "media/capture/video/linux/camera_characteristics.h" unused
https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/120001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:46: camera_characteristics_.Get().GetCameraFacing( On 2016/11/24 10:16:08, kcwu wrote: > per coding style, don't do non-trivial thing in ctor. I found the style guide changed the suggestion. It's allowed. My memory isn't updated, sorry.
https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:115: lens_facing_ = camera_characteristics_.Get().GetCameraFacing( On 2016/11/24 15:48:52, kcwu wrote: > If there are two cameras starting capture at the same time, will two > VideoCaptureDeviceChromeOS() call > camera_characteristics_.Get().GetCameraFacing() at the same time? FYI, http://stackoverflow.com/questions/5912539/stl-map-find-thread-safe
https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:48: LOG(ERROR) << "Can't find camera ID in config file: " << camera_id; On 2016/11/24 15:48:52, kcwu wrote: > "Can't find facing data in config file for camera: " > maybe you have better sentence. My point is, it is confusing if there are other > data for this camera in config file, it just lacks facing. Done. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:131: if (!GetCameraId(sub_keys[0], &camera_id)) { On 2016/11/24 15:48:52, kcwu wrote: > Please refactor to avoid duplicated code. Done. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:26: uint32_t lens_facing; On 2016/11/24 15:48:52, kcwu wrote: > I still prefer enum, though. Done. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:114: camera_characteristics_.Get().InitializeDeviceInfo(); On 2016/11/24 15:48:52, kcwu wrote: > Will VideoCaptureDeviceChromeOS be created more than once? If so, you will call > CameraCharacteistics's init more than once. Done. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:115: lens_facing_ = camera_characteristics_.Get().GetCameraFacing( On 2016/11/24 15:48:52, kcwu wrote: > If there are two cameras starting capture at the same time, will two > VideoCaptureDeviceChromeOS() call > camera_characteristics_.Get().GetCameraFacing() at the same time? Methods like find() can be called safely according to c++ spec 23.2.2 http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:141: // screen_rotation = 90, this is what front camera sees On 2016/11/24 15:48:52, kcwu wrote: > 270 ? This is 90. We want to demonstrate the difference of what front and back camera sees at the same rotation. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:154: v4l2_thread_.task_runner()->PostTask( On 2016/11/24 15:48:52, kcwu wrote: > How about call > VideoCaptureDeviceLinux::SetRotation(rotation); > then VideoCaptureDeviceLinux don't need to expose many members to children and > don't have duplicated code. Done. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.h:38: base::LazyInstance<CameraCharacteristics> camera_characteristics_; On 2016/11/24 15:48:52, kcwu wrote: > This should be static or global, otherwise no point to use LazyInstance<>. Done. https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2508803002/diff/140001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.cc:14: #include "media/capture/video/linux/camera_characteristics.h" On 2016/11/24 15:48:52, kcwu wrote: > unused Done.
https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:35: const auto& usb_id_to_camera_id_const = usb_id_to_camera_id_; Just curious, does it compile without this? https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:145: if (!base::StringToInt(value, &lens_facing)) { Please validate the range of |lens_facing|. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:152: if (value.empty()) { How about check model id is in "aaaa:bbbb" form? I'm not strong on this. Up to you. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:163: } and comment // ignore unknown/unhandled attributes. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:12: #include <vector> unused https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:20: enum LensFacing { kFacingFront = 0, kFacingBack = 1 }; enum class LensFacing. Then we can skip redundant word "Facing". enum class LensFacing { kFront = 0, kBack = 1 }; https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:20: enum LensFacing { kFacingFront = 0, kFacingBack = 1 }; Please consider const LensFacing kDefaultLensFacing = LensFacing::kFront; or enum class LensFacing { kFront = 0, kBack = 1, kDefault = 0 }; then use it for every error cases. Thus avoid using magic kFacingFront everywhere. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:27: typedef std::vector<CameraDeviceInfo> CameraDeviceInfos; unused? https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:14: #include "media/capture/video/linux/v4l2_capture_delegate.h" unused? https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.h:8: #include "base/lazy_instance.h" move to .cc https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.h:19: #include "base/lazy_instance.h" unused https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.h:57: base::Thread v4l2_thread_; // Thread used for reading data from the device. move to line 64 (keep it on original line)
https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:35: const auto& usb_id_to_camera_id_const = usb_id_to_camera_id_; On 2016/11/28 10:11:37, kcwu wrote: > Just curious, does it compile without this? It compiles. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:145: if (!base::StringToInt(value, &lens_facing)) { On 2016/11/28 10:11:37, kcwu wrote: > Please validate the range of |lens_facing|. Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:152: if (value.empty()) { On 2016/11/28 10:11:37, kcwu wrote: > How about check model id is in "aaaa:bbbb" form? > I'm not strong on this. Up to you. I think it's ok not to check. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:163: } On 2016/11/28 10:11:37, kcwu wrote: > and comment > // ignore unknown/unhandled attributes. > Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:12: #include <vector> On 2016/11/28 10:11:37, kcwu wrote: > unused Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:20: enum LensFacing { kFacingFront = 0, kFacingBack = 1 }; On 2016/11/28 10:11:37, kcwu wrote: > enum class LensFacing. > > Then we can skip redundant word "Facing". > enum class LensFacing { kFront = 0, kBack = 1 }; Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:27: typedef std::vector<CameraDeviceInfo> CameraDeviceInfos; On 2016/11/28 10:11:37, kcwu wrote: > unused? Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:14: #include "media/capture/video/linux/v4l2_capture_delegate.h" On 2016/11/28 10:11:37, kcwu wrote: > unused? Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.h:8: #include "base/lazy_instance.h" On 2016/11/28 10:11:37, kcwu wrote: > move to .cc Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.h:19: #include "base/lazy_instance.h" On 2016/11/28 10:11:37, kcwu wrote: > unused Done. https://codereview.chromium.org/2508803002/diff/160001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.h:57: base::Thread v4l2_thread_; // Thread used for reading data from the device. On 2016/11/28 10:11:37, kcwu wrote: > move to line 64 (keep it on original line) Done.
lgtm
Still work to do. Bots please. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:100: } No {} for one line bodies. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:108: DVLOG(1) << "ReadFileToString fails"; This should be DLOG() and, for more info, I'd consider DPLOG() that pastes the last system error as string afterwards. Also, s/LOG/DLOG/ elsewhere in this file, and consider DPLOG(ERROR) instead. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:148: lens_facing > static_cast<int>(CameraDeviceInfo::LensFacing::kBack)) { No casting enum class members to int or viceversa, instead, use a switch. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:18: struct CameraDeviceInfo { Make this struct public inside CameraCharacteristics. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:19: enum class LensFacing { kFront = 0, kBack = 1 }; Enum members should be uppercase see, e.g. [1] and not have the leading "k". [1] https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:22: std::string vid_pid; This member is unused. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:24: LensFacing lens_facing; This member is unused. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:27: // CameraCharacteristics reads the file /etc/camera/camera_characteristics.conf I wonder if this file format is public or is specific for CrOs. If the former, we should link to its documentation, but if the latter, then these 2 files should have the suffix _chromeos.h (e.g. [1]), so they're pulled out of builds automatically. [1] https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:59: class CameraCharacteristics { This class is now only used for parsing a file to extract and keep camera lens facing info, so the name isn't accurate. I propose calling it LensFacing or CameraFacing or similar. Following my comment before, CameraDeviceInfo should be inside this class, but CameraFacing::CameraDeviceInfo would be too verbose a name, so just change it to CameraFacing::DeviceInfo or such. On further thoughts, you don't seem to use the member fields of said Struct, so just remove it, move the enum class (corrected) to inside this class and hold on to the values as you do now in l.82 |camera_id_to_facing_|. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:23: base::LazyInstance<CameraCharacteristics> g_camera_characteristics_ = Use base::LazyInstane<>::Leaky to avoid slowing down shutdown [1]. [1] https://www.chromium.org/developers/coding-style/important-abstractions-and-d... https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:113: device_descriptor.model_id)) {} Make |lens_facing_| const and initialize it instead of assigning it. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:123: // Original frame when screen_rotation = 0 Here and elsewhere in these comments s/screen_rotation/|rotation|/ https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.h:21: #include "media/capture/video/linux/camera_characteristics.h" Not needed: move to video_capture_device_chromeos.h
https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:19: enum class LensFacing { kFront = 0, kBack = 1 }; On 2016/11/29 19:12:56, mcasas wrote: > Enum members should be uppercase see, e.g. [1] > and not have the leading "k". > > [1] > https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... FYI, chromium's style guide explicitly override the general style guide for enum naming. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
On 2016/11/30 03:04:53, kcwu wrote: > https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... > File media/capture/video/linux/camera_characteristics.h (right): > > https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... > media/capture/video/linux/camera_characteristics.h:19: enum class LensFacing { > kFront = 0, kBack = 1 }; > On 2016/11/29 19:12:56, mcasas wrote: > > Enum members should be uppercase see, e.g. [1] > > and not have the leading "k". > > > > [1] > > > https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... > > FYI, chromium's style guide explicitly override the general style guide for enum > naming. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Correct, that links says: "Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. In enums that are actually enumerations (i.e. have multiple values), continue to use this style for consistency."
https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.cc (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:100: } On 2016/11/29 19:12:55, mcasas wrote: > No {} for one line bodies. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:108: DVLOG(1) << "ReadFileToString fails"; On 2016/11/29 19:12:55, mcasas wrote: > This should be DLOG() and, for more info, I'd consider > DPLOG() that pastes the last system error as string afterwards. > > Also, s/LOG/DLOG/ elsewhere in this file, and consider > DPLOG(ERROR) instead. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.cc:148: lens_facing > static_cast<int>(CameraDeviceInfo::LensFacing::kBack)) { On 2016/11/29 19:12:55, mcasas wrote: > No casting enum class members to int or viceversa, > instead, use a switch. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/camera_characteristics.h (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:18: struct CameraDeviceInfo { On 2016/11/29 19:12:56, mcasas wrote: > Make this struct public inside CameraCharacteristics. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:19: enum class LensFacing { kFront = 0, kBack = 1 }; On 2016/11/30 03:04:53, kcwu wrote: > On 2016/11/29 19:12:56, mcasas wrote: > > Enum members should be uppercase see, e.g. [1] > > and not have the leading "k". > > > > [1] > > > https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... > > FYI, chromium's style guide explicitly override the general style guide for enum > naming. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:22: std::string vid_pid; On 2016/11/29 19:12:56, mcasas wrote: > This member is unused. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:24: LensFacing lens_facing; On 2016/11/29 19:12:56, mcasas wrote: > This member is unused. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:27: // CameraCharacteristics reads the file /etc/camera/camera_characteristics.conf On 2016/11/29 19:12:56, mcasas wrote: > I wonder if this file format is public or is specific for CrOs. > If the former, we should link to its documentation, but > if the latter, then these 2 files should have the suffix > _chromeos.h (e.g. [1]), so they're pulled out of builds > automatically. > > [1] > https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/camera_characteristics.h:59: class CameraCharacteristics { On 2016/11/29 19:12:56, mcasas wrote: > This class is now only used for parsing a file to extract > and keep camera lens facing info, so the name isn't > accurate. I propose calling it LensFacing or CameraFacing > or similar. Following my comment before, > CameraDeviceInfo should be inside this class, but > CameraFacing::CameraDeviceInfo would be too verbose > a name, so just change it to CameraFacing::DeviceInfo > or such. > > > On further thoughts, you don't seem to use the member > fields of said Struct, so just remove it, move the enum > class (corrected) to inside this class and hold on to the > values as you do now in l.82 |camera_id_to_facing_|. Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:23: base::LazyInstance<CameraCharacteristics> g_camera_characteristics_ = On 2016/11/29 19:12:56, mcasas wrote: > Use base::LazyInstane<>::Leaky to avoid slowing down shutdown [1]. > > [1] > https://www.chromium.org/developers/coding-style/important-abstractions-and-d... Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:113: device_descriptor.model_id)) {} On 2016/11/29 19:12:56, mcasas wrote: > Make |lens_facing_| const and initialize it instead > of assigning it. I don't understand. Ain't I initializing it instead of assigning it right now? https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:123: // Original frame when screen_rotation = 0 On 2016/11/29 19:12:56, mcasas wrote: > Here and elsewhere in these comments > s/screen_rotation/|rotation|/ Done. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_linux.h:21: #include "media/capture/video/linux/camera_characteristics.h" On 2016/11/29 19:12:56, mcasas wrote: > Not needed: move to video_capture_device_chromeos.h Done.
Bots please ...? https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:37: const auto& camera_id_to_facing_const = camera_id_to_facing_; I would just use the non-const versions directly, to simplify the code. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:94: bool CameraFacingChromeOS::GetCameraId(const base::StringPiece& sub_key, This method is too small to be independent, but if you want to keep it, just make it a static function and move it to an anonymous namespace inside media{} on top of the file. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:141: } What about: if (sub_keys.size() < 1) { DLOG(ERROR) << "Invalid line format: " << line: continue; } // and here assume l.132 verifies. Otherwise we get loads of nested if's and that makes the code hard to read. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:5: #ifndef MEDIA_CAPTURE_VIDEO_LINUX_CAMERA_CHARACTERISTICS_H_ Update guards. Pro-tip for the next occasion: tools/git/mass-rename.py updates these rote things and also BUILD.gn entries. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:49: class CameraFacingChromeOS { Can we have a unittests for this class? It can be simply a test that verifies a file format being correctly parsed, another one being rejected etc. That'd also force you to clean the interface, e.g. right now the file-to-parse is baked in l.70 (of the .cc file), but you'll need to modify the class to inject this location, which makes the class more observable etc. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:79: } // namespace arc s/arc/media/
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...
https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:37: const auto& camera_id_to_facing_const = camera_id_to_facing_; On 2016/12/01 17:46:56, mcasas wrote: > I would just use the non-const versions directly, > to simplify the code. We want GetCameraFacing() to be thread-safe. unordered_map has two versions of find(). One is const and the other is not, and it depends on whether the calling object is const to determine which find() to trigger. That's why I want to convert them to const variables here. http://www.cplusplus.com/reference/unordered_map/unordered_map/find/ https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:94: bool CameraFacingChromeOS::GetCameraId(const base::StringPiece& sub_key, On 2016/12/01 17:46:56, mcasas wrote: > This method is too small to be independent, > but if you want to keep it, just make it a > static function and move it to an anonymous > namespace inside media{} on top of the file. Done. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:141: } On 2016/12/01 17:46:56, mcasas wrote: > What about: > if (sub_keys.size() < 1) { > DLOG(ERROR) << "Invalid line format: " << line: > continue; > } > // and here assume l.132 verifies. > > Otherwise we get loads of nested if's and > that makes the code hard to read. Done. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:5: #ifndef MEDIA_CAPTURE_VIDEO_LINUX_CAMERA_CHARACTERISTICS_H_ On 2016/12/01 17:46:56, mcasas wrote: > Update guards. > Pro-tip for the next occasion: tools/git/mass-rename.py > updates these rote things and also BUILD.gn entries. Done. https://codereview.chromium.org/2508803002/diff/200001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:79: } // namespace arc On 2016/12/01 17:46:56, mcasas wrote: > s/arc/media/ Done.
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_...)
lgtm % comments addressed and bots happy. https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/180001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:113: device_descriptor.model_id)) {} On 2016/12/01 07:34:21, shenghao wrote: > On 2016/11/29 19:12:56, mcasas wrote: > > Make |lens_facing_| const and initialize it instead > > of assigning it. > > I don't understand. Ain't I initializing it instead of assigning it right now? You're assigning right now. As it is, the compiler will initialize |kLensFacing| in its constructor default initialization list, and then you are assigning to it. You can make the member variable const and initialize it in the member initializer list: lens_facing_(g_camera_facing_.Get().GetCameraFacing( device_descriptor.device_id, device_descriptor.model_id), in l.109. (See Scott Meyers, "Effective C++..." item 12: "Prefer initialization to assignment in constructors.") https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:115: std::vector<base::StringPiece> lines = base::SplitStringPiece( nit: const https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:119: for (base::StringPiece line : lines) { const base::StringPiece& line https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:123: } micro nit: if (line.starts_with("#")) // Ignore comment lines. continue; https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:124: std::vector<base::StringPiece> key_value = base::SplitStringPiece( const ? Also in l.133, etc. Essentially, try to const-ify as much as possible please. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:54: CameraFacingChromeOS(const char* config_file_path); const std::string& here and in l.72. base::FilePath() will work with this already. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:153: // Therefore, for back camera, we need to rotate (360 - |rotation|). All this is to say that, same as Android, rotation goes backwards for the "back" camera, that is called "environment" there. Although I'm a fan of ASCII art myself, I wonder if you could convey that meaning in a more succinct fashion here? https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.h:36: const CameraFacingChromeOS::LensFacing kLensFacing; s/kLensFacing/lens_facing_/
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 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mcasas, Do you know how to specify in BUILD.gn that a resource file is needed when running the unittest? I have tried 2 ways (in patch set 13 and 14) but nothing works. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:115: std::vector<base::StringPiece> lines = base::SplitStringPiece( On 2016/12/03 03:37:11, mcasas wrote: > nit: const Done. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:119: for (base::StringPiece line : lines) { On 2016/12/03 03:37:11, mcasas wrote: > const base::StringPiece& line Done. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:123: } On 2016/12/03 03:37:11, mcasas wrote: > micro nit: > if (line.starts_with("#")) // Ignore comment lines. > continue; Done. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.cc:124: std::vector<base::StringPiece> key_value = base::SplitStringPiece( On 2016/12/03 03:37:11, mcasas wrote: > const ? Also in l.133, etc. Essentially, try to > const-ify as much as possible please. Done. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos.h:54: CameraFacingChromeOS(const char* config_file_path); On 2016/12/03 03:37:11, mcasas wrote: > const std::string& here and in l.72. > base::FilePath() will work with this already. Done. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.cc (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.cc:153: // Therefore, for back camera, we need to rotate (360 - |rotation|). On 2016/12/03 03:37:11, mcasas wrote: > All this is to say that, same as Android, rotation goes > backwards for the "back" camera, that is called > "environment" there. > Although I'm a fan of ASCII art myself, I wonder if you > could convey that meaning in a more succinct > fashion here? My team members were confused by why back camera should rotate backward here and in android code base several times. That's why I would like to draw this and make it super clear. https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... File media/capture/video/linux/video_capture_device_chromeos.h (right): https://codereview.chromium.org/2508803002/diff/220001/media/capture/video/li... media/capture/video/linux/video_capture_device_chromeos.h:36: const CameraFacingChromeOS::LensFacing kLensFacing; On 2016/12/03 03:37:11, mcasas wrote: > s/kLensFacing/lens_facing_/ 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2508803002/diff/280001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos_unittest.cc (right): https://codereview.chromium.org/2508803002/diff/280001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos_unittest.cc:14: std::string("fake_camera_characteristics.conf")); I don't think you can access files from unittests, but might need to create it yourself like in e.g. [1]. [1] https://cs.chromium.org/chromium/src/base/files/file_unittest.cc?q=unittest+b...
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...
https://codereview.chromium.org/2508803002/diff/280001/media/capture/video/li... File media/capture/video/linux/camera_facing_chromeos_unittest.cc (right): https://codereview.chromium.org/2508803002/diff/280001/media/capture/video/li... media/capture/video/linux/camera_facing_chromeos_unittest.cc:14: std::string("fake_camera_characteristics.conf")); On 2016/12/05 18:23:19, mcasas wrote: > I don't think you can access files from unittests, but might > need to create it yourself like in e.g. [1]. > > [1] > https://cs.chromium.org/chromium/src/base/files/file_unittest.cc?q=unittest+b... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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.
The CQ bit was checked by shenghao@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2508803002/#ps320001 (title: "remove copy target")
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": 320001, "attempt_start_ts": 1481035218151800,
"parent_rev": "eab6b3fab249fe3d66ba6f91ed735ec1c563b96d", "commit_rev":
"ebd4bb1b8358e36984bf9ef7c599c73e769e87fa"}
Message was sent while issue was closed.
Description was changed from ========== Rotate frames correctly for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ========== to ========== Rotate frames correctly for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Rotate frames correctly for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. ========== to ========== Rotate frames correctly for back camera In 90 and 270 degree tablet mode, back camera used to show upside-down frames. We need to rotate the frames correctly for back cameras. BUG=56762 TEST=Verify that back camera shows correct preview in all rotations. Committed: https://crrev.com/fd4e73efe59269463f10743694597074ef2ceefb Cr-Commit-Position: refs/heads/master@{#436594} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/fd4e73efe59269463f10743694597074ef2ceefb Cr-Commit-Position: refs/heads/master@{#436594}
Message was sent while issue was closed.
Looks like you used the wrong issue number. What is the correct issue number?
Message was sent while issue was closed.
On 2016/12/06 14:49:23, PhistucK wrote: > Looks like you used the wrong issue number. What is the correct issue number? Acutally it should be chrome-os-partner:56762
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2554903002/ by hbos@chromium.org. The reason for reverting is: Causes compile failures: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 436594 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
