STR: - go to planet.mozilla.org - wait for it to load - scroll a bit - notice that we screenshot because scrolling causes paints I'm not sure what the best way to fix this is. We don't really have a good event to use for this. As a workaround we could perhaps postpone screenshotting during scrolling.

Brad spoke about putting the Java and Gecko parts of screen shots in idle handlers

cjones for the changes to add PostIdleTask to MessageLoop and Kats for the changes to widget/android/nsAppShell.cpp

(In reply to Brad Lassey [:blassey] from comment #2) > Created attachment 624383 [details] [diff] [review] > patch > > cjones for the changes to add PostIdleTask to MessageLoop and Kats for the > changes to widget/android/nsAppShell.cpp What are the conditions for being idle with this idle task?

with the previous patch, some times you can queue up several screenshots which will all happen at the end of the pan, this will prevent that.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > (In reply to Brad Lassey [:blassey] from comment #2) > > Created attachment 624383 [details] [diff] [review] > > patch > > > > cjones for the changes to add PostIdleTask to MessageLoop and Kats for the > > changes to widget/android/nsAppShell.cpp > > What are the conditions for being idle with this idle task? The conditions haven't changed, this just exposes a method to stick a task directly into the queue of tasks that are run in DoIdleWork(). DoIdleWork is called when the MessageLoop's message queue has been depleted.

(In reply to Brad Lassey [:blassey] from comment #5) > > The conditions haven't changed, this just exposes a method to stick a task > directly into the queue of tasks that are run in DoIdleWork(). DoIdleWork is > called when the MessageLoop's message queue has been depleted. Depleted for how long?

Depleted at all. The logic is here http://mxr.mozilla.org/mozilla-central/source/ipc/glue/MessagePump.cpp#140

Comment on attachment 624383 [details] [diff] [review] patch Review of attachment 624383 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsAppShell.cpp @@ +101,5 @@ > +class ScreenshotRunnable : public nsRunnable { > +public: > + ScreenshotRunnable(nsIAndroidBrowserApp* aBrowserApp, int aTabId, nsTArray<nsIntPoint>& aPoints, int aToken): > + mBrowserApp(aBrowserApp), mTabId(aTabId), mPoints(aPoints), mToken(aToken) {} > + virtual nsresult Run() { Blank line before this function would be nice. @@ +491,3 @@ > nsTArray<nsIntPoint> points = curEvent->Points(); > + nsCOMPtr<ScreenshotRunnable> sr = > + new ScreenshotRunnable(mBrowserApp, tabId, points,token); space between comma and "token".

Comment on attachment 624393 [details] [diff] [review] patch not to queue up multiple screenshots Review of attachment 624393 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoAppShell.java @@ +2271,5 @@ > + if (sPendingScreenshots.contains(ps)) > + return; > + sPendingScreenshots.add(ps); > + } > + GeckoAppShell.sendEventToGecko(GeckoEvent.createScreenshotEvent(tab.getId(), 0, 0, (int)sw, (int)sh, 0, 0, dw, dh, GeckoAppShell.SCREENSHOT_WHOLE_PAGE)); Un-indent this line. @@ +2274,5 @@ > + } > + GeckoAppShell.sendEventToGecko(GeckoEvent.createScreenshotEvent(tab.getId(), 0, 0, (int)sw, (int)sh, 0, 0, dw, dh, GeckoAppShell.SCREENSHOT_WHOLE_PAGE)); > + } > + > + static Set<PendingScreenshot> sPendingScreenshots = new HashSet<PendingScreenshot>(); Move this declaration to the top of the file. Really I would like all this screenshot stuff moved out into a separate class rather than making GeckoAppShell even more of a garbage dump than it already is... @@ +2288,5 @@ > + PendingScreenshot ps = (PendingScreenshot) obj; > + return ps.mTabId == mTabId && ps.mType == mType; > + > + > + } Random blank lines should be taken out @@ +2291,5 @@ > + > + } > + > + public int hashCode() { > + return mType + (mTabId >> 2); Is this right? Seems wrong, unless the tab id never uses the bottom two bits. @@ +2294,5 @@ > + public int hashCode() { > + return mType + (mTabId >> 2); > + } > + > + int mTabId, mType; Make these private and final. hashCode() should never depend on mutable variables. Alternatively use android.util.Pair instead of creating your own class.

Although in theory these patches should help the screenshotting by only doing it when idle, I wonder how often the idle thing gets hit in practice. It might be that it happens too often and so we're not really reducing the rate at which we take screenshots, or it might happen so rarely that we never take screenshots, or it might be that there's always an idle period right before intense scrolling so the screenshot introduces latency for painting. We should find out before landing this.

this moves most of the Screenshot logic to a new ScreenshotHandler class. Still in the same file though.

(In reply to Kartikaya Gupta (:kats) from comment #10) > Although in theory these patches should help the screenshotting by only > doing it when idle, I wonder how often the idle thing gets hit in practice. With these patches I still get screenshotting that happens during scrolling on planet.mozilla.org. So it seems in practice they are insufficient.

Comment on attachment 624787 [details] [diff] [review] patch not to queue up multiple screenshots Review of attachment 624787 [details] [diff] [review]: ----------------------------------------------------------------- If you're moving it into a separate class you might as well move the class out into a new file... :) r+ with comments addressed. ::: mobile/android/base/GeckoAppShell.java @@ +2139,5 @@ > + } > +} > + > +class ScreenshotHandler { > + static Set<PendingScreenshot> sPendingScreenshots = new HashSet<PendingScreenshot>(); make this private @@ +2243,5 @@ > + if (sPendingScreenshots.contains(ps)) > + return; > + sPendingScreenshots.add(ps); > + } > + GeckoAppShell.sendEventToGecko(GeckoEvent.createScreenshotEvent(tab.getId(), 0, 0, (int)sw, (int)sh, 0, 0, dw, dh, GeckoAppShell.SCREENSHOT_WHOLE_PAGE)); un-indent this line @@ +2264,5 @@ > + return mType + (mTabId << 2); > + } > + > + private final int mTabId; > + private final int mType; move these to the top of the class.

It seems like the best solution might be to break screenshotting up into chunks that we run on the idle handler.

Comment on attachment 624383 [details] [diff] [review] patch Please break these into separate patches in the future. >diff --git a/ipc/chromium/src/base/message_loop.cc b/ipc/chromium/src/base/message_loop.cc >--- a/ipc/chromium/src/base/message_loop.cc >+++ b/ipc/chromium/src/base/message_loop.cc >@@ -256,6 +256,13 @@ void MessageLoop::PostNonNestableDelayed > PostTask_Helper(from_here, task, delay_ms, false); > } > >+void MessageLoop::PostIdleTask( >+ const tracked_objects::Location& from_here, Task* task) { This implementation doesn't work for outside threads, so you need to DCHECK(current() == this); here. >+ deferred_non_nestable_work_queue_.push(pending_task); Putting this into the deferred_non_nestable_work_queue_ also doesn't do exactly what you want --- it won't process tasks from a nested event loop, most importantly. But I guess it's close enough. >diff --git a/ipc/chromium/src/base/message_loop.h b/ipc/chromium/src/base/message_loop.h >+ void PostIdleTask( >+ const tracked_objects::Location& from_here, Task* task); >+ Document this, in particular single-threadedness. r=me with those changes.

Comment on attachment 627389 [details] [diff] [review] patch to use only one buffer and draw dirty rects Review of attachment 627389 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoAppShell.java @@ +2151,5 @@ > float height = bottom - top; > + Log.i(LOGTAG, "update: " + top + ", " + left + ", " + width + ", " + height); > + GeckoEvent event = GeckoEvent. > + createScreenshotEvent(tab.getId(), > + (int)top, (int)left, (int)width, (int)height, Order of parameters is wrong here; should be left, top instead of top, left. @@ +2153,5 @@ > + GeckoEvent event = GeckoEvent. > + createScreenshotEvent(tab.getId(), > + (int)top, (int)left, (int)width, (int)height, > + (int)(sLastCheckerboardWidthRatio * top), > + (int)(sLastCheckerboardWidthRatio * left), Ditto. Also the one for top should be HeightRatio not WidthRatio. @@ +2170,5 @@ > synchronized(this) { > + mDirtyTop = Math.max(0, Math.min(top, mDirtyTop)); > + mDirtyLeft = Math.max(0, Math.min(left, mDirtyLeft)); > + mDirtyBottom = Math.min(sCheckerboardPageWidth, Math.max(bottom, mDirtyBottom)); > + mDirtyRight = Math.min(sCheckerboardPageWidth, Math.max(right, mDirtyRight)); These clamps need to be re-evaluated now that java supports RTL pages; the page bounds can be negative, so max(0, ...) is a bad idea. Also s/checkerboardPageWidth/checkerboardPageHeight/ for one of them. @@ +2206,4 @@ > sMaxTextureSize = maxTextureSize[0]; > if (sMaxTextureSize == 0) > return; > + sWholePageScreenshotBuffer = GeckoAppShell.allocateDirectBuffer(ScreenshotLayer.getMaxNumPixels() * 2); Why * 2? There should be a comment saying saying it's 16 bpp. @@ +2272,4 @@ > > GeckoAppShell.getHandler().post(new Runnable() { > public void run() { > + boolean needsFree = true; What is this used for? It seems like code is missing here. ::: mobile/android/base/Tab.java @@ +156,5 @@ > + } > + > + public void finalize() { > + if (mThumbnailBuffer != null) > + GeckoAppShell.freeDirectBuffer(mThumbnailBuffer); In general I don't like how we free these buffers in finalize() methods, since there's no guarantee on when they run. So far the finalizers have been on classes that live for as long as the app is running so it hasn't mattered much. Here though Tab is definitely short-lived, and I would prefer freeing the buffer in a cleanup method explicitly called from Tabs.removeTab. @@ +183,5 @@ > public void run() { > if (b != null) { > try { > + if (b.getWidth() != getThumbnailWidth() || b.getHeight() != getThumbnailHeight()) > + Log.w(LOGTAG, "thumbnail is not the right size!!!!!!!!!!!!!!"); Missing an exclamation point, you need at least 15 for this to have the right emphasis. :p ::: mobile/android/base/gfx/ScreenshotLayer.java @@ +61,2 @@ > void setBitmap(Bitmap bitmap) { > + Log.e(LOGTAG, "wrong setBitmap"); Remove log line @@ +131,5 @@ > } > } > > + void copyBuffer(ByteBuffer src, ByteBuffer dst, int size) { > + dst.asIntBuffer().put(src.asIntBuffer()); Why does this not just do dst.put(src)? @@ +139,5 @@ > + mSize = new IntSize(width, height); > + mFormat = format; > + copyBuffer(data, mBuffer, width * height * 2); > + } > + synchronized void setBitmap(Bitmap bitmap, int width, int height, int format) { Add a blank line between functions. ::: widget/android/AndroidBridge.cpp @@ +2429,4 @@ > nsPresContext::CSSPixelsToAppUnits(srcW / scale), > nsPresContext::CSSPixelsToAppUnits(srcH / scale)); > > + PRUint32 stride = bufW * 2; Add a comment for the "* 2" ::: widget/android/AndroidJavaWrappers.cpp @@ +500,5 @@ > case SCREENSHOT: { > mMetaState = jenv->GetIntField(jobj, jMetaStateField); > mFlags = jenv->GetIntField(jobj, jFlagsField); > + ReadPointArray(mPoints, jenv, jPoints, 5); > + mByteBuffer = jenv->NewGlobalRef(jenv->GetObjectField(jobj, jByteBufferField)); I don't like how this global ref is created here and then deleted in other code without clearly showing who owns it along the way. I would prefer if the AndroidGeckoEvent::ByteBuffer() getter method was renamed to RelinquishBuffer() or something, and the implementation also set mByteBuffer to null, so as to explicitly relinquish ownership to the caller. And then the DeleteGlobalRef should happen in nsAppShell after the call to AndroidBridge::TakeScreenshot, rather than at the end of AndroidBridge::TakeScreenshot, because it would be the nsAppShell code that owns the global ref. ::: widget/android/nsAppShell.cpp @@ +127,5 @@ > + rect->GetRight(&right); > + rect->GetBottom(&bottom); > + __android_log_print(ANDROID_LOG_INFO, "GeckoScreenshot", "rect: %f, %f, %f, %f", top, left, right, bottom); > + AndroidBridge::NotifyPaintedRect(top, left, bottom, right); > + //} Remove commented-out close brace.

Also diff --git a/widget/android/AndroidBridge.cpp b/widget/android/AndroidBridge.cpp --- a/widget/android/AndroidBridge.cpp +++ b/widget/android/AndroidBridge.cpp @@ -2428,21 +2429,22 @@ nsresult AndroidBridge::TakeScreenshot(n nsPresContext::CSSPixelsToAppUnits(srcW / scale), nsPresContext::CSSPixelsToAppUnits(srcH / scale)); - PRUint32 stride = dstW * 2; - PRUint32 bufferSize = dstH * stride; - - jobject buffer = Java_org_mozilla_gecko_GeckoAppShell_allocateDirectBuffer(env, NULL, bufferSize); + PRUint32 stride = bufW * 2; + PRUint32 bufferSize = bufH * stride; bufferSize is never used here.

Comment on attachment 627390 [details] [diff] [review] patch to break up screenshots into smaller chunks Review of attachment 627390 [details] [diff] [review]: ----------------------------------------------------------------- Seems OK, assuming the errors carried over from the previous patch are corrected. ::: mobile/android/base/GeckoAppShell.java @@ +2228,5 @@ > sCheckerboardPageWidth = sw; > sCheckerboardPageHeight = sh; > > + float totalSize = sw * sh; > + int numSlices = (int) Math.ceil(totalSize / 100000); magic number should be documented. @@ +2234,5 @@ > + int dstSliceSize = (int) Math.ceil(dh / numSlices); > + for (int i = 0; i < numSlices; i++) { > + scheduleCheckerboardScreenshotEvent(tab.getId(), > + 0, srcSliceSize * i, (int)sw, srcSliceSize, > + 0, dstSliceSize * i, dw, dstSliceSize, dw, dh); This loop takes care of really tall pages; what about really wide ones?

> @@ +2234,5 @@ > > + int dstSliceSize = (int) Math.ceil(dh / numSlices); > > + for (int i = 0; i < numSlices; i++) { > > + scheduleCheckerboardScreenshotEvent(tab.getId(), > > + 0, srcSliceSize * i, (int)sw, srcSliceSize, > > + 0, dstSliceSize * i, dw, dstSliceSize, dw, dh); > > This loop takes care of really tall pages; what about really wide ones? wide pages should be fine, it gets sliced based on area, not height. It'll just paint in several very short wide rects.

Comment on attachment 628199 [details] [diff] [review] patch to use only one buffer and draw dirty rects Review of attachment 628199 [details] [diff] [review]: ----------------------------------------------------------------- r- for the unaddressed RTL comments. ::: mobile/android/base/GeckoAppShell.java @@ +2169,5 @@ > synchronized(this) { > + mDirtyTop = Math.max(0, Math.min(top, mDirtyTop)); > + mDirtyLeft = Math.max(0, Math.min(left, mDirtyLeft)); > + mDirtyBottom = Math.min(sCheckerboardPageHeight, Math.max(bottom, mDirtyBottom)); > + mDirtyRight = Math.min(sCheckerboardPageWidth, Math.max(right, mDirtyRight)); See comments on previous review about this clamping code. You should rebase this patch to latest m-c and re-request review, since anything that assumes (0, 0, pageWidth, pageHeight) as the dimensions of the page is now wrong. ::: mobile/android/base/Tab.java @@ +142,5 @@ > > + public ByteBuffer getThumbnailBuffer() { > + int capacity = getThumbnailWidth() * getThumbnailHeight() * 2; > + if (mThumbnailBuffer != null && mThumbnailBuffer.capacity() == capacity) > + return mThumbnailBuffer; It feels like synchronization is needed here. getThumbnailBuffer is called from GeckoApp on the java background thread, and freeBuffer() is called on the Gecko thread, so there's potential for getThumbnailBuffer to return null or double-free or double-allocate the buffer. Or even just throw an NPE. ::: mobile/android/base/gfx/ScreenshotLayer.java @@ +131,5 @@ > } > > + void copyBuffer(ByteBuffer src, ByteBuffer dst, int size) { > + dst.asIntBuffer().put(src.asIntBuffer()); > + } Why does this not just do dst.put(src)? ::: widget/android/AndroidBridge.cpp @@ +2429,5 @@ > nsPresContext::CSSPixelsToAppUnits(srcW / scale), > nsPresContext::CSSPixelsToAppUnits(srcH / scale)); > > + PRUint32 stride = bufW * 2 /* 16 bpp */; > + PRUint32 bufferSize = bufH * stride; bufferSize is unused.

why not one more patch, huh?

Comment on attachment 628568 [details] [diff] [review] patch to use only one buffer and draw dirty rects, works with RTL Review of attachment 628568 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok except for comment below. I'm not r+'ing it until you address the remaining two comments on the last review (and which I also made on the original review). ::: mobile/android/base/FloatUtils.java @@ +20,5 @@ > > + public static boolean fuzzyEquals(RectF a, RectF b) { > + return fuzzyEquals(a.top, b.top) && fuzzyEquals(a.left, b.left) > + && fuzzyEquals(a.bottom, b.bottom) && fuzzyEquals(a.right, b.right); > + } There's already a function that does this in RectUtils. Use that instead.

Comment on attachment 628576 [details] [diff] [review] patch to chunk up large updates too Review of attachment 628576 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine

Comment on attachment 629244 [details] [diff] [review] patch to use only one buffer and draw dirty rects, works with RTL Review of attachment 629244 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Tab.java @@ +146,5 @@ > + if (mThumbnailBuffer != null && mThumbnailBuffer.capacity() == capacity) > + return mThumbnailBuffer; > + if (mThumbnailBuffer != null) > + GeckoAppShell.freeDirectBuffer(mThumbnailBuffer); // not calling freeBuffer() because it would deadlock > + return mThumbnailBuffer = GeckoAppShell.allocateDirectBuffer(capacity); Calling freeBuffer() here shouldn't deadlock. This function and freeBuffer have the same lock, and the thread executing here already has acquired the lock, so it should be able run freeBuffer() just fine.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c13c57813c https://hg.mozilla.org/integration/mozilla-inbound/rev/8acaca25f38d https://hg.mozilla.org/integration/mozilla-inbound/rev/89abfa0c0ae2 https://hg.mozilla.org/integration/mozilla-inbound/rev/f28c39eb038b https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1ff82a4780

Sorry, I had to back this out on inbound because of aborts in both XUL and Native Android: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b8fe3834b9 https://tbpl.mozilla.org/php/getParsedLog.php?id=12292452&tree=Mozilla-Inbound

https://hg.mozilla.org/mozilla-central/rev/a5c13c57813c https://hg.mozilla.org/mozilla-central/rev/8acaca25f38d https://hg.mozilla.org/mozilla-central/rev/89abfa0c0ae2 https://hg.mozilla.org/mozilla-central/rev/f28c39eb038b https://hg.mozilla.org/mozilla-central/rev/1a1ff82a4780 backed out: https://hg.mozilla.org/mozilla-central/rev/d7b8fe3834b9

why was it backed out? https://tbpl.mozilla.org/?rev=1a1ff82a4780 has green run for every test

That cset contains the backout as well. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1a1ff82a4780 has a lot of orange

ArrayDequeu was added in API level 9, this patch switches to a LinkedList. Also adds two checks to guard against divide by zero

relanded with follow up fix: https://hg.mozilla.org/mozilla-central/rev/12809c23e838 https://hg.mozilla.org/mozilla-central/rev/6cf625ab6acd https://hg.mozilla.org/mozilla-central/rev/de42c0dc4413 https://hg.mozilla.org/mozilla-central/rev/7fcb3f7d2b61 https://hg.mozilla.org/mozilla-central/rev/5ee7f5d52664 https://hg.mozilla.org/mozilla-central/rev/43e3d0333d30

Comment on attachment 624383 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Comment on attachment 624787 [details] [diff] [review] patch not to queue up multiple screenshots [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Comment on attachment 627390 [details] [diff] [review] patch to break up screenshots into smaller chunks [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Comment on attachment 628576 [details] [diff] [review] patch to chunk up large updates too [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Comment on attachment 629244 [details] [diff] [review] patch to use only one buffer and draw dirty rects, works with RTL [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Comment on attachment 629839 [details] [diff] [review] patch to fix on Froyo [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

Comment on attachment 629848 [details] [diff] [review] patch to fix XUL builds [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:

It looks like something in this patch set caused some new intermittent but very frequent failures in robocop, for example: https://tbpl.mozilla.org/php/getParsedLog.php?id=12363386&tree=Firefox 3 INFO TEST-UNEXPECTED-FAIL | testLoad | Pixel at 0, 0 - Color rgba(255,255,255,255) not close enough to expected rgb(0,0,0) https://tbpl.mozilla.org/php/getParsedLog.php?id=12362589&tree=Firefox 9 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Pixel at 100, 0 - Color rgba(0,8,8,255) not close enough to expected rgb(32,100,0) 3 INFO TEST-UNEXPECTED-FAIL | testAxisLocking | Pixel at 0, 0 - Color rgba(255,255,255,255) not close enough to expected rgb(0,0,0)

[Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: We spend too much time producing the screenshots such that it causes checkerboarding Testing completed (on m-c, etc.): just landed on m-c. I suggest it should stay on m-c until after we spin the next beta and then be uplifted to get a weeks worth of bake time before going the aurora channel. Risk to taking this patch (and alternatives if risky): Some amount of risk based on the pure volume of change here, which is why I'm suggesting some bake time. Alternative is to just not take the change. String or UUID changes made by this patch:

backed out https://hg.mozilla.org/mozilla-central/rev/3edf11eed119 due to random orange introduced and apparent panning perf regression on the droid pro

for the droid pro, it was chunking up the updates that caused the panning regression. I suspected it was because of the extra data copying and this change to copy less of the buffer seems to resolve the issue. I've pushed to try to see if it also resolves the random orange that mbrubeck reported.

that fixed the robocop mochitests, but not all others are orange https://tbpl.mozilla.org/?tree=Try&rev=f16a16b517fc

The oranges were from a java exception: 06-05 05:43:19.584 E/GeckoAppShell( 1872): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 9 ("GeckoBackgroundThread") 06-05 05:43:19.584 E/GeckoAppShell( 1872): java.lang.IllegalArgumentException 06-05 05:43:19.584 E/GeckoA recv'ing... response: ppShell( 1872): at java.nio.Buffer.limit(Buffer.java:223) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at org.mozilla.gecko.gfx.ScreenshotLayer$ScreenshotImage.copyBuffer(ScreenshotLayer.java:141) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at org.mozilla.gecko.gfx.ScreenshotLayer$ScreenshotImage.setBitmap(ScreenshotLayer.java:148) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at org.mozilla.gecko.gfx.ScreenshotLayer.se recv'ing... response: tBitmap(ScreenshotLayer.java:53) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at org.mozilla.gecko.gfx.LayerRenderer.setCheckerboardBitmap(LayerRenderer.java:138) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at org.mozilla.gecko.ScreenshotHandler$1.run(GeckoAppShell.java:2296) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at android.os.Handler.handleCallback(Handler.java:587) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at android.os.Handler.dispatchMessage(Handler.java:92) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at android.os.Looper.loop(Looper.java:123) 06-05 05:43:19.584 E/GeckoAppShell( 1872): at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31) 06-05 05:43:20.237 W/InputManagerService( 1020): Got RemoteException sending setActive(false) notification to pid 1872 uid 10033 which makes sense, for the last rect, the float math can overrun the buffer's limit. Clamping it to the limit of both the src and dst buffers (which should be eqaul) solves this issue. A try run with this change is completely green so far. https://tbpl.mozilla.org/?tree=Try&rev=114c16cb26ae

Comment on attachment 630063 [details] [diff] [review] patch to only copy the newly painted rect Assuming this one is obsolete now.

Comment on attachment 630165 [details] [diff] [review] patch to only copy the newly painted rect Review of attachment 630165 [details] [diff] [review]: ----------------------------------------------------------------- r- for XUL breakage. A couple of other comments as well. ::: mobile/android/base/GeckoAppShell.java @@ +527,5 @@ > > // Called by AndroidBridge using JNI > public static void notifyScreenShot(final ByteBuffer data, final int tabId, final int x, final int y, > + final int w, final int h, final int width, final int height, final int token) { > + ScreenshotHandler.notifyScreenShot(data, tabId, x, y, w, h, width, height, token); These variables need to be documented somewhere. Having both "w" and "width" is confusing. Also the XUL code needs to be updated so the signature matches. ::: mobile/android/base/gfx/ScreenshotLayer.java @@ +131,5 @@ > } > > + void copyBuffer(ByteBuffer src, ByteBuffer dst, int size, Rect rect, int stride) { > + int start = rect.left + rect.top * stride; > + int end = Math.min(Math.min(rect.right + rect.bottom * stride, src.capacity()), dst.limit()); size is never used in this function and doesn't make sense any more since it's calculated in the function itself. Best to take it out entirely.

Comment on attachment 630228 [details] [diff] [review] patch to only copy the newly painted rect Review of attachment 630228 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comment addressed. Note that fixing that might make the Math.min calls unnecessary, but I'm not sure. ::: mobile/android/base/gfx/ScreenshotLayer.java @@ +131,5 @@ > } > > + void copyBuffer(ByteBuffer src, ByteBuffer dst, Rect rect, int stride) { > + int start = rect.left + rect.top * stride; > + int end = Math.min(Math.min(rect.right + rect.bottom * stride, src.capacity()), dst.limit()); This should be rect.right + ((rect.bottom - 1) * stride), I think.

Triage comment: leaving on the list to get user feedback for lowend devices

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9a0d01d6ee https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ff9a5ce711 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2e226e5f1a https://hg.mozilla.org/integration/mozilla-inbound/rev/18ad05a0cc36 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0022b63efea https://hg.mozilla.org/integration/mozilla-inbound/rev/e70f09c6d8fa https://hg.mozilla.org/integration/mozilla-inbound/rev/6de8aa076a22 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8332f13f69

Same risk profile as above, its a big change. I'd like to get this on aurora soon and then we can make a call on beta

backed out for talos panning regression https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab8c859b19e

Comment on attachment 630317 [details] [diff] [review] patch for uplift Please re-nom once we've landed a followup fix on m-c.

Talked to Jeff about the Trobopan and he immediately identified the problem as doing a full texture upload for every chunked up region paint. This would be better solved by doing subtexture upload, but this can lead to even worse performance in the case where the software works around bad drivers by doing a readback followed by a full texture upload. This patch marks the last screenshot chunk with a special token and waits until it receives that chunk before doing the copy, invalidation and upload. This seems to have resolved the Trobopan regression according to try. Still waiting for Tchk2 results, but I don't see how they would be effected. https://tbpl.mozilla.org/?tree=Try&rev=488f7af70fb9

this is a rollup of all the previously reviewed patches, please the patch that's pending kats's review now.

Comment on attachment 632578 [details] [diff] [review] rollup patch for landing Review of attachment 632578 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the two comments addressed. ::: mobile/android/base/GeckoAppShell.java @@ +2326,5 @@ > + GeckoAppShell.getHandler().post(new Runnable() { > + public void run() { > + final Tab tab = Tabs.getInstance().getTab(tabId); > + if (tab == null) { > + if (token == GeckoAppShell.SCREENSHOT_CHECKERBOARD) { This check should also check for SCREENSHOT_CHECKERBOARD_AND_UPDATE @@ +2339,5 @@ > + switch (token) { > + case GeckoAppShell.SCREENSHOT_CHECKERBOARD: > + { > + synchronized (sAcumulatedRect) { > + sAcumulatedRect.union(left, top, right, bottom); There's an implicit assumption here that slices are always processed in sequential order, and are never interleaved with a different tab's slices. I think that assumption holds the way the code is currently structured (because scheduleCheckerboardScreenshotEvent always runs on the background thread regardless of which tab it's dealing with, and so the sPendingScreenshots queue is sequential), but this should be documented explicitly.

Comment on attachment 632569 [details] [diff] [review] patch to avoid texture upload on every region paint Review of attachment 632569 [details] [diff] [review]: ----------------------------------------------------------------- See my comments on the rollup patch. r=me with those comments addressed.

pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/7d21f92a5e05 this isn't as a big a win for checkerboarding as some of the earlier patches (which regressed panning), but it is a win none the less. Let's take any new work to other bugs as this one is already quite long and confusing.

https://hg.mozilla.org/mozilla-central/rev/7d21f92a5e05 https://hg.mozilla.org/mozilla-central/rev/12307c8d6028

Looks like you can have this, or you can have robocheck2, but not both. Prior to this landing on inbound, we had 7 green rck2s in a row. Starting with this, we had 12 reds in a row, and we've had a total of 8 green and 60 red since.

And https://tbpl.mozilla.org/?tree=Try&fromchange=163f9c9673fb&tochange=5c34d312d462 (tip of inbound as a control, tip of inbound with this backed out below) nicely illustrates the problem: slightly more than 50% green with this backed out, 10% green with this in.

Shuffling off some of these new issues onto bug 766643 instead.

(In reply to Phil Ringnalda (:philor) from comment #69) > And > https://tbpl.mozilla.org/ > ?tree=Try&fromchange=163f9c9673fb&tochange=5c34d312d462 (tip of inbound as a > control, tip of inbound with this backed out below) nicely illustrates the > problem: slightly more than 50% green with this backed out, 10% green with > this in. it appears that this is actually a problem with the test, see bug 756817

blassey, can you renom for aurora?