Closed Bug 688333 Opened 8 years ago Closed 8 years ago

[Skia] Import skia source files and get them building on mac

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(4 files, 4 obsolete files)

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 :(
Attached patch Build system changes for Skia (obsolete) — Splinter Review
Attached file Skia import (bz2) (obsolete) —
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.
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
Attached patch Skia Import v2 (obsolete) — Splinter Review
Added update.sh/README_MOZILLA file for easier updating of skia source code
Attachment #561625 - Attachment is obsolete: true
Attachment #562336 - Flags: review?(jmuizelaar)
Attached patch Build system changes for Skia v2 (obsolete) — Splinter Review
Added android build support
Attachment #561624 - Attachment is obsolete: true
Attachment #562337 - Flags: review?
(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?
(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.
(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?
(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.
Attachment #562336 - Flags: review?(jmuizelaar) → review-
Attachment #562336 - Attachment is obsolete: true
Attachment #562337 - Attachment is obsolete: true
Attachment #562337 - Flags: review?
Attachment #567638 - Flags: review?(jmuizelaar)
Attached patch Skia MakefileSplinter Review
Not sure if you want to review this Jeff, seems like a good start though.
Attachment #567641 - Flags: review?
Attachment #567638 - Flags: review?(jmuizelaar) → review+
Attachment #567641 - Flags: review? → review?(khuey)
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.
Attachment #567641 - Flags: review?(khuey) → review+
Thanks Kyle!

Gerv: Is there anything I need to do wrt licenses etc before landing this?
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
(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
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
Attached patch License ChangesSplinter Review
Attachment #569899 - Flags: review?(gerv)
Comment on attachment 569899 [details] [diff] [review]
License Changes

Looks great, thanks :-)

Gerv
Attachment #569899 - Flags: review?(gerv) → review+
Whiteboard: [inbound]
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)...
You need to log in before you can comment on or make changes to this bug.