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

VERIFIED FIXED in Firefox OS v2.2

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: aryx, Assigned: sotaro)

Tracking

({regression})

unspecified
mozilla37
regression
Points:
---

Firefox Tracking Flags

(b2g-v2.1 unaffected, b2g-v2.2 verified)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

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.
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
See Also: → bug 1098660
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Updated

4 years ago
Duplicate of this bug: 1102682

Comment 4

4 years ago
QA can reproduce.  see dupe bug 1102682 for more details if needed.
blocking-b2g: --- → 2.2?

Updated

4 years ago
Keywords: regression, regressionwindow-wanted
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.
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → affected
QA Contact: pcheng
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: regressionwindow-wanted
Caused by Bug 1098660 - can you take a look Mike?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(mhabicher)
QA Contact: pcheng
Sotaro, we're seeing some weird clipping on Flame: http://youtu.be/nh61n8PdVJM
Flags: needinfo?(mhabicher) → needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 9

4 years ago
I confirmed the symptom on master flame. I am going to check what is happening.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 10

4 years ago
The problem seems to happen when HwComposer is used.
(Assignee)

Updated

4 years ago
Blocks: 1097441
(Assignee)

Comment 11

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

Comment 12

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

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

Updated

4 years ago
Component: Gaia::Camera → Graphics
Product: Firefox OS → Core
(Assignee)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
Duplicate of this bug: 1105298
(Assignee)

Comment 14

4 years ago
Created attachment 8529284 [details] [diff] [review]
patch - Handle ScaleMode HwcComposer2D

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

Comment 15

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

matt, can you give a feedback to the patch?
Attachment #8529284 - Flags: feedback?(matt.woodrow)
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.
(Assignee)

Comment 17

4 years ago
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"?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 18

4 years ago
(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.
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.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 20

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

Comment 21

4 years ago
(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.
Yes, HWC will still have to handle two transforms, but I think it'll be a lot easier to understand.
(Assignee)

Updated

4 years ago
Attachment #8529284 - Flags: feedback?(matt.woodrow)
(Assignee)

Comment 23

4 years ago
Created attachment 8529855 [details] [diff] [review]
patch - Handle ScaleMode HwcComposer2D
Attachment #8529284 - Attachment is obsolete: true
(Assignee)

Comment 24

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

matt, I updated the patch. Can you feed back to the patch?
Attachment #8529855 - Flags: feedback?(matt.woodrow)
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?
Attachment #8529855 - Flags: feedback?(matt.woodrow) → feedback+
(Assignee)

Comment 26

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

Comment 27

4 years ago
Created attachment 8530369 [details]
patch - Handle ScaleMode HwcComposer2D
(Assignee)

Comment 28

4 years ago
Created attachment 8530370 [details] [diff] [review]
patch - Change to layer
Attachment #8529855 - Attachment is obsolete: true
Attachment #8530369 - Attachment is obsolete: true
(Assignee)

Comment 29

4 years ago
Created attachment 8530371 [details] [diff] [review]
patch - Change to Hwc
(Assignee)

Comment 30

4 years ago
Created attachment 8530372 [details] [diff] [review]
patch - Change to layer
Attachment #8530370 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8530371 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #8530372 - Flags: review?(sushilchauhan)
Attachment #8530372 - Flags: review?(matt.woodrow)
Attachment #8530372 - Flags: review?(matt.woodrow) → review+
Attachment #8530371 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 32

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

Current review+ is enough for check-in.
Attachment #8530372 - Flags: review?(sushilchauhan)
https://hg.mozilla.org/mozilla-central/rev/77411a7f31ed
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Comment 35

4 years ago
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
status-b2g-v2.2: affected → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Updated

4 years ago
blocking-b2g: 2.2? → ---
You need to log in before you can comment on or make changes to this bug.