Rotate camera preview image based on the original raw image orientation

ASSIGNED
Assigned to

Status

()

defect
P4
normal
ASSIGNED
5 years ago
4 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Posted 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
(Assignee)

Updated

5 years ago
Blocks: 980266
(Assignee)

Comment 2

5 years ago
Posted 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
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.

Comment 5

5 years ago
Posted file peak-profile.zip
I tried it on peak. The cpu usage of compositor is about 5%. Here is the profiling data.

Comment 6

5 years ago
Posted 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)

Updated

5 years ago
Assignee: nobody → pchang
(Assignee)

Comment 7

5 years ago
If device goes to HWC composition, I will see wrong video window.
It doesn't have problem for the GPU composition path.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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)
(Assignee)

Updated

5 years ago
Attachment #8391081 - Flags: review?(slee)
(Assignee)

Comment 10

5 years ago
(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-

Updated

5 years ago
Blocks: 984215
(Assignee)

Comment 12

5 years ago
(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
(Assignee)

Updated

5 years ago
Blocks: 984215
(Assignee)

Updated

5 years ago
Attachment #8391081 - Flags: review?(sotaro.ikeda.g)
Attachment #8391081 - Flags: review?(slee)

Updated

5 years ago
Blocks: 984216
(Assignee)

Comment 13

5 years ago
Move rotation info to GrallocImage only
(Assignee)

Updated

5 years ago
Attachment #8391081 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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-
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
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-
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 21

5 years ago
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
(Assignee)

Comment 23

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 992705

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 31

5 years ago
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.
(Assignee)

Comment 32

5 years ago
(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
(Assignee)

Updated

5 years ago
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 33

5 years ago
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)
(Assignee)

Comment 35

5 years ago
(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)
You need to log in before you can comment on or make changes to this bug.