1100110 - [camera] screen preview in video mode often doesn't use full screen size, black area

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

B2G 2.2 20141115160201 on Flame (v188 base image), git 1dd8ad8f, mozilla-central a52bf59965a0

In the camera app, if the user switches from picture mode to video mode, the preview uses approximately only 2/3 of width and height (starting in the lower left).

With m-c revision a52bf59965a0, the fix for bug 1098660 has been applied.

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

This is a limitation of the underlying camera hardware/driver. It does not support a preview size that is capable of filling the screen while recording a video -- see bug 1098028.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

There appears to be different versions of the flame hardware, as it seems to only occurs on models with supported video sizes like 1280x720.

11-17 11:13:36.259   201  1484 D QCameraParameters: int32_t qcamera::QCameraParameters::initDefaultParameters(): supported preview sizes: 1280x720,864x480,800x480,768x432,720x480,640x480,576x432,480x320,384x288,352x288,320x240,240x160,176x144,144x176
11-17 11:13:36.259   201  1484 D QCameraParameters: int32_t qcamera::QCameraParameters::initDefaultParameters(): supported video sizes: 1280x720,864x480,800x480,720x480,640x480,480x320,352x288,320x240,176x144
11-17 11:13:36.289  2547  2630 I Gecko   :  - default preview size:          1280 x 720
11-17 11:13:36.289  2547  2630 I Gecko   :  - preferred preview size for v:  1280 x 720

However I think this may actually be a graphics glitch rather than a camera specific issue. Whenever there is an animation active on the screen, it displays fine (see attached video) and when I play back the recorded video it doesn't show the clipping.

Comment 4

[Tony Chung [:tchung]]

QA can reproduce.  see dupe bug 1102682 for more details if needed.

Comment 5

[Pi Wei Cheng [:piwei] (inactive)]

This issue seems to only reproduce on 512MB memory Flame. Please correct me if any of you repro'ed this bug on 319 mem.

2.1 is unaffected. Working on the window now.

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141114160242
Gaia: 7fda06da51e7a201ad57bd8e9fa9a48941064c9a
Gecko: 55a7cb8afa81
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141114192444
Gaia: 7fda06da51e7a201ad57bd8e9fa9a48941064c9a
Gecko: ee0ef288a8da
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=55a7cb8afa81&tochange=ee0ef288a8da

Caused by Bug 1098660.

Comment 7

[Joshua Mitchell (Inactive)]

Caused by Bug 1098660 - can you take a look Mike?

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

Sotaro, we're seeing some weird clipping on Flame: http://youtu.be/nh61n8PdVJM

Comment 9

[Sotaro Ikeda [:sotaro]]

I confirmed the symptom on master flame. I am going to check what is happening.

Comment 10

[Sotaro Ikeda [:sotaro]]

The problem seems to happen when HwComposer is used.

Comment 11

[Sotaro Ikeda [:sotaro]]

I confirmed that this bug is caused by Bug 1097441. The following change caused the regression.

--------------------------------------------------------------------
   gfxPoint p = r.TopLeft() + aContainerParameters.mOffset;
   Matrix transform = Matrix::Translation(p.x, p.y);
-  transform.PreScale(r.Width() / frameSize.width,
-                     r.Height() / frameSize.height);
   layer->SetBaseTransform(gfx::Matrix4x4::From2D(transform));
+  layer->SetScaleToSize(IntSize(r.width, r.height), ScaleMode::STRETCH);
   nsRefPtr<Layer> result = layer.forget();
   return result.forget();

https://hg.mozilla.org/mozilla-central/rev/11c5034cb10b
 }

Comment 12

[Sotaro Ikeda [:sotaro]]

The above change broke the assumption of HwcUtils::PrepareLayerRects().

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcUtils.cpp#40

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle ScaleMode HwcComposer2D

By applying the patch to master flame. The problem seems to be addressed.

Comment 15

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8529284 [details] [diff] [review]
patch - Handle ScaleMode HwcComposer2D

matt, can you give a feedback to the patch?

Comment 16

[Matt Woodrow (:mattwoodrow)]

Can you explain why we need to do this?

The effective transform on the layer should already contain the scaling (ImageLayerComposite::ComputeEffectiveTransform does this), so I don't see why we need all this extra code to handle it.

Comment 17

[Sotaro Ikeda [:sotaro]]

When ScaleMode::STRETCH is used on ImageLayer, the following 2 have different scalings.
- Transform of the Layer's visible rectangle to display coordinate.
- Transform of buffer's coordinate to display coordinate.

