Defer glClear to just before draw call

Defer glClear to just before draw call

Remove some DO_DEFERRED_CLEAR call to avoid call glClear separately, like this:
glBindFramebuffer(1)
glClear
glBindFramebuffer(2)
glClear
glBindFramebuffer(1)
glDrawXXX
glBindFramebuffer(2)
glDrawXXX

These call sequences may need read and write memory back and forth.

If we call DO_DEFERRED_CLEAR just before draw call, we can bind, clear and draw in one go.
e.g.
glBindFramebuffer(1)
glClear
glDrawXXX
glBindFramebuffer(2)
glClear
glDrawXXX

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/c3c06a13e69b90d4cc1d543853504072d363ae8b

[Wasim.Abbas, 2015-05-01 11:19:06]

wasim.abbas@arm.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2015-05-04 15:12:34]

Are these removals legal? In these cases it looks to me like if we got into
these entries and haven't issued the clear then we need to, but maybe I'm
missing something.

[Joel.Liang, 2015-05-07 06:42:30]

On 2015/05/04 15:12:34, bsalomon wrote:
> Are these removals legal? In these cases it looks to me like if we got into
> these entries and haven't issued the clear then we need to, but maybe I'm
> missing something.

DO_DEFERRED_CLEAR is use for clear canvas/device at the first time it use. 
For the second time we call DO_DEFERRED_CLEAR, it will do nothing because the
'fNeedClear' variable become false.

If we can ensure call DO_DEFERRED_CLEAR before every draw call
and we always draw something before readPixels.(Never read back undefined
pixels)
It's safe to remove those DO_DEFERRED_CLEAR calls.

I saw the comments before the implementation of prepareDraw,
We have ensure call prepareDraw before every draw call and we do call
DO_DEFERRED_CLEAR within this method.

For the DO_DEFERRED_CLEAR in method onReadPixels, we can remove it unless it's
legal to read back
undefined pixels and we have defined 0 as default color.

For the DO_DEFERRED_CLEAR in method accessRenderTarget, that's the root cause
for call glClear separately. Because sometimes we just want to access the
RenderTarget object but not draw to it.

For the DO_DEFERRED_CLEAR in method flush, it's unnecessary because we neither
draw to it nor read from it.

[bsalomon, 2015-05-07 14:21:54]

On 2015/05/07 06:42:30, joel.liang wrote:
> On 2015/05/04 15:12:34, bsalomon wrote:
> > Are these removals legal? In these cases it looks to me like if we got into
> > these entries and haven't issued the clear then we need to, but maybe I'm
> > missing something.
> 
> DO_DEFERRED_CLEAR is use for clear canvas/device at the first time it use. 
> For the second time we call DO_DEFERRED_CLEAR, it will do nothing because the
> 'fNeedClear' variable become false.
> 
> If we can ensure call DO_DEFERRED_CLEAR before every draw call
> and we always draw something before readPixels.(Never read back undefined
> pixels)
> It's safe to remove those DO_DEFERRED_CLEAR calls.
> 
> I saw the comments before the implementation of prepareDraw,
> We have ensure call prepareDraw before every draw call and we do call
> DO_DEFERRED_CLEAR within this method.
> 
> For the DO_DEFERRED_CLEAR in method onReadPixels, we can remove it unless it's
> legal to read back
> undefined pixels and we have defined 0 as default color.

If fNeedClear is true then the device is defined to have a color of 0, so I
don't think we can remove this one.

> 
> For the DO_DEFERRED_CLEAR in method accessRenderTarget, that's the root cause
> for call glClear separately. Because sometimes we just want to access the
> RenderTarget object but not draw to it.

Curious when this happens. I'm not opposed to removing the clear here but want
to understand the pattern that leads to calling accessRenderTarget() on device
we haven't rendered to yet.

> 
> For the DO_DEFERRED_CLEAR in method flush, it's unnecessary because we neither
> draw to it nor read from it.

flush() is supposed to be called when the client is ready to composite the
device's texture, so we're assuming that the client is about to read from it.
Are we calling flush() in cases where the device hasn't been rendered to yet?

[Joel.Liang, 2015-05-08 09:09:39]

On 2015/05/07 14:21:54, bsalomon wrote:
> On 2015/05/07 06:42:30, joel.liang wrote:
> > On 2015/05/04 15:12:34, bsalomon wrote:
> > > Are these removals legal? In these cases it looks to me like if we got
into
> > > these entries and haven't issued the clear then we need to, but maybe I'm
> > > missing something.
> > 
> > DO_DEFERRED_CLEAR is use for clear canvas/device at the first time it use. 
> > For the second time we call DO_DEFERRED_CLEAR, it will do nothing because
the
> > 'fNeedClear' variable become false.
> > 
> > If we can ensure call DO_DEFERRED_CLEAR before every draw call
> > and we always draw something before readPixels.(Never read back undefined
> > pixels)
> > It's safe to remove those DO_DEFERRED_CLEAR calls.
> > 
> > I saw the comments before the implementation of prepareDraw,
> > We have ensure call prepareDraw before every draw call and we do call
> > DO_DEFERRED_CLEAR within this method.
> > 
> > For the DO_DEFERRED_CLEAR in method onReadPixels, we can remove it unless
it's
> > legal to read back
> > undefined pixels and we have defined 0 as default color.
> 
> If fNeedClear is true then the device is defined to have a color of 0, so I
> don't think we can remove this one.

OK, if we defined to have default color of 0, we should keep this
DO_DEFERRED_CLEAR.
Although it's rare to read back pixels without draw anything to it.

