998019 - [Camera][Madai] Glitches in preview when switching from picture to video mode

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Diego Marcos [:dmarcos]]

The preview is resized when switching from video to picture and the transition is not smooth. It's more noticeable on front camera (see attached video)

Comment 1

[Diego Marcos [:dmarcos]]

Attachment: videoPictureSwitch.MOV

Comment 2

[David Flanagan [:djf]]

It would be nice to fix this before shipping. If there's a simple patch, I'll set the approval flag for 1.4 uplift.

We already have some kind of CSS transition to hide these mode switching glitches, right? Does it just need to be tweaked to handle this case?

Diego: whoever worked on the existing transition code is probably the right person to take this bug as well.

Comment 3

[Diego Marcos [:dmarcos]]

Wilson is taking this

Comment 4

[Wilson Page [:wilsonpage]]

Attachment: video

I tried disabling all other DOM work around the point where the <video> is resized, but the glitch still persists. It seems the browser is doing some painting async which means that we see a few frames painted of the unsized video after it has been resized.

Comment 5

[Wilson Page [:wilsonpage]]

I think I finally understand the cause of this bug. It is a result of a delay on the Gecko side when switching streams. Currently this is the sequence of events:

1. Fade out the viewfinder
2. Re-configure the camera hardware
3. Wait for the preview 'started' state
4. Fade the viewfinder back in
5. Observe the stream change (glitch)

EXPECTED

Gecko should only announce the stream has 'started' once the new stream has started flowing.

I believe this is a regression since 1.4 on the Gecko side, as we used to be able to safely rely on this event in 1.3.

Comment 6

[Wilson Page [:wilsonpage]]

Attachment: simplified-test-branch

Comment 7

[Wilson Page [:wilsonpage]]

That attached branch removes a lot of noise to make the problem more apparent and clearer to debug.

Comment 8

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

I just flashed my Madai with:
- gecko: b2g_autogen_ephemeral_branch:851c53529d1d651b3a277930236e3ebc62b63ad8
- gaia: master:bbc76bcdc937bdbaf3935351ba2d532449c63bf8

...and I don't see any viewfinder glitching on either camera. The transition is clean.

Comment 9

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

Wilson, you're seeing this on Madai, right?

Comment 10

[Diego Marcos [:dmarcos]]

Madai? Are you talking about QRD?

Comment 11

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

(In reply to Diego Marcos [:dmarcos] from comment #10)
>
> Madai? Are you talking about QRD?

Sorry, yes--I keep conflating them.

Comment 12

[Diego Marcos [:dmarcos]]

I don't have one. Wilson does but he hasn't been able to flash it. If you can help him he can test this out.

Comment 13

[Wilson Page [:wilsonpage]]

mike: You won't see the glitch on master as we are delaying the fade in for 300ms, in the hope that the stream will have changed by then. In some cases (like Diego's) this isn't long enough. If you flash the Gaia branch (attachment 8411163 [details]) that amended to make the underlying issue clearer.

Comment 14

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

Attachment: (Mildly annotated) logcat showing several cycles

Comment 15

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

The following change in controllers/viewfinder.js can make it easier to see the glitches (at the expense of the viewfinder being shrunk and improperly rotated):

  ViewfinderController.prototype.configurePreview = function() {
    var camera = this.app.settings.cameras.selected('key');
    var isFrontCamera = camera === 'front';
    var sensorAngle = this.camera.getSensorAngle();
    var previewSize = this.camera.previewSize();

-   this.viewfinder.updatePreview(previewSize, sensorAngle, isFrontCamera);
  };

Comment 16

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

Sotaro, can you take a quick look at this one? There's a visual glitch where it looks like it takes the <video> tag a few frames to recognize a change in frame size when switching from picture mode (640x480 frames) to video mode (1280x720 frames).

From the logcat, it looks like the camera is responding properly to the size change request, so I'm thinking there might be something in the upper layers of Gecko with a delayed update.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Add some logs

I did debugging by applying the patch and by enabling "Dump layers tree".

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: logout when the problem happens

