Rotate frames according to camera orientation

Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=684375, chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.

Review-Url: https://codereview.chromium.org/2648743002
Cr-Commit-Position: refs/heads/master@{#446285}
Committed: https://chromium.googlesource.com/chromium/src/+/306b9e55cc83f18e30023e1af00f2f7abc649c6e

[shenghao, 2017-01-23 07:28:51]

Description was changed from

==========
Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.
==========

to

==========
Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.
==========

[shenghao, 2017-01-23 07:28:51]

shenghao@chromium.org changed reviewers:
+ mcasas@chromium.org, posciak@chromium.org, wuchengli@chromium.org

[wuchengli, 2017-01-23 09:43:27]

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
File media/capture/video/linux/camera_config_chromeos.cc (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
media/capture/video/linux/camera_config_chromeos.cc:91: return
kLensFacingDefault;
Use kCameraIdNotFound = -1

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
File media/capture/video/linux/camera_config_chromeos.h (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
media/capture/video/linux/camera_config_chromeos.h:63: const std::string&
model_id) const;
Document what the orientation is. Copy the documentation from Android.

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
File media/capture/video/linux/camera_config_chromeos_unittest.cc (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
media/capture/video/linux/camera_config_chromeos_unittest.cc:31:
std::string("04f2:b53a")));
Check orientation.

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/video_capture_device_chromeos.h (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/video_capture_device_chromeos.h:37: const int
orientation_;
s/orientation_/camera_orientaton_/. Otherwise, it's not clear it's display or
camera orientation.

[shenghao, 2017-01-23 12:06:58]

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
File media/capture/video/linux/camera_config_chromeos.cc (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
media/capture/video/linux/camera_config_chromeos.cc:91: return
kLensFacingDefault;
On 2017/01/23 09:43:27, wuchengli wrote:
> Use kCameraIdNotFound = -1

Done.

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
File media/capture/video/linux/camera_config_chromeos.h (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
media/capture/video/linux/camera_config_chromeos.h:63: const std::string&
model_id) const;
On 2017/01/23 09:43:27, wuchengli wrote:
> Document what the orientation is. Copy the documentation from Android.

Done.

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
File media/capture/video/linux/camera_config_chromeos_unittest.cc (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/c...
media/capture/video/linux/camera_config_chromeos_unittest.cc:31:
std::string("04f2:b53a")));
On 2017/01/23 09:43:27, wuchengli wrote:
> Check orientation.

Done.

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/video_capture_device_chromeos.h (right):

https://codereview.chromium.org/2648743002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/video_capture_device_chromeos.h:37: const int
orientation_;
On 2017/01/23 09:43:27, wuchengli wrote:
> s/orientation_/camera_orientaton_/. Otherwise, it's not clear it's display or
> camera orientation.

Done.

[mcasas, 2017-01-23 21:44:57]

Please make a crbug.com/ to track this
work (or reuse the appropriate one).

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos.cc (right):

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:71: auto camera_id_iter =
camera_id_to_orientation_const.find(camera_id);
You shouldn't encode the type of the variable
or its const-ness in the name, so just
s/camera_id_iter/camera_id/ and
s/camera_id_to_orientation_const/camera_id_to_orientation/

(that's usual in Win programming, but not in Chromium)

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:88: // Can't find Usb ID.
Fall back to use model_id.
s/model_id/|model_id|/

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:98: return camera_id;
The code in this and GetOrientation() methods is hard
to read due to the nested if()s. Prefer early return and
do not define aliases just for constness, e.g.:

  const auto& usb_id =...
  if (usb_id != usb_id_to_camera_id_.end())
    return usb_id->second;

  const model_id = ...
  if (model_id != model_id_to_camera_id_.end())
    return usb_id->second;

  DLOG(ERROR) << "..."
  return kCameraIdNotFound;

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos.h (right):

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.h:74: CAPTURE_EXPORT int
GetOrientation(const std::string& device_id,
You don't need CAPTURE_EXPORT in every method,
the class one is sufficient.

[wuchengli, 2017-01-24 04:18:23]

lgtm

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos_unittest.cc (right):

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos_unittest.cc:41: }
Also check orientation here.

[shenghao, 2017-01-24 08:19:08]

Description was changed from

==========
Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.
==========

to

==========
Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=684375, chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.
==========

[shenghao, 2017-01-24 14:29:52]

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos.cc (right):

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:71: auto camera_id_iter =
camera_id_to_orientation_const.find(camera_id);
On 2017/01/23 21:44:57, mcasas wrote:
> You shouldn't encode the type of the variable
> or its const-ness in the name, so just
> s/camera_id_iter/camera_id/ and
> s/camera_id_to_orientation_const/camera_id_to_orientation/
> 
> (that's usual in Win programming, but not in Chromium)

Done.

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:88: // Can't find Usb ID.
Fall back to use model_id.
On 2017/01/23 21:44:57, mcasas wrote:
> s/model_id/|model_id|/

Done.

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:98: return camera_id;
On 2017/01/23 21:44:57, mcasas wrote:
> The code in this and GetOrientation() methods is hard
> to read due to the nested if()s. Prefer early return and
> do not define aliases just for constness, e.g.:
> 
>   const auto& usb_id =...
>   if (usb_id != usb_id_to_camera_id_.end())
>     return usb_id->second;
> 
>   const model_id = ...
>   if (model_id != model_id_to_camera_id_.end())
>     return usb_id->second;
> 
>   DLOG(ERROR) << "..."
>   return kCameraIdNotFound;
> 
> 

Done, except that I have to keep the const temporary variable. Otherwise find()
will become a non-const method, which conflicts the constness of GetCameraId()

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos.h (right):

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.h:74: CAPTURE_EXPORT int
GetOrientation(const std::string& device_id,
On 2017/01/23 21:44:57, mcasas wrote:
> You don't need CAPTURE_EXPORT in every method,
> the class one is sufficient.

Done.

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos_unittest.cc (right):

https://codereview.chromium.org/2648743002/diff/20001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos_unittest.cc:41: }
On 2017/01/24 04:18:22, wuchengli wrote:
> Also check orientation here.

Done.

[mcasas, 2017-01-25 16:51:49]

lgtm with nit.

https://codereview.chromium.org/2648743002/diff/40001/media/capture/video/lin...
File media/capture/video/linux/camera_config_chromeos.cc (right):

https://codereview.chromium.org/2648743002/diff/40001/media/capture/video/lin...
media/capture/video/linux/camera_config_chromeos.cc:89: }
nit: no {} for one-line bodies, here and in l.93.

[shenghao, 2017-01-26 07:01:43]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2017-01-26 07:01:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
wuchengli@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2648743002/#ps60001
(title: "removed {} for one-line blocks")

[commit-bot: I haz the power, 2017-01-26 07:02:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 08:42:14]

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

[commit-bot: I haz the power, 2017-01-26 08:42:15]

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...)

[shenghao, 2017-01-26 09:03:14]

The CQ bit was checked by shenghao@chromium.org

[commit-bot: I haz the power, 2017-01-26 09:03:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 09:46:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485421394262230,
"parent_rev": "7bab3265dc08756009c0374803bbbdd11fa6a1e8", "commit_rev":
"306b9e55cc83f18e30023e1af00f2f7abc649c6e"}

[commit-bot: I haz the power, 2017-01-26 09:47:02]

Description was changed from

==========
Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=684375, chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.
==========

to

==========
Rotate frames according to camera orientation

Camera orientation information is available in
camera_characteristics.conf file. When rotating frames, we should
consider both the display rotation and the camera orientation in the
config file.

BUG=684375, chrome-os-partner:61414
TEST=On reef, verify that back camera shows correct preview when in
tablet mode.

Review-Url: https://codereview.chromium.org/2648743002
Cr-Commit-Position: refs/heads/master@{#446285}
Committed:
https://chromium.googlesource.com/chromium/src/+/306b9e55cc83f18e30023e1af00f...
==========

[commit-bot: I haz the power, 2017-01-26 09:47:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/306b9e55cc83f18e30023e1af00f...