Avoid to resolve CMAA for read and draw without content changed or with content committed.

Avoid to resolve CMAA for read and draw without content changed or with content committed.

CMAA will be triggered by every readPixels call, even content does not be  changed or committed.

BUG=535198
TEST=conformance/readinging/fbo-remains-unchanged-after-read-pixels.html

Review-Url: https://codereview.chromium.org/2597933002
Cr-Commit-Position: refs/heads/master@{#467915}
Committed: https://chromium.googlesource.com/chromium/src/+/87ff6dd93ccb11c339b9f9278b6d2ce18a05f32d

[xinghua.cao, 2016-12-22 07:19:40]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 07:19:57]

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-12-22 08:21:31]

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

[commit-bot: I haz the power, 2016-12-22 08:21:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xinghua.cao, 2016-12-22 08:30:06]

Description was changed from

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

BUG=535198
==========

to

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not changed
or content committed.
If so, the result is not expected and reduce performance.

BUG=535198
==========

[xinghua.cao, 2016-12-22 08:33:04]

Description was changed from

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not changed
or content committed.
If so, the result is not expected and reduce performance.

BUG=535198
==========

to

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not changed
or content committed.
If so, the result is not expected and reduce performance.

BUG=535198

TEST=conformance/rendering/gl-scissor-test.html
==========

[xinghua.cao, 2016-12-22 08:33:05]

xinghua.cao@intel.com changed reviewers:
+ dongseong.hwang@chromium.org, qiankun.miao@intel.com, yunchao.he@intel.com

[xinghua.cao, 2016-12-23 05:12:54]

On 2016/12/22 08:33:05, xinghua.cao wrote:
> mailto:xinghua.cao@intel.com changed reviewers:
> + mailto:dongseong.hwang@chromium.org, mailto:qiankun.miao@intel.com,
mailto:yunchao.he@intel.com

Please help to review it, thank you.

[Yang Gu, 2016-12-28 14:27:51]

I think we need a dedicated bug to clarify the problem here, and a webgl test
case to cover it.

[dshwang, 2017-03-21 20:51:28]

sorry for delaying review. I didn't notice because of chromium.org account. The
CL looks good. Could you file new issue and create new webgl conformance test
for it?

[dshwang, 2017-03-21 20:51:52]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com
- dongseong.hwang@chromium.org

[xinghua.cao, 2017-04-07 10:06:45]

Description was changed from

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not changed
or content committed.
If so, the result is not expected and reduce performance.

BUG=535198

TEST=conformance/rendering/gl-scissor-test.html
==========

to

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not be 
changed or committed.

BUG=535198

TEST=conformance/readinging/fbo-remains-unchanged-after-read-pixels.html
==========

[xinghua.cao, 2017-04-07 10:06:46]

xinghua.cao@intel.com changed reviewers:
- dongseong.hwang@intel.com, qiankun.miao@intel.com, yunchao.he@intel.com

[xinghua.cao, 2017-04-07 10:10:26]

xinghua.cao@intel.com changed reviewers:
+ dongseong.hwang@intel.com, kbr@chromium.org, qiankun.miao@intel.com,
yunchao.he@intel.com

[xinghua.cao, 2017-04-07 10:10:39]

On 2017/03/21 20:51:28, dshwang wrote:
> sorry for delaying review. I didn't notice because of http://chromium.org
account. The
> CL looks good. Could you file new issue and create new webgl conformance test
> for it?

Hi, dshwang, I had added a case in conformance test for this issue,
https://github.com/KhronosGroup/WebGL/pull/2354

Hi, ken, please help to review it, thank you.

[xinghua.cao, 2017-04-07 10:40:14]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 10:40:28]

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, 2017-04-07 12:33:13]

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

[commit-bot: I haz the power, 2017-04-07 12:33:14]

Dry run: This issue passed the CQ dry run.

[dshwang, 2017-04-08 02:16:33]

thx for new webgl conformance test.

https://codereview.chromium.org/2597933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:971: if
(m_contentsChanged) {
I think it's better to change
m_antiAliasingMode != None && !m_contentsChangeResolved
in DrawingBuffer::resolveIfNeeded().
What do you think?

[Ken Russell (switch to Gerrit), 2017-04-12 22:08:56]

kbr@chromium.org changed reviewers:
+ kainino@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2017-04-12 22:08:57]

I have to defer to Kai or Mo to review this carefully.

[Zhenyao Mo, 2017-04-12 23:38:08]

https://codereview.chromium.org/2597933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:971: if
(m_contentsChanged) {
On 2017/04/08 02:16:32, dshwang wrote:
> I think it's better to change
> m_antiAliasingMode != None && !m_contentsChangeResolved
> in DrawingBuffer::resolveIfNeeded().
> What do you think?

I agree !m_contentsChangeResolved is a better condition.  However, this is
already a condition in resolveMultisampleFramebufferInternal() which is called
by resolveIfNeeded().  So why do we want to repeat it here? (Maybe I missed
something?)

[xinghua.cao, 2017-04-13 10:49:44]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 10:49:54]

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

[xinghua.cao, 2017-04-13 10:50:24]

https://codereview.chromium.org/2597933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:971: if
(m_contentsChanged) {
On 2017/04/12 23:38:08, Zhenyao Mo wrote:
> On 2017/04/08 02:16:32, dshwang wrote:
> > I think it's better to change
> > m_antiAliasingMode != None && !m_contentsChangeResolved
> > in DrawingBuffer::resolveIfNeeded().
> > What do you think?
> 
> I agree !m_contentsChangeResolved is a better condition.  However, this is
> already a condition in resolveMultisampleFramebufferInternal() which is called
> by resolveIfNeeded().  So why do we want to repeat it here? (Maybe I missed
> something?)

It seems a repeating condition here, thank you.

[commit-bot: I haz the power, 2017-04-13 12:05:33]

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

[commit-bot: I haz the power, 2017-04-13 12:05:34]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2017-04-13 17:36:06]

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1020: if
(anti_aliasing_mode_ != kNone && !contents_change_resolved_)
This is a behavioral change.  Because before even if contents_change_resolved_
is true, we still bind fbo_ (line $1019), but now we don't.

Is it OK? Please do a bit careful study and provide an answer (with explanation)
here.

[xinghua.cao, 2017-04-26 10:46:37]

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1020: if
(anti_aliasing_mode_ != kNone && !contents_change_resolved_)
On 2017/04/13 17:36:06, Zhenyao Mo wrote:
> This is a behavioral change.  Because before even if contents_change_resolved_
> is true, we still bind fbo_ (line $1019), but now we don't.
> 
> Is it OK? Please do a bit careful study and provide an answer (with
explanation)
> here.

Zhenyao, I had made a more carefully investigation on this issue. List my
comment as below,
I think DrawingBuffer::ResolveMultisampleFramebufferInternal is used to only
resolve the multisampled buffer into back_color_buffer_ texture.
ResolveIfNeeded function is called by
DrawingBuffer::PrepareTextureMailboxInternal,
DrawingBuffer::CopyToPlatformTexture and
DrawingBuffer::ResolveAndBindForReadAndDraw.
DrawingBuffer::PrepareTextureMailboxInternal and
DrawingBuffer::CopyToPlatformTexture only operate back_color_buffer_ texture, do
not operate fbo, and do not require bind to fbo_.
DrawingBuffer::ResolveAndBindForReadAndDraw had already binded fbo_ to
GL_FRAMEBUFFER target after resolving multisample, because it need to read and
draw fbo_.
In summary, I think binding fbo_ is only needed if we want to operate the fbo_.
if we only want to operate attached texture, does not need bind fbo_, and I
think only read and draw need to bind fbo_ to GL_FRAMEBUFFER after resolve
multisample. And if one function wants operate fbo_, it should explicitly bind
fbo_ in its body, like DrawingBuffer::ResolveAndBindForReadAndDraw. Is my
understanding right?
I think that binding fbo_ (line $1014) is only useful for
ApplyScreenSpaceAntialiasingCHROMIUM (CMAA operation, anti_aliasing_mode is
kScreenSpaceAntialiasing) if GL_FRAMEBUFFER target is currently binding to zero,
if not, it is already binding on fbo_.

By the way, I have another question. Why should bind to fbo_ after every resolve
multisample? If it was originally binding to multisample_fbo_, after resolve
multsample, it is binding to fbo_. Is that right?

[Ken Russell (switch to Gerrit), 2017-04-28 00:10:00]

LGTM

