WebGL: optimize to draw WebGL on external destination. e.g. 2D Canvas.

WebGL: optimize to draw WebGL on external destination. e.g. 2D Canvas.

DrawingBuffer::paintCompositedResultsToCanvas()/copyToPlatformTexture() reuses
the mailbox which was created for the compositor, instead of creating a temporary mailbox.
It reduces mailbox creation as well as avoids context switching and glFlush.

BUG=331181
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184774

[dshwang, 2014-10-20 14:22:17]

dongseong.hwang@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[dshwang, 2014-10-20 14:22:48]

bajones@, could you review?

[bajones, 2014-10-20 17:49:23]

On 2014/10/20 at 14:22:48, dongseong.hwang wrote:
> bajones@, could you review?

Wouldn't this fail if paintCompositedResultsToCanvas gets called before
prepareMailbox? Is that possible? The fact that the tryjobs came back green is
encouraging, but not foolproof. If that is provably never the case then this
looks okay to me, though I can't give an owner's review. I might still prefer to
have an error case in paintCompositedResultsToCanvas to create a temporary
mailbox if the frontbuffer mailbox hasn't been set yet. 

kbr@ is out this week, so he may be slow to review.

[dshwang, 2014-10-20 19:00:26]

Thanks for review.

On 2014/10/20 17:49:23, bajones wrote:
> Wouldn't this fail if paintCompositedResultsToCanvas gets called before
> prepareMailbox? Is that possible? The fact that the tryjobs came back green is
> encouraging, but not foolproof. If that is provably never the case then this
> looks okay to me, though I can't give an owner's review. I might still prefer
to
> have an error case in paintCompositedResultsToCanvas to create a temporary
> mailbox if the frontbuffer mailbox hasn't been set yet.

See the below comment. In short answer, paintCompositedResultsToCanvas must be
after prepareMailbox.

> kbr@ is out this week, so he may be slow to review.

In this case, can other webgl owners review? e.g. you or zmo@

https://codereview.chromium.org/665073002/diff/1/Source/platform/graphics/gpu...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (left):

https://codereview.chromium.org/665073002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
m_context->produceTextureDirectCHROMIUM(m_frontColorBuffer.textureId,
GL_TEXTURE_2D, bufferMailbox->mailbox.name);
As the method name, this method exists for copying composited result i.e.
|m_frontColorBuffer|. If this CL has a bug, the bug exists already because this
line uses |m_frontColorBuffer|
After reading whole client, I make sure this method cannot be called before
prepareMailbox().

[bajones, 2014-10-20 20:08:19]

On 2014/10/20 at 19:00:26, dongseong.hwang wrote:
> Thanks for review.
> 
> On 2014/10/20 17:49:23, bajones wrote:
> > Wouldn't this fail if paintCompositedResultsToCanvas gets called before
> > prepareMailbox? Is that possible? The fact that the tryjobs came back green
is
> > encouraging, but not foolproof. If that is provably never the case then this
> > looks okay to me, though I can't give an owner's review. I might still
prefer to
> > have an error case in paintCompositedResultsToCanvas to create a temporary
> > mailbox if the frontbuffer mailbox hasn't been set yet.
> 
> See the below comment. In short answer, paintCompositedResultsToCanvas must be
after prepareMailbox.
> 
> > kbr@ is out this week, so he may be slow to review.
> 
> In this case, can other webgl owners review? e.g. you or zmo@
> 
>
https://codereview.chromium.org/665073002/diff/1/Source/platform/graphics/gpu...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (left):
> 
>
https://codereview.chromium.org/665073002/diff/1/Source/platform/graphics/gpu...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
m_context->produceTextureDirectCHROMIUM(m_frontColorBuffer.textureId,
GL_TEXTURE_2D, bufferMailbox->mailbox.name);
> As the method name, this method exists for copying composited result i.e.
|m_frontColorBuffer|. If this CL has a bug, the bug exists already because this
line uses |m_frontColorBuffer|
> After reading whole client, I make sure this method cannot be called before
prepareMailbox().

Neither zmo@ or I are Blink-side owners of the code in question, so we can't
give you the necessary review bits (though it certainly doesn't hurt to have us
take a look.) kbr@ is really the right reviewer for this content, sorry for the
delay.

[dshwang, 2014-10-20 20:33:31]

On 2014/10/20 20:08:19, bajones wrote:
> Neither zmo@ or I are Blink-side owners of the code in question, so we can't
> give you the necessary review bits (though it certainly doesn't hurt to have
us
> take a look.) kbr@ is really the right reviewer for this content, sorry for
the
> delay.

I see. Thank you for explaining. I can wait for kbr@ :)

[dshwang, 2014-10-30 06:16:02]

kbr@, could you review this small CL?

[Ken Russell (switch to Gerrit), 2014-10-31 00:54:21]

Looks good, but one question.

https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:487:
context->waitSyncPoint(mailbox.syncPoint);
Forgive my ignorance, but is it legal to wait multiple times on the same sync
point? I assume it is, but this'll be the first time we do this in this class.

[dshwang, 2014-10-31 06:44:10]

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

[dshwang, 2014-10-31 06:46:07]

https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:487:
context->waitSyncPoint(mailbox.syncPoint);
On 2014/10/31 00:54:21, Ken Russell wrote:
> Forgive my ignorance, but is it legal to wait multiple times on the same sync
> point? I assume it is, but this'll be the first time we do this in this class.

Yes, it's legal. For example, video frame is used by both compositor and WebGL,
which wait for the sync point of the video frame. +danakj

[Ken Russell (switch to Gerrit), 2014-10-31 21:32:54]

lgtm

https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:487:
context->waitSyncPoint(mailbox.syncPoint);
On 2014/10/31 06:46:07, dshwang wrote:
> On 2014/10/31 00:54:21, Ken Russell wrote:
> > Forgive my ignorance, but is it legal to wait multiple times on the same
sync
> > point? I assume it is, but this'll be the first time we do this in this
class.
> 
> Yes, it's legal. For example, video frame is used by both compositor and
WebGL,
> which wait for the sync point of the video frame. +danakj

Right, confirmed by reading CHROMIUM_sync_point.txt.

[dshwang, 2014-11-01 16:09:35]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-11-01 16:10:13]

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

[dshwang, 2014-11-01 16:12:25]

On 2014/10/31 21:32:54, Ken Russell wrote:
> lgtm
> 
>
https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/665073002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:487:
> context->waitSyncPoint(mailbox.syncPoint);
> On 2014/10/31 06:46:07, dshwang wrote:
> > On 2014/10/31 00:54:21, Ken Russell wrote:
> > > Forgive my ignorance, but is it legal to wait multiple times on the same
> sync
> > > point? I assume it is, but this'll be the first time we do this in this
> class.
> > 
> > Yes, it's legal. For example, video frame is used by both compositor and
> WebGL,
> > which wait for the sync point of the video frame. +danakj
> 
> Right, confirmed by reading CHROMIUM_sync_point.txt.

Thank you for review!

[commit-bot: I haz the power, 2014-11-01 16:38:40]

Committed patchset #2 (id:20001) as 184774