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)

ARM
Android
defect

Tracking

(firefox11 fixed, fennec11+)

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
fennec 11+ ---

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+
Details | Diff | Splinter Review
(deleted), patch
pcwalton
: review+
Details | Diff | Splinter Review
(deleted), patch
pcwalton
: review+
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
pcwalton
: review+
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)
How close is snorp's stuff? If it's close then that might just fix the issue.
Priority: -- → P3
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.
Attached patch Part 1 - Add tiled buffer support (obsolete) (deleted) — Splinter Review
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)
Attached patch Part 2 - Add MultiTileLayer class to Java code (obsolete) (deleted) — Splinter Review
This patch adds a new layer type that uses multiple SingleTileLayer's and uploads from a tiled image buffer.
Attachment #585230 - Flags: feedback?(pwalton)
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)
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!
Attached patch Part 1 - Add tiled buffer support (obsolete) (deleted) — Splinter Review
Attachment #585229 - Attachment is obsolete: true
Attachment #585229 - Flags: feedback?(pwalton)
Attached patch Part 2 - Add MultiTileLayer class to Java code (obsolete) (deleted) — Splinter Review
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)
Attachment #585233 - Attachment is obsolete: true
Attachment #585233 - Flags: feedback?(pwalton)
Attached patch Part 4 - Use tiles to update asynchronously (obsolete) (deleted) — Splinter Review
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 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 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 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+
Rebased. I'll add the cairo recording surface optimisation in a further patch.
Attachment #585476 - Attachment is obsolete: true
Attachment #586021 - Flags: review?(pwalton)
Attached patch Part 2 - Add MultiTileLayer class to Java code (obsolete) (deleted) — Splinter Review
Rebased, comments addressed.
Attachment #585477 - Attachment is obsolete: true
Attachment #586022 - Flags: review?(pwalton)
A complete version of this patch, rebased. I also un-broke thumbnailing.
Attachment #585478 - Attachment is obsolete: true
Attachment #586023 - Flags: review?(pwalton)
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)
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.
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.
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)
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)
Attached patch Part X - Use recording surface (deleted) — Splinter Review
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.
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.
(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 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 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 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 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 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?
Upload the right patch, sorry about that.
Attachment #586022 - Attachment is obsolete: true
Attachment #586183 - Flags: review?(pwalton)
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+
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.
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.
(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).
Whoops, that limit comment was actually meant for the code in MultiTileLayer that implements CairoBitmap - the rest still applies though.
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)
(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).
(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?
(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.
(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...
(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.
We could easily make this interruptable too, even better!
(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.
tracking-fennec: --- → 11+
Chris, please request approval-aurora on these patches
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+
(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.
(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.
(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.
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?
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?
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?
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 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+
Attachment #586024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #586106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #586183 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: