Make GrTextureStripAtlas flush pending IO on newly acquired texture

Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262233002

Committed: https://skia.googlesource.com/skia/+/95243ebb68cae9ff4821ea57b9a32a194f2a87cf

[ajuma, 2016-08-22 15:41:38]

Description was changed from

==========
Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
==========

to

==========
Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262233002
==========

[ajuma, 2016-08-22 16:29:37]

Description was changed from

==========
Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262233002
==========

to

==========
Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262233002
==========

[ajuma, 2016-08-22 16:29:37]

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

[ajuma, 2016-08-22 16:30:37]

What's a good place to add a test for this?

[bsalomon, 2016-08-22 16:52:58]

request for comment, otherwise lgtm.

https://codereview.chromium.org/2262233002/diff/1/src/gpu/effects/GrTextureSt...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

https://codereview.chromium.org/2262233002/diff/1/src/gpu/effects/GrTextureSt...
src/gpu/effects/GrTextureStripAtlas.cpp:213:
fDesc.fContext->flushSurfaceIO(fTexture);
Could you add a comment here that we need to do this because we will be issuing
writes to the surface using kDontFlush_PixelOpsFlag?

[bsalomon, 2016-08-22 16:56:32]

On 2016/08/22 16:30:37, ajuma wrote:
> What's a good place to add a test for this?

It's going to be a bit tricky. I guess we could draw to a texture pulled from
the cache, draw from it to another destination, release the texture back to the
cache, make a texture strip atlas with params that will find the texture, and
then draw using a locked row.

As for location, maybe add tests/GrTextureStipAtlasTest.cpp?

[ajuma, 2016-08-23 19:45:28]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 19:45:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 19:48:21]

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

[commit-bot: I haz the power, 2016-08-23 19:48:22]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[ajuma, 2016-08-23 19:56:20]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 19:56:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ajuma, 2016-08-23 19:56:43]

Patchset #3 (id:40001) has been deleted

[ajuma, 2016-08-23 19:56:50]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-23 20:01:46]

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

[commit-bot: I haz the power, 2016-08-23 20:01:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ajuma, 2016-08-23 20:32:34]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 20:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ajuma, 2016-08-23 20:32:46]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-08-23 20:52:14]

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

[commit-bot: I haz the power, 2016-08-23 20:52:14]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-08-24 13:12:39]

Added a test, ptal.

https://codereview.chromium.org/2262233002/diff/1/src/gpu/effects/GrTextureSt...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

https://codereview.chromium.org/2262233002/diff/1/src/gpu/effects/GrTextureSt...
src/gpu/effects/GrTextureStripAtlas.cpp:213:
fDesc.fContext->flushSurfaceIO(fTexture);
On 2016/08/22 16:52:57, bsalomon wrote:
> Could you add a comment here that we need to do this because we will be
issuing
> writes to the surface using kDontFlush_PixelOpsFlag?

Done.

[bsalomon, 2016-08-24 13:31:10]

nice test

https://codereview.chromium.org/2262233002/diff/80001/tests/GrTextureStripAtl...
File tests/GrTextureStripAtlasTest.cpp (right):

https://codereview.chromium.org/2262233002/diff/80001/tests/GrTextureStripAtl...
tests/GrTextureStripAtlasTest.cpp:24: GrTexture* texture =
context->textureProvider()->createTexture(desc, SkBudgeted::kYes, nullptr, 0);
nit, we wrap lines at 100 cols.

https://codereview.chromium.org/2262233002/diff/80001/tests/GrTextureStripAtl...
tests/GrTextureStripAtlasTest.cpp:45: GrTextureStripAtlas* atlas =
GrTextureStripAtlas::GetAtlas(atlasDesc);
Can we check that the atlas's texture is the same as 'texture' above?

[ajuma, 2016-08-24 14:23:37]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 14:23:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 14:48:59]

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

[commit-bot: I haz the power, 2016-08-24 14:49:00]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-08-24 14:56:09]

https://codereview.chromium.org/2262233002/diff/80001/tests/GrTextureStripAtl...
File tests/GrTextureStripAtlasTest.cpp (right):

https://codereview.chromium.org/2262233002/diff/80001/tests/GrTextureStripAtl...
tests/GrTextureStripAtlasTest.cpp:24: GrTexture* texture =
context->textureProvider()->createTexture(desc, SkBudgeted::kYes, nullptr, 0);
On 2016/08/24 13:31:10, bsalomon wrote:
> nit, we wrap lines at 100 cols.

Fixed.

https://codereview.chromium.org/2262233002/diff/80001/tests/GrTextureStripAtl...
tests/GrTextureStripAtlasTest.cpp:45: GrTextureStripAtlas* atlas =
GrTextureStripAtlas::GetAtlas(atlasDesc);
On 2016/08/24 13:31:10, bsalomon wrote:
> Can we check that the atlas's texture is the same as 'texture' above?

Added a check that the texture is the same when we're not in
preferVRAMUseOverFlushes mode.

[bsalomon, 2016-08-24 15:09:20]

lgtm, thanks for fixing this!

[ajuma, 2016-08-24 15:18:02]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2016-08-24 15:18:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 15:19:05]

Description was changed from

==========
Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262233002
==========

to

==========
Make GrTextureStripAtlas flush pending IO on newly acquired texture

GrTextureStripAtlas uses its own lock counts to protect against
overwriting its own earlier writes, but that doesn't protect against
IO that was pending when a texture was first acquired.

BUG=chromium:637678
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2262233002

Committed:
https://skia.googlesource.com/skia/+/95243ebb68cae9ff4821ea57b9a32a194f2a87cf
==========

[commit-bot: I haz the power, 2016-08-24 15:19:07]

Committed patchset #3 (id:100001) as
https://skia.googlesource.com/skia/+/95243ebb68cae9ff4821ea57b9a32a194f2a87cf

[mtklein_C, 2016-08-24 16:56:57]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2016-08-24 16:56:58]

LSAN says this is leaking memory:
https://luci-milo.appspot.com/swarming/task/30d636e0f1be5410/steps/dm/0/stdout

[bsalomon, 2016-08-24 17:17:07]

On 2016/08/24 16:56:58, mtklein_C wrote:
> LSAN says this is leaking memory:
> https://luci-milo.appspot.com/swarming/task/30d636e0f1be5410/steps/dm/0/stdout

Oh yeah, looks like the atlas object is leaked.

[bsalomon, 2016-08-24 17:17:25]

On 2016/08/24 17:17:07, bsalomon wrote:
> On 2016/08/24 16:56:58, mtklein_C wrote:
> > LSAN says this is leaking memory:
> >
https://luci-milo.appspot.com/swarming/task/30d636e0f1be5410/steps/dm/0/stdout
> 
> Oh yeah, looks like the atlas object is leaked.

In the new test

[ajuma, 2016-08-24 17:39:54]

On 2016/08/24 17:17:25, bsalomon wrote:
> On 2016/08/24 17:17:07, bsalomon wrote:
> > On 2016/08/24 16:56:58, mtklein_C wrote:
> > > LSAN says this is leaking memory:
> > >
> https://luci-milo.appspot.com/swarming/task/30d636e0f1be5410/steps/dm/0/stdout
> > 
> > Oh yeah, looks like the atlas object is leaked.
> 
> In the new test

Whoops, looks like I forgot to call unlockRow. Putting up a fix.