1158396 - [Gallery] Gesturing through 'large view' images may flash other images across the screen in the opposite direction

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20150424_1515.txt

Description:
When the user is gesturing through their gallery in 'large view' of images, they may observe images flash across the screen in the other direction. 

[speculation]
This may be caused by a buffer of the current screen being exceeded, flushing the UI with an image that had slid past the screen earlier, or setting up the UI for future gestures by processing the images to display from other side of the 'active' image.
Below demonstration observes at least 2 instances of this occurring, and it may be when the gallery has to shift 'rows' of gallery images, as 'E' (as observed in the gallery in the first few seconds of the video' is positioned on the far right of the 3rd row

video demonstration:
0:06 - 0:07 || between images E -> C
0:34 - 0:37 || between images E -> C

PreReq:
* handful of images in gallery (tested with 17)
Repro Steps:
1) Update a Flame to 20150423010203
2) Open the 'Gallery' app
3) Tap the first image (top-left)
4) Slide from right to left on the image to gesture-slide to the next image
5) Repeat step 4 through to end
6) Repeat step 4/5 in reverse from bottom to top

Actual:
Non-active images will flash across screen when gesturing through gallery

Expected:
Only actives will appear on screen; no sudden flashes


Environmental Variables:
Device: Flame 3.0
Build ID: 20150423010203
Gaia: 9d4f756aa35cb7f030a92f3c1f65fb55254ddd1d
Gecko: 0b202671c9e2
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Device: Flame 2.2
BuildID: 20150424002507
Gaia: b838d0e7c163e66660dcb6e387d8339944a7a30e
Gecko: 5fe76b26e55f
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
----------------------------------------------
Issue reproes, but appears to have lower repro/visibility on 2.1 and 2.0 devices

Device: Flame 2.1
BuildID: 20150424001201
Gaia: bbe983b4e8bebfec26b3726b79568a22d667223c
Gecko: 6a68a038146a
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 34.0 (2.1) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0
BuildID: 20150424000201
Gaia: 84898cadf28b1a1fcd03b726cff658de470282f0
Gecko: ff96615f8cfc
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 32.0 (2.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
----------------------------------------------

Repro frequency: 3/10 (need couple gestures to execute, seems after a 'row' [3rd image])
See attached: 
video- https://youtu.be/XBtCCdla1Lk
logcat

Comment 1

[Peter Bylenga [:PBylenga]]

NI on component owner for nomination decision and assignment.

Comment 2

[No-Jun Park [:njpark]]

This sounds like memory related, pinging :djf, wondering whether it is a graphics issue or gallery app isssue.

Comment 3

[David Flanagan [:djf]]

No-Jun,

If this goes back to 2.0, it is almost certainly an issue in the Gallery app. There have been lots of gecko changes since then. If this is happening consistently, then I'd bet it is my fault.

It looks like Oliver was swiping quite quickly, and I guess there is a bug where swipes that happen while an image is still being animated off screen get the app messed up. I'd guess that this like those bugs that happen when people tap on buttons more quickly than the app can respond to the tap and we get weird behavior.

I'll take this. If I can fix it quickly and the fix is small, I will request uplift to 2.2. Swiping quickly might be something that users do commonly to find an image they're looking for, it would be good to make it work.

No-Jun: I'll let you decide whether to nominate it as 2.2?

Oliver: I love your alphabetic test images. Are you able and willing to share those? If so, please attach to the bug or email them to me.

Comment 4

[No-Jun Park [:njpark]]

Hmmm, I don't think it's a blocker since it has been around for so long (and considering the cutoff date is so close), but it certainly belongs to the category of "it would be really great if we can fix it".

Comment 5

[Oliver Nelson [:oliverthor]]

Attachment: gallery_flicker_pictures.zip

Of course, I honestly thought I had attached them initially. If you have any issue, ni? me again and I can email them to you just as well.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1158396 > mozilla-b2g:master

Comment 7

[David Flanagan [:djf]]

Attachment: possible fix for the bug

Punam,

I can't reproduce the bug, so I'm not sure if this patch fixes it. You might want to wait for confirmation from QA before reviewing. But if this does fix it, I would love to get the fix uplifted, so please review quickly, if you can.

This patch addresses two issues:

1) We were not ignoring swipe events from the gesture detector while transitioning from one image to another. 

2) We were using setTimeout() to clear the transitioning flag but using a transitionend event to clear the transition property on the frames. So if the setTimeout fired before the transitionend event arrived, there was a window in which we could start another transition before the animation styles for the previous transition had been cleared.  If nextFile() was getting called twice in a row and the second call was during this brief window, then the second call would be moving the previousFrame to the other side of the screen to make it the nextFrame. This is not supposed to be animated, but if the styles from the first call had not yet been cleared (by the transitionend event from the first call) then that image would appear to fly across the screen in the wrong direction, which is what seems to be shown in the video.

There was some relatively recent change (I forget the bug number) to CSS animation timing that I've seen blamed for a number of recent regressions, so perhaps it has also made this bug easier to reproduce. That would be consistent with QAs finding that this is harder to reproduce in 2.0 and 2.1.

Comment 8

[David Flanagan [:djf]]

Oliver,

I was never able to reproduce the bug, but I'm hoping that the attached patch fixes it. Could you check the patch? If it solves the problem I'd like to ask for uplift approval before Wednesday's deadline.

Comment 9

[Oliver Nelson [:oliverthor]]

Attachment: logcat_PR_29749.txt

David,

So I thought the patch was clean at first, I have not been able to reproduce this issue after applying your pull request. However, it seems to have introduced an initial design appearance to that invariably displays itself as a draw bug:

