Last Comment Bug 688333 - [Skia] Import skia source files and get them building on mac
: [Skia] Import skia source files and get them building on mac
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks: 687187 731384
  Show dependency treegraph
 
Reported: 2011-09-21 17:12 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2012-02-28 13:02 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Build system changes for Skia (11.88 KB, patch)
2011-09-21 17:26 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Skia import (bz2) (1.15 MB, application/x-bzip2)
2011-09-21 17:27 PDT, Matt Woodrow (:mattwoodrow)
no flags Details
Skia Import v2 (1.35 MB, patch)
2011-09-25 16:17 PDT, Matt Woodrow (:mattwoodrow)
jmuizelaar: review-
Details | Diff | Review
Build system changes for Skia v2 (12.58 KB, patch)
2011-09-25 16:18 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Skia update.sh script (3.91 KB, patch)
2011-10-17 17:49 PDT, Matt Woodrow (:mattwoodrow)
jmuizelaar: review+
Details | Diff | Review
Import of Skia files (1.34 MB, patch)
2011-10-17 17:50 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Skia Makefile (13.13 KB, patch)
2011-10-17 17:51 PDT, Matt Woodrow (:mattwoodrow)
khuey: review+
Details | Diff | Review
License Changes (3.95 KB, patch)
2011-10-26 21:09 PDT, Matt Woodrow (:mattwoodrow)
gerv: review+
Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) 2011-09-21 17:12:35 PDT
We should import Skia into our source tree and get it building so we can use it as an Azure backend.

Not sure who I should be getting to review this, and what else I need to do regarding license issues etc.

