826249 - [Gallery] Low performance during image exposure edit options, Image disappears after Edit options: Effect or Borders

Status: VERIFIED FIXED

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

Description

[Leo]

1. Title : Low performance during image exposure edit options, Image disappears after Edit options: Effect or Borders
2. Precondition : atleast one image has to be in SD card and need to be opened
3. Tester's Action : Gallery->Open Image->Edit Image->Change Exposure
4. Detailed Symptom (ENG.) : When ever user try to change exposure of an Image, it is taking time to apply effect,  Image disappears after Edit options: Effect or Borders
5. Detailed Symptom (KOR.) : 
6. Expected : changing exposure of an image should be seamlessly fast,  Image should not disappear after Edit options: Effect or Borders
7.Reproducibility: Y
1)Frequency Rate : 100%
8.Comparison Results : 
1)Model Comparing : 
9. Attached files: 
1)Log : 
2)Test Contents : 
3)Video file :

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch for this issue

This is a patch for the image disappearing when clicking an effect or a border. It also improves general speed of effects and exposure, but there is still work to be done there.

After quite some testing I tracked this down to the moment that the filter is applied (through WebGL, gl.texImage2D line). During this moment (which runs on the main thread, can't offload GL to a worker at the moment) the background images of the buttons disappear as well, even though there is no actual change on them. This is not only due to buttons or anything, but seem to happen on all background-images on the current view. Maybe a gecko flake or anything.

I worked around the above by writing the image information to a canvas object embedded within the button, as an  tag crashes B2G desktop there. This works great, the effects are also faster applied on the Unagi and no redraws are triggered. Also I now delay the rendering of these elements until the panel is actually used.

Comment 2

[Jon Hylands [:jhylands]]

janjongboom:

I've also worked on this, and just created a patch that deals specifically with the exposure slider performance. We should coordinate our efforts to ensure both patches can co-exist.

Comment 3

[Jon Hylands [:jhylands]]

Attachment: Patch for exposure slider performance

Comment 4

[David Flanagan [:djf]]

Comment on attachment 719591 [details]
Patch for exposure slider performance

Jon,

Really nice work for your first foray into this app and this kind of coding. I'm cancelling the review for now. See my comments on github for some more work to be done.

Comment 5

[David Flanagan [:djf]]

(In reply to janjongboom from comment #1)
> Created attachment 718983 [details] [diff] [review]
> Patch for this issue
> 
> This is a patch for the image disappearing when clicking an effect or a
> border. It also improves general speed of effects and exposure, but there is
> still work to be done there.

Your description of this patch on github helped me to understand what the bug is. It really is quite noticeable. Wow.

> 
> After quite some testing I tracked this down to the moment that the filter
> is applied (through WebGL, gl.texImage2D line). During this moment (which
> runs on the main thread, can't offload GL to a worker at the moment) the
> background images of the buttons disappear as well, even though there is no
> actual change on them. This is not only due to buttons or anything, but seem
> to happen on all background-images on the current view. Maybe a gecko flake
> or anything.

It does seem like a bug in gecko or in our webgl implementation. Would you be willing to open a separate bug about this because I'd like our graphics people to be aware of it instead of just working around it in gaia.

> I worked around the above by writing the image information to a canvas
> object embedded within the button, as an  tag crashes B2G desktop there.
> This works great, the effects are also faster applied on the Unagi and no
> redraws are triggered. Also I now delay the rendering of these elements
> until the panel is actually used.

I'd rather not resort to using canvas element there if we can avoid it. Jon's patch (work in progress) is going to make applying effects much, much faster. Would you test with his patch and see if it prevents or minimizes this bug?

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

@Jon: _O_ great stuff. The image flickering is also gone due to the improved speed even on the Unagi phone on large photo's. I'll add some comments to his PR.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 719591 [details]
Patch for exposure slider performance

Jon,

r- again, but this seems much closer. I think you're almost done. Most of my comments on github are nits. My only really substantive concern is about the way image memory is not being actively released.

I'd forgotten that edit mode never handled orientation changes correctly, so let's leave that alone here and handle it as a separate bug.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 719591 [details]
Patch for exposure slider performance

Minus again. Sorry!

I've tried running the code this time. There is something weird going on when cropping now.

I go into edit mode, tap Crop to enter crop mode. Then tap 3:2 to choose landscape cropping. Then tap Crop again to perform a crop. When I do that, the crop is done correctly, but the crop frame switches to 2:3 portrait mode and it goes outside the bounds of the newly cropped image. Similar things happen when I select other crop sizes.

I love the new speed of the exposure slider, though!

Comment 9

[David Flanagan [:djf]]

Comment on attachment 719591 [details]
Patch for exposure slider performance

Nice work, Jon!

Comment 10

[David Flanagan [:djf]]

Merged into master: https://github.com/mozilla-b2g/gaia/commit/a28105c34ae2b5f879e76894b8d69f5c2d1f78be

This is a really nice performance boost for editing images, so I'm nominating it for inclusion in Leo.

Comment 11

[[:fabrice] Fabrice Desré]

Please request approval for uplift, we definitely want that in b2g18!

Comment 12

[David Flanagan [:djf]]

Comment on attachment 719591 [details]
Patch for exposure slider performance

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression, just a long overdue update. The gallery editing features were implemented back in the days when we had tiny sample images before the camera was working, so I was able to edit the entire image on every slider adjustment in real time. Now that we've got big images, we needed to update the code to operate on previews instead.


User impact if declined: image editing will feel sluggish, particularly adjusting exposure.

Testing completed: yes, locally.

Risk to taking this patch (and alternatives if risky): its not very risky, but in the worst case, there might be some editing features that do not work right.

String or UUID changes made by this patch: none

Comment 13

[Alex Keybl [:akeybl]]

Adding qawanted to test master around image editing before uplifting these changes. If we don't find any regressions, please remove NO_UPLIFT from the whiteboard.

Comment 14

[David Flanagan [:djf]]

Bug 848625 is a tef+ bug that will be very hard to uplift without uplifting this first.  The risk of screwing up that merge is higher than the risk of uplifting this patch.

This has been on master for 2 weeks now.  Can I get approval to uplift it?

Setting needinfo on John... Has image editing been tested sufficiently with this patch per comments 13 and 14?

Comment 15

[David Flanagan [:djf]]

Nominating this for tef because bug 848625 is tef+ and the fix for that bug is built on top of the fix for this bug.

The two bugs are intertwined. They could be separated, but I think that the risk of trying to uplift bug 848625 without this bug is greater than the risk of uplifting this one.

See the previous comment: this patch has been on master for > 2weeks. It is a really nice performance fix and seems to be stable.

Comment 16

[Michael Vines [:m1] [:evilmachines]]

(tef+: increases risk of fixing a blocker, 2 week soak time, and it helps with perf)

Comment 17

[David Flanagan [:djf]]

Clearing the need info on jhammink and removing NO_UPLIFT in the whiteboard since this is now tef+.

jhford: after you uplift this, would you try to uplift 848625 again?

Comment 18

[amiller]

Does not make sense to create a regression issue.

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 719591 [details]
Patch for exposure slider performance

tef+, so this will be uplifted.

Comment 20

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Per comment 17, also removing qawanted request to test for uplift necessity. If it's still needed. please replace.

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

v1.0.1: 6c3237ab46b0c35fd7a058f849d44f4979044321

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

There are a lot of merge conflicts on v1-train.  These might be able to be resolved with 

  cd gaia
  git checkout v1-train
  git cherry-pick -x -m1 a28105c
  <resolve conflict>
  git commit

Comment 23

[David Flanagan [:djf]]

Merged into v1-train: https://github.com/mozilla-b2g/gaia/commit/22480d9f3bfe740604d99c9da7d7a94a83020e3e

Comment 24

[mdaniloff]

Verified Fixed on Unagi:
editing image (exposure) is fast with no delay and no image deletion occurs
Unagi Build ID: 20130328070202
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c516d7e67150
Gaia: d40dcdd112f12e2a5a0d1de46451670918fd4369

Comment 25

[Shally Li（leave from Mozilla）]

Per comment 24,I clear "Verifyme".

Comment 26

[Shally Li（leave from Mozilla）]

If someone feels convenient,please verify on "b2g18".