Move code to dump GrSurface as a png to helper files

Move code to dump GrSurface as a png to helper files

Create a new set of helper functions in SkImagesSupport.h. These functions use the
image decoders and encoders from images library. In cases where images library
is not part of the Skia library, these functions can be stubbed out.

For example, Chromium would compile src/ports/SkImagesSupport_noimages.cpp,
since it does not compile in the image decoders or encoders.

BUG=skia:2992

[Kimmo Kinnunen, 2014-10-20 12:46:13]

kkinnunen@nvidia.com changed reviewers:
+ jvanverth@google.com

[jvanverth1, 2014-10-20 14:32:21]

jvanverth@google.com changed reviewers:
+ bsalomon@google.com

[jvanverth1, 2014-10-20 14:32:21]

Only one comment, but I think you'll need Brian to approve it, as it changes the
interface.

https://codereview.chromium.org/665913002/diff/1/src/gpu/GrSurfacePriv.h
File src/gpu/GrSurfacePriv.h (right):

https://codereview.chromium.org/665913002/diff/1/src/gpu/GrSurfacePriv.h#newc...
src/gpu/GrSurfacePriv.h:12: #include "../images/SkImagesSupport.h"
I'm not sure this belongs in here, as there aren't any dependencies in this
header. I'd think that you'd include SkImagesSupport.h in the files that need to
save pixels, rather than GrSurfacePriv.h.

[scroggo, 2014-10-20 15:27:37]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2014-10-20 15:27:39]

not lgtm

https://codereview.chromium.org/665913002/diff/1/src/gpu/GrAADistanceFieldPat...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/665913002/diff/1/src/gpu/GrAADistanceFieldPat...
src/gpu/GrAADistanceFieldPathRenderer.cpp:31: #ifdef SK_DEVELOPER
This is a good change that can go in separately.

https://codereview.chromium.org/665913002/diff/1/src/gpu/GrSurfacePriv.h
File src/gpu/GrSurfacePriv.h (right):

https://codereview.chromium.org/665913002/diff/1/src/gpu/GrSurfacePriv.h#newc...
src/gpu/GrSurfacePriv.h:12: #include "../images/SkImagesSupport.h"
On 2014/10/20 14:32:21, jvanverth1 wrote:
> I'm not sure this belongs in here, as there aren't any dependencies in this
> header. I'd think that you'd include SkImagesSupport.h in the files that need
to
> save pixels, rather than GrSurfacePriv.h.

Some more comments:

We do not fully qualify path names like this - if we need SkImagesSupport.h, its
folder should be in include_dirs in the gyp file, and the include should just be
the filename ("SkImagesSupport.h").

That said, we do not generally include files in src/.

Further, gpu should not be dependent on images.

[Kimmo Kinnunen, 2014-10-20 16:27:39]

On 2014/10/20 15:27:39, scroggo wrote:
> not lgtm
....
> We do not fully qualify path names like this - if we need SkImagesSupport.h,
its
> folder should be in include_dirs in the gyp file, and the include should just
be
> the filename ("SkImagesSupport.h").
> 
> That said, we do not generally include files in src/.

Yeah, sorry for that. I forgot to clean that hunk..

> Further, gpu should not be dependent on images.

Right.. So currently there's a call in gpu to image encoders. This patch removes
the call to image encoder.

Essentially I *am* trying to remove the dependency between gpu and images.
Should I try this some other way?

[scroggo, 2014-10-20 17:47:43]

On 2014/10/20 16:27:39, Kimmo Kinnunen wrote:
> On 2014/10/20 15:27:39, scroggo wrote:
> > not lgtm
> ....
> > We do not fully qualify path names like this - if we need SkImagesSupport.h,
> its
> > folder should be in include_dirs in the gyp file, and the include should
just
> be
> > the filename ("SkImagesSupport.h").
> > 
> > That said, we do not generally include files in src/.
> 
> Yeah, sorry for that. I forgot to clean that hunk..
> 
> > Further, gpu should not be dependent on images.
> 
> Right.. So currently there's a call in gpu to image encoders. This patch
removes
> the call to image encoder.

Touche. We actually have a sort of weird situation where gpu (as well as core,
see
SkImage::encodeData) depends on the interface of SkImageEncoder. To solve this
(whether
or not this is correct), we moved the header for SkImageEncoder (and Decoder) to
core,
with an empty implementation in SkImageDecoder_empty.cpp which is used by
chromium.

> 
> Essentially I *am* trying to remove the dependency between gpu and images.
> Should I try this some other way?

It appears to me you've made the dependency more convoluted. With patch set 1,
gpu
depends on a private header in images, and the implementation depends on gpu.

