reland "extract some common code from PictureRenderer"

reland "extract some common code from PictureRenderer"

re-lands https://codereview.chromium.org/273703006/

NOTRY=True

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

[epoger, 2014-05-08 17:32:29]

Ben- How can I see the evidence of the problem on my development machine?  (I.e.
without building for Android, uploading to a device, and running GM on the
device)

Here's what I tried, but you can see I got the same results in workspaces with
and without this change:


epoger@wpgntat-ubiq141:~/src/skia/white/skia$ make
/usr/local/google/home/epoger/src/skia/white/skia/gyp_skia
GYP_GENERATORS environment variable not set, using default
GYP_GENERATORS is "ninja"
Updating projects from gyp files...
ninja -C out/Debug most
ninja: Entering directory `out/Debug'
ninja: no work to do.
epoger@wpgntat-ubiq141:~/src/skia/white/skia$ head -21 gyp/gm.gyp | tail -5
      'dependencies': [
        'skia_lib.gyp:skia_lib',
        'jsoncpp.gyp:jsoncpp',
      ],
      'direct_dependent_settings': {
epoger@wpgntat-ubiq141:~/src/skia/white/skia$ nm --print-file-name $(find
out/Debug -name '*.[ao]') | grep gEmptyRec
out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE


epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ make
/usr/local/google/home/epoger/src/skia/blue/skia/gyp_skia
GYP_GENERATORS environment variable not set, using default
GYP_GENERATORS is "ninja"
Updating projects from gyp files...
ninja -C out/Debug most
ninja: Entering directory `out/Debug'
ninja: no work to do.
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ head -21 gyp/gm.gyp | tail -5
      'dependencies': [
        'skia_lib.gyp:skia_lib',
        'jsoncpp.gyp:jsoncpp',
        'utils.gyp:utils',
      ],
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ nm --print-file-name $(find
out/Debug -name '*.[ao]') | grep gEmptyRec
out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE

[bungeman-skia, 2014-05-08 18:13:23]

On 2014/05/08 17:32:29, epoger wrote:
> Ben- How can I see the evidence of the problem on my development machine? 
(I.e.
> without building for Android, uploading to a device, and running GM on the
> device)
> 
> Here's what I tried, but you can see I got the same results in workspaces with
> and without this change:
> 
> 
> epoger@wpgntat-ubiq141:~/src/skia/white/skia$ make
> /usr/local/google/home/epoger/src/skia/white/skia/gyp_skia
> GYP_GENERATORS environment variable not set, using default
> GYP_GENERATORS is "ninja"
> Updating projects from gyp files...
> ninja -C out/Debug most
> ninja: Entering directory `out/Debug'
> ninja: no work to do.
> epoger@wpgntat-ubiq141:~/src/skia/white/skia$ head -21 gyp/gm.gyp | tail -5
>       'dependencies': [
>         'skia_lib.gyp:skia_lib',
>         'jsoncpp.gyp:jsoncpp',
>       ],
>       'direct_dependent_settings': {
> epoger@wpgntat-ubiq141:~/src/skia/white/skia$ nm --print-file-name $(find
> out/Debug -name '*.[ao]') | grep gEmptyRec
> out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
> _ZN8SkString9gEmptyRecE
> out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
> _ZN8SkString9gEmptyRecE
> 
> 
> epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ make
> /usr/local/google/home/epoger/src/skia/blue/skia/gyp_skia
> GYP_GENERATORS environment variable not set, using default
> GYP_GENERATORS is "ninja"
> Updating projects from gyp files...
> ninja -C out/Debug most
> ninja: Entering directory `out/Debug'
> ninja: no work to do.
> epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ head -21 gyp/gm.gyp | tail -5
>       'dependencies': [
>         'skia_lib.gyp:skia_lib',
>         'jsoncpp.gyp:jsoncpp',
>         'utils.gyp:utils',
>       ],
> epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ nm --print-file-name $(find
> out/Debug -name '*.[ao]') | grep gEmptyRec
> out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
> _ZN8SkString9gEmptyRecE
> out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
> _ZN8SkString9gEmptyRecE

