Closed Bug 976397 Opened 10 years ago Closed 2 years ago

Rotate camera preview image based on the original raw image orientation

Categories

(Core :: Layout, defect, P4)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED INACTIVE

People

(Reporter: pchang, Unassigned)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(3 files, 5 obsolete files)

From bug 970183, the raw data from camera sensor is always under landscape mode. It displayed the wrong direction under portrait mode if app didn't configure the rotation.

Create this bug to add rotation attribute on image itself and configure compositor to do the rotation.

https://bugzilla.mozilla.org/show_bug.cgi?id=970183#c0
Attached patch patch_bug976397 v1 (obsolete) — Splinter Review
Attached patch that could rotate the image layer of camera preview.
But it will change the size of video tag (width and hight switch)

Based on video tag spec, we couldn't change the video tag size(video.width/video.height) but we could adjust the video(image layer) ratio to align the video tag size.
Priority: -- → P4
Blocks: 980266
Attached patch rotate preview window v2 (obsolete) — Splinter Review
Update patch
 - Sync to latest code
 - Rotate image based on the image's rotation flag
 - Keep the same aspect video ratio to align video tag size

Checking this patch works if video tag size didn't setup.
Attachment #8381882 - Attachment is obsolete: true
Oops...the video tag size will refer the video frame size if app didn't configure it.
Therefore, attachment 8388369 [details] [diff] [review] will get the landscape size but display with portrait mode.
One more thing to mention that attachment 8388369 [details] [diff] [review] reduce the user space CPU usage from 70 to 50% on my leo device. But I saw compositor thread consume about 40~50% CPU usage. Will create another bug for tracking.
Attached file peak-profile.zip
I tried it on peak. The cpu usage of compositor is about 5%. Here is the profiling data.
Attached file unagi-perf.zip
This profiling data is from unagi. The compositor thread in b2g is also about 50%. It seems that the cpu time spends on memcpy.
Assignee: nobody → pchang
If device goes to HWC composition, I will see wrong video window.
It doesn't have problem for the GPU composition path.
(In reply to StevenLee[:slee] from comment #6)
> Created attachment 8389604 [details]
> unagi-perf.zip
> 
> This profiling data is from unagi. The compositor thread in b2g is also
> about 50%. It seems that the cpu time spends on memcpy.

Create bug 983540 to follow up.
Most camera sensors of mobile devices are mounted as landscape mode and the output is the landscape frames. Therefore, we need to rotate the image if device is under portrait mode. 

For camera app, the rotation already handles by camera app. 
For WebRTC which is cross-platform feature, we need to add image rotation support inside gecko.

Create this patch to do image rotation through layer transform to reduce the CPU usage.
Attachment #8388369 - Attachment is obsolete: true
Attachment #8391081 - Flags: review?(sotaro.ikeda.g)
Attachment #8391081 - Flags: review?(roc)
Attachment #8391081 - Flags: review?(slee)
(In reply to peter chang[:pchang][:peter] from comment #9)
> Created attachment 8391081 [details] [diff] [review]
> add image rotation support based on raw image
> 
> Most camera sensors of mobile devices are mounted as landscape mode and the
> output is the landscape frames. Therefore, we need to rotate the image if
> device is under portrait mode. 
> 
> For camera app, the rotation already handles by camera app. 
> For WebRTC which is cross-platform feature, we need to add image rotation
> support inside gecko.
> 
> Create this patch to do image rotation through layer transform to reduce the
> CPU usage.

Note: The size of video tag will be refer to the video intrinsic size if no video tag width/height setup. Current patch still keep this behavior.

For example, the portrait video with size (320, 240). The default video tag size is still (320, 240) although it is the portrait video.

roc, do you think we need to modify the intrinsic size for this special video? (like change size to (240,320))
Comment on attachment 8391081 [details] [diff] [review]
add image rotation support based on raw image

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

This patch doesn't handle the other consumers of Images such as canvas drawImage(), MediaRecorder and WebRTC PeerConnection.

It seems to me that the best approach would be to move the rotation information into GrallocImage. I think the Stride and Skip members of PlanarYCbCrImage::Data can be modified to represent a rotated image directly (with negative stride/skip as necessary). You'd need to modify the consumers of GrallocImage/PlanarYCbCrImage to make sure they can handle this. It may also be useful to add the rotation as a direct member of PlanarCbCrImage.
Attachment #8391081 - Flags: review?(roc) → review-
Blocks: 984215
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Comment on attachment 8391081 [details] [diff] [review]
> add image rotation support based on raw image
> 
> Review of attachment 8391081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch doesn't handle the other consumers of Images such as canvas
> drawImage(), MediaRecorder and WebRTC PeerConnection.
> 
This bug only focus on rotate image on compositor side based on the ram image orientation.
For the MediaRecorder or WebRTC PerrConnection, there are different bugs to cover.

For Canvas drawImage, I will create the bug to follow up.

> It seems to me that the best approach would be to move the rotation
> information into GrallocImage. I think the Stride and Skip members of
> PlanarYCbCrImage::Data can be modified to represent a rotated image directly
> (with negative stride/skip as necessary). You'd need to modify the consumers
> of GrallocImage/PlanarYCbCrImage to make sure they can handle this. It may
> also be useful to add the rotation as a direct member of PlanarCbCrImage.

Since the camera preview on b2g is using GrallocImage, I think move rotation info to GrallocImage is a better choice.
No longer blocks: 984215
Blocks: 984215
Attachment #8391081 - Flags: review?(sotaro.ikeda.g)
Attachment #8391081 - Flags: review?(slee)
Blocks: 984216
Move rotation info to GrallocImage only
Attachment #8391081 - Attachment is obsolete: true
Move rotation to GrallocImage
Attachment #8392049 - Attachment is obsolete: true
Attachment #8392068 - Flags: review?(roc)
Comment on attachment 8392068 [details] [diff] [review]
add image rotation support based on raw image v2

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

We shouldn't have to change nsVideoFrame. Instead, the GrallocImage's reported width and height should be swapped if necessary, and the compositor code that composites gralloc images should do the rotation there.
Attachment #8392068 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Comment on attachment 8392068 [details] [diff] [review]
> add image rotation support based on raw image v2
> 
> Review of attachment 8392068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We shouldn't have to change nsVideoFrame. Instead, the GrallocImage's
> reported width and height should be swapped if necessary, and the compositor
> code that composites gralloc images should do the rotation there.

Moving rotation support into compositor is almost done, but maybe move to clientImageLayer is a better option because content(WebRTC) knows when the image needs to rotate but this information doesn't pass to compositor.
Move the image rotation support to layer only.
Attachment #8392068 - Attachment is obsolete: true
Attachment #8396845 - Flags: review?(roc)
Comment on attachment 8396845 [details] [diff] [review]
add image rotation support based on raw image v3

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

This patch is much better than the previous one. However, I don't think we should modify SetBaseTransform like this. It violates the invariant that GetBaseTransform returns what was set by SetBaseTransform.

Instead I think the rotation should be passed through to the compositor as part of the image data. Then the compositor should apply it, either in ImageLayerComposite::RenderLayer or ImageHost::Composite.

::: gfx/layers/GrallocImages.h
@@ +189,1 @@
>    RefPtr<GrallocTextureClientOGL> mTextureClient;

Reorder the fields so that pointers are first, followed by mRotation, followed by mBufferAllocated. Saves memory.

::: gfx/layers/ImageContainer.h
@@ +542,5 @@
>  
>    /**
> +   * Returns the rotation of current active image if exists
> +   */
> +  int GetRotation();

Document what this returns.

However, I don't think we should need to modify ImageContainer for this bug.
Attachment #8396845 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 8396845 [details] [diff] [review]
> add image rotation support based on raw image v3
> 
> Review of attachment 8396845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is much better than the previous one. However, I don't think we
> should modify SetBaseTransform like this. It violates the invariant that
> GetBaseTransform returns what was set by SetBaseTransform.
> 
> Instead I think the rotation should be passed through to the compositor as
> part of the image data. Then the compositor should apply it, either in
> ImageLayerComposite::RenderLayer or ImageHost::Composite.
> 
roc, since we need to support both HWC/GPU composition, ImageLayerComposite::RenderLayer or ImageHost::Composite doesn't work for HWC.
How about put the transform calculation at ImageLayerComposite::ComputeEffectiveTransforms[1]?

By the way, HWC will query the GetShadowVisibleRegion to calculate the correct crop region. It requires to provide the rotated visibleRegion. How do you think to hack the ImageLayerComposite::GetShadowVisibleRegion to apply the rotation? Or apply the visibleRegion rotation inside HwcComposer2D::PrepareLayerList[2].

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageLayerComposite.cpp#115
[2]http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#192
> ::: gfx/layers/GrallocImages.h
> @@ +189,1 @@
> >    RefPtr<GrallocTextureClientOGL> mTextureClient;
> 
> Reorder the fields so that pointers are first, followed by mRotation,
> followed by mBufferAllocated. Saves memory.
> 
Will do.
> ::: gfx/layers/ImageContainer.h
> @@ +542,5 @@
> >  
> >    /**
> > +   * Returns the rotation of current active image if exists
> > +   */
> > +  int GetRotation();
> 
> Document what this returns.
> 
> However, I don't think we should need to modify ImageContainer for this bug.

Sure, I will remove from ImageContainer.
Flags: needinfo?(roc)
(In reply to peter chang[:pchang][:peter] from comment #19)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> > Comment on attachment 8396845 [details] [diff] [review]
> > add image rotation support based on raw image v3
> > 
> > Review of attachment 8396845 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch is much better than the previous one. However, I don't think we
> > should modify SetBaseTransform like this. It violates the invariant that
> > GetBaseTransform returns what was set by SetBaseTransform.
> > 
> > Instead I think the rotation should be passed through to the compositor as
> > part of the image data. Then the compositor should apply it, either in
> > ImageLayerComposite::RenderLayer or ImageHost::Composite.
> > 
> roc, since we need to support both HWC/GPU composition,
> ImageLayerComposite::RenderLayer or ImageHost::Composite doesn't work for
> HWC.

You can change HWC as well. You could add the rotation to LayerRenderState and copy it into there in HwcComposer2D::PrepareLayerList.

> By the way, HWC will query the GetShadowVisibleRegion to calculate the
> correct crop region. It requires to provide the rotated visibleRegion. How
> do you think to hack the ImageLayerComposite::GetShadowVisibleRegion to
> apply the rotation? Or apply the visibleRegion rotation inside
> HwcComposer2D::PrepareLayerList[2].

The latter.
Flags: needinfo?(roc)
For the image rotation, we need to pass it from content to b2g process.

After discussing with nical and roc, I'm going to add struct ImageAttrbuite to include PictureRect[1] and rotation info and replace UpdatePictureRect()[2] with new UpdateImageAttribute().

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh#336
[2]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ImageBridgeChild.cpp#175
(In reply to Randy Lin [:rlin] from comment #22)
> It should define the DYNAMIC_GUM_ROTATION, right?
> http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/
> MediaEngineWebRTCVideo.cpp#561

I think the DYNAMIC_GUM_ROTATION means WebRTC could accept rotation changes after WebRTC peer connection start. Now is disable by default.
(In reply to peter chang[:pchang][:peter] from comment #23)
> (In reply to Randy Lin [:rlin] from comment #22)
> > It should define the DYNAMIC_GUM_ROTATION, right?
> > http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/
> > MediaEngineWebRTCVideo.cpp#561
> 
> I think the DYNAMIC_GUM_ROTATION means WebRTC could accept rotation changes
> after WebRTC peer connection start. Now is disable by default.

Right - we disabled that until MediaRecorder can handle dynamic rotation.  (Note: this means that once a call starts, you can't rotate the phone - or rather once you do it won't adapt to the new position)
I think mp4 muxer cannot support the frame size change on the fly....
(In reply to Randy Lin [:rlin] from comment #25)
> I think mp4 muxer cannot support the frame size change on the fly....

Right.  Per discussion when we landed the patch with DYNAMIC_GUM_ROTATION turned off, what the MediaRecorder needs to do is: 
if rotation == initial rotation, just encode
if rotation != initial rotation, scale-and-inset-with-black-bars (90 degree rotates).  Note that whether the black bars are top/bottom or left/right will depend on the initial rotation.  I'm assuming 180 rotate can be handled automatically as there's no resolution change.

Note that PeerConnection would really benefit from DYNAMIC_GUM_ROTATION
So we also need to do the rotation as bug:984215 if we turn on this feature. 
The benefit is remote user can see the head in the right angle whenever the caller change the phone in portrait or landscape mode, is it?
(In reply to Randy Lin [:rlin] from comment #27)
> So we also need to do the rotation as bug:984215 if we turn on this feature. 
> The benefit is remote user can see the head in the right angle whenever the
> caller change the phone in portrait or landscape mode, is it?

yes
Fill in the black bar may cause the performance issue. I may measure it first.
(In reply to Randy Lin [:rlin] from comment #29)
> Fill in the black bar may cause the performance issue. I may measure it
> first.

black-fill of the sidebars should be minor - and if we're smart about it may only have to be done once (use a persistent buffer to rotate-and-resize into, with back bars set when it's allocated).  Even if we can't do that, I don't think that's the perf blocker.  The rotate-and-resize is the only real perf issue here I think.
Depends on: 992705
Status: NEW → ASSIGNED
Just rebased to latest m-c. Right now the Rotation information is hidden inside LayerRenderState which could be used by GPU and HWC. But I still have the issue to pass rotation info from ImageHost(CompositableHost) to GrallocTextureHost(TextureHost) during each frame update.

Consider how to pass rotation info to GrallocTextureHost(TextureHost) in a generic way.
(In reply to peter chang[:pchang][:peter] from comment #31)
> Just rebased to latest m-c. Right now the Rotation information is hidden
> inside LayerRenderState which could be used by GPU and HWC. But I still have
> the issue to pass rotation info from ImageHost(CompositableHost) to
> GrallocTextureHost(TextureHost) during each frame update.
> 
> Consider how to pass rotation info to GrallocTextureHost(TextureHost) in a
> generic way.

To support the rotation during GPU or HWC composition, the LayserRenderState is a good place to hold the rotation info. We need to pass the info from ImageHost(CompositableHost) to GrallocTextureHost(TextureHost).

My idea is to add the following new texture flags in CompositorTypes.h[1] and also pass the texture flag(if changed) from ImageHost to GrallocTextureHost during the composition. Now the texture flag is updated only at TextureHost creation and there are some comments[2] that want to remove the textureflag API. 

  Do you know why we want to remove those APIs and do you have any comment about my idea to pass rotation by texture flag?

[New texture flags]
TEXTURE_FLIP_H
TEXTURE_FLIP_V    (TEXTURE_NEEDS_Y_FLIP)
TEXTURE_ROT_90
TEXTURE_ROT_180 = TEXTURE_FLIP_H|TEXTURE_FLIP_V
TEXTURE_ROT_270 = TEXTURE_ROT_90|TEXTURE_ROT_180

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/CompositorTypes.h#31
[2]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.h#377
Flags: needinfo?(nical.bugzilla)
nical, I would like to know your feedback on comment32.
(In reply to peter chang[:pchang][:peter] from comment #32)
> (In reply to peter chang[:pchang][:peter] from comment #31)
> > Just rebased to latest m-c. Right now the Rotation information is hidden
> > inside LayerRenderState which could be used by GPU and HWC. But I still have
> > the issue to pass rotation info from ImageHost(CompositableHost) to
> > GrallocTextureHost(TextureHost) during each frame update.
> > 
> > Consider how to pass rotation info to GrallocTextureHost(TextureHost) in a
> > generic way.
> 
> To support the rotation during GPU or HWC composition, the LayserRenderState
> is a good place to hold the rotation info. We need to pass the info from
> ImageHost(CompositableHost) to GrallocTextureHost(TextureHost).
> 
> My idea is to add the following new texture flags in CompositorTypes.h[1]
> and also pass the texture flag(if changed) from ImageHost to
> GrallocTextureHost during the composition. Now the texture flag is updated
> only at TextureHost creation and there are some comments[2] that want to
> remove the textureflag API. 
> 
>   Do you know why we want to remove those APIs and do you have any comment
> about my idea to pass rotation by texture flag?
> 
> [New texture flags]
> TEXTURE_FLIP_H
> TEXTURE_FLIP_V    (TEXTURE_NEEDS_Y_FLIP)
> TEXTURE_ROT_90
> TEXTURE_ROT_180 = TEXTURE_FLIP_H|TEXTURE_FLIP_V
> TEXTURE_ROT_270 = TEXTURE_ROT_90|TEXTURE_ROT_180
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/CompositorTypes.
> h#31
> [2]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> TextureHost.h#377

TextureFlags are set before sharing the TextureClient with the compositor, and can't be modified after connection is established. I would like that it remains that way because changing some flags while the texture is shared could cause race conditions (for example the flags that define how the texture is to be destroyed).
So if for a given Texture the rotation can change during its lifetime I think TextureFlags aren't a good place to store it.
Also, TextureClient/Host as much as possible should remain focused on abstracting out the surface type rather than storing how it is composited (which is the Compositable's job).

Here's my suggestion: the Message OpUseTexture could contain an optional struct that adds some information such as specific rotation when needed. This will be needed to attach timestamps to textures for some of our video compositing code paths, so we'll need something like this anyway. This seems to me like a better place. When the ImageClient handles the UseTexture message, it can store the appropriate rotation.

Does this sound good to you?
Flags: needinfo?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #34)
> TextureFlags are set before sharing the TextureClient with the compositor,
> and can't be modified after connection is established. I would like that it
> remains that way because changing some flags while the texture is shared
> could cause race conditions (for example the flags that define how the
> texture is to be destroyed).
> So if for a given Texture the rotation can change during its lifetime I
> think TextureFlags aren't a good place to store it.
> Also, TextureClient/Host as much as possible should remain focused on
> abstracting out the surface type rather than storing how it is composited
> (which is the Compositable's job).
> 
> Here's my suggestion: the Message OpUseTexture could contain an optional
> struct that adds some information such as specific rotation when needed.
> This will be needed to attach timestamps to textures for some of our video
> compositing code paths, so we'll need something like this anyway. This seems
> to me like a better place. When the ImageClient handles the UseTexture
> message, it can store the appropriate rotation.
> 
> Does this sound good to you?

In my WIP, I already change OpUpdatePictureRect to OpUpdateImageAttribute to pass rotation through IPC. It will be great to merge OpUpdateImageAttribute to OpUseTexture.

  But, my question here is how to pass the rotation from ImageHost(CompositableHost) to GrallocTextureHost(TextureHost). The reason to do that is to provide the correct LayerRenderState for GPU or HWC composition. In my previous plan, texture flag is a good candidate to do that.

Another dirty way[1] is to hack the LayerTrenderState that returns from TextureHost.

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp#212

nical, how do you think?
Flags: needinfo?(nical.bugzilla)
I prefer the dirty way :) for the reason I exposed in comment 34. I think it's fine to have part of the data required by LayerRenderState come from the CompositableHost since LayerRenderState contains information about more than the texture data itself (orientation not being information about how the data so stored, but about how the data is composited).
I admit that there are a few TextureFlags that are bad examples of what I said, but I'd like to move them out in the long run.
Flags: needinfo?(nical.bugzilla)

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

The bug assignee didn't login in Bugzilla in the last 7 months.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: howareyou322 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dholbert)

Looks like this was FirefoxOS/Boot2Gecko-specific. Likely no longer relevant/reproducible on supported hardware.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(dholbert)
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: