Adding support to trunk for building Skia using the Android NDK.

Adding support to trunk for building Skia using the Android NDK.

This CL depends on a subsequent CL to add the appropriate NDK
toolchain and system sources to the skia repo.

Review URL: http://codereview.appspot.com/5306089/
Committed: http://code.google.com/p/skia/source/detail?r=2592

[bsalomon, 2011-11-02 17:39:52]

The parts I understand LGTM.

http://codereview.appspot.com/5306089/diff/1/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/gpu.gyp#newcode336
gyp/gpu.gyp:336: '-DGL_GLEXT_PROTOTYPES',
Maybe these should just be defined in the file that pulls Android's GL/EGL
headers. If that works then people who aren't using our gyp file won't have
missing extension declarations.

[epoger, 2011-11-02 18:32:00]

http://codereview.appspot.com/5306089/diff/1/bench/BenchTimer.cpp
File bench/BenchTimer.cpp (right):

http://codereview.appspot.com/5306089/diff/1/bench/BenchTimer.cpp#newcode13
bench/BenchTimer.cpp:13: #elif defined(SK_BUILD_FOR_UNIX) ||
defined(SK_BUILD_FOR_ANDROID)
So I guess you need to grep through our code base to look for any other #if
SK_BUILD_FOR_UNIX and make a change similar to this one?

Would it perhaps make sense to make SK_BUILD_FOR_ANDROID a subset of
SK_BUILD_FOR_UNIX?  (if SK_BUILD_FOR_ANDROID is defined, then you are guaranteed
that SK_BUILD_FOR_UNIX is defined also)

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp
File gyp/SampleApp.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode262
gyp/SampleApp.gyp:262: 'type': 'shared_library',
Is the type definition specific to SampleApp, or could this be moved out into
common_conditions.gypi?

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode276
gyp/SampleApp.gyp:276: '../include/utils',
Are you sure you need to add ../include/utils?   What is android-specific about
this need?

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode278
gyp/SampleApp.gyp:278: 'cflags': [
Is the cflags definition specific to SampleApp, or could this be moved out into
common_conditions.gypi?

http://codereview.appspot.com/5306089/diff/1/gyp/android_system.gyp
File gyp/android_system.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/android_system.gyp#newcode6
gyp/android_system.gyp:6: # toolchain and they are not expected to be present on
the host OS.
As I recall, you originally tried putting this android-specific gyp file out in
the ../../android_tools directory, but that caused gyp to build a bogus
makefile.  Right?

If so, please add a comment to that effect here.

P.S. Have you tried working around that gyp problem by making this a thin shell
that just includes ../../android_tools/blah.gypi ?

http://codereview.appspot.com/5306089/diff/1/gyp/bench.gypi
File gyp/bench.gypi (right):

http://codereview.appspot.com/5306089/diff/1/gyp/bench.gypi#newcode46
gyp/bench.gypi:46: 'conditions': [
I think you've inserted this wrong.  As it stands, this conditional will only be
evaluated if 'skia_os not in ["linux", "freebsd", "openbsd", "solaris",
"android"]' is false.

Better to copy the 'skia_os != "win"' stanza below, and use it as a template.

http://codereview.appspot.com/5306089/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

http://codereview.appspot.com/5306089/diff/1/gyp/common_conditions.gypi#newco...
gyp/common_conditions.gypi:155: '-lstdc++',
Does Linux not need these same libraries?  (Maybe some of this should be shared
with Linux?)

http://codereview.appspot.com/5306089/diff/1/include/core/SkPreConfig.h
File include/core/SkPreConfig.h (right):

http://codereview.appspot.com/5306089/diff/1/include/core/SkPreConfig.h#newco...
include/core/SkPreConfig.h:47: /* Even if the user only defined the NDK variant
we still need to build
What's the difference between SK_BUILD_FOR_ANDROID and SK_BUILD_FOR_ANDROID_NDK
?  (You can just tell me, if it's not worth augmenting this comment.)

http://codereview.appspot.com/5306089/diff/1/src/ports/FontHostConfiguration_...
File src/ports/FontHostConfiguration_android.h (right):

http://codereview.appspot.com/5306089/diff/1/src/ports/FontHostConfiguration_...
src/ports/FontHostConfiguration_android.h:23: * The FontFamily data structure is
created during parsing and handed back to Skia to fold
>80 character lines showing up ugly in code review tool

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_FreeType.cpp
File src/ports/SkFontHost_FreeType.cpp (right):

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_FreeType.cp...
src/ports/SkFontHost_FreeType.cpp:325: #if
defined(FT_FSTYPE_RESTRICTED_LICENSE_EMBEDDING) &&
!defined(SK_BUILD_FOR_ANDROID_NDK)
Please add a comment stating why this flavor is appropriate for
SK_BUILD_FOR_ANDROID_NDK (but not SK_BUILD_FOR_ANDROID)...

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp
File src/ports/SkFontHost_android.cpp (right):

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:5: ** Licensed under the Apache License,
Version 2.0 (the "License");
So this isn't covered under the same terms in the LICENSE file?

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:160: //        SkDebugf("remove_from_family(%p)
face not found", face);
is there a reason not to leave in the SkDebugf()?

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:418: /*  Load info from a configuration file
that populates the system/fallback font structures
>80 char lines ugly in review tool

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:421: //   
load_font_info_xml("/system/etc/system_fonts.xml");
Please remove this call, or don't, or explain why it's sitting there commented
out. :-)

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:605: //      SkDebugf("--- fonthost custom
deserialize %d %d\n", face->style(), len);
why not leave the SkDebugf()'s in place?

[DerekS, 2011-11-02 19:19:14]

http://codereview.appspot.com/5306089/diff/1/bench/BenchTimer.cpp
File bench/BenchTimer.cpp (right):

http://codereview.appspot.com/5306089/diff/1/bench/BenchTimer.cpp#newcode13
bench/BenchTimer.cpp:13: #elif defined(SK_BUILD_FOR_UNIX) ||
defined(SK_BUILD_FOR_ANDROID)
Not necessarily, there may be some cases were we need to add the Android check
in addition to the Unix check but not everywhere.  For instance we handle
SkWindow.h, SkEvent.h, and SkNativeGLContext.h differently on Linux than on
Android.  It may be easier to just special case those definitions with...

#if defined(SK_BUILD_FOR_UNIX) && !defined(SK_BUILD_FOR_ANDROID)

What do you think?


On 2011/11/02 18:32:00, epoger wrote:
> So I guess you need to grep through our code base to look for any other #if
> SK_BUILD_FOR_UNIX and make a change similar to this one?
> 
> Would it perhaps make sense to make SK_BUILD_FOR_ANDROID a subset of
> SK_BUILD_FOR_UNIX?  (if SK_BUILD_FOR_ANDROID is defined, then you are
guaranteed
> that SK_BUILD_FOR_UNIX is defined also)

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp
File gyp/SampleApp.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode262
gyp/SampleApp.gyp:262: 'type': 'shared_library',
This is specific to the Android sample app.  I'm planning on moving the
/trunk/android_sample to /android/sampleApp which will house the Java code
needed to run the Android SampleApp.  That java code will just load this
sharedLib and use JNI to run the rest.

On 2011/11/02 18:32:00, epoger wrote:
> Is the type definition specific to SampleApp, or could this be moved out into
> common_conditions.gypi?

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode276
gyp/SampleApp.gyp:276: '../include/utils',
It is only needed by SampleApp_android.cpp so this seemed like the right place
to put it.  I can move it to the standard include directory since that won't
hurt anything. 

On 2011/11/02 18:32:00, epoger wrote:
> Are you sure you need to add ../include/utils?   What is android-specific
about
> this need?

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode278
gyp/SampleApp.gyp:278: 'cflags': [
I don't think this is necessary so I can remove it for now.

On 2011/11/02 18:32:00, epoger wrote:
> Is the cflags definition specific to SampleApp, or could this be moved out
into
> common_conditions.gypi?

http://codereview.appspot.com/5306089/diff/1/gyp/android_system.gyp
File gyp/android_system.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/android_system.gyp#newcode6
gyp/android_system.gyp:6: # toolchain and they are not expected to be present on
the host OS.
On 2011/11/02 18:32:00, epoger wrote:
> As I recall, you originally tried putting this android-specific gyp file out
in
> the ../../android_tools directory, but that caused gyp to build a bogus
> makefile.  Right?
> 
> If so, please add a comment to that effect here.
> 
> P.S. Have you tried working around that gyp problem by making this a thin
shell
> that just includes ../../android_tools/blah.gypi ?

Done.

http://codereview.appspot.com/5306089/diff/1/gyp/bench.gypi
File gyp/bench.gypi (right):

http://codereview.appspot.com/5306089/diff/1/gyp/bench.gypi#newcode46
gyp/bench.gypi:46: 'conditions': [
Actually, this is in the "else" portion of that condition. I agree that it's
confusing so I'll rewrite this rule as a separate one.

On 2011/11/02 18:32:00, epoger wrote:
> I think you've inserted this wrong.  As it stands, this conditional will only
be
> evaluated if 'skia_os not in ["linux", "freebsd", "openbsd", "solaris",
> "android"]' is false.
> 
> Better to copy the 'skia_os != "win"' stanza below, and use it as a template.

http://codereview.appspot.com/5306089/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

http://codereview.appspot.com/5306089/diff/1/gyp/common_conditions.gypi#newco...
gyp/common_conditions.gypi:155: '-lstdc++',
These are implied on linux, but must be explicitly called out on Android.

On 2011/11/02 18:32:00, epoger wrote:
> Does Linux not need these same libraries?  (Maybe some of this should be
shared
> with Linux?)

http://codereview.appspot.com/5306089/diff/1/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/gpu.gyp#newcode336
gyp/gpu.gyp:336: '-DGL_GLEXT_PROTOTYPES',
Actually, I found that they weren't required in the NDK so I removed them.

On 2011/11/02 17:39:53, bsalomon wrote:
> Maybe these should just be defined in the file that pulls Android's GL/EGL
> headers. If that works then people who aren't using our gyp file won't have
> missing extension declarations.

http://codereview.appspot.com/5306089/diff/1/include/core/SkPreConfig.h
File include/core/SkPreConfig.h (right):

http://codereview.appspot.com/5306089/diff/1/include/core/SkPreConfig.h#newco...
include/core/SkPreConfig.h:47: /* Even if the user only defined the NDK variant
we still need to build
The non-ndk one relies on internals known only when built as part of the OS to
gain extra speed. The goal is to get those benefits using the NDK build so that
all users of Skia on Android get these benefits.

On 2011/11/02 18:32:00, epoger wrote:
> What's the difference between SK_BUILD_FOR_ANDROID and
SK_BUILD_FOR_ANDROID_NDK
> ?  (You can just tell me, if it's not worth augmenting this comment.)

http://codereview.appspot.com/5306089/diff/1/src/ports/FontHostConfiguration_...
File src/ports/FontHostConfiguration_android.h (right):

http://codereview.appspot.com/5306089/diff/1/src/ports/FontHostConfiguration_...
src/ports/FontHostConfiguration_android.h:23: * The FontFamily data structure is
created during parsing and handed back to Skia to fold
Unfortunately, this is pulled directly from the ICS Android repository.  I'll
update it to follow our rules and it should be picked up when we do the next
merge into the android tree.

On 2011/11/02 18:32:00, epoger wrote:
> >80 character lines showing up ugly in code review tool

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp
File src/ports/SkFontHost_android.cpp (right):

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:5: ** Licensed under the Apache License,
Version 2.0 (the "License");
This is pulled directly from the Android tree so I'm not sure what license we
should be using.

On 2011/11/02 18:32:00, epoger wrote:
> So this isn't covered under the same terms in the LICENSE file?

http://codereview.appspot.com/5306089/diff/1/src/ports/SkFontHost_android.cpp...
src/ports/SkFontHost_android.cpp:160: //        SkDebugf("remove_from_family(%p)
face not found", face);
Again this is a straight copy from Android.  I'll look into cleaning it up more.

On 2011/11/02 18:32:00, epoger wrote:
> is there a reason not to leave in the SkDebugf()?

[epoger, 2011-11-02 21:26:25]

LGTM

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp
File gyp/SampleApp.gyp (right):

http://codereview.appspot.com/5306089/diff/1/gyp/SampleApp.gyp#newcode276
gyp/SampleApp.gyp:276: '../include/utils',
On 2011/11/02 19:19:14, djsollen wrote:
> It is only needed by SampleApp_android.cpp so this seemed like the right place
> to put it.  I can move it to the standard include directory since that won't
> hurt anything. 
> 
> On 2011/11/02 18:32:00, epoger wrote:
> > Are you sure you need to add ../include/utils?   What is android-specific
> about
> > this need?
> 

Please try removing it (if you didn't already); I suspect it is already provided
for you by line 150 of utils.gyp since this target depends on utils.gyp.

OK with me for you to commit with or without this, though.