ImageLayerComposite::ComputeEffectiveTransform is calculated based on Image size. But "Layer's visible rectangle" is calculated based on mScaleToSize size. Current HwcComposer2D's implementation transforms "Layer's visible rectangle" to display coordinate. Therefore we need another transform for it.

Is there another easy way to transform correctly "Layer's visible rectangle"?

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> When ScaleMode::STRETCH is used on ImageLayer, the following 2 have
> different scalings.
> - Transform of the Layer's visible rectangle to display coordinate.
> - Transform of buffer's coordinate to display coordinate.
> 

Before bug 1097441 change, they were same.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Ah, ok I understand now.

So, what ImageLayerComposite::ComputeEffectiveTransforms is doing is invalid. The effective transform on a layer converts from the local coordinate space (in which the visible region is specified) into that of the nearest ancestor with an intermediate surface.

The transform computed for mScaleToSize is a transform from buffer space into local layer space. Including this in the effective transform isn't correct, and is invalidating the visible region which is what you're seeing.

I think rather than having this change the effective transform, we instead just want to include this buffer transform in the value we pass to mImageHost->Composite. We'd also want to include the 'buffer to layer space' transform in the LayerRenderState we give to HWC.



(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Before bug 1097441 change, they were same.

It was equally broken before, we just didn't use ScaleMode for video.

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> (In reply to Sotaro Ikeda [:sotaro] from comment #18)
> > Before bug 1097441 change, they were same.

Bug 998019 is similar to bug 1097441. It is blocked by Bug 1093110.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> I think rather than having this change the effective transform, we instead
> just want to include this buffer transform in the value we pass to
> mImageHost->Composite. We'd also want to include the 'buffer to layer space'
> transform in the LayerRenderState we give to HWC.

Does it mean to revive layer's pre-scaling? It it is not, HWC continue to handle 2 transforms.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Yes, HWC will still have to handle two transforms, but I think it'll be a lot easier to understand.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle ScaleMode HwcComposer2D

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8529855 [details] [diff] [review]
patch - Handle ScaleMode HwcComposer2D

matt, I updated the patch. Can you feed back to the patch?

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8529855 [details] [diff] [review]
patch - Handle ScaleMode HwcComposer2D

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

::: gfx/layers/Layers.h
@@ +1387,5 @@
>     * such ancestor), but for BasicLayers it's different.
>     */
>    const gfx::Matrix4x4& GetEffectiveTransform() const { return mEffectiveTransform; }
>  
> +  virtual const gfx::Matrix4x4& GetEffectiveTransformForBuffer() const { return mEffectiveTransform; }

Add a comment about the difference between this and GetEffectiveTransform.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +150,5 @@
>    // transform.
>    bool Setup2DTransform()
>    {
>      // Will return an identity matrix for 3d transforms.
> +    if (mLayer->GetType() == Layer::TYPE_IMAGE) {

Can't we just always use GetEffectiveTransformForBuffer here, since it's virtual?

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Comment on attachment 8529855 [details] [diff] [review]
> patch - Handle ScaleMode HwcComposer2D
> 
> Review of attachment 8529855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.h
> @@ +1387,5 @@
> >     * such ancestor), but for BasicLayers it's different.
> >     */
> >    const gfx::Matrix4x4& GetEffectiveTransform() const { return mEffectiveTransform; }
> >  
> > +  virtual const gfx::Matrix4x4& GetEffectiveTransformForBuffer() const { return mEffectiveTransform; }
> 
> Add a comment about the difference between this and GetEffectiveTransform.

Update in a next patch.

> 
> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +150,5 @@
> >    // transform.
> >    bool Setup2DTransform()
> >    {
> >      // Will return an identity matrix for 3d transforms.
> > +    if (mLayer->GetType() == Layer::TYPE_IMAGE) {
> 
> Can't we just always use GetEffectiveTransformForBuffer here, since it's
> virtual?

Yes, I forgot about it. I will update in a next patch.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle ScaleMode HwcComposer2D

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change to layer

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change to Hwc

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change to layer

Comment 31

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=dffbcdef4dfa

Comment 32

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8530372 [details] [diff] [review]
patch - Change to layer

Current review+ is enough for check-in.

Comment 33

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77411a7f31ed

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77411a7f31ed

Comment 35

[sarsenyev]

Verified the issue is fixed on 2.2 Flame

The camera opens always in fullscreen mode, when toggle video/camera option

Flame 2.2

Device: Flame 2.2 Master (512mb)(Kitkat Base)(Full Flash)
BuildID: 20141209040203
Gaia: 9e0b96c7b61c7ff943876ca93e2596d972437b80
Gecko: acf5660d2048
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0