993019 - [Camera] [Gallery] orientation response is slow after launching from Camera app

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Tony Chung [:tchung]]

Possibly related to bug 988594.   but since this is in the gallery app, filing a separate bug for now.

The performance of redrawing the screen will crawl after trying to re-orient the gallery app.   Launching the gallery app from scratch is fine, but it's after navigating through the camera app and triggering the web activity to Gallery does the responsiveness crawl.

See screencast: http://youtu.be/cCqsFJKBVnA

Repro:
1) install 1.4 nightly build on Buri
Gaia      86de7fcce674ef6196d68e7e23552d219a3d72db
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/6e028297be14
BuildID   20140407000203
Version   30.0a2
ro.build.version.incremental=eng.tclxa.20131223.163538
2) launch camera, take a picture,  then launch gallery from the image you took
3) Rotate the device on the gallery app.  verify the redraw is slow to respond.

Comment 1

[David Flanagan [:djf]]

Tony: what happens if you launch gallery, return to homescreen, launch camera, take a picture, return to homescreen, and then go back into gallery from the homescreen.  Does the slowdown occur in that case? I'm trying to figure out whether it is related to the activity, or just related to having camera and gallery running at the same time.  Does rotation of other apps (like the browser) slow down when the camera is running in the background?  Does it matter whether you take a picture or not?

If this really is specific to activities, then I don't think we can do anything about it on the Media team. I'm not sure who could fix it but I usually ask Alive about things like this.

But if it is not related to activities, I wonder if we're doing something wrong in camera.  Camera does monitor orientation changes itself, manually, and maybe we're not turning that feature off when we go to the background.  If the camera app is busy trying to handle orientation changes in the background, that could slow down orientation changes in the foreground app, I suppose.  Wilson or Justin, are either of you familiar with that code and do you have time to investigate this? (If you're busy, this is probably something we could ask Jim to look into.)

Justin: you've reported seeing serious graphics slowdowns with the latest gecko.  Is that only on master, or is 1.4 also affected?  And does what Tony is reporting here seem related to what you've seen?

Comment 2

[Justin D'Arcangelo [:justindarc]]

David: The graphics performance issues I'm seeing is only with latest Gecko on master, but 1.4 seems to be ok. This issue here seems to be in Gallery from what I'm seeing in the video demo. Also, when I tried reproducing it, I was able to see the delay in rotating Gallery, but I then opened the task manager and killed the Camera app to see if there would be any effect from that and there seemed to be none.

Comment 3

[Wilson Page [:wilsonpage]]

djf: AFAIK we are not turning off the orientation listeners when the camera app loses focus. I am fairly familiar with that code. If it is the source of the problem, it should be simple enough to unbind these listeners.

Comment 4

[Tony Chung [:tchung]]

Attachment: logcat

More testing per comment 1.    I'll attach the logcat of what i see so far.

During the rotation, it seems this line appears the most:
04-07 16:29:43.069: E/memalloc(136): /dev/pmem: No more pmem available
04-07 16:29:43.069: W/memalloc(136): Falling back to ashmem

Did the following:
1) launch camera, take a pic
2) hop over to gallery, rotate device.  Frame redraw is slow like my first comment.
3) hit Home button.  (gallery and Camera process is still running)
4) launch Gallery again, and rotate device
5) the orientation responsivenness seems after reopening gallery (remember, gallery and camera proc is still running)

So not sure why reopening gallery app makes the orientation lag go away .   but it certainly happens 100% when doing steps 1-2 everytime without exiting.

Comment 5

[Tony Chung [:tchung]]

(In reply to Tony Chung [:tchung] from comment #4)
> Created attachment 8402871 [details]
> logcat
> 
> More testing per comment 1.    I'll attach the logcat of what i see so far.
> 
> During the rotation, it seems this line appears the most:
> 04-07 16:29:43.069: E/memalloc(136): /dev/pmem: No more pmem available
> 04-07 16:29:43.069: W/memalloc(136): Falling back to ashmem
> 
> Did the following:
> 1) launch camera, take a pic
> 2) hop over to gallery, rotate device.  Frame redraw is slow like my first
> comment.
> 3) hit Home button.  (gallery and Camera process is still running)
> 4) launch Gallery again, and rotate device
> 5) the orientation responsivenness seems *BETTER* after reopening gallery (remember,
> gallery and camera proc is still running)

sorry missing context in step 5 earlier.  added the *BETTER*  adjective describing it now

Comment 6

[David Flanagan [:djf]]

Tony, 

I don't understand the "hop over to gallery" part. Did you do that via activity or by going to homescreen and launching gallery?

Comment 7

[Tony Chung [:tchung]]