Here's what I would suggest:
Keep SkSaveGrSurfacePrivPixelsAsPNG in gpu, with a full implementation that
depends
on SkImageEncoder's interface. If images/ is built, EncodeFile will write to a
file.
If SkImageDecoder_empty is built, nothing will be written. Or another
implementation
for SkImageEncoder::EncodeFile could be written (e.g. to use Chromium's
encoders).

This is consistent with other uses (e.g. SkImage::encodeData). It also separates
the
reading from the writing. I don't imagine an implementation of
SkSaveGrSurfacePrivPixelsAsPNG would want to read the pixels differently, so
that
seems like a clean way to do it. If you are concerned about wasting time reading
the
pixels when you won't end up writing them, (which I'm not yet convinced is an
important
optimization) you could call SkImageEncoder::Create(kPNG_Type) first, and not
bother
reading if it returns NULL.

Lastly, it means that images does not need to include gpu headers.

[Kimmo Kinnunen, 2014-10-20 18:30:20]

On 2014/10/20 17:47:43, scroggo wrote:
> On 2014/10/20 16:27:39, Kimmo Kinnunen wrote:
> > On 2014/10/20 15:27:39, scroggo wrote:
> > > not lgtm
> > ....
> > > We do not fully qualify path names like this - if we need
SkImagesSupport.h,
> > its
> > > folder should be in include_dirs in the gyp file, and the include should
> just
> > be
> > > the filename ("SkImagesSupport.h").
> > > 
> > > That said, we do not generally include files in src/.
> > 
> > Yeah, sorry for that. I forgot to clean that hunk..
> > 
> > > Further, gpu should not be dependent on images.
> > 
> > Right.. So currently there's a call in gpu to image encoders. This patch
> removes
> > the call to image encoder.
> 
> Touche. We actually have a sort of weird situation where gpu (as well as core,
> see
> SkImage::encodeData) depends on the interface of SkImageEncoder. To solve this
> (whether
> or not this is correct), we moved the header for SkImageEncoder (and Decoder)
to
> core,
> with an empty implementation in SkImageDecoder_empty.cpp which is used by
> chromium.

This would remove the SkImage::encodeData:
https://codereview.chromium.org/656503003/

[Kimmo Kinnunen, 2014-10-20 18:50:09]

On 2014/10/20 17:47:43, scroggo wrote:
> If SkImageDecoder_empty is built, nothing will be written. Or another
> implementation
> for SkImageEncoder::EncodeFile could be written (e.g. to use Chromium's
> encoders).


This code is actually run with plain skia compile. If there's _empty.cpp file,
it will not be run. This is clear to everyone, and so I sense there's a bit of
resistance to the actual goal. Maybe we can simplify this a bit:
 If you do not care for this goal and wish to retain status quo (no unit tests /
dm results with chromium), then just say so. This is not a personal goal of
mine, so I can just drop the patches if this is uncomfortable. Suggesting
running stuff from _empty.cpp is not going to solve it -- dm does not encode
result pngs with _empty.cpp implementation.

[scroggo, 2014-10-20 19:11:28]

On 2014/10/20 18:50:09, Kimmo Kinnunen wrote:
> On 2014/10/20 17:47:43, scroggo wrote:
> > If SkImageDecoder_empty is built, nothing will be written. Or another
> > implementation
> > for SkImageEncoder::EncodeFile could be written (e.g. to use Chromium's
> > encoders).
> 
> 
> This code is actually run with plain skia compile. If there's _empty.cpp file,
> it will not be run. This is clear to everyone, and so I sense there's a bit of
> resistance to the actual goal.

Sorry, I did not mean to offend. This was actually not clear to me.

> Maybe we can simplify this a bit:
>  If you do not care for this goal and wish to retain status quo (no unit tests
/
> dm results with chromium), then just say so. This is not a personal goal of
> mine, so I can just drop the patches if this is uncomfortable. Suggesting
> running stuff from _empty.cpp is not going to solve it -- dm does not encode
> result pngs with _empty.cpp implementation.

In general, I support more testing. I think I understand the architectural
decisions
a little better now that I have looked through all of your CLs. That said, I
think
it will be worthwhile to have a discussion about it. I'll set something up for
tomorrow. In the meantime, is there a document you can point me to that has more
explanation that the bug?

[Kimmo Kinnunen, 2014-10-21 08:14:46]

On 2014/10/20 19:11:28, scroggo wrote:
> In general, I support more testing. I think I understand the architectural
> decisions
> a little better now that I have looked through all of your CLs. That said, I
> think
> it will be worthwhile to have a discussion about it. I'll set something up for
> tomorrow.

Ok.

> In the meantime, is there a document you can point me to that has more
> explanation that the bug?

I didn't think this would be such a contested feature to warrant such. I tried
to write something here, not sure if it's of any help:
https://docs.google.com/document/d/1pUbdJ6OSNK--y2rSjPRkUMYCCx7jg6CjcLB_AjZyv...