gpu: play skp files through the command buffer

gpu: play skp files through the command buffer

Small utility program that can take an skp, or directory of skps and
GPU rasterizes them to png, through chrome's command buffer.

BUG=

Committed: https://crrev.com/6daedb499182a5b6b0bcd54004afb7ebb3c3443b
Cr-Commit-Position: refs/heads/master@{#334928}

Committed: https://crrev.com/5c1da6de3bf4a9b3ebee5f3ea77f42ee2f8db4bb
Cr-Commit-Position: refs/heads/master@{#335042}

[hendrikw, 2015-06-05 01:27:23]

Patchset #3 (id:40001) has been deleted

[hendrikw, 2015-06-05 01:27:27]

Patchset #2 (id:20001) has been deleted

[hendrikw, 2015-06-05 01:27:36]

Patchset #2 (id:60001) has been deleted

[hendrikw, 2015-06-05 01:27:45]

Patchset #1 (id:1) has been deleted

[hendrikw, 2015-06-11 18:29:09]

hendrikw@chromium.org changed reviewers:
+ piman@chromium.org, vmiura@chromium.org

[hendrikw, 2015-06-11 18:29:10]

Hi hi, here's a small program that takes a bunch of skp files and spits out a
png for each by gpu rasterizing it using skia and the command buffer.

Currently the decoders come from blink, but there is active work to move these
to skia, so hopefully the blink dependency can be removed later.

There's still a bit of work to do:
1. Make it work on android (not tested at all, might just work)
2. have it spit out skia DM compatible json file
3. look into the black list (is it used? If not, what needs to be done to make
it used)

PTAL, thanks (or pass the review to whomever you think would be more appropriate
to look at this)

[piman, 2015-06-12 01:05:30]

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
File gpu/skia_runner/in_process_graphics_system.cc (right):

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:24: NonDiscardableMemory(size_t
size) : data_(size) {}
nit: explicit.

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:30: mutable std::vector<uint8_t>
data_;
Since you don't resize the data, maybe you can use scoped_ptr<uint8_t[]>?
I'd also suggest using a const_cast<> in data() rather than using mutable here.

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:42: NonDiscardableMemoryAllocator
g_discardable;
nit: non-POD statics are disallowed by the style guide. You can use LazyInstance
for example. Or can you make it a field of InProcessGraphicsSystem?

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:81:
base::ThreadTaskRunnerHandle::IsSet();
This is kinda unfortunate - it sounds like a data race... Could you fix it
closer to the source of the problem, i.e. in
gpu/command_buffer/service/in_process_command_buffer.cc? E.g. by making sure
this is called before g_default_service.Get() is called, possibly by introducing
an intermediate function called from the service_ initializer in the
InProcessCommandBuffer constructor.

[hendrikw, 2015-06-12 18:57:49]

PTAL

I tried moving the IsSet as you suggested, let me know if this is what you had
in mind.

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
File gpu/skia_runner/in_process_graphics_system.cc (right):

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:24: NonDiscardableMemory(size_t
size) : data_(size) {}
On 2015/06/12 01:05:30, piman (Very slow to review) wrote:
> nit: explicit.

Done.

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:30: mutable std::vector<uint8_t>
data_;
On 2015/06/12 01:05:30, piman (Very slow to review) wrote:
> Since you don't resize the data, maybe you can use scoped_ptr<uint8_t[]>?
> I'd also suggest using a const_cast<> in data() rather than using mutable
here.

hm, I generally try to stay away from 'new' when an stl container handles it,
but in this case, I don't feel strongly one way or the other.  done.

https://codereview.chromium.org/1149893010/diff/100001/gpu/skia_runner/in_pro...
gpu/skia_runner/in_process_graphics_system.cc:42: NonDiscardableMemoryAllocator
g_discardable;
On 2015/06/12 01:05:30, piman (Very slow to review) wrote:
> nit: non-POD statics are disallowed by the style guide. You can use
LazyInstance
> for example. Or can you make it a field of InProcessGraphicsSystem?

Done.

[hendrikw, 2015-06-12 18:58:17]

hendrikw@chromium.org changed reviewers:
+ thakis@chromium.org

[hendrikw, 2015-06-12 18:58:17]

+thakis for all.gyp change.

[piman, 2015-06-12 19:19:38]

lgtm

https://codereview.chromium.org/1149893010/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1149893010/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:213: const
scoped_refptr<Service>& service);
nit: you can make this a free function in the .cc file, since you're not using
any field.

[hendrikw, 2015-06-12 22:54:22]

https://codereview.chromium.org/1149893010/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1149893010/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:213: const
scoped_refptr<Service>& service);
On 2015/06/12 19:19:37, piman (Very slow to review) wrote:
> nit: you can make this a free function in the .cc file, since you're not using
> any field.

