1065957 - [B2G] Correct photo orientation

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alfredo Yang (:alfredo)]

Use camera control parameter CAMERA_PARAM_PICTURE_ROTATION to correct the photo orientation.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: b2g_photo_orientation_change

Comment 2

[John Lin [:jhlin][:jolin]]

Comment on attachment 8489227 [details] [diff] [review]
b2g_photo_orientation_change

Review of attachment 8489227 [details] [diff] [review]:
-----------------------------------------------------------------

Always set rotation value before |TakePicture()| seems simpler to me. But overall this looks fine.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +100,5 @@
>      : mCameraControl(nullptr)
>      , mCallbackMonitor("WebRTCCamera.CallbackMonitor")
>      , mRotation(0)
>      , mBackCamera(false)
> +    , mOrientationChanged(true)

Better to explain why this is true by default. To ensure that camera rotation value will be set according to the screen orientation when gUM is called?

@@ +203,5 @@
>    nsresult TakePhoto(PhotoCallback* aCallback) MOZ_OVERRIDE;
>  
> +  // It sets the correct photo orientation via camera parameter according to
> +  // current screen orientation.
> +  nsresult PhotoOrientationUpdate();

Start method name with a verb: UpdatePhotoOrientation().

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +799,5 @@
>           mRotation, mCaptureIndex, mBackCamera, mCameraAngle));
>    }
>  #endif
> +
> +  mOrientationChanged = true;

Doesn't this need to be added before |#endif|?

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8489227 [details] [diff] [review]
b2g_photo_orientation_change

Review of attachment 8489227 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +100,5 @@
>      : mCameraControl(nullptr)
>      , mCallbackMonitor("WebRTCCamera.CallbackMonitor")
>      , mRotation(0)
>      , mBackCamera(false)
> +    , mOrientationChanged(true)

+1, maybe set this during the StartImpl?

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +1088,5 @@
> +
> +  hal::ScreenConfiguration config;
> +  hal::GetCurrentScreenConfiguration(&config);
> +
> +  // The rotation angle is clockwise.

Does this calculation apply to other devices? Maybe have a try on n4 or n5 or others chip vender's devices.

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: b2g_photo_orientation_change

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8489264 [details] [diff] [review]
b2g_photo_orientation_change

Review of attachment 8489264 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +1110,5 @@
> +  orientation = (mBackCamera ? orientation : (-orientation));
> +
> +  ICameraControlParameterSetAutoEnter batch(mCameraControl);
> +  // It changes the orientation value in EXIF information only.
> +  mCameraControl->Set(CAMERA_PARAM_PICTURE_ROTATION, orientation);

We don't honor EXIF orientation data by default (for good reasons). But I guess that's OK here.

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: b2g_photo_orientation_change

Thanks for review.

Rebase.

Try result.
https://tbpl.mozilla.org/?tree=Try&rev=35d23e574367

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de7c7bbb5679

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de7c7bbb5679