(In reply to David Flanagan [:djf] from comment #6)
> Tony, 
> 
> I don't understand the "hop over to gallery" part. Did you do that via
> activity or by going to homescreen and launching gallery?

sorry, "hop" means clicked the gallery activity button from camera preview.

Comment 8

[David Flanagan [:djf]]

(In reply to Wilson Page [:wilsonpage] from comment #3)
> djf: AFAIK we are not turning off the orientation listeners when the camera
> app loses focus. I am fairly familiar with that code. If it is the source of
> the problem, it should be simple enough to unbind these listeners.

I've investigated. We're not turning them off explicitly, but they stop firing automatically when the camera is in the background, including when we've launched an activity.  So that is not the issue.

Comment 9

[David Flanagan [:djf]]

What I see is that switching orientation in Gallery and Video apps now causes two redraws instead of one. First, the size of the screen changes and we get a redraw, then the screen rotates and we get another redraw. The resize event handler is only invoked once, however.

I've got a WIP version of Camera that unlocks the orientation for previews, and it has the same problem.

Also, I sometimes see this in browser.  If I just open the browser, and rotate, I it rotates smoothly.  But if I type "about:mozilla" into the browser URL bar, I get a failed google search (because I haven't turned on wifi). Then, if I rotate the phone, I get the ugly two-step rotation.

This looks like a system app bug to me.  Alive, could you take a look?

Comment 10

[David Flanagan [:djf]]

I forgot to mention that orientation changes in 1.4 seem smooth. This looks like a regression in 1.5.

Comment 11

[David Flanagan [:djf]]

I just noticed that Tony reported this bug for 1.4.  The symptom he is showing is exactly what I see in 1.5, but in 1.5 I don't have to use camera at all. If I just launch the gallery, I'll see the bad doubled-draw on orientation change right away with no camera and no activity required.

I'm using a nexus 4 for this.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Can we get a regression range on this?
I suspect we are resizing more apps than we need on a previous change.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Put into the queue.

Comment 14

[Jason Smith [:jsmith]]

Switching to QA Wanted to test on 1.3.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

What I saw from the browser app case:

E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.579]request RESIZE...active? ,true
E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.581] will resize...
E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.582]force RESIZE...
E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.586] publishing internal event: withoutkeyboard
E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.593] publishing internal event: resize
E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.595] publishing external event: resize
E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/app_window.js:753 in aw_debug: [Dump: AppWindow][Browser][11374.596]W:,480,H:,300

The browser app is resized only once by AppWindowManager. And then gecko rotate the screen.
That means:
1. We unlock screen orientation at certain timing.
2. Gecko fires 'resize' event on system app's window.
3. AppWindowManager tells current app to resize to Width: 480/Height: 320-20
4. The screen really rotates(by gecko).

I suspect this is a gecko issue: gecko fires resize event before it really rotates the screen.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

I am going to deassign myself since I don't think gaia is doing something wrong here.
If regression window investigation finds it's system app relevant then feel free to assign to me again.

