Make static global initialization (flattening only) optional

Add a compile time option to choose between static global initializers used by flattening functions, and those same functions called once at flattening time.

Add a dummy SkEffects_none.cpp so that tools can build Skia without including effects. 

Add a first cut at the SkGlobalInitialization port that Chromium might use.

Add some missing flattening to ColorFilters and others.

[epoger, 2011-12-12 21:36:50]

clever...

http://codereview.appspot.com/5487050/diff/1001/gyp/ports.gyp
File gyp/ports.gyp (right):

http://codereview.appspot.com/5487050/diff/1001/gyp/ports.gyp#newcode51
gyp/ports.gyp:51: #            '../src/ports/SkFontHost_FreeType.cpp',
what's with the  commented- out lines here?

[caryclark1, 2011-12-12 21:58:03]

http://codereview.appspot.com/5487050/diff/1001/gyp/ports.gyp
File gyp/ports.gyp (right):

http://codereview.appspot.com/5487050/diff/1001/gyp/ports.gyp#newcode51
gyp/ports.gyp:51: #            '../src/ports/SkFontHost_FreeType.cpp',
These lines were copied from core.gyp. I assume that some of the commented out
lines (e.g., freetype) may be enabled as options for the Mac in the future.

On 2011/12/12 21:36:50, epoger wrote:
> what's with the  commented- out lines here?

[caryclark1, 2011-12-13 15:53:14]

Here's what's required for this change to make a difference in Chromium:

--- skia/skia.gyp	(working copy)
*** 327,328 ****
--- 327,329 ----
          '../third_party/skia/src/effects/SkDiscretePathEffect.cpp',
+         '../third_party/skia/src/effects/SkEffects.cpp',
          '../third_party/skia/src/effects/SkEmbossMask.cpp',
*** 482,483 ****
--- 483,486 ----
          '../third_party/skia/src/ports/SkFontHost_win.cpp',
+         '../third_party/skia/src/ports/SkGlobalInitialization_chromium.cpp',
+         #'../third_party/skia/src/ports/SkGlobalInitialization_default.cpp',
          #'../third_party/skia/src/ports/SkImageDecoder_CG.cpp',
*** 602,603 ****
--- 605,607 ----
          '../third_party/skia/include/effects/SkDrawExtraPathEffect.h',
+         '../third_party/skia/include/effects/SkEffects.h',
          '../third_party/skia/include/effects/SkEmbossMaskFilter.h',
*** 872,873 ****
--- 876,878 ----
                  'SK_USE_MAC_CORE_TEXT',
+                 'SK_ALLOW_STATIC_GLOBAL_INITIALIZERS=0',

With SK_ALLOW_STATIC_GLOBAL_INITIALIZERS unset or set to 1,
dump-static-initializers.py reports:

./third_party/skia/include/core/SkThread.h 00000000023d5890
third_party/skia/include/core/SkThread.h 00000000023d5430
..
third_party/skia/include/core/SkFlattenable.h 00000000023d54d0
third_party/skia/src/core/SkBitmapProcShader.h 00000000023d53d0
third_party/skia/include/core/SkMallocPixelRef.h 00000000023d5400
third_party/skia/src/core/SkTypefaceCache.h 00000000023d5480
third_party/skia/src/effects/SkGradientShader.cpp 00000000023d5710
third_party/skia/src/effects/SkDashPathEffect.cpp 00000000023d56e0
third_party/skia/src/gpu/GrGpu.cpp 00000000023d57c0
third_party/skia/src/gpu/SkGrTexturePixelRef.cpp 00000000023d5840
..
third_party/skia/src/effects/SkCornerPathEffect.cpp 00000000023d56b0
third_party/skia/src/effects/SkLayerDrawLooper.cpp 00000000023d5790
third_party/skia/src/effects/SkBlurMaskFilter.cpp 00000000023d5590
third_party/skia/src/effects/SkColorFilters.cpp 00000000023d55c0
third_party/skia/include/effects/SkBlurImageFilter.h 00000000023d5560

(non-Skia globals omitted)

With SK_ALLOW_STATIC_GLOBAL_INITIALIZERS=0, dump-static-initializers.py reports:

./third_party/skia/include/core/SkThread.h 00000000023d5940
third_party/skia/include/core/SkThread.h 00000000023d57d0
..
third_party/skia/src/core/SkTypefaceCache.h 00000000023d5820
third_party/skia/src/gpu/GrGpu.cpp 00000000023d5870
third_party/skia/src/gpu/SkGrTexturePixelRef.cpp 00000000023d58f0

[caryclark1, 2011-12-13 16:38:45]

I was mistaken: The Chromium build does not require SkEffects.cpp / SkEffects.h
-- please ignore those changes in skia.gyp above.

[reed1, 2011-12-14 15:36:29]

1. can we checkin anything unrelated to global initialization, like the ports
gyp? Just trying to make the size more manageable if possible.