Get log by applying attachment 8412324 [details] [diff] [review] and enabling enabling "Dump layers tree".

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Created attachment 8412325 [details]
> logout when the problem happens
> 
> Get log by applying attachment 8412324 [details] [diff] [review] and
> enabling enabling "Dump layers tree".

I got the log by using master nexus-4.

Comment 20

[Sotaro Ikeda [:sotaro]]

From attachment 8412325 [details], the following seems to happen. At [7], one preview frame is rendered by inconsistent state.

-[1] ImageHost renders front camera camera preview (w=640, h=640).
     ImageLayerComposite's visible is (x=0, y=0, w=640, h=480).
-[2] camera app chane the preview to front camera recording mode.
-[3] camera preview size is changed to (w=1280, h=720) 
-[4] VideoFrameContainer::SetCurrentFrame() is called by new video frame (w=1280, h=720) 
-[5] VideoFrameContainer trigger reflow
-[6] ImageClient send the new video frame to ImageHost via ImageBridge
-[7] ImageHost renders front camera camera preview (w=1280, h=720) .
     ImageLayerComposite's visible is still (x=0, y=0, w=640, h=480). ************* old data
-[8] reflow on camera app
-[9] Update new layer data to ImageLayerComposite
-[10]ImageHost renders front camera camera preview (w=1280, h=720) .
     ImageLayerComposite's visible is (x=86, y=0, w=1108, h=720). ************* updated data

Comment 21

[Sotaro Ikeda [:sotaro]]

Layer data is updated by reflow on main thread and then forwarded to Host side via ClientLayerManager. Camera preview frame is forwarded to Host side via ImageBridge on ImageBridge thread. This difference made the inconsistent state between ImageHost and ImageLayerComposite for a short time.

Comment 22

[Sotaro Ikeda [:sotaro]]

Changed to a correct component.

Comment 24

[Wilson Page [:wilsonpage]]

Any news on this? Why is this being unassigned?

Comment 25

[Sotaro Ikeda [:sotaro]]

Sorry, I was busy for other bugs. I could have a time for this bug since this week.

Comment 26

[Sotaro Ikeda [:sotaro]]

Sorry, changed by mistake.

Comment 30

[Josh Cheng [:josh]]

The same problem happen on Woodduck and reported by partner.

Hi Hubert,
qawanted for branch check on Flame FFOS 2.0. Thanks!

Comment 31

[Hubert Lu[:hlu] <[e-mail]>]

Hi Josh,
 On flame v2.0, I could reproduce this issue symptom "The preview is resized when switching from video to picture and the transition is not smooth."

== Build Information - Flame ==
Gaia-Rev        9791940d5093c37336dcd85f7e66e9a8445a218e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/00d3c843aeec
Build-ID        20141010000201
Version         32.0
Device-Name     flame-kk
FW-Release      4.4.2
FW-Incremental  34
FW-Date         Tue Sep 30 14:06:36 CST 2014
Bootloader      L1TC00011840

Comment 33

[Sotaro Ikeda [:sotaro]]

I am going to implement a patch from tomorrow.

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add Image size generation handling

Comment 35

[Sotaro Ikeda [:sotaro]]