Mo's out this week so I'll finish this review for him.

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1020: if
(anti_aliasing_mode_ != kNone && !contents_change_resolved_)
On 2017/04/26 10:46:37, xinghua.cao wrote:
> On 2017/04/13 17:36:06, Zhenyao Mo wrote:
> > This is a behavioral change.  Because before even if
contents_change_resolved_
> > is true, we still bind fbo_ (line $1019), but now we don't.
> > 
> > Is it OK? Please do a bit careful study and provide an answer (with
> explanation)
> > here.
> 
> Zhenyao, I had made a more carefully investigation on this issue. List my
> comment as below,
> I think DrawingBuffer::ResolveMultisampleFramebufferInternal is used to only
> resolve the multisampled buffer into back_color_buffer_ texture.
> ResolveIfNeeded function is called by
> DrawingBuffer::PrepareTextureMailboxInternal,
> DrawingBuffer::CopyToPlatformTexture and
> DrawingBuffer::ResolveAndBindForReadAndDraw.
> DrawingBuffer::PrepareTextureMailboxInternal and
> DrawingBuffer::CopyToPlatformTexture only operate back_color_buffer_ texture,
do
> not operate fbo, and do not require bind to fbo_.
> DrawingBuffer::ResolveAndBindForReadAndDraw had already binded fbo_ to
> GL_FRAMEBUFFER target after resolving multisample, because it need to read and
> draw fbo_.
> In summary, I think binding fbo_ is only needed if we want to operate the
fbo_.
> if we only want to operate attached texture, does not need bind fbo_, and I
> think only read and draw need to bind fbo_ to GL_FRAMEBUFFER after resolve
> multisample. And if one function wants operate fbo_, it should explicitly bind
> fbo_ in its body, like DrawingBuffer::ResolveAndBindForReadAndDraw. Is my
> understanding right?

This sounds correct. One note: DrawingBuffer::PrepareTextureMailboxInternal
calls ResolveIfNeeded and immediately calls FinishPrepareTextureMailboxGpu.
FinishPrepareTextureMailboxGpu calls AttachColorBufferToReadFramebuffer -- which
sounds like it's relying on the currently bound framebuffer -- but in fact it
binds fbo_ itself.

> I think that binding fbo_ (line $1014) is only useful for
> ApplyScreenSpaceAntialiasingCHROMIUM (CMAA operation, anti_aliasing_mode is
> kScreenSpaceAntialiasing) if GL_FRAMEBUFFER target is currently binding to
zero,
> if not, it is already binding on fbo_.

I agree. It looks like that could be done only if we're going to call
ApplyScreenSpaceAntialiasingCHROMIUM.

> By the way, I have another question. Why should bind to fbo_ after every
resolve
> multisample? If it was originally binding to multisample_fbo_, after resolve
> multsample, it is binding to fbo_. Is that right?

I don't see a good reason for this and think this could be changed.

[xinghua.cao, 2017-04-28 01:58:48]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 02:00:02]

Dry run: CQ is trying da patch.

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

[dshwang, 2017-04-28 02:08:29]

non-owner lgtm too. thank you!

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1020: if
(anti_aliasing_mode_ != kNone && !contents_change_resolved_)
On 2017/04/28 00:10:00, Ken Russell wrote:
> On 2017/04/26 10:46:37, xinghua.cao wrote:
> > I think that binding fbo_ (line $1014) is only useful for
> > ApplyScreenSpaceAntialiasingCHROMIUM (CMAA operation, anti_aliasing_mode is
> > kScreenSpaceAntialiasing) if GL_FRAMEBUFFER target is currently binding to
> zero,
> > if not, it is already binding on fbo_.
> 
> I agree. It looks like that could be done only if we're going to call
> ApplyScreenSpaceAntialiasingCHROMIUM.

I think so.

> > By the way, I have another question. Why should bind to fbo_ after every
> resolve
> > multisample? If it was originally binding to multisample_fbo_, after resolve
> > multsample, it is binding to fbo_. Is that right?
> 
> I don't see a good reason for this and think this could be changed.

I agree. as ResolveIfNeeded() is private method, public caller must take care of
fbo state.

