Closed Bug 593310 Opened 14 years ago Closed 13 years ago

show checkerboard for waiting-to-be-filled regions with browser elements as browsing surface

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android
defect

Tracking

(fennec2.0b4+)

VERIFIED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: blassey, Assigned: cjones)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
This works, but its pretty simple so I assume it can be made faster.
Attachment #471797 - Flags: review?(mark.finkle)
I assumed we were moving this checkerboard handling into the platform?
I thought so too, but got a different impression when I asked cjones about it.
Comment on attachment 471797 [details] [diff] [review]
patch

Why change the background position at all? This probably is a really bad idea for performance.
Attachment #471797 - Flags: review-
(In reply to comment #3)
> Comment on attachment 471797 [details] [diff] [review]
> patch
> 
> Why change the background position at all? This probably is a really bad idea
> for performance.

So that the chrckerbpard moves with the page and we don't look stalled. This is the behavior we've always had in fennec
Fwiw, this performs just fine on the nexus one
tracking-fennec: --- → 2.0b1+
OS: Linux → Android
Hardware: x86_64 → ARM
I heard some feedback that there might be a percieved performance issue. I'd like to not rush this for b1 and talk to roc (and others) about alternatives. Moving to beta2.
tracking-fennec: 2.0b1+ → 2.0b2+
Assignee: nobody → webapps
Brad mentioned that we could use mozAnimationFrame to make this faster. Let's try it.
Priority: -- → P1
Should be easy enough. Calls to scrollers are all mozAnimationFrame rate limited now anyways. Also, there are some other shadow layers changes afloat that may improve checkerboard perf as well.
If we want to stick with a checkerboard-style (i.e. repeated pattern, doesn't have to be a checkerboard) background, probably the maximally efficient way to implement that at the layers level is to create a PatternLayer type and allow the frontend to control which image is used for the source.  Then we would set a visible area for the PatternLayer that fills whatever the shadow layer tree leaves empty, and draw the pattern with EXTEND_REPEAT.  We would only need to allocate enough memory for the pattern image itself, not for the size of the area it occupied.  This area can fluctuate wildly and can cover the entire <browser> surface.  Drawing it to should be just a series of memcpy()s basically, so perf shouldn't suffer from the compression.  This would work really well in GL too: we could allocate a texture the size of the pattern and use GL_REPEAT.

We could probably fold ColorLayer into PatternLayer to save code.

roc, how does that sound?
Folding ColorLayer into PatternLayer probably isn't a good idea.

How about adding a repeat flag to ImageLayer? Setting it would cause the image to be repeated to fill the entire visible region. The top-left of the image would still be anchored to 0,0 in the layer's coordinate system.
Ah, that sounds good.  That should be a nice optimization for background-attached with a repeated image as well.
Actually, to get the sampling of repeated images right, instead of a "repeat flag" the new parameter should be a "tiling rectangle". The image is conceptually tiled to fill the rectangle before being clipped/transformed/sampled. You can't just manipulate the visible region or clip rect to get the effect of the tiling rectangle, because the tiling rectangle can restrict which pixels of the source image are sampled.
OK, I think I understand what you mean.  We can still implement that efficiently with cairo, right?  (Seems easy in theory.)
No, it's actually a huge pain to implement with cairo. See imgFrame::Draw. It's the right API though.
But you can implement the simple cases you care about --- such as when the tile rect contains the entire visible region --- very easily and just use a slow temporary-surface path for the other cases.
I thought about this some more, and we'd save at best about 1MB per <browser remote> with the tiling optimization.  That's nothing to sneeze at, but before deciding if it's worthwhile, can someone please write out the UX goals for the checkerboard?

 - what coordinate space is the checkerboard relative to
 - how does it behave when the page is zoomed
 - how is it rendered when there's a non-rectangular undefined region

If this is going to be complicated to implement, I'd rather keep it in XUL/HTML.
tracking-fennec: 2.0b2+ → 2.0b3+
Comment on attachment 471797 [details] [diff] [review]
patch

Let's give this a go and see what feedback we get.
Attachment #471797 - Flags: review?(mark.finkle) → review+
Comment on attachment 471797 [details] [diff] [review]
patch

Clearing Ben's r- since we have had no better patches since this patch
Attachment #471797 - Flags: review-
Whiteboard: [fennec-checkin-postb2][has-patch]
pushed the front-end patch after some unbitrotting:
http://hg.mozilla.org/mobile-browser/rev/55388b6a8d7d

Let's see how this works
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb2][has-patch]
The checkerboard doesn't move in a local desktop build of mine, it just stays fixed wrt viewport.  My impression was that the goal of the checkerboard was to shot the user that panning events were being processed even though new pixels weren't rendered yet.  If that's so, I'm not sure this impl quite does the trick because the entire screen can stay unchanged for an arbitrarily long time during panning.
(In reply to comment #21)
> The checkerboard doesn't move in a local desktop build of mine, it just stays
> fixed wrt viewport.  My impression was that the goal of the checkerboard was to
> shot the user that panning events were being processed even though new pixels
> weren't rendered yet.  If that's so, I'm not sure this impl quite does the
> trick because the entire screen can stay unchanged for an arbitrarily long time
> during panning.

That is so, so it sounds like this bit-rotted over the last 2.5 months.
Attached patch bitrot fix (obsolete) — Splinter Review
This patch fixes some bitrot that kept the checkerboard from moving
Attachment #489871 - Flags: review?(mbrubeck)
Comment on attachment 489871 [details] [diff] [review]
bitrot fix

Could you factor out the 2 repeated lines into a new method, updateBackgroundPosition(x,y)?  r+ with that change.
Attachment #489871 - Flags: review?(mbrubeck) → review+
verified FIXED on builds:

Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101111 Namoroka/4.0b8pre Fennec/4.0b3pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101111 Namoroka/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED
This seems to have regressed panning performance. Backing out. We can try to land again after bug 606730 lands.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
With the patches from both bug 606730 and this bug applied, panning smoothness is no longer noticeably regressed, but the repainting delay after panning still seems to be regressed.
Moving to blocking final. We should have some improvements landed by then that might make this work better.
tracking-fennec: 2.0b3+ → 2.0+
tracking-fennec: 2.0+ → 2.0b4+
We need to decide how best to approach this issue. The front-end patch was backed out again due to perceived panning regressions. We also show checkerboard (or gray areas without this patch) much more often than say Fennec 1.1

Can we also, increase the size of our render area to decrease the checkerboarding? I assume there is a memory size cost if we increase our render area.
(In reply to comment #17)
> I thought about this some more, and we'd save at best about 1MB per <browser
> remote> with the tiling optimization.  That's nothing to sneeze at, but before
> deciding if it's worthwhile, can someone please write out the UX goals for the
> checkerboard?
> 
>  - what coordinate space is the checkerboard relative to
>  - how does it behave when the page is zoomed
>  - how is it rendered when there's a non-rectangular undefined region
> 
> If this is going to be complicated to implement, I'd rather keep it in
> XUL/HTML.

The basic goal of the checkerboard is to let the user know something is happening when panning outpaces rendering. A checkerboard impl goals include:
* Show motion when panning. The appearance of motion is a visual queue to the user that something is happening.
* Do not zoom. The checkerboard should not participate in any content zooming and should always be rendered at a fixed scale.
* I'm not sure how often non-rectangular regions would be encountered, but the checkerboard should act as a background to any undefined/unrendered area of the screen.

Any other issues people can think of?
stechz, have you started on this?  If not, I'll probably have time to poke next week.
(In reply to comment #32)
> * Show motion when panning. The appearance of motion is a visual queue to the
> user that something is happening.
> * Do not zoom. The checkerboard should not participate in any content zooming
> and should always be rendered at a fixed scale.

So from this, it sounds like this spec might suffice
 - "checkerboard space" point <0, 0> == document <0, 0>
 - checkerboard space has units of device pixels, independent of document-space scaling/app-units etc.
 - the checkerboard area is conceptually infinite

This would mean that
 - panning a <browser>'s content by <dx, dy> device pixels also translates the checkerboard top-left point within the <browser> by <dx, dy>
 - if fuzzy-zooming a <browser>'s content changes its document origin by <dx, dy> device pixels, then the checkerboard top-left translates by <dx, dy>

The second implication above means that if, say, the viewport top-left of web content were <100, 100>, and the entire <browser> were checkerboard, and the user pinch-zoomed, then the checkerboard would move translate horizontally and vertically during the pinch-zoom animation.  I'm not sure if this squares with comment 32.
No, have not started yet. Feel free to claim.
Assignee: ben → nobody
Assignee: nobody → jones.chris.g
See long comment in BuildBackgroundPatternFor().

Before anyone objects, yes GetBackgroundImage() is probably the grossest bit of code ever written, but figuring out how to get a background down from the frontend to RenderFrameParent shouldn't block the initial landing, I don't think.  Might be a long discussion there.  The placeholder image is the existing checkerboard.png image from mobile-browser, shrunk to 16x16, and converted to rgb565.

This patch also includes
 - rebuild ContainerLayer for shadow tree each time, instead of only when things changed.  Should be fast and caching got a lot more complicated by this patch.
 - removal of assertions made obsolete by this patch
 - removal of some dead code
 - minor indentation and style fixes

I'll get an android build up after dinner.
Attachment #471797 - Attachment is obsolete: true
Attachment #489871 - Attachment is obsolete: true
Attachment #505624 - Flags: review?(tnikkel)
Attachment #505624 - Flags: review?(ben)
Should also note we can tune perf by changing the size of the background image, to fit the cairo path we're hitting here, but this seemed fast enough on my galaxy S already.  Will poke more with the upcoming build.
Build with a 16x16 pattern

http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm-16x16.apk

and one with a 32x32

http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm-32x32.apk

The 32x32 one seems a bit more distracting to me, but maybe we could lighten the gray or something.
Comment on attachment 505624 [details] [diff] [review]
Bug 593310: Add initial support for drawing a moving, default background where shadow-layer content is undefined (as efficiently as possible)

>+already_AddRefed<gfxASurface>
>+GetBackgroundImage()
>+{
>+  // XXX TODO FIXME/bug XXXXXX: this is obviously a hacky placeloader
>+  // impl.  Unclear how the background pattern source should be set.

Hmm. How about a URI as a preference for the checkerboard image?

It otherwise looks good to me, but I'm no expert of this code. Nice cleanup too.
Attachment #505624 - Flags: review?(ben)
(In reply to comment #41)
> Hmm. How about a URI as a preference for the checkerboard image?

OK, this is the discussion I wanted to postpone :(.

 - for this implementation to be simple+fast with GL (out of the box anyway), the dimensions of the image need to be a power of 2
 - what's the maximum allowed image size?
 - what formats do we allow?  is SVG OK?
 - what happens if the URI is invalid or doesn't meet one of the above criteria?

If we wanted to do this right, I also think the background should be specified as CSS-ily as possible.  At minimum, a <browser> property, at best, re-use background-repeat but fast-path it.
I missed the above comment. I have no objections to landing this with generating the background image (and filing a followup).
OK; I thought you cleared the review request as a "virtual r-".

Just to be clear, my point in wanting to put off this discussion is, if for beta4 the checkerboard background is going to just be a dirty hack, one is good as the next to me.  A pref implies some modicum of support, which I don't believe we want to be true :).
Attachment #505624 - Flags: review+
(In reply to comment #43)
> I missed the above comment. I have no objections to landing this with
> generating the background image (and filing a followup).

Agreed. We can work out this part later. Maybe looking to see how other C++
code acquires image resources.
My favorite is fast-path for CSS background-image repeat property.
(In reply to comment #45)
> Agreed. We can work out this part later. Maybe looking to see how other C++
> code acquires image resources.

Getting the image bits isn't a problem, but there are several others.  Filed bug 627828.
(In reply to comment #40)
> Build with a 16x16 pattern
> 
> http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm-16x16.apk
> 
> and one with a 32x32
> 
> http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm-32x32.apk
> 
> The 32x32 one seems a bit more distracting to me, but maybe we could lighten
> the gray or something.

Any updates on which size is preferred?
(In reply to comment #49)
> (In reply to comment #40)
> > Build with a 16x16 pattern
> > 
> > http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm-16x16.apk
> > 
> > and one with a 32x32
> > 
> > http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm-32x32.apk
> > 
> > The 32x32 one seems a bit more distracting to me, but maybe we could lighten
> > the gray or something.
> 
> Any updates on which size is preferred?

I think the 16x16 works well. The 32x32 was harder for my eyes to track while panning.
Comment on attachment 506618 [details] [diff] [review]
Add initial support for drawing a moving, default background where shadow-layer content is undefined (as efficiently as possible), v2

There's no attempt to retain the checkerboard image layer. For GL this is going to re-upload the checkerboard pattern every time. Is that OK?

Otherwise looks good.
Attachment #506618 - Flags: review?(tnikkel) → review+
(In reply to comment #51)
> There's no attempt to retain the checkerboard image layer. For GL this is going
> to re-upload the checkerboard pattern every time. Is that OK?

Probably not.  See bug 628566 comment 0.
http://hg.mozilla.org/mozilla-central/rev/8d160fb8f2db
http://hg.mozilla.org/mozilla-central/rev/bfd44071cce9
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
FTR, I landed with the 16x16 checkerboard, but I've still got the patch for 32x32 if we want to switch at the last minute.  It's pretty trivial to do regardless.
I'm not seeing the checkerboard pattern in Fennec trunk builds.  Do we need to enable something in mobile-browser?
Nope.  I've been running local builds on my phone since last week without problems, but they weren't always with up-to-date m-b.  Rebuilding now.
Yeah, I'm not seeing it either with m-b tip.  Will look after lunch.
I just landed a followup fix for bug 627273 that wfm locally.  Mind giving it a try?
I posted a build that's ~beta4 candidate here

http://people.mozilla.com/~cjones/fennec-4.0b4pre.en-US.eabi-arm.apk

m-c f2ac3e4722c4, m-b da68f67cc6ad
Depends on: 629475
Commenting on bug Bug 624444 and got to this one.

I was unaware of this improvement; Now I know the idea is to give the user the perception of "something's still happening". 


However, when I first tried b4, the checker pattern immediately made me feel something was wrong. While using b3 I never had the need for the application to tell me it was still alive; Now I have the application permanently reminding me that it's working.


My perception is: beta4 is much slower than beta3. Turns out that it's mostly wrong perception (but it's still very important).


As an analogy with the desktop firefox, when a page is loading we see the missing spots as white bg as they get filled, and that's what I'm used to.


I vote for going back to good'ol white (or whatever is the bg color of the page)
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110915
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.