(In reply to Hubert Lu[:hlu] <[e-mail]> from comment #31)
> Hi Josh,
>  On flame v2.0, I could reproduce this issue symptom "The preview is resized
> when switching from video to picture and the transition is not smooth."

From the above description, I am not sure it is taking about this bug. This bug is not about transition smoothness.

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add Image size generation handling

Comment 37

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Add some logs

Comment 38

[Sotaro Ikeda [:sotaro]]

This bug is about inconsistency between video image size and layer setting. It happen because video image is delivered via ImageBridge(by different thread).

Comment 39

[Sotaro Ikeda [:sotaro]]

In current mozCamera implementation, same preview size is used when camera hal supports separate preview and video sizes.

Comment 40

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #39)
> In current mozCamera implementation, same preview size is used when camera
> hal supports separate preview and video sizes.

It is implemented by Bug 1014877. 

On nexus-4, camera preview size is always 1280*720.
On flame-kk, camera preview size is always 864*480.

Comment 41

[Sotaro Ikeda [:sotaro]]

From Bug 1014877 comment 59, wookstock also become to use same camera preview size.

Comment 42

[Sotaro Ikeda [:sotaro]]

josh, from Bug 1014877 comment 59 is there a still reason to block 1054172 and 1080481?

Comment 43

[Sotaro Ikeda [:sotaro]]

(In reply to Hubert Lu[:hlu] <[e-mail]> from comment #31)
> Hi Josh,
>  On flame v2.0, I could reproduce this issue symptom "The preview is resized
> when switching from video to picture and the transition is not smooth."
> 
> == Build Information - Flame ==
> Gaia-Rev        9791940d5093c37336dcd85f7e66e9a8445a218e
> Gecko-Rev      
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/00d3c843aeec
> Build-ID        20141010000201
> Version         32.0
> Device-Name     flame-kk
> FW-Release      4.4.2
> FW-Incremental  34
> FW-Date         Tue Sep 30 14:06:36 CST 2014
> Bootloader      L1TC00011840

This problem seems to related to Bug 1014877.

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Add some logs

Comment 45

[Sotaro Ikeda [:sotaro]]

By disabling Bug 1014877, the glitch can be easily confirmed.

Comment 46

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Image size generation handling

Comment 47

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2gv2.1 - Add Image size generation handling

Comment 48

[Josh Cheng [:josh]]

(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> josh, from Bug 1014877 comment 59 is there a still reason to block 1054172
> and 1080481?

Hi Sotaro,
This bug is initially reported by partner in bug 1070799 with title "Preview shake when switch betwen camera and camcorder" which I think is same problem with bug. You can check the video attachment in bug 1070799. We thought bug 1014877 can fix this issue but there is still slightly shake when switching between picture and video mode per https://bugzilla.mozilla.org/show_bug.cgi?id=1070799#c9 and https://bugzilla.mozilla.org/show_bug.cgi?id=1014877#c59. Do you mean patch for this bug will also not fix this "slightly shake" problem?
Thanks!

Comment 49

[Sotaro Ikeda [:sotaro]]

I do not think this bug fix "slightly shake" problem. If camera preview size is always same size, this bug have no effect to it.

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Image size generation handling

Update some comments.

Comment 51

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Image size generation handling

Remove unnecessary log.

Comment 52

[Nicolas Silva [:nical]]

Comment on attachment 8508130 [details] [diff] [review]
patch - Add Image size generation handling

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

Sorry for the review lag. I would prefer something a bit more generic like the notion of a "SyncPoint" with a sync id, instead of making it specific to size (I assume will eventually bump into other properties of ImageLayers that need to be synchronized with main thread transactions), but I think that the implementation would be the same (or almost), just with different names. So we'll consider this if the needs come.

Comment 53

[Sotaro Ikeda [:sotaro]]

Thanks for the review:-) I am going to update to more generic name.

Comment 54

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Image size generation handling

Apply the comment. Carry "r=nical".

Comment 55

[Sotaro Ikeda [:sotaro]]

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

reftests failed :-(

Comment 56

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Image size generation handling

Carry "r=nical". Update how to set ImageLayer atributte.

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add Image size generation handling

Limit only to async ImageContainer. Carry "r=nical".

Comment 58

[Sotaro Ikeda [:sotaro]]

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

Comment 59

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #58)
> https://tbpl.mozilla.org/?tree=Try&rev=b1130f16ed5c

Some tryserver failures exist around "layout + video" tests.

Comment 60

[Sotaro Ikeda [:sotaro]]

Set to "2.0M?" based on comment 49.

Comment 61

[Josh Cheng [:josh]]

Triage:
Minor user impact and not happen after 2.1. Not fix in 2.0M.

Comment 62

[Sotaro Ikeda [:sotaro]]

Bug 1097441 fixed a part of the problem.

Comment 63

[Josh Cheng [:josh]]

Woodduck shipped and issue not happen on 2.2. Close as wont fix.