Closed
Bug 766668
Opened 12 years ago
Closed 12 years ago
Screenshot buffer-copy assumes 0,0 is the top-left corner of the page
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox16 fixed)
RESOLVED
FIXED
Firefox 16
Tracking | Status | |
---|---|---|
firefox16 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(1 file, 1 obsolete file)
1.79 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•12 years ago
|
||
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.
Attachment #635003 -
Flags: review?(blassey.bugs)
Comment 2•12 years ago
|
||
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).
Assignee | ||
Comment 3•12 years ago
|
||
Yeah, that was the wrong place to put that fix. This one is better, I think.
Attachment #635003 -
Attachment is obsolete: true
Attachment #635003 -
Flags: review?(blassey.bugs)
Attachment #635503 -
Flags: review?(blassey.bugs)
Updated•12 years ago
|
Attachment #635503 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4571fdeaa65e
status-firefox16:
--- → fixed
Target Milestone: --- → Firefox 16
Comment 5•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4571fdeaa65e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•