Closed Bug 993019 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

30 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.3 --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: tchung, Assigned: jerry)

References

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

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.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(dflanagan)
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?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(tchung)
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(dflanagan)
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.
Flags: needinfo?(jdarcangelo)
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.
Flags: needinfo?(wilsonpage)
Attached file 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.
Flags: needinfo?(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
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?
Flags: needinfo?(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.
Flags: needinfo?(tchung)
(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.
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?
Component: Gaia::Camera → Gaia::System::Window Mgmt
Flags: needinfo?(alive)
I forgot to mention that orientation changes in 1.4 seem smooth. This looks like a regression in 1.5.
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.
Can we get a regression range on this?
I suspect we are resizing more apps than we need on a previous change.
Put into the queue.
Assignee: nobody → alive
Flags: needinfo?(alive)
Switching to QA Wanted to test on 1.3.
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.
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?
Assignee: alive → nobody
Flags: needinfo?(dflanagan)
QA Contact: mvaughan
Attached video 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.
(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.
Flags: needinfo?(dflanagan) → needinfo?(alive)
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 19 indicates this is a gecko issue.
I am c.c.ing Viral since he had worked on orientation part.
Component: Gaia::System::Window Mgmt → General
Flags: needinfo?(alive)
Blocks: 977757
Component: General → Graphics: Layers
Product: Firefox OS → Core
Version: unspecified → 30 Branch
(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.
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.
Flags: needinfo?(dflanagan)
Probably not the above, I'm having trouble consistently reproducing this so it's difficult to say if it makes a difference.
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.
Flags: needinfo?(dflanagan)
(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.
Flags: needinfo?(praghunath)
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.
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Attached patch rotate.patch (obsolete) — Splinter Review
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 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.
Attachment #8410901 - Flags: review?(bugmail.mozilla)
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.
Attachment #8410901 - Flags: review?(bugmail.mozilla) → review+
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.
format patch for hg
Attachment #8410901 - Attachment is obsolete: true
Whiteboard: [leave open]
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.
Keywords: checkin-needed
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
Keywords: checkin-needed
Duplicate of this bug: 1000916
Attachment #8412546 - Flags: feedback?(cku)
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
Attachment #8412546 - Flags: feedback?(cku) → feedback+
And good work, thanks
Hi Milan, do you know who is suitable to review this patch? compositorparent.cpp
Flags: needinfo?(milan)
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?
Flags: needinfo?(mvaughan)
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.
Attachment #8411773 - Flags: review+
Attachment #8411773 - Flags: checkin+
Attachment #8412546 - Flags: review?(nical.bugzilla)
Attachment #8412546 - Flags: review?(matt.woodrow)
Flags: needinfo?(milan)
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"
Attachment #8412546 - Flags: review?(nical.bugzilla) → review+
(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!
Flags: needinfo?(mvaughan) → needinfo?(hshih)
These are gecko patches, so we can't help out here with pre-testing.
Flags: needinfo?(hshih)
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.
Attachment #8412546 - Flags: review?(matt.woodrow) → review+
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.
Flags: needinfo?(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
Flags: needinfo?(tchung)
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
Flags: needinfo?(hshih)
(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
Flags: needinfo?(hshih) → needinfo?(kwierso)
Hi Ryan,

Is this try result enough to prove?
https://tbpl.mozilla.org/?tree=Try&rev=3ef763fd50bc
Flags: needinfo?(ryanvm)
LGTM
Flags: needinfo?(kwierso)
Keywords: checkin-needed
Flags: needinfo?(ryanvm)
Bug 1003870 - Another bug that we filed to improve rotation performance base on what Jerry learn while fixing this bug.
https://hg.mozilla.org/mozilla-central/rev/3beda9264d51
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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.
Flags: needinfo?(praghunath)
You need to log in before you can comment on or make changes to this bug.