** After pull request ** 
Transitioning between images quickly may leave some images undrawn when passing through or when landing on for 1-5 seconds at a time, or they may not draw until user interaction.
* https://youtu.be/JsVqiYaP7JM

I have not seen the flashing of this bug (1158396) which is great, but I wanted to be sure you're aware of this new behavior. Uploaded video and attached logcat of the issue.

Comment 10

[David Flanagan [:djf]]

Oliver,

Yes, I saw that too, but it occurs with today's nightly build even without my patch, so I think it is unrelated. 

It didn't happen for me when my phone was configured with 1024mb of memory, and only started happening when I went down to 319mb.

If you see it on on 2.2, please file a new bug, needinfo me, and nominate it as a blocker. But there have been a number of big gecko image decoding changes on master and the gallery app has not yet been updated to accomodate those changes, so this kind of instability on master doesn't worry me too much.

Comment 11

[David Flanagan [:djf]]

Punam: from comment #9, it sounds like the patch is good, so please review.

Comment 12

[Punam Dahiya [:pdahiya]]

Comment on attachment 8598191 [details] [review]
possible fix for the bug

Thanks David for the patch, it looks good and has r+.

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8598187 [details] [review]
[gaia] davidflanagan:bug1158396 > mozilla-b2g:master

I requested review on the attachment I added manually instead of auto-lander's version of it, so I'm carrying Punam's r+ over to this version so I can get it auto landed.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8598187 [details] [review]
[gaia] davidflanagan:bug1158396 > mozilla-b2g:master

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #): not a regression; a long-standing race condition

[User impact] if declined: rapid swiping through a series of photos may sometimes show an image being aninated in the wrong direction. Not a really severe bug, but I'm nominating it for uplift just because I expect 2.2 to be a long-lived branch.

[Testing completed]: yes, QA has verified the patch

[Risk to taking this patch] (and alternatives if risky): The patch may reduce the maximum speed at which the user can swipe through a long set of photos. Because this patch changes some timing details there is some risk of introducing unexpected behavior. I expect that any regressions will be pretty obvious, however, and the patch is easy to back out.

[String changes made]: none

Comment 15

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/8ea6a31ca2da46063728f5686fc1c7791a3dc162

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 8598187 [details] [review]
[gaia] davidflanagan:bug1158396 > mozilla-b2g:master

Approving this considering the fact that the fallouts will be easily observed. If we end up identifying blocking fallouts, I'd rather live with this being backed-out on 2.2. For now, approving. Please let QA know different scenarios to test other than our regular smoketests so this code path is extensively tested.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/dbf31baf93bcbfb9ad29fc9ffe6e8aea3afb111f

Comment 18

[Oliver Nelson [:oliverthor]]

This issue appears to still be occurring on master/3.0 for flame devices, but is fixed on 2.2; verifying for that branch. This was tested for both 1024mb and 319mb memory against both branches.
---------------------

Issue REPRODUCES on 3.0 for flame devices
Results: Swift swiping through gallery may result in images flashing across screen in opposite direction.

Environmental Variables:
Device: Flame 3.0 [1024mb & 319mb]
Build ID: 20150518010206
Gaia: afea16de7a76c3b6d15c35fb4c37bac71c8ddc6a
Gecko: 35918b0441b4
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0
******************************

Issue DOES NOT REPRO on 2.2 for flame devices
Results: Swift swiping through images in gallery will only display the present image to be viewed.

Device: Flame 2.2
BuildID: 20150517162505 [1024mb & 319mb]
Gaia: f73891b8fcc5f34de81868640754f7cc331fa709
Gecko: 8785a53b8d6e
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 19

[David Flanagan [:djf]]

That's very weird. The patch is on both master and 2.2. I'm not sure why it would fix one and not the other. Leaving the needinfo set so I remember to come back to this.

Comment 20

[Lance（leave from Mozilla）]

Attachment: Verified video: Flame KK master.3GP

This bug has been verified as "Pass" on the latest build of Flame KK master&v2.5 by the STR in comment 0.
Actual result: 
Only actives will appear on screen，no sudden flashes.
Occurrence rate: 0/10.

Device: Flame KK master （1024mb&319mb） (Pass)
Build ID               20151221150211
Gaia Revision          14aefb2519becfa32f31bcc3c9c995693421f19c
Gaia Date              2015-12-21 06:34:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/8c9825377d0d8115e9fed64fd3700b1e54700dbe
Gecko Version          46.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151221.182728
Firmware Date          Mon Dec 21 18:27:42 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK v2.5 （1024mb&319mb） (Pass)
Build ID               20151218180935
Gaia Revision          eeed1451e0e48b63abe3199e4d6906adc2a762d2
Gaia Date              2015-12-17 14:53:36
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/766600655dfb1b67c380b9da4f4b19acb505a858
Gecko Version          44.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151218.172320
Firmware Date          Fri Dec 18 17:23:30 UTC 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK master (Pass)
Build ID               20151221204910
Gaia Revision          14aefb2519becfa32f31bcc3c9c995693421f19c
Gaia Date              2015-12-21 06:34:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/8c9825377d0d8115e9fed64fd3700b1e54700dbe
Gecko Version          46.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151221.200242
Firmware Date          Mon Dec 21 20:02:51 UTC 2015
Bootloader             s1

Device: Aries KK v2.5 (Pass)
Build ID               20151222010746
Gaia Revision          42aa9b91a231572138509bbd942d918d0293110a
Gaia Date              2015-12-21 22:48:19
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9c525eef094b710e8ea6d00613200282acf9b682
Gecko Version          44.0
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151222.001825
Firmware Date          Tue Dec 22 00:18:33 UTC 2015
Bootloader             s1