Closed Bug 731637 Opened 10 years ago Closed 10 years ago

robocop on tegras hit OOM for PixelTest due to getPaintedSurface() array creation

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

(Whiteboard: [android][tegra][android_tier_1])

Attachments

(1 file, 1 obsolete file)

we create a 100x100 array of pixels for verification, but this causes an OOM error on the tegras.  We should erite this to the file and use java.nio.MappedByteBuffer to retrieve the data.
Blocks: 723667
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #603242 - Flags: review?(bugmail.mozilla)
Comment on attachment 603242 [details] [diff] [review]
write painted surface information to a file (1.0)

>diff -r 3f3308c7960c build/mobile/robocop/FennecNativeDriver.java.in
>+        String mapFile = "/mnt/sdcard/pixels.map";
>+
>+        // write row to file

This comment seems out of place

>+        FileOutputStream fos = null;
>+        DataOutputStream dos = null;
>+        try {
>+            fos = new FileOutputStream(mapFile);
>+            dos = new DataOutputStream(fos);
>+        } catch(IOException e) {

nit: space between "catch" and "("

>+            Log.e("Robocop", "exception initializing pixel writer on: " + mapFile);
>+        }
>+
>         for (int y = h - 1; y >= 0; y--) {
>             for (int x = 0; x < w; x++) {
>                 int agbr = pixelBuffer.get();
>-                pixels[y][x] = (agbr & 0xFF00FF00) | ((agbr >> 16) & 0x000000FF) | ((agbr << 16) & 0x00FF0000);
>+                try {
>+                    dos.writeInt((agbr & 0xFF00FF00) | ((agbr >> 16) & 0x000000FF) | ((agbr << 16) & 0x00FF0000));
>+                } catch(IOException e) {
>+                    Log.e("Robocop", "exception writing pixels to: " + mapFile);
>+                }
>             }
>         }
>-        return pixels;
>+        try {
>+            if (dos != null) {
>+                dos.flush();
>+                dos.close();
>+            }
>+        } catch (IOException ex) {
>+            ex.printStackTrace();
>+        }
>+        return new PaintedSurface(mapFile, w, h);

So rather than have three different try/catch blocks, I think you should just wrap pretty much all the code starting with "fos = new FileOutputStream" and ending with "return new PaintedSurface" in a single try block. If we get an IOException at any point during this function there's no guarantee of what state the file will be in, so we should just log an error and fail or return null.

The finally clause of the block should flush dos and close fos (not dos). According to the javadoc for DataOutputStream flushing it will also flush the underlying stream (i.e. fos) but it doesn't override close(), so doing dos.close() will not actually close fos and release the file handle.

>diff -r 3f3308c7960c build/mobile/robocop/PaintedSurface.java.in
>+#filter substitution
>+/* ***** BEGIN LICENSE BLOCK *****

Use the MPL 2.0 license in new code. See http://www.mozilla.org/MPL/headers/ for what to replace this license block with.

>+public class PaintedSurface {
>+    String mFileName = null;
>+    int mWidth = -1;
>+    int mHeight = -1;
>+    MappedByteBuffer mPixelBuffer = null;

These should be private.

>+    public final int getPixelAt(int y, int x) {

It would probably be better to change the parameter order to x, y. I used [y][x] with the array because that's the common convention for pixels in an array (so that reading rows doesn't cause a bajillion page faults), but for function calls x, y is the common convention. You'll have to change all the places that call this function and swap the parameter order.

>diff -r 3f3308c7960c mobile/android/base/tests/PixelTest.java.in
> 
>-    protected final int[][] loadAndPaint(String url) {
>+
>+    protected final PaintedSurface loadAndPaint(String url) {

Extra blank line not needed here.
Attachment #603242 - Flags: review?(bugmail.mozilla) → review-
updated with all feedback!
Attachment #603242 - Attachment is obsolete: true
Attachment #603356 - Flags: review?(bugmail.mozilla)
Comment on attachment 603356 [details] [diff] [review]
write painted surface information to a file (2.0)

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

Nice, r=me
Attachment #603356 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/35056a47d906
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.