good point, done

[Nico, 2015-06-14 21:45:07]

https://codereview.chromium.org/1149893010/diff/140001/gpu/skia_runner/skia_r...
File gpu/skia_runner/skia_runner.gyp (right):

https://codereview.chromium.org/1149893010/diff/140001/gpu/skia_runner/skia_r...
gpu/skia_runner/skia_runner.gyp:6: 'variables': {
Set chromium_code to 1 here (cs.chromium.org for "chromium_code file:gyp" for
oodles of examples), this enables the usual warning level

https://codereview.chromium.org/1149893010/diff/140001/gpu/skia_runner/skia_r...
gpu/skia_runner/skia_runner.gyp:14: '<(DEPTH)/base/base.gyp:base',
<(DEPTH) is for .gypi files (where the DEPTH is different depending on what
includes the gypi), in .gyp files you can just say ../.. (since it's known)

[hendrikw, 2015-06-15 16:55:23]

PTAL

https://codereview.chromium.org/1149893010/diff/140001/gpu/skia_runner/skia_r...
File gpu/skia_runner/skia_runner.gyp (right):

https://codereview.chromium.org/1149893010/diff/140001/gpu/skia_runner/skia_r...
gpu/skia_runner/skia_runner.gyp:6: 'variables': {
On 2015/06/14 21:45:07, Nico wrote:
> Set chromium_code to 1 here (http://cs.chromium.org for "chromium_code
file:gyp" for
> oodles of examples), this enables the usual warning level

Done.

https://codereview.chromium.org/1149893010/diff/140001/gpu/skia_runner/skia_r...
gpu/skia_runner/skia_runner.gyp:14: '<(DEPTH)/base/base.gyp:base',
On 2015/06/14 21:45:07, Nico wrote:
> <(DEPTH) is for .gypi files (where the DEPTH is different depending on what
> includes the gypi), in .gyp files you can just say ../.. (since it's known)

Ah, I was wondering about that, thanks!

Done.

[Nico, 2015-06-15 18:54:38]

gyp files look fine, but:

https://codereview.chromium.org/1149893010/diff/160001/gpu/skia_runner/skia_r...
File gpu/skia_runner/skia_runner.gyp (right):

https://codereview.chromium.org/1149893010/diff/160001/gpu/skia_runner/skia_r...
gpu/skia_runner/skia_runner.gyp:11: # GN version: //gpu:skia_runner
did you forget to add the BUILD.gn file for this?

[hendrikw, 2015-06-15 18:57:13]

https://codereview.chromium.org/1149893010/diff/160001/gpu/skia_runner/skia_r...
File gpu/skia_runner/skia_runner.gyp (right):

https://codereview.chromium.org/1149893010/diff/160001/gpu/skia_runner/skia_r...
gpu/skia_runner/skia_runner.gyp:11: # GN version: //gpu:skia_runner
On 2015/06/15 18:54:38, Nico wrote:
> did you forget to add the BUILD.gn file for this?

Forgot to write it! :)

I'll try to figure out how to run GN and write one.

[hendrikw, 2015-06-15 23:02:54]

hendrikw@chromium.org changed reviewers:
+ dpranke@chromium.org

[hendrikw, 2015-06-15 23:02:55]

Added a gn version of the build.

+dpranke@ for root gn file.

[Dirk Pranke, 2015-06-15 23:51:52]

lgtm

[hendrikw, 2015-06-16 19:41:18]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-06-16 19:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1149893010/#ps180001
(title: "+gn")

[hendrikw, 2015-06-16 19:41:23]

The CQ bit was unchecked by hendrikw@chromium.org

[Nico, 2015-06-16 19:42:20]

gyp lgtm

[hendrikw, 2015-06-16 19:42:47]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-06-16 19:44:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149893010/180001

[commit-bot: I haz the power, 2015-06-16 19:56:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 19:56:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[hendrikw, 2015-06-16 21:21:53]

hendrikw@chromium.org changed reviewers:
+ bsalomon@google.com

[hendrikw, 2015-06-16 21:21:54]

Hi, I had some failures with ios (running try job now)

Nico, can you take another look at all.gyp
Dirk, can you take another look at BUILD.gn

Brian, apparently I need your permission to add use skia in a brand new program,
please give me your blessing :D

Thanks!!!

[Nico, 2015-06-16 21:24:34]

all.gyp lgtm

[Dirk Pranke, 2015-06-16 22:16:36]

lgtm. frankly, all of //BUILD.gn is !is_ios at this point, but this change is
fine :).

[bsalomon, 2015-06-16 22:32:32]

On 2015/06/16 21:21:54, hendrikw wrote:
> Hi, I had some failures with ios (running try job now)
> 
> Nico, can you take another look at all.gyp
> Dirk, can you take another look at BUILD.gn
> 
> Brian, apparently I need your permission to add use skia in a brand new
program,
> please give me your blessing :D

lgtm :)

> 
> Thanks!!!

[bsalomon, 2015-06-16 22:35:35]

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
File gpu/skia_runner/sk_picture_rasterizer.cc (right):

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
gpu/skia_runner/sk_picture_rasterizer.cc:41: skia::RefPtr<GrTexture>
gr_texture(skia::AdoptRef(
Can you use SkSurface::NewWrappedRenderTarget rather than going through
GrTexture?

We want to deprecate public access to GrTexture and GrRenderTarget.

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
gpu/skia_runner/sk_picture_rasterizer.cc:82: SkCanvas canvas(bitmap);
And SkSurface::NewRaster here?

[hendrikw, 2015-06-17 01:21:16]

Patchset #8 (id:220001) has been deleted

[hendrikw, 2015-06-17 01:22:09]

bsalomon@, PTAL, thanks!

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
File gpu/skia_runner/sk_picture_rasterizer.cc (right):

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
gpu/skia_runner/sk_picture_rasterizer.cc:41: skia::RefPtr<GrTexture>
gr_texture(skia::AdoptRef(
On 2015/06/16 22:35:35, bsalomon wrote:
> Can you use SkSurface::NewWrappedRenderTarget rather than going through
> GrTexture?
> 
> We want to deprecate public access to GrTexture and GrRenderTarget.

Ok, SkSurface::NewWrappedRenderTarget doesn't really seem to serve the same
purpose, I'll try SkSurface::NewRenderTarget instead?

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
gpu/skia_runner/sk_picture_rasterizer.cc:82: SkCanvas canvas(bitmap);
On 2015/06/16 22:35:35, bsalomon wrote:
> And SkSurface::NewRaster here?

hm, not sure I follow, NewRaster returns a surface, it doesn't look like I need
one here?

Should I be using SkSurfaces in place of SkBitmap?

[bsalomon, 2015-06-17 14:58:18]

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
File gpu/skia_runner/sk_picture_rasterizer.cc (right):

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
gpu/skia_runner/sk_picture_rasterizer.cc:41: skia::RefPtr<GrTexture>
gr_texture(skia::AdoptRef(
On 2015/06/17 01:22:09, hendrikw wrote:
> On 2015/06/16 22:35:35, bsalomon wrote:
> > Can you use SkSurface::NewWrappedRenderTarget rather than going through
> > GrTexture?
> > 
> > We want to deprecate public access to GrTexture and GrRenderTarget.
> 
> Ok, SkSurface::NewWrappedRenderTarget doesn't really seem to serve the same
> purpose, I'll try SkSurface::NewRenderTarget instead?

Oh, yes, you're right. NewRenderTarget is the right replacement here. Also this
should probably return an SkImage (which can be drawn into a SkCanvas in the
below function).

https://codereview.chromium.org/1149893010/diff/200001/gpu/skia_runner/sk_pic...
gpu/skia_runner/sk_picture_rasterizer.cc:82: SkCanvas canvas(bitmap);
On 2015/06/17 01:22:09, hendrikw wrote:
> On 2015/06/16 22:35:35, bsalomon wrote:
> > And SkSurface::NewRaster here?
> 
> hm, not sure I follow, NewRaster returns a surface, it doesn't look like I
need
> one here?
> 
> Should I be using SkSurfaces in place of SkBitmap?

The way to "modernize" users of SkBitmap is to replace source bitmaps with
SkImages and dst bitmap wrapped in canvases with SkSurfaces. Here the thing to
do is create a raster surface, draw the tile SkImages to it, and return a
SkImage snapshot at the end (SkSurface::newImageSnapshot()).

[hendrikw, 2015-06-17 20:22:58]

modernized!

bsalomon@, PTAL, thanks!

[bsalomon, 2015-06-17 20:27:11]

On 2015/06/17 20:22:58, hendrikw wrote:
> modernized!
> 
> bsalomon@, PTAL, thanks!

lgtm!

[hendrikw, 2015-06-17 20:31:56]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-06-17 20:31:56]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
dpranke@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1149893010/#ps260001
(title: "skbitmap -> skimage")

[commit-bot: I haz the power, 2015-06-17 20:34:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149893010/260001

[commit-bot: I haz the power, 2015-06-17 21:39:13]

Committed patchset #9 (id:260001)

[commit-bot: I haz the power, 2015-06-17 21:40:20]

Patchset 9 (id:??) landed as
https://crrev.com/6daedb499182a5b6b0bcd54004afb7ebb3c3443b
Cr-Commit-Position: refs/heads/master@{#334928}

[hendrikw, 2015-06-17 23:23:54]

A revert of this CL (patchset #9 id:260001) has been created in
https://codereview.chromium.org/1183513006/ by hendrikw@chromium.org.

The reason for reverting is: Causes a build failure on continuous mac bots
(succeeds on try bots though).

[hendrikw, 2015-06-18 14:05:49]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-06-18 14:05:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, thakis@chromium.org, bsalomon@google.com,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1149893010/#ps280001
(title: "now with proper mac headers")

[commit-bot: I haz the power, 2015-06-18 14:05:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149893010/280001

[commit-bot: I haz the power, 2015-06-18 15:09:11]

Committed patchset #10 (id:280001)

[commit-bot: I haz the power, 2015-06-18 15:10:11]

Patchset 10 (id:??) landed as
https://crrev.com/5c1da6de3bf4a9b3ebee5f3ea77f42ee2f8db4bb
Cr-Commit-Position: refs/heads/master@{#335042}

[Nico, 2015-06-19 18:10:52]

This makes the clang/win/gn bot fail to link:

http://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%28dbg%29/build...

FAILED: C:/b/depot_tools/python276_bin/python.exe gyp-win-tool link-wrapper
environment.x64 False link.exe /nologo /OUT:skia_runner.exe
/PDB:skia_runner.exe.pdb @skia_runner.exe.rsp
skia_runner.in_process_graphics_system.obj : error LNK2019: unresolved external
symbol "__declspec(dllimport) public: __cdecl
gpu::gles2::ContextCreationAttribHelper::ContextCreationAttribHelper(void)"
(__imp_??0ContextCreationAttribHelper@gles2@gpu@@QEAA@XZ) referenced in function
"private: class scoped_ptr<class gpu::GLInProcessContext,struct
base::DefaultDeleter<class gpu::GLInProcessContext> > __cdecl
skia_runner::InProcessGraphicsSystem::CreateInProcessContext(void)const "
(?CreateInProcessContext@InProcessGraphicsSystem@skia_runner@@AEBA?AV?$scoped_ptr@VGLInProcessContext@gpu@@U?$DefaultDeleter@VGLInProcessContext@gpu@@@base@@@@XZ)

skia_runner.exe : fatal error LNK1120: 1 unresolved externals

I guess the .gn file is missing some dependency?