749568 - Scrolling is not smooth on mobile pages

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Fennec/15.0a1 2012-04-26 / Aurora/14.0a2 tinderbox build 8fb4d0f15a18
Device: HTC Desire (Android 2.2)

Steps to reproduce:
1. Open Nightly.
2. Go to a mobile page. e.g. msnbc.com or espn.com
3. Wait for the page to load.
4. Flick scroll the page in both landscape and portrait.

Expected results:
Scrolling is smooth.

Actual results:
Scrolling is choppy, jumping from one position to another instead of having a smooth transition through the content.

Please see the video: http://youtu.be/a9O-QaVEUdg

Comment 1

[Tony Chung [:tchung]]

qawanted: MozQA or SV, can you guys try a few other phones (preferably older ones) and report back?   triage would like to know if its specific to the HTC Desire device.

Comment 2

[Adrian Tamas (:AdrianT)]

Issue _is_ reproducible on Nightly/15.0a1 2012-04-29 on HTC Desire (Android 2.2), HTC Desire Z (Android 2.3). 
Issue _is not_ reproducible on LG Optimus 2X (Android 2.2), Samsung Galaxy Nexus (Android 4.0.2), Samsung Captivate (Android 2.2).

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Using my G2 I see slowness on msnbc.com seeing a couple 100+ and 200+ ms paint times. I don't see jank in espn.com most paint times are below 75ms. MSNBC is a heavy page with some elements that are hidden by the show more stories drop downs.

Comment 4

[Tony Chung [:tchung]]

the jank happens for me on mobile twitter when scrolling up and down my wall.

HTC Sensation, 2.3.4, Adreno220, 4-30-2012 latest-aurora build

Comment 5

[Johnathan Nightingale [:johnath]]

I see this on a galaxy nexus with the linkedin mobile site as well (albeit not as bad).

Discussed in triage today, and JP suggests assigning it to BenWa to profile so that we understand what's happening, and that we leave it on the noms list pending that information. Done and done.

Comment 6

[Benoit Girard (:BenWa)]

This isn't easy to profile without me spending a some non trivial amount of time improving our profiler, something I plan to do in Q3 (or over some weekend).

I do know with confidence that this is a case of upload jank and needs some form of progressively upload to solve.

I will discuss with JP today if we're willing to uplift some form of progressive upload to aurora.

Comment 7

[Benoit Girard (:BenWa)]

The HTC Desire performs really poorly when uploading a large number of tiles. Here a log with time in ms:

kI/Gecko   ( 3044): Time to upload 34
I/Gecko   ( 3044): Time to upload 144
I/Gecko   ( 3044): Time to upload 110
I/Gecko   ( 3044): Time to upload 146
I/Gecko   ( 3044): Time to upload 72
I/Gecko   ( 3044): Time to upload 36
I/Gecko   ( 3044): Time to upload 100
I/Gecko   ( 3044): Time to upload 39
I/Gecko   ( 3044): Time to upload 35
I/Gecko   ( 3044): Time to upload 232
I/Gecko   ( 3044): Time to upload 34
I/Gecko   ( 3044): Time to upload 93
I/Gecko   ( 3044): Time to upload 228
I/Gecko   ( 3044): Time to upload 80

Comment 8

[JP Rosevear [:jpr]]

Jeff is going to dig into this a bit more specifically.

Comment 9

[Aaron Train [:aaronmt]]

I also see this on the Galaxy Note (Android 2.3) browsing a site such as http://i.reddit.com

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Aaron Train [:aaronmt] from comment #9)
> I also see this on the Galaxy Note (Android 2.3) browsing a site such as
> http://i.reddit.com

It would be good to know if you find the scrolling worse on mobile site then regular sites. We did some quick comparisons and it didn't seem noticeably different.

Comment 11

[Damon Sicore (:damons)]

Can we get a reduced testcase from QA?  i.e., not something we have to chase on reddit.com, etc.

Comment 12

[Tony Chung [:tchung]]