The cairo makefile change shouldn't be in here, but we fail to link on mac without (and fail to link everywhere else with it). For some reason this patch is stopping the cairo deflate code from being able to link with zlib. It's looking for the original symbols, and we only export prefixed ones (like _MOZ_Z_deflateInit) from our copy of zlib. I really have no idea how this patch could affect this :(
Comment 1 Matt Woodrow (:mattwoodrow) 2011-09-21 17:26:11 PDT
Created attachment 561624 [details] [diff] [review]
Build system changes for Skia
Comment 2 Matt Woodrow (:mattwoodrow) 2011-09-21 17:27:15 PDT
Created attachment 561625 [details]
Skia import (bz2)

This was too big to attach to the bugzilla as a patch file. This patch just imports all the files we need into gfx/skia.
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-09-23 10:47:49 PDT
So it seems unlikely that we'll be able to import Skia without patching it. I'd like the update process to be better than we have with cairo. The third-party libraries we have in media/ are good example of the way to do this. i.e the README_MOZILLA and update.sh scripts
Comment 4 Matt Woodrow (:mattwoodrow) 2011-09-25 16:17:34 PDT
Created attachment 562336 [details] [diff] [review]
Skia Import v2

Added update.sh/README_MOZILLA file for easier updating of skia source code
Comment 5 Matt Woodrow (:mattwoodrow) 2011-09-25 16:18:46 PDT
Created attachment 562337 [details] [diff] [review]
Build system changes for Skia v2

Added android build support
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-10-03 11:29:17 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Created attachment 562337 [details] [diff] [review] [diff] [details] [review]
> Build system changes for Skia v2
> 
> Added android build support

It seems like the shell script could do better than a long list of files?
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-10-03 11:33:57 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> > Created attachment 562337 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Build system changes for Skia v2
> > 
> > Added android build support
> 
> It seems like the shell script could do better than a long list of files?

Also, I wouldn't bother uploading the actual skia parts of the patch because I won't be reviewing them.
Comment 8 Matt Woodrow (:mattwoodrow) 2011-10-03 18:21:25 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> 
> It seems like the shell script could do better than a long list of files?

This is what the media/ script files were doing, what would you suggest instead?

Copy the whole folders?
Comment 9 Jeff Muizelaar [:jrmuizel] 2011-10-03 19:39:04 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> > 
> > It seems like the shell script could do better than a long list of files?
> 
> This is what the media/ script files were doing, what would you suggest
> instead?

I think the number of files in the media scripts is a lot more manageable and they likely need to copy some files and leave others.
 
> Copy the whole folders?

Doing that and using globs would do the job.
Comment 10 Matt Woodrow (:mattwoodrow) 2011-10-17 17:49:12 PDT
Created attachment 567638 [details] [diff] [review]
Skia update.sh script
Comment 11 Matt Woodrow (:mattwoodrow) 2011-10-17 17:50:26 PDT
Created attachment 567640 [details] [diff] [review]
Import of Skia files
Comment 12 Matt Woodrow (:mattwoodrow) 2011-10-17 17:51:40 PDT
Created attachment 567641 [details] [diff] [review]
Skia Makefile

Not sure if you want to review this Jeff, seems like a good start though.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-19 11:02:32 PDT
Comment on attachment 567641 [details] [diff] [review]
Skia Makefile

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

Looks fine, a few nits.

::: gfx/Makefile.in
@@ +55,5 @@
> +endif
> +
> +ifeq (android,$(MOZ_WIDGET_TOOLKIT))
> +DIRS        += skia
> +endif

Collapse these two conditions to

ifeq (,$(filter-out cocoa android,$(MOZ_WIDGET_TOOLKIT)))

::: gfx/skia/Makefile.in
@@ +48,5 @@
> +EXPORT_LIBRARY  = 1
> +
> +EXPORTS_NAMESPACES = skia
> +
> +DEFINES += -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0

Should these really all be hardcoded here?

@@ +52,5 @@
> +DEFINES += -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0
> +
> +LOCAL_INCLUDES += -I$(srcdir)/include/core -I$(srcdir)/include/config -I$(srcdir)/include/ports -I$(srcdir)/src/core -I$(srcdir)/include/images -I$(srcdir)/include/utils/mac -I$(srcdir)/include/views -I$(srcdir)/include/effects
> +
> +VPATH += $(srcdir)/src/core $(srcdir)/src/ports $(srcdir)/src/opts $(srcdir)/src/effects

Use multiple lines for these please.  e.g.

FOO = \
  BAR \
  BAZ \
  $(NULL)

@@ +55,5 @@
> +
> +VPATH += $(srcdir)/src/core $(srcdir)/src/ports $(srcdir)/src/opts $(srcdir)/src/effects
> +
> +EXPORTS_skia = \
> +	include/core/Sk64.h \

If you're VPATHing all of this above, you shouldn't need to say include/core on all of these, right?

@@ +60,5 @@
> +	include/core/SkAutoKern.h \
> +	include/core/SkBitmap.h \
> +	include/core/SkBlitRow.h \
> +	include/core/SkBlitter.h \
> +    include/core/SkBounder.h \

Consistent indentation please.

::: toolkit/library/libxul-config.mk
@@ +330,5 @@
>  	$(MOZ_APP_EXTRA_LIBS) \
>  	$(SQLITE_LIBS) \
>  	$(NULL)
>  
> +

Don't add this extra line.

@@ +365,5 @@
> +endif
> +
> +ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> +EXTRA_DSO_LDOPTS += $(MOZ_SKIA_LIBS)
> +endif

Same about the collapsed conditional.
Comment 14 Matt Woodrow (:mattwoodrow) 2011-10-20 18:54:34 PDT
Thanks Kyle!

Gerv: Is there anything I need to do wrt licenses etc before landing this?
Comment 15 Gervase Markham [:gerv] 2011-10-25 03:03:55 PDT
Skia is under the Apache License 2.0, according to its Wikipedia page. Is that correct?

We will need to update about:license to account for this. We don't have any other Apache-licensed code yet, but recently when we made the decision to move to MPL 2, that made it possible to take Apache-2-licensed code.

Once you have done the import, can you file a mozilla.org/Licensing bug on me to update about:licence?

Gerv
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-10-25 11:21:18 PDT
(In reply to Gervase Markham [:gerv] from comment #15)
> Skia is under the Apache License 2.0, according to its Wikipedia page. Is
> that correct?

Nope. It has recently been changed to a BSD style license. See:
http://code.google.com/p/skia/source/browse/trunk/LICENSE
Comment 17 Gervase Markham [:gerv] 2011-10-26 01:26:58 PDT
OK :-) Still, we need to update about:licence. If you could produce a patch for me to review (it should be obvious what to do - just make sure they are in alphabetical order) that would be awesome. 

Gerv
Comment 18 Matt Woodrow (:mattwoodrow) 2011-10-26 21:09:55 PDT
Created attachment 569899 [details] [diff] [review]
License Changes
Comment 19 Gervase Markham [:gerv] 2011-10-27 01:18:05 PDT
Comment on attachment 569899 [details] [diff] [review]
License Changes

Looks great, thanks :-)

Gerv
Comment 20 Matt Woodrow (:mattwoodrow) 2011-10-28 00:16:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/656a231abf23
Comment 23 neil@parkwaycc.co.uk 2012-01-29 03:49:20 PST
The bug summary says mac although my Linux gcc 4.3.2 build breaks because of trailing commas in various header files (core/SkAdvancedTypefaceMetrics.h, core/SkCanvas.h, core/SkDevice.h, core/SkFlattenable.h, core/SkMaskFilter.h, core/SkPaint.h, core/SkTypes.h, effects/SkLayerDrawLooper.h)...

Note You need to log in before you can comment on or make changes to this bug.