> 
> > 
> > For the DO_DEFERRED_CLEAR in method accessRenderTarget, that's the root
cause
> > for call glClear separately. Because sometimes we just want to access the
> > RenderTarget object but not draw to it.
> 
> Curious when this happens. I'm not opposed to removing the clear here but want
> to understand the pattern that leads to calling accessRenderTarget() on device
> we haven't rendered to yet.

Here is the backtrace for one of the patterns we calling accessRenderTarget()
without rendered to the device.
#0  SkGpuDevice::accessRenderTarget at ../../../../src/gpu/SkGpuDevice.cpp:304
#1  SkCanvas::getGrContext          at ../../../../src/core/SkCanvas.cpp:1562
#2  SkMultiPictureDraw::add         at
../../../../src/core/SkMultiPictureDraw.cpp:76
#3  create_content                  at ../../../../gm/multipicturedraw.cpp:372

> 
> > 
> > For the DO_DEFERRED_CLEAR in method flush, it's unnecessary because we
neither
> > draw to it nor read from it.
> 
> flush() is supposed to be called when the client is ready to composite the
> device's texture, so we're assuming that the client is about to read from it.
> Are we calling flush() in cases where the device hasn't been rendered to yet?

Just like the DO_DEFERRED_CLEAR in onReadPixels, if we defined to have default
color of 0,
we should keep this DO_DEFERRED_CLEAR too.
This is the case I found that we calling flush() where the device hasn't been
rendered to.
#0  SkGpuDevice::flush                  at
../../../../src/gpu/SkGpuDevice.cpp:1927
#1  SkCanvas::flush                     at ../../../../src/core/SkCanvas.cpp:627
#2  SkDeferredDevice::flush             at
../../../../src/utils/SkDeferredCanvas.cpp:367
#3  SkDeferredDevice::newImageSnapshot  at
../../../../src/utils/SkDeferredCanvas.cpp:409
#4  SkDeferredCanvas::newImageSnapshot  at
../../../../src/utils/SkDeferredCanvas.cpp:635
#5  TestDeferredCanvasSetSurface        at
../../../../tests/DeferredCanvasTest.cpp:837
#6  test_DeferredCanvas_GPU             at
../../../../tests/DeferredCanvasTest.cpp:943
#7  run_test                            at ../../../../dm/DM.cpp:638

[bsalomon, 2015-05-12 19:08:02]

lgtm

[bsalomon, 2015-05-14 15:18:19]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-05-14 15:18:26]

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

[commit-bot: I haz the power, 2015-05-14 15:24:57]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/c3c06a13e69b90d4cc1d543853504072d363ae8b

[robertphillips, 2015-05-14 15:53:44]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1136393005/ by robertphillips@google.com.

The reason for reverting is: This CL seems to be triggering the assert: 
src/gpu/SkGpuDevice.cpp:319: failed assertion "!fNeedClear"
on, at least, Mac & Ubuntu.

.

[robertphillips, 2015-05-14 15:56:12]

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

[robertphillips, 2015-05-14 15:56:13]

Here is the Mac call stack:

unit test  SkImage_NewFromTexture../../src/gpu/SkGpuDevice.cpp:319: failed
assertion "!fNeedClear"

Signal 11:
_sigtramp (+0x1a)
SkGpuDevice::replaceRenderTarget(bool) (+0x71)
SkSurface_Gpu::onCopyOnWrite(SkSurface::ContentChangeMode) (+0xeb)
SkSurface_Base::aboutToDraw(SkSurface::ContentChangeMode) (+0xd0)
SkCanvas::predrawNotify() (+0x3a)
SkCanvas::internalDrawPaint(SkPaint const&) (+0x34)
SkCanvas::onDrawPaint(SkPaint const&) (+0x10e)
SkCanvas::drawPaint(SkPaint const&) (+0x32)
SkCanvas::drawColor(unsigned int, SkXfermode::Mode) (+0x192)
SkCanvas::clear(unsigned int) (+0x20)
TestSurfaceCopyOnWrite(skiatest::Reporter*, SurfaceType, GrContext*) (+0x284)
test_Surface(skiatest::Reporter*, GrContextFactory*) (+0x168)
run_test(skiatest::Test*) (+0xf9)
run_enclave_and_gpu_tests(SkTArray<Task, false>*) (+0x50)
(anonymous namespace)::ThreadPool::Loop(void*) (+0xee)
thread_start(void*) (+0x54)
_pthread_body (+0x8a)
_pthread_struct_init (+0x0)

from:
http://build.chromium.org/p/client.skia/builders/Test-Mac10.9-Clang-MacMini6....

Here is Ubuntu:
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...

Here is Windows:
http://build.chromium.org/p/client.skia/builders/Test-Win7-MSVC-ShuttleA-GPU-...

[robertphillips, 2015-05-14 16:42:00]

This CL also seems to have introduced a visual artifact in the convex_poly_clip
on the Nexus5 (a small extra triangle appearing in the bottom row).

[robertphillips, 2015-05-14 16:43:11]

And added a stray rectangle to the imagealphathreshold GM.

[Joel.Liang, 2015-05-18 08:46:22]

On 2015/05/14 16:42:00, robertphillips wrote:
> This CL also seems to have introduced a visual artifact in the
convex_poly_clip
> on the Nexus5 (a small extra triangle appearing in the bottom row).

On 2015/05/14 16:43:11, robertphillips wrote:
> And added a stray rectangle to the imagealphathreshold GM.

I can not reproduce this two artifacts on Nexus5 with Android 4.4.2.
Which Android version you are using for GM tests?