Closed Bug 1100110 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified

People

(Reporter: aryx, Assigned: sotaro)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

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
Closed: 5 years ago
Resolution: --- → WONTFIX
See Also: → 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 → ---
Duplicate of this bug: 1102682
QA can reproduce.  see dupe bug 1102682 for more details if needed.
blocking-b2g: --- → 2.2?
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.
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)
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)
I confirmed the symptom on master flame. I am going to check what is happening.
Flags: needinfo?(sotaro.ikeda.g)
The problem seems to happen when HwComposer is used.
Blocks: 1097441
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
 }
The above change broke the assumption of HwcUtils::PrepareLayerRects().

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcUtils.cpp#40
Component: Gaia::Camera → Graphics
Product: Firefox OS → Core
Assignee: nobody → sotaro.ikeda.g
Duplicate of this bug: 1105298
By applying the patch to master flame. The problem seems to be addressed.
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.
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)
(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)
(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.
(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.
Attachment #8529284 - Flags: feedback?(matt.woodrow)
Attachment #8529284 - Attachment is obsolete: true
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+
(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.
Attached file patch - Handle ScaleMode HwcComposer2D (obsolete) —
Attached patch patch - Change to layer (obsolete) — Splinter Review
Attachment #8529855 - Attachment is obsolete: true
Attachment #8530369 - Attachment is obsolete: true
Attachment #8530370 - Attachment is obsolete: true
Attachment #8530371 - Flags: review?(matt.woodrow)
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+
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
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
blocking-b2g: 2.2? → ---
You need to log in before you can comment on or make changes to this bug.