adding preabandon flag to DM

adding preabandon flag to DM

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/5f5a8d7599b8e248633ac122294b7a01401fedcb

[joshualitt, 2015-02-25 17:14:45]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com, mtklein@google.com

[joshualitt, 2015-02-25 17:14:46]

A change to add a --preAbandonGpuContext flag to DM for use in valgrind.

This patch passes valgrind cleanly on my box.

[bsalomon, 2015-02-25 17:52:10]

mostly lg, minor stuff inline

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp#newcode170
dm/DMSrcSink.cpp:170: dst->allocPixels(info);
does the allocPixels need to move? AFAIK it is doing a CPU malloc for the
readback.

https://codereview.chromium.org/929243004/diff/80001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:438: //SkErrorInternals::SetError(
kInternalError_SkError,
Does this print or do something worse? I think the error handler is settable..
maybe DM could set a /dev/null error handler when the flag set.

https://codereview.chromium.org/929243004/diff/80001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:951: //SkDebugf("Unable to create device for layer.");
Maybe convert to an SkErrorInternals::Error?

https://codereview.chromium.org/929243004/diff/80001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:56: #define RETURN_BOOL_IF_ABANDONED if (!fDrawBuffer) {
return false; }
RETURN_FALSE_IF_ABANDONED?

https://codereview.chromium.org/929243004/diff/80001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:1889: // SkDebugf("---- failed to create compatible
device texture [%d %d]\n",
SkErrorInternals?

https://codereview.chromium.org/929243004/diff/80001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/gpu/SkGr.cpp#newcode563
src/gpu/SkGr.cpp:563: // Pre abandon gpu context will trigger this excessively
SkEI?

[mtklein, 2015-02-25 18:09:06]

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp#newcode170
dm/DMSrcSink.cpp:170: dst->allocPixels(info);
On 2015/02/25 17:52:10, bsalomon wrote:
> does the allocPixels need to move? AFAIK it is doing a CPU malloc for the
> readback.

Right.  It's always plain-old-pixels.  I wouldn't think it'd need an erase call
either.

https://codereview.chromium.org/929243004/diff/80001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:438: //SkErrorInternals::SetError(
kInternalError_SkError,
On 2015/02/25 17:52:10, bsalomon wrote:
> Does this print or do something worse? I think the error handler is settable..
> maybe DM could set a /dev/null error handler when the flag set.

As far as I can tell, the default is just an SkDebug().

https://codereview.chromium.org/929243004/diff/80001/tools/flags/SkCommonFlag...
File tools/flags/SkCommonFlags.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/tools/flags/SkCommonFlag...
tools/flags/SkCommonFlags.cpp:42: DEFINE_bool(preAbandonGpuContext, false,
"Abandons the GrContext before running the test.");
Are you planning on adding this flag to nanobench too?  If not, it can go right
in DMSrcSink.cpp.

[joshualitt, 2015-02-25 19:00:42]

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp#newcode170
dm/DMSrcSink.cpp:170: dst->allocPixels(info);
On 2015/02/25 18:09:06, mtklein wrote:
> On 2015/02/25 17:52:10, bsalomon wrote:
> > does the allocPixels need to move? AFAIK it is doing a CPU malloc for the
> > readback.
> 
> Right.  It's always plain-old-pixels.  I wouldn't think it'd need an erase
call
> either.

Valgrind was unhappy with us using it uninitialized, so I think I do need to do
something to it.  Thoughts?

https://codereview.chromium.org/929243004/diff/80001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:438: //SkErrorInternals::SetError(
kInternalError_SkError,
On 2015/02/25 18:09:06, mtklein wrote:
> On 2015/02/25 17:52:10, bsalomon wrote:
> > Does this print or do something worse? I think the error handler is
settable..
> > maybe DM could set a /dev/null error handler when the flag set.
> 
> As far as I can tell, the default is just an SkDebug().

I can set a noop error handler for preabandon.  The issue is we don't want to
spam the logs with these errors.

https://codereview.chromium.org/929243004/diff/80001/tools/flags/SkCommonFlag...
File tools/flags/SkCommonFlags.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/tools/flags/SkCommonFlag...
tools/flags/SkCommonFlags.cpp:42: DEFINE_bool(preAbandonGpuContext, false,
"Abandons the GrContext before running the test.");
On 2015/02/25 18:09:06, mtklein wrote:
> Are you planning on adding this flag to nanobench too?  If not, it can go
right
> in DMSrcSink.cpp.

I think we will want to run nanobench with this flag as well just to be safe. 
I'll do the nanobench changes in a separate CL.

[mtklein, 2015-02-25 19:05:40]

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp#newcode170
dm/DMSrcSink.cpp:170: dst->allocPixels(info);
On 2015/02/25 19:00:42, joshualitt wrote:
> On 2015/02/25 18:09:06, mtklein wrote:
> > On 2015/02/25 17:52:10, bsalomon wrote:
> > > does the allocPixels need to move? AFAIK it is doing a CPU malloc for the
> > > readback.
> > 
> > Right.  It's always plain-old-pixels.  I wouldn't think it'd need an erase
> call
> > either.
> 
> Valgrind was unhappy with us using it uninitialized, so I think I do need to
do
> something to it.  Thoughts?

Don't call eraseArea()?  Why are we doing that?

[joshualitt, 2015-02-25 19:10:11]

On 2015/02/25 19:05:40, mtklein wrote:
> https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp
> File dm/DMSrcSink.cpp (right):
> 
>
https://codereview.chromium.org/929243004/diff/80001/dm/DMSrcSink.cpp#newcode170
> dm/DMSrcSink.cpp:170: dst->allocPixels(info);
> On 2015/02/25 19:00:42, joshualitt wrote:
> > On 2015/02/25 18:09:06, mtklein wrote:
> > > On 2015/02/25 17:52:10, bsalomon wrote:
> > > > does the allocPixels need to move? AFAIK it is doing a CPU malloc for
the
> > > > readback.
> > > 
> > > Right.  It's always plain-old-pixels.  I wouldn't think it'd need an erase
> > call
> > > either.
> > 
> > Valgrind was unhappy with us using it uninitialized, so I think I do need to
> do
> > something to it.  Thoughts?
> 
> Don't call eraseArea()?  Why are we doing that?

feedback incorporated

[bsalomon, 2015-02-25 19:13:07]

lgtm

[mtklein, 2015-02-25 20:16:22]

lgtm too

We'll want to skip -w on the Valgrind bots before landing this right?

[joshualitt, 2015-02-25 20:17:33]

On 2015/02/25 20:16:22, mtklein wrote:
> lgtm too
> 
> We'll want to skip -w on the Valgrind bots before landing this right?

Yea, for the preAbandon run of dm / nanobench we will definitely want to skip
-w.

[joshualitt, 2015-02-25 22:02:12]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-25 22:02:48]

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

[commit-bot: I haz the power, 2015-02-25 22:09:52]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/5f5a8d7599b8e248633ac122294b7a01401fedcb