create simple skia app for android using jni

create simple skia app for android using jni

Committed: http://code.google.com/p/skia/source/detail?r=9437

[djsollen, 2013-06-03 18:24:45]

move this code into platform_tools/android/ndk_demo_app.  Also don't include the
libskia_android.so in the CL, but rather put a README file with instructions on
how to build this using just the NDK tools.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/buildm...
File experimental/HelloSkia/buildme.sh (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/buildm...
experimental/HelloSkia/buildme.sh:1: NDK_DEBUG=1 ndk-build && ant debug && adb
install -r bin/HelloSkia-debug.apk
remove this file or clean it up and document what each step is supposed to do.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
File experimental/HelloSkia/jni/Android.mk (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:3: # libskia
add comments here about what this code snippet does.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:6: LOCAL_MODULE := skia_local
rename to skia_android

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:16: LOCAL_MODULE := HelloSkia
rename to hello_skia_ndk.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:21: LOCAL_LDLIBS := -ljnigraphics
comment on why this is needed.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:24:
/usr/local/google/home/zachr/src/skia_so/trunk/include/core
give the user instructions on where they are supposed to get these include
files. Should they copy them into their jni directory under
skia/includes/[core/config/...]?

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
File experimental/HelloSkia/jni/helloskia.cpp (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:16: JNIEXPORT void JNICALL
Java_com_example_HelloSkiaActivity_drawIntoBitmap(JNIEnv* env,
add a brief comment explaining what this method does.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:18: {
move to previous line

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:27: SkString path("skhello.png");
remove this unless you plan on drawing it.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:28: SkString text("Skia is the Best!");
define text where you use it not at the beginning of the function.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:31: SkPaint paint;
same as above (define it closer to where it is used).

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:44: // Draw something "interesting"
let's draw some more interesting stuff. You can probably get some ideas from
looking at our sample slides.  Also put a comment next to each interesting group
of drawing ops describing what is drawn.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:52: SkBitmap canvasBitmap;
you shouldn't need this at all.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:57: for (uint32_t row = 0; row <
dstInfo.height; row++)
this isn't very efficient. We should just have our canvas write directly into. 
We should just create the SkSurface using NewRasterDirect(...).

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/res/la...
File experimental/HelloSkia/res/layout/main.xml (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/res/la...
experimental/HelloSkia/res/layout/main.xml:10: android:text="Hello World,
HelloSkiaActivity"
why do you need a text view? Can't we draw it all through Skia?

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/src/co...
File experimental/HelloSkia/src/com/example/HelloSkiaActivity.java (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/src/co...
experimental/HelloSkia/src/com/example/HelloSkiaActivity.java:44: Bitmap
skiaBitmap = Bitmap.createBitmap(400, 400, Bitmap.Config.ARGB_8888);
make the bitmap the same size as the canvas you are given.

[Zach Reizner, 2013-06-03 22:54:25]

SkAutoGraphics causes a SkAssert to fail when used too many times, so it was
removed for the sake of the animation

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/buildm...
File experimental/HelloSkia/buildme.sh (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/buildm...
experimental/HelloSkia/buildme.sh:1: NDK_DEBUG=1 ndk-build && ant debug && adb
install -r bin/HelloSkia-debug.apk
On 2013/06/03 18:24:45, djsollen wrote:
> remove this file or clean it up and document what each step is supposed to do.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
File experimental/HelloSkia/jni/Android.mk (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:3: # libskia
On 2013/06/03 18:24:45, djsollen wrote:
> add comments here about what this code snippet does.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:6: LOCAL_MODULE := skia_local
On 2013/06/03 18:24:45, djsollen wrote:
> rename to skia_android

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:16: LOCAL_MODULE := HelloSkia
On 2013/06/03 18:24:45, djsollen wrote:
> rename to hello_skia_ndk.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/An...
experimental/HelloSkia/jni/Android.mk:21: LOCAL_LDLIBS := -ljnigraphics
On 2013/06/03 18:24:45, djsollen wrote:
> comment on why this is needed.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
File experimental/HelloSkia/jni/helloskia.cpp (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:16: JNIEXPORT void JNICALL
Java_com_example_HelloSkiaActivity_drawIntoBitmap(JNIEnv* env,
On 2013/06/03 18:24:45, djsollen wrote:
> add a brief comment explaining what this method does.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:18: {
On 2013/06/03 18:24:45, djsollen wrote:
> move to previous line 

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:26: SkAutoGraphics ag;
This was removed because repeated constructions/destructions cause SkAssert to
trip after a certain number of times

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:27: SkString path("skhello.png");
On 2013/06/03 18:24:45, djsollen wrote:
> remove this unless you plan on drawing it.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:28: SkString text("Skia is the Best!");
On 2013/06/03 18:24:45, djsollen wrote:
> define text where you use it not at the beginning of the function.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:31: SkPaint paint;
On 2013/06/03 18:24:45, djsollen wrote:
> same as above (define it closer to where it is used).

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:44: // Draw something "interesting"
On 2013/06/03 18:24:45, djsollen wrote:
> let's draw some more interesting stuff. You can probably get some ideas from
> looking at our sample slides.  Also put a comment next to each interesting
group
> of drawing ops describing what is drawn.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:52: SkBitmap canvasBitmap;
On 2013/06/03 18:24:45, djsollen wrote:
> you shouldn't need this at all.

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/jni/he...
experimental/HelloSkia/jni/helloskia.cpp:57: for (uint32_t row = 0; row <
dstInfo.height; row++)
On 2013/06/03 18:24:45, djsollen wrote:
> this isn't very efficient. We should just have our canvas write directly into.

> We should just create the SkSurface using NewRasterDirect(...).

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/res/la...
File experimental/HelloSkia/res/layout/main.xml (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/res/la...
experimental/HelloSkia/res/layout/main.xml:10: android:text="Hello World,
HelloSkiaActivity"
On 2013/06/03 18:24:45, djsollen wrote:
> why do you need a text view? Can't we draw it all through Skia?

Done.

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/src/co...
File experimental/HelloSkia/src/com/example/HelloSkiaActivity.java (right):

https://codereview.chromium.org/16336004/diff/1/experimental/HelloSkia/src/co...
experimental/HelloSkia/src/com/example/HelloSkiaActivity.java:44: Bitmap
skiaBitmap = Bitmap.createBitmap(400, 400, Bitmap.Config.ARGB_8888);
On 2013/06/03 18:24:45, djsollen wrote:
> make the bitmap the same size as the canvas you are given.

Done.

[djsollen, 2013-06-04 00:26:34]

It is getting pretty close.  I would add a README file to this directory that
has instructions for...

1) how to build the skia lib and copy the necessary files into thei jni folder
(e.g. run android_make -d ...)

2) how to run ndk-build

3) how to build the app

4) how to deploy the app

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
File experimental/HelloSkia/jni/Android.mk (right):

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
experimental/HelloSkia/jni/Android.mk:3: # set this to wherever your skia
directory is located, with out trailing slashes
I think that for clarity we should have them copy the headers into
jni/skia/include.  That way if they update their skia tree it is clear that they
would also need to update these files.

Additionally, have them put the shared library in jni/skia/ as well.

Now this file won't be dependent upon any variables being set, just that they
did the work ahead of time to put content into the correct directories.

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
File experimental/HelloSkia/jni/helloskia.cpp (right):

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
experimental/HelloSkia/jni/helloskia.cpp:13: static float get_seconds()
doesn't look like you call this function

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
experimental/HelloSkia/jni/helloskia.cpp:44: 
keep the basic text drawing as well, but I like your animation idea! That is
interesting. :)

[Zach Reizner, 2013-06-04 14:35:45]

In the README I reference the Android quick start guide in the online
documentation to avoid duplicating the build instructions. Let me know if this
satisfactory.

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
File experimental/HelloSkia/jni/helloskia.cpp (right):

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
experimental/HelloSkia/jni/helloskia.cpp:13: static float get_seconds()
On 2013/06/04 00:26:35, djsollen wrote:
> doesn't look like you call this function

Done.

https://codereview.chromium.org/16336004/diff/9001/experimental/HelloSkia/jni...
experimental/HelloSkia/jni/helloskia.cpp:44: 
On 2013/06/04 00:26:35, djsollen wrote:
> keep the basic text drawing as well, but I like your animation idea! That is
> interesting. :)

Done.

[djsollen, 2013-06-04 19:08:29]

the code looks good to me.  The only thing that needs to be done is to move this
out of experimental and into platform_tools/android/ndk_demo_app

[Zach Reizner, 2013-06-04 19:21:23]

On 2013/06/04 19:08:29, djsollen wrote:
> the code looks good to me.  The only thing that needs to be done is to move
this
> out of experimental and into platform_tools/android/ndk_demo_app

should we not call it into platform_tools/android/hello_skia_ndk because that is
what we call the module in Android.mk

[djsollen, 2013-06-04 19:30:29]

how about we create a new folder called examples/ and put it in there under
hello_skia_app

[djsollen, 2013-06-04 20:17:20]

this on is my fault, sorry.  I intended it to be
platform_tools/android/examples.

[djsollen, 2013-06-04 20:50:21]

lgtm

[djsollen, 2013-06-04 20:50:56]

now that you have an lgtm you can click the commit button and it should go the
the skia commit queue (CQ).

[commit-bot: I haz the power, 2013-06-04 20:54:15]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zachr@google.com/16336004/32001

[commit-bot: I haz the power, 2013-06-04 20:54:19]

Can't process patch for file
platform_tools/android/examples/hello_skia_app/res/drawable-hdpi/ic_launcher.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[commit-bot: I haz the power, 2013-06-04 20:54:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zachr@google.com/16336004/32001

[commit-bot: I haz the power, 2013-06-04 20:54:42]

Can't process patch for file
platform_tools/android/examples/hello_skia_app/res/drawable-hdpi/ic_launcher.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[djsollen, 2013-06-04 21:05:46]

lgtm with the removed images.  I'll upload them in the morning.

[commit-bot: I haz the power, 2013-06-04 21:06:16]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zachr@google.com/16336004/36002

[commit-bot: I haz the power, 2013-06-04 21:13:33]

Change committed as 9437