Last Comment Bug 766668 - Screenshot buffer-copy assumes 0,0 is the top-left corner of the page
: Screenshot buffer-copy assumes 0,0 is the top-left corner of the page
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 16
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks: 766643
  Show dependency treegraph
 
Reported: 2012-06-20 12:09 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-06-23 05:47 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Take the screenshot rect relative to the page rect (5.59 KB, patch)
2012-06-20 12:15 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Take the screenshot rect relative to the page rect (v2) (1.79 KB, patch)
2012-06-21 15:26 PDT, Kartikaya Gupta (email:kats@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-06-20 12:09:06 PDT
The buffer-copy code in ScreenshotLayer's copyBuffer function assumes that the page has a top-left corner of 0,0, which is not necessarily the case (specifically for RTL pages). This causes the buffer-copy to underflow in some cases (e.g. screenshotting a RTL page). Here is some logcat output from debugging printfs I inserted:

06-20 18:56:00.556 D/staktrace(32610): Calling setbitmap with 2097152 bytes of data; 512x2048 and rect Rect(-376, 0 - 136, 2025)
06-20 18:56:00.556 W/dalvikvm(32610): threadid=11: thread exiting with uncaught exception (group=0x40a671f8)
06-20 18:56:00.564 E/GeckoAppShell(32610): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 7908 ("GeckoBackgroundThread")
06-20 18:56:00.564 E/GeckoAppShell(32610): java.lang.IllegalArgumentException: Bad position (limit 2097152): -376
06-20 18:56:00.564 E/GeckoAppShell(32610):  at java.nio.Buffer.positionImpl(Buffer.java:364)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at java.nio.Buffer.position(Buffer.java:358)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at org.mozilla.gecko.gfx.ScreenshotLayer$ScreenshotImage.copyBuffer(ScreenshotLayer.java:136)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at org.mozilla.gecko.gfx.ScreenshotLayer$ScreenshotImage.setBitmap(ScreenshotLayer.java:145)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at org.mozilla.gecko.gfx.ScreenshotLayer.setBitmap(ScreenshotLayer.java:53)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at org.mozilla.gecko.gfx.LayerRenderer.setCheckerboardBitmap(LayerRenderer.java:138)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at org.mozilla.gecko.ScreenshotHandler$1.run(GeckoAppShell.java:2440)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at android.os.Handler.handleCallback(Handler.java:605)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at android.os.Handler.dispatchMessage(Handler.java:92)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at android.os.Looper.loop(Looper.java:137)
06-20 18:56:00.564 E/GeckoAppShell(32610):  at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-20 12:15:08 PDT
Created attachment 635003 [details] [diff] [review]
Take the screenshot rect relative to the page rect

There's probably a better way to do this farther up the chain. This is mostly a WIP so I not crash and keep finding other issues.
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-06-21 03:46:54 PDT
Comment on attachment 635003 [details] [diff] [review]
Take the screenshot rect relative to the page rect

Review of attachment 635003 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +133,5 @@
> +        void copyBuffer(ByteBuffer src, ByteBuffer dst, RectF pageRect, Rect rect, int stride) {
> +            // when converting pageRect's top/left coordinates from floats to integers, we do a
> +            // (int) truncation in order to be consistent with the code in ScreenshotHandler.screenshotWholePage
> +            int start = ((rect.top - (int)pageRect.top) * stride) + (rect.left - (int)pageRect.left);
> +            int end = ((rect.bottom - 1 - (int)pageRect.top) * stride) + (rect.right - (int)pageRect.left);

does this work? My understanding is that pageRect is in page coordinates and rect is in buffer coordinates (so [sx, sy, sw, sh] vs. [dx, dy, dw, dh] in terms of TakeScreenshot).
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-21 15:26:55 PDT
Created attachment 635503 [details] [diff] [review]
Take the screenshot rect relative to the page rect (v2)

Yeah, that was the wrong place to put that fix. This one is better, I think.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-22 06:33:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4571fdeaa65e
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:47:37 PDT
https://hg.mozilla.org/mozilla-central/rev/4571fdeaa65e

Note You need to log in before you can comment on or make changes to this bug.