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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: write painted surface information to a file (1.0)

Comment 2

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

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.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: write painted surface information to a file (2.0)

updated with all feedback!

Comment 4

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

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

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35056a47d906

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/35056a47d906