(In reply to David Flanagan [:djf] from comment #9)
> What I see is that switching orientation in Gallery and Video apps now
> causes two redraws instead of one.

BTW, how do you know the redraw count is 1 before(before which commit?) and it's 2 now?

Comment 17

[Matthew Vaughan]

Attachment: Video of issue.

(In reply to Jason Smith [:jsmith] from comment #14)
> Switching to QA Wanted to test on 1.3.

This issue does *not* reproduce on the 04/12/14 1.3 build. I can reproduce this issue using the 1.4 and Master builds though.

Device: Buri v1.3 MOZ RIL
BuildID: 20140412164002
Gaia: d98c34ba0f36e3c48bd53e5a2fa5e8fb7021ca43
Gecko: 8d8cbf029f89
Version: 28.0
Firmware Version: v1.2-device.cfg


Just to be clear with what this bug is about, I have attached a video showing what I understand the issue to be. Please let me know if this bug is about another issue. The video shows a Buri using the 04/14/14 1.4 build.

Comment 18

[David Flanagan [:djf]]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #16)

> 
> BTW, how do you know the redraw count is 1 before(before which commit?) and
> it's 2 now?

Just visually. In the video (and in my own testing) I see two redraws.  Sometimes (like in the browser before loading a page) there is only 1 redraw.

Alive: if this is a gecko problem, do you know who might be able to work on it?  Or even what the proper component for the bug is?  I don't even know what part of gecko handles orientation.

Comment 19

[Matthew Vaughan]

This looks to be a gecko issue.

last working gaia/first broken gecko = REPRO
Gaia: 5de5cab90e1a83d81c0fc506912696d85b1865fe
Gecko: 45253e02bde3

first broken gaia/last working gecko = NO REPRO
Gaia: 5de5cab90e1a83d81c0fc506912696d85b1865fe
Gecko: 6175ca5a9da4


B2G INBOUND:
- Last Working -
Device: Buri ENG Master (1.4) MOZ RIL
BuildID: 20140307085255
Gaia: 5de5cab90e1a83d81c0fc506912696d85b1865fe
Gecko: 6175ca5a9da4
Version: 30.0a1
Firmware Version: v1.2-device.cfg

- First Broken -
Device: Buri ENG Master (1.4) MOZ RIL
BuildID: 20140307091726
Gaia: 5de5cab90e1a83d81c0fc506912696d85b1865fe
Gecko: 45253e02bde3
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Push log: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=6175ca5a9da4&tochange=45253e02bde3

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

comment 19 indicates this is a gecko issue.
I am c.c.ing Viral since he had worked on orientation part.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #20)
> comment 19 indicates this is a gecko issue.

Bug 977757, which is in the regression range enabled will-change for Gaia.  Until then, apps could specify will-change and nothing would happen.  One (obvious) possibility is that will-change is not properly implemented in Gecko.  However, the other is that will-change is not properly being used in Gaia. Both are explainable by the regression range, even though the changes are in Gecko only.

We'll take a look, but just a heads up.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Gallery is asking for a separate layer for each thumbnail, if I'm reading gallery.css correctly (and there are good chances I'm not, but I see will-change on #thumbnails), and that is probably not good. Asking David randomly, please forward NI to the right person?) if that is in fact what we're doing, and if so, if we have a good reason for it.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Probably not the above, I'm having trouble consistently reproducing this so it's difficult to say if it makes a difference.

Comment 24

[David Flanagan [:djf]]

Milan,

I don't know anything about will-change or its impact on layers. Someone added that in the last month or two to gallery, to improve scrolling performance, I thought.   The #thumbnails element is the scrolling container that holds all the thumbnails.  Is that not the right place to use will-change?

Note, though, that this bug is not limited to Gallery. I see it in Camera and Browser as well.

Comment 25

[Kai-Chih Hu [:khu]]

(In reply to Matthew Vaughan from comment #17)
> 
> Just to be clear with what this bug is about, I have attached a video
> showing what I understand the issue to be. Please let me know if this bug is
> about another issue. The video shows a Buri using the 04/14/14 1.4 build.

Thanks. The video helps! 

What I am curious is: is this bug a hard 1.4 blocker? 
It looks like a performance regression.

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I can see this issue(2 step change) on my helix device(gecko and gaia master branch). If this is related to will-change, I will investigate the usage in gallery and gecko implementation.

Comment 27

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: rotate.patch

I still debug for this issue, but we need to fix this part first.

The "mReadyForCompose" needs to init for every frame.
Without this, we need to wait the mForceCompositionTask to compose, even we finish all rotate task.
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#630

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8410901 [details] [diff] [review]
rotate.patch

Kats, could you review this part? We should land it now if it makes sense, while we're waiting for the rest of the work.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8410901 [details] [diff] [review]
rotate.patch

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

Good catch. This looks like an error in the refactoring in bug 861727.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks for the quick review.  Jerry, can you land this patch, let's make sure it makes it to Aurora, but leave this bug open until the full solution is in.

Comment 31

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: reset_compose_state.patch r=bugmail.mozilla

format patch for hg

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land attachment 8411773 [details] [diff] [review] for 1.4 and master.

This patch just fix one part of this issue. Still debugging.
See comment 30.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82bec52d6915

Friendly reminder that your commit message should be explaining what your patch is doing, not restating the problem it's fixing :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82bec52d6915

Comment 36

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: add rotation condition checking for cross-process

We should check the content[1] side rotation info as b2g[2] do.

[1]
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=compositorparent.cpp&case=true#1252
[2]
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=compositorparent.cpp&case=true#748

Comment 37

[u459114]

Comment on attachment 8412546 [details] [diff] [review]
add rotation condition checking for cross-process

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

::: gfx/layers/ipc/CompositorParent.h
@@ +129,5 @@
>    virtual void ScheduleComposition();
>    void NotifyShadowTreeTransaction(uint64_t aId, bool aIsFirstPaint, bool aScheduleComposite);
>  
>    /**
> +   * Check the rotation info and schedule the rendering task if needs.

Check rotation info and schedule a rendering task if needs

Comment 38

[u459114]

And good work, thanks

Comment 39

[u459114]

Hi Milan, do you know who is suitable to review this patch? compositorparent.cpp

Comment 40

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The attachment 8412546 [details] [diff] [review] just solves the "two steps rotation" problem(visual problem) in attachment 8406215 [details].
I think it's not related to the performance issue.

The attachment 8411773 [details] [diff] [review] might be related to performance.
Without that, we need to wait the mForceCompositionTask to compose, even all app is rotated.

Matthew, could you try both of these patches?

Comment 41

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Wait for attachment 8412546 [details] [diff] [review] try result:
https://tbpl.mozilla.org/?tree=Try&rev=1bc1ca3fdff0

Comment 42

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8411773 [details] [diff] [review]
reset_compose_state.patch r=bugmail.mozilla

Just tagging this patch as reviewed and checked in so that we don't lose track.

Comment 43

[Nicolas Silva [:nical]]

Comment on attachment 8412546 [details] [diff] [review]
add rotation condition checking for cross-process

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

Looks good to me

::: gfx/layers/ipc/CompositorParent.h
@@ +129,5 @@
>    virtual void ScheduleComposition();
>    void NotifyShadowTreeTransaction(uint64_t aId, bool aIsFirstPaint, bool aScheduleComposite);
>  
>    /**
> +   * Check the rotation info and schedule the rendering task if needs.

nit: "if needed"

Comment 44

[Matthew Vaughan]

(In reply to Jerry Shih[:jerry] from comment #40)
> The attachment 8412546 [details] [diff] [review] just solves the "two steps
> rotation" problem(visual problem) in attachment 8406215 [details].
> I think it's not related to the performance issue.
> 
> The attachment 8411773 [details] [diff] [review] might be related to
> performance.
> Without that, we need to wait the mForceCompositionTask to compose, even all
> app is rotated.
> 
> Matthew, could you try both of these patches?

Hello Jerry,

Since this looks to be a gecko issue (and therefore makes them gecko patches), we are unfortunately not set up to apply gecko patches here... only gaia patches. If you know how we can setup our machines to do this, or maybe and easy way to apply the patches, we welcome the info. 

If these patches are for gaia, could you please provide the github branch name for each patch?

Thank you!

Comment 45

[Jason Smith [:jsmith]]

These are gecko patches, so we can't help out here with pre-testing.

Comment 46

[No-Jun Park [:njpark]]

Applied the gecko patch to the tip of the master, and reflashed gecko to Buri device.  reset_compose_state.patch was already applied, so only needed to apply debug_rotate.patch.  Tested by following STR from Comment.

Before applying the patch, I saw what Tony saw from Comment 4, but after the patch, there is no odd hesitation behavior during the orientation change.  Orientation change still takes ~1 second, but it is clean now.

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: add rotation condition checking for cross-process v2, r=matt.woodrow,nical.bugzilla

update comment

Comment 48

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land attachment 8413533 [details] [diff] [review] 1.4 and master.

try result:
https://tbpl.mozilla.org/?tree=Try&rev=3ef763fd50bc

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1201268ce090

Comment 50

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Tony,
I think these patches fix the visual glitch in the video attachment 8406215 [details].
How about the performance problem?
I try the old gecko revision, but I don't get a significant performance difference.

Comment 51

[Tony Chung [:tchung]]

Thanks Jerry.  I trust no-jun's pretesting has helped confirmed the fix, and we will ultimately retest this when it gets uplifted to 1.4 today.

Tony

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in https://hg.mozilla.org/mozilla-central/rev/b681a6daea3b (along with backing out bug 996508) because it seemed like one or both of them made Android J3 tests frequently fail with the error from https://tbpl.mozilla.org/php/getParsedLog.php?id=38639894&tree=Mozilla-Inbound

Comment 53

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Wes Kocher (:KWierso) from comment #52)
> Backed this out in https://hg.mozilla.org/mozilla-central/rev/b681a6daea3b
> (along with backing out bug 996508) because it seemed like one or both of
> them made Android J3 tests frequently fail with the error from
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38639894&tree=Mozilla-
> Inbound

This is my try result for android j3 test
https://tbpl.mozilla.org/?tree=Try&rev=3ef763fd50bc

I don't see the same error in
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2fac94005f86

Comment 54

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Ryan,

Is this try result enough to prove?
https://tbpl.mozilla.org/?tree=Try&rev=3ef763fd50bc

Comment 55

[Ryan VanderMeulen [:RyanVM]]

LGTM

Comment 56

[u459114]

Bug 1003870 - Another bug that we filed to improve rotation performance base on what Jerry learn while fixing this bug.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3beda9264d51

Comment 58

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3beda9264d51

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/322e52597116
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/f032016ac1a9

Comment 60

[Preeti Raghunath(:Preeti)]

(In reply to Kevin Hu [:khu] from comment #25)
> (In reply to Matthew Vaughan from comment #17)
> > 
> > Just to be clear with what this bug is about, I have attached a video
> > showing what I understand the issue to be. Please let me know if this bug is
> > about another issue. The video shows a Buri using the 04/14/14 1.4 build.
> 
> Thanks. The video helps! 
> 
> What I am curious is: is this bug a hard 1.4 blocker? 
> It looks like a performance regression.

Yes Kevin.