(In reply to Damon Sicore (:damons) from comment #11)
> Can we get a reduced testcase from QA?  i.e., not something we have to chase
> on reddit.com, etc.

Martijn or Aaron, can you guys reproduce and create a testcase here?

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

fyi, it can also be produced in planet.mozilla.org

Comment 15

[JP Rosevear [:jpr]]

Jeff, we still think its good to profile this.  Renom if you find specific problems or if you find any specific issues.

Comment 17

[JP Rosevear [:jpr]]

Any useful data here Jeff?

Comment 18

[Jeff Muizelaar [:jrmuizel]]

I get 41ms paints msnbc.com on my Galaxy Nexus. Planet.mozilla.org is also quite smooth for me. This bug needs a better test case or specific device the problem is narrowed down to.

Comment 19

[Aaron Train [:aaronmt]]

I see some skipping scrolling on http://neowin.net (Nightly 05/08, GN 4.0.4)

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add GetScreenVisibleRegion

Comment 22

[Ali Juma [:ajuma]]

Comment on attachment 624080 [details] [diff] [review]
Part 1: Add GetScreenVisibleRegion

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

r+ with comments addressed.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +322,5 @@
>    shadow->SetShadowClipRect(aLayer->GetClipRect());
>  
> +  nsIntRegion screen = aLayer->GetVisibleRegion();
> +  nsIntRect layerScreenBounds = aScreenBounds;
> +  transform.TransformBounds(aScreenBounds);

aScreenBounds = transform.TransformBounds(aScreenBounds);

@@ +336,5 @@
> +static void
> +SetShadowProperties(Layer* aLayer)
> +{
> +  SetShadowProperties(aLayer, nsIntRect(0, 0, 1, 1), gfx3DMatrix());
> +}

Do we need this? It doesn't seem to be called right now.

@@ +400,5 @@
>      nsIntPoint metricsScrollOffset(0, 0);
>      if (metrics->IsScrollable())
>        metricsScrollOffset = metrics->mViewportScrollOffset;
>  
> +    //nsIntSize scaledSize = nsIntSize(size.width / mXScale, size.height / mYScale);

Remove.

@@ +401,5 @@
>      if (metrics->IsScrollable())
>        metricsScrollOffset = metrics->mViewportScrollOffset;
>  
> +    //nsIntSize scaledSize = nsIntSize(size.width / mXScale, size.height / mYScale);
> +    nsIntSize scaledSize = nsIntSize(size.width, size.height);

Can we just use size here, or did you mean to scale before assigning to scaledSize?

@@ +410,4 @@
>      ViewTransform treeTransform(-scrollCompensation, mXScale, mYScale);
>      shadow->SetShadowTransform(gfx3DMatrix(treeTransform) * currentTransform);
>    } else {
> +    nsIntSize scaledSize = nsIntSize(size.width * mXScale, size.height * mYScale);

If this is intentionally computed differently than scaledSize above in the case above, a comment explaining why would be helpful.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +793,5 @@
>  
>    mGLContext->fEnable(LOCAL_GL_SCISSOR_TEST);
>  
>    // If the Java compositor is being used, this clear will be done in
> +  // DrawWindow. Make sure the bits used here match up with those used

Is this change intentional? The existing comment seems correct.

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 624080 [details] [diff] [review]
Part 1: Add GetScreenVisibleRegion

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +322,5 @@
>    shadow->SetShadowClipRect(aLayer->GetClipRect());
>  
> +  nsIntRegion screen = aLayer->GetVisibleRegion();
> +  nsIntRect layerScreenBounds = aScreenBounds;
> +  transform.TransformBounds(aScreenBounds);

This is indeed a no-op right now? How come this didn't give any issues? aScreenBounds is also const so just assigning to it wouldn't work I think.

@@ +410,4 @@
>      ViewTransform treeTransform(-scrollCompensation, mXScale, mYScale);
>      shadow->SetShadowTransform(gfx3DMatrix(treeTransform) * currentTransform);
>    } else {
> +    nsIntSize scaledSize = nsIntSize(size.width * mXScale, size.height * mYScale);

Indeed, these three different compution methods (including the commented one), confuse me a little. It would seem to me all should be * mX/YScale. But that could be a little naive since I don't know this code very well.

Comment 25

[Benoit Girard (:BenWa)]

(In reply to Bas Schouten (:bas) from comment #23)
> Comment on attachment 624080 [details] [diff] [review]
> Part 1: Add GetScreenVisibleRegion
> 
> Review of attachment 624080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +322,5 @@
> >    shadow->SetShadowClipRect(aLayer->GetClipRect());
> >  
> > +  nsIntRegion screen = aLayer->GetVisibleRegion();
> > +  nsIntRect layerScreenBounds = aScreenBounds;
> > +  transform.TransformBounds(aScreenBounds);
> 
> This is indeed a no-op right now? How come this didn't give any issues?
> aScreenBounds is also const so just assigning to it wouldn't work I think.

I don't understand this feedback. We don't assign to aScreenBounds so I don't see the problem.

> @@ +410,4 @@
> >      ViewTransform treeTransform(-scrollCompensation, mXScale, mYScale);
> >      shadow->SetShadowTransform(gfx3DMatrix(treeTransform) * currentTransform);
> >    } else {
> > +    nsIntSize scaledSize = nsIntSize(size.width * mXScale, size.height * mYScale);
> 
> Indeed, these three different compution methods (including the commented
> one), confuse me a little. It would seem to me all should be * mX/YScale.
> But that could be a little naive since I don't know this code very well.

The commented one should be removed. The difference between the first and second branch is that the second branch doesn't have access to the frame metrics.

Comment 26

[Ali Juma [:ajuma]]

(In reply to Benoit Girard (:BenWa) from comment #25)
> (In reply to Bas Schouten (:bas) from comment #23)
> > Comment on attachment 624080 [details] [diff] [review]
> > Part 1: Add GetScreenVisibleRegion
> > 
> > Review of attachment 624080 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/ipc/CompositorParent.cpp
> > @@ +322,5 @@
> > >    shadow->SetShadowClipRect(aLayer->GetClipRect());
> > >  
> > > +  nsIntRegion screen = aLayer->GetVisibleRegion();
> > > +  nsIntRect layerScreenBounds = aScreenBounds;
> > > +  transform.TransformBounds(aScreenBounds);
> > 
> > This is indeed a no-op right now? How come this didn't give any issues?
> > aScreenBounds is also const so just assigning to it wouldn't work I think.
> 
> I don't understand this feedback. We don't assign to aScreenBounds so I
> don't see the problem.

The problem is the line

  transform.TransformBounds(aScreenBounds);

has no effect since its a const method taking a const argument (see http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfx3DMatrix.cpp#644) and we're doing nothing with the return value. I suggested doing

  aScreenBounds = transform.TransformBounds(aScreenBounds); 

but as Bas points out we can't do that either, since aScreenBounds is const. So the question is whether we really need something like this line, and if so, why we aren't seeing any issues right now without it.

Comment 27

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Testcase-wanted removed as there are URLs and STRs to reproduce the panning issue.  Discussion is already going on with panning, so it doesn't seem like there needs to be a test case.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Jeff, are you still looking at this?

Comment 29

[Aaron Train [:aaronmt]]

As reported in comment #0, this is a non-issue currently on recent builds. There's an outdated patch attached here but I suspect this is WONT-FIX given improvement in this area over the past year in other bugs; please re-open if not the case.