[xinghua.cao, 2017-04-28 02:40:08]

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1070: if
(source_buffer == kFrontBuffer && front_color_buffer_) {
Hi, all, another question here.
I think if context is currently binding to default fbo, GL_FRAMEBUFFER target
must be binded either fbo_ or multisample_fbo_, and never binded to zero.
But here if this condition is true, will create a new fbo object, when calling
gl_->DeleteFramebuffers(1, &fbo) below, GL_FRAMEBUFFER target is implicitly
binded to zero. Binding status had been changed here.
Is it right here?

[dshwang, 2017-04-28 02:48:42]

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1020: if
(anti_aliasing_mode_ != kNone && !contents_change_resolved_)
On 2017/04/28 02:08:29, dshwang wrote:
> On 2017/04/28 00:10:00, Ken Russell wrote:
> > On 2017/04/26 10:46:37, xinghua.cao wrote:
> > > I think that binding fbo_ (line $1014) is only useful for
> > > ApplyScreenSpaceAntialiasingCHROMIUM (CMAA operation, anti_aliasing_mode
is
> > > kScreenSpaceAntialiasing) if GL_FRAMEBUFFER target is currently binding to
> > zero,
> > > if not, it is already binding on fbo_.
> > 
> > I agree. It looks like that could be done only if we're going to call
> > ApplyScreenSpaceAntialiasingCHROMIUM.
> 
> I think so.
> 
> > > By the way, I have another question. Why should bind to fbo_ after every
> > resolve
> > > multisample? If it was originally binding to multisample_fbo_, after
resolve
> > > multsample, it is binding to fbo_. Is that right?
> > 
> > I don't see a good reason for this and think this could be changed.
> 
> I agree. as ResolveIfNeeded() is private method, public caller must take care
of
> fbo state.

ResolveMultisampleFramebufferInternal() set fbo dirty to state_restorer_, so
it's ok to bind any FBO in this moment.

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1070: if
(source_buffer == kFrontBuffer && front_color_buffer_) {
On 2017/04/28 02:40:08, xinghua.cao wrote:
> Hi, all, another question here.
> I think if context is currently binding to default fbo, GL_FRAMEBUFFER target
> must be binded either fbo_ or multisample_fbo_, and never binded to zero.
> But here if this condition is true, will create a new fbo object, when calling
> gl_->DeleteFramebuffers(1, &fbo) below, GL_FRAMEBUFFER target is implicitly
> binded to zero. Binding status had been changed here.
> Is it right here?

public methods are guard by |state_restorer_| and line1069 checks fbo is dirty.

[xinghua.cao, 2017-04-28 02:56:02]

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2597933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1070: if
(source_buffer == kFrontBuffer && front_color_buffer_) {
On 2017/04/28 02:48:42, dshwang wrote:
> On 2017/04/28 02:40:08, xinghua.cao wrote:
> > Hi, all, another question here.
> > I think if context is currently binding to default fbo, GL_FRAMEBUFFER
target
> > must be binded either fbo_ or multisample_fbo_, and never binded to zero.
> > But here if this condition is true, will create a new fbo object, when
calling
> > gl_->DeleteFramebuffers(1, &fbo) below, GL_FRAMEBUFFER target is implicitly
> > binded to zero. Binding status had been changed here.
> > Is it right here?
> 
> public methods are guard by |state_restorer_| and line1069 checks fbo is
dirty.

Yes, here is no problem. Thank you @dshwang.

[commit-bot: I haz the power, 2017-04-28 05:43:12]

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

[commit-bot: I haz the power, 2017-04-28 05:43:13]

Dry run: This issue passed the CQ dry run.

[xinghua.cao, 2017-04-28 06:00:12]

The CQ bit was checked by xinghua.cao@intel.com

[commit-bot: I haz the power, 2017-04-28 06:00:27]

CQ is trying da patch.

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

[Yang Gu, 2017-04-28 06:01:21]

Yang is on vacation from Apr 24 to May 1, and may have no email access during
this period of time.

[commit-bot: I haz the power, 2017-04-28 06:04:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493359212306970,
"parent_rev": "840f5cbdf11218a2eeb85e5f94573907a612f45b", "commit_rev":
"87ff6dd93ccb11c339b9f9278b6d2ce18a05f32d"}

[commit-bot: I haz the power, 2017-04-28 06:04:43]

Description was changed from

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not be 
changed or committed.

BUG=535198

TEST=conformance/readinging/fbo-remains-unchanged-after-read-pixels.html
==========

to

==========
Avoid to resolve CMAA for read and draw without content changed or with content
committed.

CMAA will be triggered by every readPixels call, even content does not be 
changed or committed.

BUG=535198

TEST=conformance/readinging/fbo-remains-unchanged-after-read-pixels.html

Review-Url: https://codereview.chromium.org/2597933002
Cr-Commit-Position: refs/heads/master@{#467915}
Committed:
https://chromium.googlesource.com/chromium/src/+/87ff6dd93ccb11c339b9f9278b6d...
==========

[commit-bot: I haz the power, 2017-04-28 06:04:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/87ff6dd93ccb11c339b9f9278b6d...