755070 - Scrolling causes after paint notifications which causes screenshotting which causes checkerboarding

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 2

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

Attachment: patch

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

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(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?

Comment 4

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

Attachment: patch not to queue up multiple screenshots

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.

Comment 5

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

(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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(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?

Comment 7

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

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 11

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

Attachment: patch not to queue up multiple screenshots

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

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(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 13

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

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

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 18

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

Attachment: patch to use only one buffer and draw dirty rects

Comment 19

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

Attachment: patch to break up screenshots into smaller chunks

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

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 22

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 23

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

> @@ +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 24

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

Attachment: patch to use only one buffer and draw dirty rects

Comment 25

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 26

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

Attachment: patch to use only one buffer and draw dirty rects, works with RTL

Comment 27

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

Attachment: patch to chunk up large updates too

why not one more patch, huh?

Comment 28

[Kartikaya Gupta (email:[e-mail])]

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 29

[Kartikaya Gupta (email:[e-mail])]

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 30

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

Attachment: patch to use only one buffer and draw dirty rects, works with RTL

Comment 31

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 32

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

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

Comment 33

[Matt Brubeck (:mbrubeck)]

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

Comment 34

[(no longer active)]

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

Comment 35

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

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

Comment 36

[Kartikaya Gupta (email:[e-mail])]

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

Comment 37

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

Attachment: patch to fix on Froyo

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

Comment 38

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

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 39

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

Attachment: patch to fix XUL builds

Comment 40

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

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 41

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

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 42

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

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 43

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

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 44

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

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 45

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

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 46

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

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:

Comment 47

[Matt Brubeck (:mbrubeck)]

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)

Comment 48

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

[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:

Comment 49

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

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

Comment 50

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

Attachment: patch to only copy the newly painted rect

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.

Comment 51

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

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

Comment 52

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

Attachment: patch to only copy the newly painted rect

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 53

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 630063 [details] [diff] [review]
patch to only copy the newly painted rect

Assuming this one is obsolete now.

Comment 54

[Kartikaya Gupta (email:[e-mail])]

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 55

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

Attachment: patch to only copy the newly painted rect

Comment 56

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 57

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

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

Comment 58

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

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

Comment 59

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

Attachment: patch for uplift

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

Comment 60

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

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

Comment 61

[Alex Keybl [:akeybl]]

Comment on attachment 630317 [details] [diff] [review]
patch for uplift

Please re-nom once we've landed a followup fix on m-c.

Comment 62

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

Attachment: patch to avoid texture upload on every region paint

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

Comment 63

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

Attachment: rollup patch for landing

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

Comment 64

[Kartikaya Gupta (email:[e-mail])]

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 65

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 66

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

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.

Comment 67

[Ed Morley [:emorley]]

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

Comment 68

[Phil Ringnalda (:philor)]

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.

Comment 69

[Phil Ringnalda (:philor)]

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.

Comment 70

[Kartikaya Gupta (email:[e-mail])]

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

Comment 71

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

(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

Comment 72

[Joe Drew (not getting mail)]

blassey, can you renom for aurora?

Comment 73

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1956208c7b4a

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1956208c7b4a