I think you might be able to see this on Linux with 'python gyp_skia
-Dskia_shared_lib=1'. I think this just has to do with what happens in
skia_lib.gyp.

[epoger, 2014-05-08 18:52:33]

On 2014/05/08 18:13:23, bungeman1 wrote:
> I think you might be able to see this on Linux with 'python gyp_skia
> -Dskia_shared_lib=1'. I think this just has to do with what happens in
> skia_lib.gyp.

Still not seeing any evidence of the problem on my Linux machine... maybe I'm
barking up the wrong tree with gEmptyRec ?

epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ make clean
rm -rf out xcodebuild
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ GYP_DEFINES="skia_shared_lib=1"
make
/usr/local/google/home/epoger/src/skia/blue/skia/gyp_skia
GYP_GENERATORS environment variable not set, using default
GYP_GENERATORS is "ninja"
Updating projects from gyp files...
ninja -C out/Debug most
ninja: Entering directory `out/Debug'
[2258/2258] STAMP obj/gyp/most.actions_depends.stamp
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ head -21 gyp/gm.gyp | tail -5
      'dependencies': [
        'skia_lib.gyp:skia_lib',
        'jsoncpp.gyp:jsoncpp',
        'utils.gyp:utils',
      ],
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ nm --print-file-name $(find
out/Debug -name '*.[ao]') | grep gEmptyRec
out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE

[epoger, 2014-05-08 18:59:27]

Here's one of the bots that failed with the original CL:
http://108.170.220.21:10117/builders/Test-Android-Nexus7-Tegra3-Arm7-Debug/bu...

excerpt:
[08:55:18.141778] reading resources from
/storage/emulated/legacy/skiabot/skia_resources
[08:55:18.642431] drawing... android_paint [500 500]
[08:55:18.642569] ../../../../src/core/SkString.cpp:243: failed assertion
"fRec->fLength > 0"
[08:55:19.143221] Segmentation fault 
[08:55:19.143317] EXIT_CODE is 139

[epoger, 2014-05-08 19:54:20]

Derek showed me how to find evidence of the problem on my Linux machine.  From
the results below, it seems like we already have similar problems with skimage
if run on Android...

[Details below... now that I know how to see evidence of the problem, I'll fix
it and upload a patchset soon.]


Before the CL:
epoger@wpgntat-ubiq141:~/src/skia/white/skia$ GYP_DEFINES="skia_shared_lib=1"
make
/usr/local/google/home/epoger/src/skia/white/skia/gyp_skia
GYP_GENERATORS environment variable not set, using default
GYP_GENERATORS is "ninja"
Updating projects from gyp files...
ninja -C out/Debug most
ninja: Entering directory `out/Debug'
[2256/2256] STAMP obj/gyp/most.actions_depends.stamp
epoger@wpgntat-ubiq141:~/src/skia/white/skia$ head -21 gyp/gm.gyp | tail -5
      'dependencies': [
        'skia_lib.gyp:skia_lib',
        'jsoncpp.gyp:jsoncpp',
      ],
      'direct_dependent_settings': {
epoger@wpgntat-ubiq141:~/src/skia/white/skia$ nm --print-file-name $(find
out/Debug -name '*.[ao]') | grep gEmptyRec | sort
out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
epoger@wpgntat-ubiq141:~/src/skia/white/skia$ nm --print-file-name $(find
out/Debug -executable -type f) | grep gEmptyRec | sort
out/Debug/lib/libskia.so:00000000005c7730 R _ZN8SkString9gEmptyRecE
out/Debug/lua_app:000000000062a720 R _ZN8SkString9gEmptyRecE
out/Debug/lua_pictures:00000000006352e0 R _ZN8SkString9gEmptyRecE
out/Debug/skimage:00000000005eb0e0 R _ZN8SkString9gEmptyRecE


After the CL:
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ GYP_DEFINES="skia_shared_lib=1"
make
/usr/local/google/home/epoger/src/skia/blue/skia/gyp_skia
GYP_GENERATORS environment variable not set, using default
GYP_GENERATORS is "ninja"
Updating projects from gyp files...
ninja -C out/Debug most
ninja: Entering directory `out/Debug'
ninja: no work to do.
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ head -21 gyp/gm.gyp | tail -5
      'dependencies': [
        'skia_lib.gyp:skia_lib',
        'jsoncpp.gyp:jsoncpp',
        'utils.gyp:utils',
      ],
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ nm --print-file-name $(find
out/Debug -name '*.[ao]') | grep gEmptyRec | sort
out/Debug/libskia_core.a:core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
out/Debug/obj/src/core/core.SkString.o:00000000000000e0 R
_ZN8SkString9gEmptyRecE
epoger@wpgntat-ubiq141:~/src/skia/blue/skia$ nm --print-file-name $(find
out/Debug -executable -type f) | grep gEmptyRec | sort
out/Debug/bbh_shootout:000000000060aee0 R _ZN8SkString9gEmptyRecE
out/Debug/bench_pictures:0000000000611ee0 R _ZN8SkString9gEmptyRecE
out/Debug/gm:00000000007174c0 R _ZN8SkString9gEmptyRecE
out/Debug/lib/libskia.so:00000000005c7bd0 R _ZN8SkString9gEmptyRecE
out/Debug/lua_app:000000000062a760 R _ZN8SkString9gEmptyRecE
out/Debug/lua_pictures:0000000000635300 R _ZN8SkString9gEmptyRecE
out/Debug/pinspect:00000000005d1640 R _ZN8SkString9gEmptyRecE
out/Debug/render_pictures:000000000060e3e0 R _ZN8SkString9gEmptyRecE
out/Debug/skimage:00000000005eb420 R _ZN8SkString9gEmptyRecE

[epoger, 2014-05-08 20:17:14]

Ben- please review patchset 2.  I'll send it to some trybots to make sure, but
from my local testing it looks like it should be fine.

https://codereview.chromium.org/278633002/diff/10001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/278633002/diff/10001/gm/gm_expectations.cpp#n...
gm/gm_expectations.cpp:226: SkAutoDataUnref
dataRef(SkData::NewFromFileName(jsonPath));
On 2014/05/08 16:29:50, bungeman1 wrote:
> https://codereview.chromium.org/273703006/diff/40001/gm/gm_expectations.cpp
> File gm/gm_expectations.cpp (right):
> 
>
https://codereview.chromium.org/273703006/diff/40001/gm/gm_expectations.cpp#n...
> gm/gm_expectations.cpp:234: SkAutoDataUnref
> dataRef(SkDataUtils::ReadFileIntoSkData(inFile));
> Why isn't this just SkData::NewFromFileName(jsonPath);?

Thanks for the tip!  Last time I touched this stuff, I don't think that function
existed.

Now I don't need to add SkDataUtils.[h,cpp] at all.

https://codereview.chromium.org/278633002/diff/10001/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/278633002/diff/10001/gyp/tools.gyp#newcode184
gyp/tools.gyp:184: 'skia_lib.gyp:skia_lib',
Fixed multiple places where this dependency should not have been.

[djsollen, 2014-05-08 20:19:52]

lgtm

[bungeman-skia, 2014-05-08 20:23:00]

The gyp change lgtm, as does using the SkData factory. Can't say I know much
about the rest of it though :-)

[epoger, 2014-05-08 20:24:17]

On 2014/05/08 20:23:00, bungeman1 wrote:
> The gyp change lgtm, as does using the SkData factory. Can't say I know much
> about the rest of it though :-)

Sure, sure.  As long as you're happy with the diffs between patchsets 1 and 2,
we're good.

(I'm gonna wait for the trybots to finish before committing, anyway.)

[epoger, 2014-05-09 03:17:05]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-05-09 03:18:26]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/278633002/10001

[commit-bot: I haz the power, 2014-05-09 03:18:44]

Change committed as 14662