2. I like the macroization of the Registrar code. I have a little more trouble
with the automatic calling of it. Explicitly calling a bunch of Init() calls is
not thread-safe (it was when it was a global initializer). Can we stage this
change by first just having a compile flag that builds skia with Init()
functions instead of globals?  That alone should be testable locally by skia (w/
and w/o the ALLOW_GLOBAL compile flag).

Perhaps we'll end up writing lazy, thread-safe, automatic ways to call the
inits, or perhaps we'll put that onus on the client (e.g. Chrome). We may not
know where to make that call yet, but if Chrome wants this feature, its seems
reasonable that they help us find the safe place to do this sort of
initialization... besides, for the moment they actually don't need to init their
flattenables at all, since they don't use that feature yet.

[caryclark1, 2011-12-14 21:54:44]

On 2011/12/14 15:36:29, reed1 wrote:
> 1. can we checkin anything unrelated to global initialization, like the ports
> gyp? Just trying to make the size more manageable if possible.
Done.

> 2. I like the macroization of the Registrar code. I have a little more trouble
> with the automatic calling of it. Explicitly calling a bunch of Init() calls
is
> not thread-safe (it was when it was a global initializer).
True, didn't consider that.

 Can we stage this
> change by first just having a compile flag that builds skia with Init()
> functions instead of globals? 
I don't understand. What would that look like?

> That alone should be testable locally by skia (w/
> and w/o the ALLOW_GLOBAL compile flag).
That's the test I make now (w, w/o). Are you seying you want something more
automatic that allows running both ways from a tool or buildbot?

> 
> Perhaps we'll end up writing lazy, thread-safe, automatic ways to call the
> inits, or perhaps we'll put that onus on the client (e.g. Chrome). We may not
> know where to make that call yet, but if Chrome wants this feature, its seems
> reasonable that they help us find the safe place to do this sort of
> initialization...
I'm confused by this. 

> besides, for the moment they actually don't need to init their
> flattenables at all, since they don't use that feature yet.
I assume that it would be sufficient to do the following:
- note that calling the Init() functions is a not thread safe
- remove code that calls Init() lazily
- note that Chromium doesn't need to call Init() today
- add SkDebugf/ SkAssert if Chromium starts calling flattening code
Would this be satisfactory?

[reed1, 2011-12-14 22:01:33]

I assume that it would be sufficient to do the following:
- note that calling the Init() functions is a not thread safe
- remove code that calls Init() lazily
- note that Chromium doesn't need to call Init() today
- add SkDebugf/ SkAssert if Chromium starts calling flattening code
Would this be satisfactory?

------

I think so, where I think we only need the assert if Chrome starts to use
picture serialization (which needs the registered procs for deserialization):
just flattening/unflattening is fine, as it doesn't touch the registrar.

[caryclark1, 2011-12-15 13:08:10]

It looks like SkBitmap::flatten() does read the Registrar when it has a
fPixelRef.

Instead of tying it to picture playback, how about if I add to
FactoryToName and NameToFactory something like
#if !SK_ALLOW_STATIC_GLOBAL_INITIALIZATION
    if (!gCount) {
       SkDebugf("%s has no registered name/factory pairs. Call
SkGraphics::InitializeGlobals() at process iniitialization time.",
__FUNCTION__);
    }
#endif


On Wed, Dec 14, 2011 at 5:01 PM, <reed@google.com> wrote:

> I assume that it would be sufficient to do the following:
> - note that calling the Init() functions is a not thread safe
> - remove code that calls Init() lazily
> - note that Chromium doesn't need to call Init() today
> - add SkDebugf/ SkAssert if Chromium starts calling flattening code
> Would this be satisfactory?
>
> ------
>
> I think so, where I think we only need the assert if Chrome starts to
> use picture serialization (which needs the registered procs for
> deserialization): just flattening/unflattening is fine, as it doesn't
> touch the registrar.
>
>
>
http://codereview.appspot.com/**5487050/<http://codereview.appspot.com/5487050/>
>

[reed1, 2011-12-15 14:03:38]

http://codereview.appspot.com/5487050/diff/9001/include/core/SkGraphics.h
File include/core/SkGraphics.h (right):

http://codereview.appspot.com/5487050/diff/9001/include/core/SkGraphics.h#new...
include/core/SkGraphics.h:59: #if !SK_ALLOW_STATIC_GLOBAL_INITIALIZERS
Is this different from the caller's perspective from having them call
SkGraphics::Init()? i.e. do we need both functions?

[caryclark1, 2011-12-15 14:13:42]

http://codereview.appspot.com/5487050/diff/9001/include/core/SkGraphics.h
File include/core/SkGraphics.h (right):

http://codereview.appspot.com/5487050/diff/9001/include/core/SkGraphics.h#new...
include/core/SkGraphics.h:59: #if !SK_ALLOW_STATIC_GLOBAL_INITIALIZERS
On 2011/12/15 14:03:38, reed1 wrote:
> Is this different from the caller's perspective from having them call
> SkGraphics::Init()? i.e. do we need both functions?

Removed InitializeGlobals; moved the comment to Init

[reed1, 2011-12-15 14:14:48]

lgtm