Closed
Bug 709120
Opened 13 years ago
Closed 13 years ago
There is often a perceptible delay when the screen is updated during panning
Categories
(Firefox for Android Graveyard :: General, defect, P3)
Tracking
(firefox11 fixed, fennec11+)
RESOLVED
FIXED
Firefox 12
People
(Reporter: cwiiis, Assigned: cwiiis)
References
Details
(Whiteboard: [inbound])
Attachments
(6 files, 10 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
pcwalton
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pcwalton
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pcwalton
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
pcwalton
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Often-times when panning, there are jerks as checker-boarded areas are filled in.
Doing some investigation, these jerks correspond with texture-upload time (where we block), but that texture upload time can vary wildly. I expect this is down to saturating the bus.
Experimenting a bit, I've found that adding a small delay after Gecko draws can really help even out these times and improve perceived performance, especially on single-core devices it seems.
A delay is a bit rough though, I expect that waiting for a draw, and then doing the texture upload while blocking Gecko may have a similar effect.
Experimental patch attached.
Attachment #580443 -
Flags: feedback?(pwalton)
Comment 1•13 years ago
|
||
How close is snorp's stuff? If it's close then that might just fix the issue.
Updated•13 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•13 years ago
|
||
Just to note that snorp's DirectTexture work (bug 670930) completely fixes this issue, but unfortunately isn't available on all devices (notably, any device with an Adreno chipset, it seems).
pcwalton is working on a patch to use horizontal double-buffered texture slices and threaded texture upload using multiple contexts/texture sharing.
I'd like to investigate doing something similar without the threading/double-buffering (which may be a memory concern/concurrency headache) when there's time.
ajuma has pointed out that doing sub-image uploads on some chipsets is much much slower than doing a whole texture upload, to the point where it sometimes isn't worth doing at all - this is something that needs to be considered and investigated. Thankfully, the Adreno is not one of these chipsets.
I have a patch in bug 711959 that would allow the delay patch on this bug to be implemented more elegantly, I think, if this is something worth doing at all.
Assignee | ||
Comment 3•13 years ago
|
||
This patch adds optional support for rendering to a tiled buffer. This could be sped up slightly, I believe, by using Cairo recording surfaces instead of calling draw multiple times.
Assignee: nobody → chrislord.net
Attachment #585229 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 4•13 years ago
|
||
This patch adds a new layer type that uses multiple SingleTileLayer's and uploads from a tiled image buffer.
Attachment #585230 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 5•13 years ago
|
||
This uses MultiTileLayer instead of SingleTileLayer, but is really only for testing and very much WIP - the chosen tile size is arbitrary and this breaks image thumbnailing.
Due to how things are structured, this code will not be activated when gralloc is used.
Attachment #585233 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 6•13 years ago
|
||
This is the start of some work that will hopefully speed things up on devices that:
- Can't use gralloc
- Have possibly slow sub-image upload
- Can't specify the image stride when doing sub-image upload
The three patches I just submitted add support for rendering to a tiled buffer and having a tiled texture layer.
The next step is to make tile updates lazy so that only tiles that are intersecting the screen are uploaded, and other tiles are uploaded in idle time, or as-needed.
Any and all feedback is appreciated!
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #585229 -
Attachment is obsolete: true
Attachment #585229 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 8•13 years ago
|
||
As well as rebase, fix a few bugs (notably, tile resolution not being correctly set).
Attachment #585230 -
Attachment is obsolete: true
Attachment #585230 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #585233 -
Attachment is obsolete: true
Attachment #585233 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 10•13 years ago
|
||
This patch only synchronously updates tiles that intersect with the screen, other tiles are updated 1-by-1 in subsequent redraws. Also stops drawing tiles that aren't on-screen.
With this patch, scrolling is a lot smoother on my HTC Flyer, but there appear to be some drawing glitches occasionally when scrolling fast that I haven't worked out yet.
Comment 11•13 years ago
|
||
Comment on attachment 585476 [details] [diff] [review]
Part 1 - Add tiled buffer support
Review of attachment 585476 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/src/android/AndroidJavaWrappers.h
@@ +531,5 @@
> FORCED_RESIZE = 16,
> ACTIVITY_START = 17,
> BROADCAST = 19,
> VIEWPORT = 20,
> + TILE_SIZE = 22, /* Leaving space for EXPOSE */
I think I'm going to get rid of EXPOSE, so this isn't necessary.
::: widget/src/android/nsWindow.cpp
@@ +1227,5 @@
> + break;
> + } else {
> + if (sHasDirectTexture) {
> + // XXX: lock only the dirty rect above and pass it in here
> + DrawTo(targetSurface);
It would be nice to only redraw the changed tiles when we're using gralloc. Tiling is useful even with gralloc for this reason. (Can be a followup, of course.)
Attachment #585476 -
Flags: feedback+
Comment 12•13 years ago
|
||
Comment on attachment 585477 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code
Review of attachment 585477 [details] [diff] [review]:
-----------------------------------------------------------------
So when I coded the StripLayer I didn't use a separate SingleTileLayer for each strip; rather, the StripLayer drew each strip manually. Your way may well be cleaner though.
::: mobile/android/base/gfx/MultiTileLayer.java
@@ +43,5 @@
> +import android.graphics.Point;
> +import android.graphics.Rect;
> +import android.util.Log;
> +import java.nio.ByteBuffer;
> +import java.nio.ByteOrder;
ByteOrder is not used and can be removed.
@@ +48,5 @@
> +import java.util.ArrayList;
> +import javax.microedition.khronos.opengles.GL10;
> +
> +/**
> + * Encapsulates the logic needed to draw a single textured tile.
Comment needs to be updated.
@@ +104,5 @@
> +
> + private void validateTiles() {
> + IntSize size = getSize();
> +
> + if (!size.equals(mBufferSize)) {
nit:
if (size.equals(mBufferSize))
return;
Saves a level of indentation.
@@ +106,5 @@
> + IntSize size = getSize();
> +
> + if (!size.equals(mBufferSize)) {
> + // If the area hasn't change, we can re-use the tiles
> + if (size.getArea() != mBufferSize.getArea()) {
Might want to factor this if out into a separate function to save some more indentation. Or not, it's up to you.
Attachment #585477 -
Flags: feedback+
Comment 13•13 years ago
|
||
Comment on attachment 585478 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient
Review of attachment 585478 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, as long as the commented-out code gets removed.
::: mobile/android/base/GeckoEvent.java
@@ +78,5 @@
> public static final int GECKO_EVENT_SYNC = 15;
> public static final int ACTIVITY_START = 17;
> public static final int BROADCAST = 19;
> public static final int VIEWPORT = 20;
> + public static final int TILE_SIZE = 22; // Leaving space for EXPOSE
See above re. EXPOSE.
Attachment #585478 -
Flags: feedback+
Assignee | ||
Comment 14•13 years ago
|
||
Rebased. I'll add the cairo recording surface optimisation in a further patch.
Attachment #585476 -
Attachment is obsolete: true
Attachment #586021 -
Flags: review?(pwalton)
Assignee | ||
Comment 15•13 years ago
|
||
Rebased, comments addressed.
Attachment #585477 -
Attachment is obsolete: true
Attachment #586022 -
Flags: review?(pwalton)
Assignee | ||
Comment 16•13 years ago
|
||
A complete version of this patch, rebased. I also un-broke thumbnailing.
Attachment #585478 -
Attachment is obsolete: true
Attachment #586023 -
Flags: review?(pwalton)
Assignee | ||
Comment 17•13 years ago
|
||
Rebased and fixed up. There was a problem in that sometimes tiles are drawn without being updated, and updating asyncronously means you could expose tiles with incorrect data. I've now taken care to mark which tiles are valid so that invalid tiles are never drawn.
Attachment #585480 -
Attachment is obsolete: true
Attachment #586024 -
Flags: review?(pwalton)
Assignee | ||
Comment 18•13 years ago
|
||
Irritatingly, there seems to be some kind of rendering glitch where very occasionally a tile looks like its data was uploaded with an incorrect stride. I've no idea what's causing this, it's most visible on very animated sites (such as animalsbeingdicks.com).
I don't think this should stop this hitting central, but it certainly shouldn't hit aurora unless we find the source of that problem. It seems to only happen with sub-image updates, and could feasibly be a driver bug, but I hope not.
Assignee | ||
Comment 19•13 years ago
|
||
So it ends up that recording surfaces are much slower than just issuing multiple draw commands, so these patches stand as is - will do a try run.
Assignee | ||
Comment 20•13 years ago
|
||
Noticed that the tiling path is still capping to the maximum texture size, when it doesn't need to - fixed.
Attachment #586023 -
Attachment is obsolete: true
Attachment #586023 -
Flags: review?(pwalton)
Attachment #586091 -
Flags: review?(pwalton)
Assignee | ||
Comment 21•13 years ago
|
||
Whoops, bad patch (uploaded before testing properly, silly mistake, fixed)
Attachment #586091 -
Attachment is obsolete: true
Attachment #586091 -
Flags: review?(pwalton)
Attachment #586106 -
Flags: review?(pwalton)
Assignee | ||
Comment 22•13 years ago
|
||
I'm adding this to the patch so it doesn't get lost, in case it's useful in the future. This uses cairo recording surface and then paints those to the tiles, instead of invoking multiple draws. This would be faster, but unfortunately our Cairo isn't new enough :(
For reference, we would need development cairo - and apparently none of the development snapshots have what we need yet, so for all intents and purposes, we'd need Cairo 1.12.
Assignee | ||
Comment 23•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9de9e6c83d69 - Try run is green, but this is to be expected as none of this code is exercised if there is gralloc support.
This confirms that these patches don't break the gralloc path at least, I'm going to push another try run with a patch to force the non-gralloc path and see how that does.
Comment 24•13 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #18)
> Irritatingly, there seems to be some kind of rendering glitch where very
> occasionally a tile looks like its data was uploaded with an incorrect
> stride. I've no idea what's causing this, it's most visible on very animated
> sites (such as animalsbeingdicks.com).
>
> I don't think this should stop this hitting central, but it certainly
> shouldn't hit aurora unless we find the source of that problem. It seems to
> only happen with sub-image updates, and could feasibly be a driver bug, but
> I hope not.
If we're tiling, can we just not use glTexSubImage2D()? Some drivers have reaalllly insanely stupidly slow versions of glTexSubImage2D(), and if we tile we can just reupload a tile at a time and it won't cost us much.
Comment 25•13 years ago
|
||
Comment on attachment 586021 [details] [diff] [review]
Part 1 - Add tiled buffer support
Review of attachment 586021 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #586021 -
Flags: review?(pwalton) → review+
Comment 26•13 years ago
|
||
Comment on attachment 586022 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code
Review of attachment 586022 [details] [diff] [review]:
-----------------------------------------------------------------
I'd like to move to async texture upload at some point on a separate thread, but this can be a followup.
r=me with nit.
::: mobile/android/base/gfx/Layer.java
@@ +162,5 @@
>
> /**
> * Subclasses may override this method to perform custom layer updates. This will be called
> * with the transaction lock held. Subclass implementations of this method must call the
> * superclass implementation.
nit: Describe what the return value does here.
::: mobile/android/base/gfx/LayerRenderer.java
@@ +172,5 @@
> + updated &= mShadowLayer.update(gl, pageContext);
> + updated &= mCheckerboardLayer.update(gl, screenContext);
> + updated &= mFrameRateLayer.update(gl, screenContext);
> + updated &= mVertScrollLayer.update(gl, pageContext);
> + updated &= mHorizScrollLayer.update(gl, pageContext);
Bitwise-and works on booleans? Crazy.
Attachment #586022 -
Flags: review?(pwalton) → review+
Comment 27•13 years ago
|
||
Comment on attachment 586024 [details] [diff] [review]
Part 4 - Use tiles to update asynchronously
Review of attachment 586024 [details] [diff] [review]:
-----------------------------------------------------------------
This looks like it's the same as part 2 above...
Attachment #586024 -
Flags: review?(pwalton) → review+
Comment 28•13 years ago
|
||
Comment on attachment 586022 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code
This is identical to 4; I think this is the wrong patch.
Attachment #586022 -
Flags: review+ → review-
Comment 29•13 years ago
|
||
Comment on attachment 586106 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient
Review of attachment 586106 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +248,5 @@
> return null;
> try {
> Bitmap b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
> CairoUtils.cairoFormatTobitmapConfig(mFormat));
> + if (mTileLayer instanceof MultiTileLayer) {
nit: Could you factor this out into a separate function? The nesting is getting awfully deep here.
@@ +270,5 @@
> + tile.recycle();
> +
> + // Progress the buffer to the next tile
> + tileBuffer.position(tileSize.getArea() * bpp);
> + tileBuffer = tileBuffer.slice();
Should the order of these two statements be reversed? You don't want to update the position of the previous tile buffer, you want to make a new view on the current data and update its position, right?
Assignee | ||
Comment 30•13 years ago
|
||
Upload the right patch, sorry about that.
Attachment #586022 -
Attachment is obsolete: true
Attachment #586183 -
Flags: review?(pwalton)
Comment 31•13 years ago
|
||
Comment on attachment 586183 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code
Review of attachment 586183 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
::: mobile/android/base/gfx/MultiTileLayer.java
@@ +213,5 @@
> + }
> +
> + @Override
> + public void draw(RenderContext context) {
> + for (SingleTileLayer layer : mTiles) {
nit: no {}
Attachment #586183 -
Flags: review?(pwalton) → review+
Comment 32•13 years ago
|
||
Could we, in the near future, prioritize tiles in regions where no tile at all is being displayed (in other words, where there's a checkerboard) before we rerender dirty tiles when panning? This could eliminate lots of perceived checkerboarding during panning on sites that are slow to render, as the renderer would start to fill in the areas that aren't rendered yet first.
Assignee | ||
Comment 33•13 years ago
|
||
Try run with forced tiling path is green: https://tbpl.mozilla.org/?tree=Try&rev=252f5e353d1e
I'm going to assume that you meant to r+ part 3 too if that's not too presumptious (going on the prior comments here and your twitter) - just so I can get this on inbound before the weekend.
Assignee | ||
Comment 34•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #29)
> Comment on attachment 586106 [details] [diff] [review]
> Part 3 - Use MultiTileLayer instead of SingleTileLayer in
> GeckoSoftwareLayerClient
>
> Review of attachment 586106 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> @@ +248,5 @@
> > return null;
> > try {
> > Bitmap b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
> > CairoUtils.cairoFormatTobitmapConfig(mFormat));
> > + if (mTileLayer instanceof MultiTileLayer) {
>
> nit: Could you factor this out into a separate function? The nesting is
> getting awfully deep here.
Sure, sounds reasonable.
> @@ +270,5 @@
> > + tile.recycle();
> > +
> > + // Progress the buffer to the next tile
> > + tileBuffer.position(tileSize.getArea() * bpp);
> > + tileBuffer = tileBuffer.slice();
>
> Should the order of these two statements be reversed? You don't want to
> update the position of the previous tile buffer, you want to make a new view
> on the current data and update its position, right?
No, if you slice a positioned buffer, you end up with a buffer whose position zero is the position that was previously set. This is handy so that code that gets this buffer can call position itself without having to worry about the current position of the buffer. Really, I should set the limit of the buffer too (and in fact, I'll do that before pushing this).
Assignee | ||
Comment 35•13 years ago
|
||
Whoops, that limit comment was actually meant for the code in MultiTileLayer that implements CairoBitmap - the rest still applies though.
Assignee | ||
Comment 36•13 years ago
|
||
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a664e3507340
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a363485a656
http://hg.mozilla.org/integration/mozilla-inbound/rev/32d494dce342
http://hg.mozilla.org/integration/mozilla-inbound/rev/a1f1e4d685c0
Whiteboard: [inbound]
Assignee | ||
Comment 37•13 years ago
|
||
Comment on attachment 580443 [details] [diff] [review]
Add a delay after Gecko sends a drawing update
Removing feedback request for this - while it seems to help, tiling/gralloc pretty much solve the problem this bug is titled after. Further optimisation can be dealt with in new bugs.
Attachment #580443 -
Flags: feedback?(pwalton)
Assignee | ||
Comment 38•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #32)
> Could we, in the near future, prioritize tiles in regions where no tile at
> all is being displayed (in other words, where there's a checkerboard) before
> we rerender dirty tiles when panning? This could eliminate lots of perceived
> checkerboarding during panning on sites that are slow to render, as the
> renderer would start to fill in the areas that aren't rendered yet first.
Yes, I was thinking of doing something along these lines. What we could now do reasonably easily, is alter refreshTileMetrics so that it just marks tiles as needing refreshing instead of immediately doing it, and store invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid state - they're in the correct place, but their image data may be out of date. Most of the time, this will look correct.
Then, we continue to prioritise dirty tiles that are intersecting with the viewport, but instead of moving the viewport bit-by-bit when scrolling, we just move it an entire viewport-height/width at a time, in anticipation of where it will be in the future. The newly-exposed tiles can replace the tiles that are off-screen, and the semi-valid tiles that intersect with the viewport can remain there. This way, we have a virtual buffer size that can be twice the real buffer size (between consecutive updates).
The down-side is that when scrolling, animated parts of the page may appear to pause, and possibly only part of a page will pause (so you may see half a video pause, but the bottom half continue, for example). We could either just allow this (I don't think it's a big deal really), or perhaps set some flags when scrolling so that animations are temporarily paused (if this is feasible).
The other down-side is that I don't think this would work with gralloc (it requires the texture and buffer to not share memory), so I'm eager to explore other avenues first.
Another alternative is to do this and have a layer size that's greater than the buffer size - this makes the timing/viewport manipulation a bit less tricky (but again, doesn't necessarily help much for the gralloc case, at least how we currently use it).
Comment 39•13 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #38)
> Yes, I was thinking of doing something along these lines. What we could now
> do reasonably easily, is alter refreshTileMetrics so that it just marks
> tiles as needing refreshing instead of immediately doing it, and store
> invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> state - they're in the correct place, but their image data may be out of
> date. Most of the time, this will look correct.
What about using per-tile viewport metrics? (At least, as far as position is concerned -- I don't mind if we throw away all the tiles during zooming.) Then we know the origin of each tile in page coordinates, and we can keep around the old tiles during redraw after panning. We prioritize the new tiles that are appearing in the checkerboarded regions before redrawing the tiles with invalid content.
> The other down-side is that I don't think this would work with gralloc (it
> requires the texture and buffer to not share memory), so I'm eager to
> explore other avenues first.
I'd like to see whether we can move gralloc to tile as well. Is there any reason why the buffers for each tile must be contiguous, if we're painting a tile at a time anyhow?
Assignee | ||
Comment 40•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #39)
> (In reply to Chris Lord [:cwiiis] from comment #38)
> > Yes, I was thinking of doing something along these lines. What we could now
> > do reasonably easily, is alter refreshTileMetrics so that it just marks
> > tiles as needing refreshing instead of immediately doing it, and store
> > invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> > state - they're in the correct place, but their image data may be out of
> > date. Most of the time, this will look correct.
>
> What about using per-tile viewport metrics? (At least, as far as position is
> concerned -- I don't mind if we throw away all the tiles during zooming.)
> Then we know the origin of each tile in page coordinates, and we can keep
> around the old tiles during redraw after panning. We prioritize the new
> tiles that are appearing in the checkerboarded regions before redrawing the
> tiles with invalid content.
Yes, this is kind of what I mean - the tile's origin and resolution act as its metrics and we can derive their position from those. I think we should still draw synchronously any changed tiles within the viewport (unless we have devices that struggle even with that?) That said, pre-empting the viewport position as I suggest would have this effect anyway.
> > The other down-side is that I don't think this would work with gralloc (it
> > requires the texture and buffer to not share memory), so I'm eager to
> > explore other avenues first.
>
> I'd like to see whether we can move gralloc to tile as well. Is there any
> reason why the buffers for each tile must be contiguous, if we're painting a
> tile at a time anyhow?
No reason at all, I was talking to snorp about this earlier - we could pass in a tile map instead of a pointer to make this work with gralloc. If we start with optimisations like this, we definitely want to make this work with gralloc too.
Comment 41•13 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #40)
> (In reply to Patrick Walton (:pcwalton) from comment #39)
> > (In reply to Chris Lord [:cwiiis] from comment #38)
> > > Yes, I was thinking of doing something along these lines. What we could now
> > > do reasonably easily, is alter refreshTileMetrics so that it just marks
> > > tiles as needing refreshing instead of immediately doing it, and store
> > > invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> > > state - they're in the correct place, but their image data may be out of
> > > date. Most of the time, this will look correct.
> >
> > What about using per-tile viewport metrics? (At least, as far as position is
> > concerned -- I don't mind if we throw away all the tiles during zooming.)
> > Then we know the origin of each tile in page coordinates, and we can keep
> > around the old tiles during redraw after panning. We prioritize the new
> > tiles that are appearing in the checkerboarded regions before redrawing the
> > tiles with invalid content.
>
> Yes, this is kind of what I mean - the tile's origin and resolution act as
> its metrics and we can derive their position from those. I think we should
> still draw synchronously any changed tiles within the viewport (unless we
> have devices that struggle even with that?)
As long as we don't know of any pages where that leads to bad performance, sure, I agree that's desirable. A good test case here is http://taskjs.org/, which has a gigantic, screen-sized, Cairo- (and Skia)-killing gradient. Our paint times (as well as those of the stock browser) are horribly slow there and we may have to resort to drawing a tile at a time, so that the user can see something instead of staring at a checkerboard...
Assignee | ||
Comment 42•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #41)
> (In reply to Chris Lord [:cwiiis] from comment #40)
> > (In reply to Patrick Walton (:pcwalton) from comment #39)
> > > (In reply to Chris Lord [:cwiiis] from comment #38)
> > > > Yes, I was thinking of doing something along these lines. What we could now
> > > > do reasonably easily, is alter refreshTileMetrics so that it just marks
> > > > tiles as needing refreshing instead of immediately doing it, and store
> > > > invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> > > > state - they're in the correct place, but their image data may be out of
> > > > date. Most of the time, this will look correct.
> > >
> > > What about using per-tile viewport metrics? (At least, as far as position is
> > > concerned -- I don't mind if we throw away all the tiles during zooming.)
> > > Then we know the origin of each tile in page coordinates, and we can keep
> > > around the old tiles during redraw after panning. We prioritize the new
> > > tiles that are appearing in the checkerboarded regions before redrawing the
> > > tiles with invalid content.
> >
> > Yes, this is kind of what I mean - the tile's origin and resolution act as
> > its metrics and we can derive their position from those. I think we should
> > still draw synchronously any changed tiles within the viewport (unless we
> > have devices that struggle even with that?)
>
> As long as we don't know of any pages where that leads to bad performance,
> sure, I agree that's desirable. A good test case here is http://taskjs.org/,
> which has a gigantic, screen-sized, Cairo- (and Skia)-killing gradient. Our
> paint times (as well as those of the stock browser) are horribly slow there
> and we may have to resort to drawing a tile at a time, so that the user can
> see something instead of staring at a checkerboard...
Oh I see, you're suggesting making the Gecko painting async per-tile too? I guess we could do that - perhaps start a timer when beginDrawing has, and after a certain time (like 50ms, say) just paint whatever tiles have finished and the rest as they come in. I like the idea, I'll have a look at doing this next week.
Assignee | ||
Comment 43•13 years ago
|
||
We could easily make this interruptable too, even better!
Comment 44•13 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #42)
> Oh I see, you're suggesting making the Gecko painting async per-tile too? I
> guess we could do that - perhaps start a timer when beginDrawing has, and
> after a certain time (like 50ms, say) just paint whatever tiles have
> finished and the rest as they come in. I like the idea, I'll have a look at
> doing this next week.
Yeah, something like that. Coupled with painting the checkerboarded tiles first this could have a huge positive impact on our checkerboarding.
Updated•13 years ago
|
tracking-fennec: --- → 11+
Comment 45•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a664e3507340
https://hg.mozilla.org/mozilla-central/rev/4a363485a656
https://hg.mozilla.org/mozilla-central/rev/32d494dce342
https://hg.mozilla.org/mozilla-central/rev/a1f1e4d685c0
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Updated•13 years ago
|
status-firefox11:
--- → affected
Comment 46•13 years ago
|
||
Chris, please request approval-aurora on these patches
Comment 47•13 years ago
|
||
Comment on attachment 586106 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient
Review of attachment 586106 [details] [diff] [review]:
-----------------------------------------------------------------
r+ to get this out of my queue.
Attachment #586106 -
Flags: review?(pwalton) → review+
Comment 48•13 years ago
|
||
(In reply to Brad Lassey [:blassey] from comment #46)
> Chris, please request approval-aurora on these patches
And talk about the risks associated with the patches.
Comment 49•13 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #48)
> (In reply to Brad Lassey [:blassey] from comment #46)
> > Chris, please request approval-aurora on these patches
>
> And talk about the risks associated with the patches.
Just weighing in: This is less risky than gralloc because it uses no private APIs. It adds some risk, mostly in the possibility of transient incorrect display, because of the complexity of managing multiple tiles over a single tile as we do today.
Assignee | ||
Comment 50•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #49)
> (In reply to Mark Finkle (:mfinkle) from comment #48)
> > (In reply to Brad Lassey [:blassey] from comment #46)
> > > Chris, please request approval-aurora on these patches
> >
> > And talk about the risks associated with the patches.
>
> Just weighing in: This is less risky than gralloc because it uses no private
> APIs. It adds some risk, mostly in the possibility of transient incorrect
> display, because of the complexity of managing multiple tiles over a single
> tile as we do today.
This is correct, and would be my concern. It would take some refactoring for this to land without snorp's gralloc work, but I see that's re-landing now, so I'll request this as well.
Assignee | ||
Comment 51•13 years ago
|
||
Comment on attachment 586021 [details] [diff] [review]
Part 1 - Add tiled buffer support
[Approval Request Comment]
User impact if declined: Unable to apply further tiling-related patches
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): None
Attachment #586021 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 52•13 years ago
|
||
Comment on attachment 586183 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code
[Approval Request Comment]
User impact if declined: Unable to apply further tiling-related patches
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): None
Attachment #586183 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 53•13 years ago
|
||
Comment on attachment 586106 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient
[Approval Request Comment]
User impact if declined: Longer pauses during panning on certain devices
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): Small risk of rectangular areas of the screen looking incorrect for a short while on certain devices
Attachment #586106 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 54•13 years ago
|
||
Comment on attachment 586024 [details] [diff] [review]
Part 4 - Use tiles to update asynchronously
[Approval Request Comment]
User impact if declined: Pauses during panning on certain devices
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): Small risk of rectangular areas of the page looking incorrect for a short while on certain devices
Attachment #586024 -
Flags: approval-mozilla-aurora?
Comment 55•13 years ago
|
||
Comment on attachment 586021 [details] [diff] [review]
Part 1 - Add tiled buffer support
[Triage Comment]
Mobile only - approved for Aurora.
Attachment #586021 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #586024 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #586106 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #586183 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 56•13 years ago
|
||
Marking as dependent on bug 670930 - these patches are based on top of those and require significant rebasing to work without. I understand that that may land, but default to being switched off (which is fine for this).
Depends on: 670930
Comment 57•13 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•