WebGL: Don't destroy mailbox textures in the destructor until they're released.

WebGL: Don't destroy mailbox textures in the destructor until they're released.

This CL fixes the bug that WebGL makes the compositor get following gl error.
[20074:20074:0317/145233:1532049594488:ERROR:gles2_cmd_decoder.cc(10071)]
[.Compositor-0x731c2a2e9a0]GL ERROR :GL_INVALID_OPERATION :
glConsumeTextureCHROMIUM: invalid mailbox name

Currently, DrawingBuffer deletes all textures, even if some textures are
used by the clients. This CL makes "release callback" be able to delete remaining
textures even after the DrawingBuffer is destructed.

This pattern is very similar to that used by accelerated 2d canvas and HW Video. See Canvas2DLayerBridge.

TEST=DrawingBufferTest.verifyDestructionCompleteAfterAllMailboxesReleased,
Reload http://browsertests.herokuapp.com/webgl/webglPerformance.html in all platforms.
BUG=344393
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172520

[dshwang, 2014-02-17 20:41:09]

Working in progress.

This CL relieves following err msgs:
[20400:20400:0217/223233:44920417703:ERROR:gles2_cmd_decoder.cc(10067)]
[.Compositor-0x374d957f0c60]GL ERROR :GL_INVALID_OPERATION :
glConsumeTextureCHROMIUM: invalid mailbox name
[20400:20400:0217/223233:44920417943:ERROR:gles2_cmd_decoder.cc(5994)]
[.Compositor-0x374d957f0c60]RENDER WARNING: texture bound to texture unit 0 is
not renderable. It maybe non-power-of-2 and have incompatible texture filtering
or is not 'texture complete'
[20400:20400:0217/223233:44920421226:ERROR:gles2_cmd_decoder.cc(5994)]
[.Compositor-0x374d957f0c60]RENDER WARNING: texture bound to texture unit 0 is
not renderable. It maybe non-power-of-2 and have incompatible texture filtering
or is not 'texture complete'

You can reproduce it
1. > content_shell http://browsertests.herokuapp.com/webgl/webglPerformance.html
2. load another page.

crrev.com/163773007/ and crrev.com/167893008/ are preparation of this CL.
Accelerated 2D Canvas and Accelerated Video also need something like this CL.

Before going ahead, could I listen to your opinion? mainly because now
WebExternalTextureLayerClient API is hard to use.

[Justin Novosad, 2014-02-18 18:07:33]

https://codereview.chromium.org/169933002/diff/1/Source/core/html/canvas/WebG...
File Source/core/html/canvas/WebGLRenderingContext.h (right):

https://codereview.chromium.org/169933002/diff/1/Source/core/html/canvas/WebG...
Source/core/html/canvas/WebGLRenderingContext.h:388:
blink::WebGraphicsContext3D* m_context;
IMHO: the ownership model would be clearer and more robust if you got rid of
this member, and instead accessed the context through m_drawingBuffer.

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

https://codereview.chromium.org/169933002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:234: releaseSelf(mailbox);
naming nit: releaseSelfIfNeeded or releaseSelfIfFinalMailboxReleased

[Ken Russell (switch to Gerrit), 2014-02-18 18:35:27]

Sorry but I think this is the wrong way of approaching this problem. Having the
DrawingBuffer instance keep itself alive is confusing and error-prone. If the
compositor decides for some reason to not send mailboxReleased messages during
page teardown then the DrawingBuffer instances will be leaked for the lifetime
of the renderer process.

A better approach would be to rethink how WebExternalTextureMailbox instances
specifically are deleted during page teardown. If the compositor's guaranteed to
delete the textures associated with all outstanding mailboxes then the
DrawingBuffer can just stop deleting them. Perhaps piman, danakj and others can
advise.

[piman, 2014-02-18 19:28:18]

On 2014/02/18 18:35:27, Ken Russell wrote:
> Sorry but I think this is the wrong way of approaching this problem. Having
the
> DrawingBuffer instance keep itself alive is confusing and error-prone. If the
> compositor decides for some reason to not send mailboxReleased messages during
> page teardown then the DrawingBuffer instances will be leaked for the lifetime
> of the renderer process.

No, I think this is the correct approach, similar to what we did for Canvas (and
other places in Chrome).
The compositor guarantees it will send mailboxReleased. 

> A better approach would be to rethink how WebExternalTextureMailbox instances
> specifically are deleted during page teardown. If the compositor's guaranteed
to
> delete the textures associated with all outstanding mailboxes then the
> DrawingBuffer can just stop deleting them. Perhaps piman, danakj and others
can
> advise.

The compositor will delete its mapping of the mailbox, but that's not the issue
here.
Textures are refcounted, so the client (here DrawingBuffer) also needs to delete
its mapping.

However, if it deletes its mapping before the compositor had a chance to map
(consume) it, the texture will go away, leading to the compositor failing to map
it. So the client needs to keep it alive until the compositor returned it.

[dshwang, 2014-02-18 20:45:54]

On 2014/02/18 19:28:18, piman wrote:
> On 2014/02/18 18:35:27, Ken Russell wrote:
> > Sorry but I think this is the wrong way of approaching this problem. Having
> the
> > DrawingBuffer instance keep itself alive is confusing and error-prone. If
the
> > compositor decides for some reason to not send mailboxReleased messages
during
> > page teardown then the DrawingBuffer instances will be leaked for the
lifetime
> > of the renderer process.

To avoid this, we can use timer as fallback.
For example, after 5 sec, we forcely delete the DrawingBuffer instances with
ASSERT_NOT_REACHED().
Even if we trust the compositor must call mailboxReleased, we can remove the
last possibility of leak.

How do you think about timer fallback?

> No, I think this is the correct approach, similar to what we did for Canvas
(and
> other places in Chrome).
> The compositor guarantees it will send mailboxReleased. 

Thank you for clarifying.


> > A better approach would be to rethink how WebExternalTextureMailbox
instances
> > specifically are deleted during page teardown. If the compositor's
guaranteed
> to
> > delete the textures associated with all outstanding mailboxes then the
> > DrawingBuffer can just stop deleting them. Perhaps piman, danakj and others
> can
> > advise.

I considered WebExternalTextureMailbox deletes remained mailboxes after
DrawingBuffer is deleted. It is hard because WebGraphicsContext3D was also
deleted with DrawingBuffer.
Moreover, WebExternalTextureMailbox needs to consider software/hardware mailbox
and WebGL/Canvas.
So I decided DrawingBuffer is the best place.

> The compositor will delete its mapping of the mailbox, but that's not the
issue
> here.
> Textures are refcounted, so the client (here DrawingBuffer) also needs to
delete
> its mapping.
> 
> However, if it deletes its mapping before the compositor had a chance to map
> (consume) it, the texture will go away, leading to the compositor failing to
map
> it. So the client needs to keep it alive until the compositor returned it.

Thank you for detail explanation. I'll polish this approach and add tests.


On 2014/02/18 18:07:33, junov wrote:
>
https://codereview.chromium.org/169933002/diff/1/Source/core/html/canvas/WebG...
> File Source/core/html/canvas/WebGLRenderingContext.h (right):
> 
>
https://codereview.chromium.org/169933002/diff/1/Source/core/html/canvas/WebG...
> Source/core/html/canvas/WebGLRenderingContext.h:388:
> blink::WebGraphicsContext3D* m_context;
> IMHO: the ownership model would be clearer and more robust if you got rid of
> this member, and instead accessed the context through m_drawingBuffer.

Yes, I transfer the ownership of context from WebGLRenderingContext to
DrawingBuffer.
So that's good idea, but imho m_context is needed for performance.

m_context can be replaced to webGraphicsContext3D(), which can not be inlined,
because WebGLRenderingContext forward declares DrawingBuffer.
blink::WebGraphicsContext3D* WebGLRenderingContext::webGraphicsContext3D() const
{
    return isContextLost() ? 0 : m_drawingBuffer->context();
}

m_context is used ~530 times in WebGLRenderingContext. I think if I replace
m_context, WebGL maybe impact performance badly.


>
https://codereview.chromium.org/169933002/diff/1/Source/platform/graphics/gpu...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/169933002/diff/1/Source/platform/graphics/gpu...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:234: releaseSelf(mailbox);
> naming nit: releaseSelfIfNeeded or releaseSelfIfFinalMailboxReleased

Thank you for good name, I buy it :)

[piman, 2014-02-18 20:54:53]

On Tue, Feb 18, 2014 at 12:45 PM, <dongseong.hwang@intel.com> wrote:

> On 2014/02/18 19:28:18, piman wrote:
>
>> On 2014/02/18 18:35:27, Ken Russell wrote:
>> > Sorry but I think this is the wrong way of approaching this problem.
>> Having
>> the
>> > DrawingBuffer instance keep itself alive is confusing and error-prone.
>> If
>>
> the
>
>> > compositor decides for some reason to not send mailboxReleased messages
>>
> during
>
>> > page teardown then the DrawingBuffer instances will be leaked for the
>>
> lifetime
>
>> > of the renderer process.
>>
>
> To avoid this, we can use timer as fallback.
> For example, after 5 sec, we forcely delete the DrawingBuffer instances
> with
> ASSERT_NOT_REACHED().
> Even if we trust the compositor must call mailboxReleased, we can remove
> the
> last possibility of leak.
>
> How do you think about timer fallback?


No, please no timer.


>
>
>  No, I think this is the correct approach, similar to what we did for
>> Canvas
>>
> (and
>
>> other places in Chrome).
>> The compositor guarantees it will send mailboxReleased.
>>
>
> Thank you for clarifying.
>
>
>
>  > A better approach would be to rethink how WebExternalTextureMailbox
>>
> instances
>
>> > specifically are deleted during page teardown. If the compositor's
>>
> guaranteed
>
>> to
>> > delete the textures associated with all outstanding mailboxes then the
>> > DrawingBuffer can just stop deleting them. Perhaps piman, danakj and
>> others
>> can
>> > advise.
>>
>
> I considered WebExternalTextureMailbox deletes remained mailboxes after
> DrawingBuffer is deleted. It is hard because WebGraphicsContext3D was also
> deleted with DrawingBuffer.
> Moreover, WebExternalTextureMailbox needs to consider software/hardware
> mailbox
> and WebGL/Canvas.
> So I decided DrawingBuffer is the best place.
>
>
>  The compositor will delete its mapping of the mailbox, but that's not the
>>
> issue
>
>> here.
>> Textures are refcounted, so the client (here DrawingBuffer) also needs to
>>
> delete
>
>> its mapping.
>>
>
>  However, if it deletes its mapping before the compositor had a chance to
>> map
>> (consume) it, the texture will go away, leading to the compositor failing
>> to
>>
> map
>
>> it. So the client needs to keep it alive until the compositor returned it.
>>
>
> Thank you for detail explanation. I'll polish this approach and add tests.
>
>
>
> On 2014/02/18 18:07:33, junov wrote:
>
> https://codereview.chromium.org/169933002/diff/1/Source/core/html/canvas/
> WebGLRenderingContext.h
>
>> File Source/core/html/canvas/WebGLRenderingContext.h (right):
>>
>
>
> https://codereview.chromium.org/169933002/diff/1/Source/core/html/canvas/
> WebGLRenderingContext.h#newcode388
>
>> Source/core/html/canvas/WebGLRenderingContext.h:388:
>> blink::WebGraphicsContext3D* m_context;
>> IMHO: the ownership model would be clearer and more robust if you got rid
>> of
>> this member, and instead accessed the context through m_drawingBuffer.
>>
>
> Yes, I transfer the ownership of context from WebGLRenderingContext to
> DrawingBuffer.
> So that's good idea, but imho m_context is needed for performance.
>
> m_context can be replaced to webGraphicsContext3D(), which can not be
> inlined,
> because WebGLRenderingContext forward declares DrawingBuffer.
> blink::WebGraphicsContext3D* WebGLRenderingContext::webGraphicsContext3D()
> const
> {
>     return isContextLost() ? 0 : m_drawingBuffer->context();
> }
>
> m_context is used ~530 times in WebGLRenderingContext. I think if I replace
> m_context, WebGL maybe impact performance badly.
>
>
>
>
> https://codereview.chromium.org/169933002/diff/1/Source/
> platform/graphics/gpu/DrawingBuffer.cpp
>
>> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
>>
>
>
> https://codereview.chromium.org/169933002/diff/1/Source/
> platform/graphics/gpu/DrawingBuffer.cpp#newcode234
>
>> Source/platform/graphics/gpu/DrawingBuffer.cpp:234: releaseSelf(mailbox);
>> naming nit: releaseSelfIfNeeded or releaseSelfIfFinalMailboxReleased
>>
>
> Thank you for good name, I buy it :)
>
>
> https://codereview.chromium.org/169933002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2014-02-18 21:05:23]

If we want to make sure the implementation is leak proof, we could always
implement an instance counter (debug builds only) and assert that it is zero
when the renderer terminates.

[dshwang, 2014-02-19 19:38:32]

Accelerated 2D Canvas already handles this situation. Canvas2DLayerBridge is
destructed after all mailboxes are released.
I follow Canvas2DLayerBridge's pattern for DrawingBuffer.

Add DrawingBufferTest for this change.
This CL waits for http://crrev.com/163773007/ and http://crrev.com/167893008/

On 2014/02/18 20:54:53, piman wrote:
> No, please no timer.

got it!


On 2014/02/18 21:05:23, junov wrote:
> If we want to make sure the implementation is leak proof, we could always
> implement an instance counter (debug builds only) and assert that it is zero
> when the renderer terminates.

Thank you. I use WTF::RefCountedLeakCounter to check leak like other important
objects.

[Justin Novosad, 2014-02-19 21:10:17]

This change seems reasonable to me. It is analogous to the solution we shipped
for 2D canvas several months ago.

[Ken Russell (switch to Gerrit), 2014-03-04 03:21:10]

I'm sorry for holding up this review. If accelerated 2D canvas is already doing
this, then OK, let's make the change for WebGL.

Is this going to require rebaselining after
https://codereview.chromium.org/163773007/ lands?

[Ken Russell (switch to Gerrit), 2014-03-06 02:06:38]

Please send this out for re-review once the dependent CL
https://codereview.chromium.org/163773007/ has re-landed. Thanks.

[dshwang, 2014-04-07 19:20:23]

On 2014/03/06 02:06:38, Ken Russell wrote:
> Please send this out for re-review once the dependent CL
> https://codereview.chromium.org/163773007/ has re-landed. Thanks.

Hi, I landed crrev.com/163773007 and then update this CL.
could you review again?

[Justin Novosad, 2014-04-09 15:34:34]

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
m_textureMailboxes.remove(j);
I think this is unnecessary complexity. Wouldn't it be sufficient to just have
m_textureMailboxes.clear() in the destructor?  Deleting just the textures as
they are released is good enough IMHO. You could remove the entire inner for
loop here, and avoid O(N^2)

[dshwang, 2014-04-09 18:05:50]

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
m_textureMailboxes.remove(j);
Thank you for review.

As you mentioned, "nested for" is not actually needed. I did it to clarify code
flow.
For example,
1. if m_textureMailboxes.isEmpty(), this method deletes m_context and m_layer
although the destructor can delete them.
2. the destructor ensures ASSERT(m_textureMailboxes.isEmpty());

m_textureMailboxes.size() is usually 3 and m_recycledMailboxes.size() is 1 or 2.
So I think 6 loop is not very heavy.

[Justin Novosad, 2014-04-09 18:49:07]

On 2014/04/09 18:05:50, dshwang wrote:
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
> m_textureMailboxes.remove(j);
> Thank you for review.
> 
> As you mentioned, "nested for" is not actually needed. I did it to clarify
code
> flow.
> For example,
> 1. if m_textureMailboxes.isEmpty(), this method deletes m_context and m_layer
> although the destructor can delete them.
> 2. the destructor ensures ASSERT(m_textureMailboxes.isEmpty());
> 
> m_textureMailboxes.size() is usually 3 and m_recycledMailboxes.size() is 1 or
2.
> So I think 6 loop is not very heavy.

I agree that the performance argument is weak. because N is bound to be small.
I agree with this change, but I will let Ken have the last word since he knows
this code better.

[Ken Russell (switch to Gerrit), 2014-04-15 02:24:01]

A few remaining concerns.

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
m_textureMailboxes.remove(j);
On 2014/04/09 18:05:50, dshwang wrote:
> As you mentioned, "nested for" is not actually needed. I did it to clarify
code
> flow.
> For example,
> 1. if m_textureMailboxes.isEmpty(), this method deletes m_context and m_layer
> although the destructor can delete them.
> 2. the destructor ensures ASSERT(m_textureMailboxes.isEmpty());

I think keeping this code is a good idea only to enable the assertion in the
destructor. Otherwise the state management of the m_parentDrawingBuffer pointer
in the m_recycledMailboxes and m_textureMailboxes vectors, especially while
destruction is in progress, is really confusing.

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:587: m_layer.clear();
This eager clearing of m_context and m_layer adds complexity. It isn't done in
releaseMailbox, above. Please remove this block and let the destructor take care
of clearing out the context and layer.

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.h:75: RefPtr<DrawingBuffer>
m_parentDrawingBuffer;
Please document the semantics.

"This keeps the parent drawing buffer alive as long as the compositor is
referring to one of the mailboxes it produced. The parent drawing buffer is
cleared when the MailboxInfo is added to the m_recycledMailboxes vector, and
also when the MailboxInfo is released while destruction is in progress."

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.h:87: // Destruction will be
completed after all mailboxes are received.
received -> released

https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.h:152: void
mailboxReleasedInDestructionInProgress(const blink::WebExternalTextureMailbox&);
Please rename this to "mailboxReleasedWhileDestructionInProgress" to improve the
grammar.

[Ken Russell (switch to Gerrit), 2014-04-15 02:25:36]

BTW, I made some grammatical changes to the CL description. Hope that's OK.

[dshwang, 2014-04-17 01:48:02]

Thank you for review! I'm OOO. I'll update this CL soon.

On 2014/04/15 02:24:01, Ken Russell wrote:
> A few remaining concerns.
> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:541:
> m_textureMailboxes.remove(j);
> On 2014/04/09 18:05:50, dshwang wrote:
> > As you mentioned, "nested for" is not actually needed. I did it to clarify
> code
> > flow.
> > For example,
> > 1. if m_textureMailboxes.isEmpty(), this method deletes m_context and
m_layer
> > although the destructor can delete them.
> > 2. the destructor ensures ASSERT(m_textureMailboxes.isEmpty());
> 
> I think keeping this code is a good idea only to enable the assertion in the
> destructor. Otherwise the state management of the m_parentDrawingBuffer
pointer
> in the m_recycledMailboxes and m_textureMailboxes vectors, especially while
> destruction is in progress, is really confusing.

I'll rethink to find more intuitive way.

> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:587: m_layer.clear();
> This eager clearing of m_context and m_layer adds complexity. It isn't done in
> releaseMailbox, above. Please remove this block and let the destructor take
care
> of clearing out the context and layer.

Ok

> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> File Source/platform/graphics/gpu/DrawingBuffer.h (right):
> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.h:75: RefPtr<DrawingBuffer>
> m_parentDrawingBuffer;
> Please document the semantics.
> 
> "This keeps the parent drawing buffer alive as long as the compositor is
> referring to one of the mailboxes it produced. The parent drawing buffer is
> cleared when the MailboxInfo is added to the m_recycledMailboxes vector, and
> also when the MailboxInfo is released while destruction is in progress."

Thank you for good paragraph.

> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.h:87: // Destruction will be
> completed after all mailboxes are received.
> received -> released
> 
>
https://codereview.chromium.org/169933002/diff/190001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.h:152: void
> mailboxReleasedInDestructionInProgress(const
blink::WebExternalTextureMailbox&);
> Please rename this to "mailboxReleasedWhileDestructionInProgress" to improve
the
> grammar.

On 2014/04/15 02:25:36, Ken Russell wrote:
> BTW, I made some grammatical changes to the CL description. Hope that's OK.

Thank you a lot :)

[dshwang, 2014-04-19 00:58:05]

Hi, I tried to make code more clear.
Could you review again?

https://codereview.chromium.org/169933002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/169933002/diff/210001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:552:
deleteMailbox(m_recycledMailboxQueue.takeLast());
now m_recycledMailboxQueue just keeps mailbox name to check which mailbox can be
reused. It doesn't keep parent drawing buffer and texture id anymore, so code is
more clear now.

[Ken Russell (switch to Gerrit), 2014-04-20 00:27:41]

Thanks for pushing this through. LGTM with one request.

https://codereview.chromium.org/169933002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/169933002/diff/210001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:87: inline bool operator==(const
blink::WebExternalTextureMailbox& a, const blink::WebExternalTextureMailbox& b)
This really belongs in public/platform/WebExternalTextureMailbox.h, similarly to
how operator== is defined for WebString. Could you please move it there? You'll
need to add an OWNER, perhaps abarth or dglazkov (jamesr's out right now).

[dshwang, 2014-04-22 09:50:54]

On 2014/04/20 00:27:41, Ken Russell wrote:
> Thanks for pushing this through. LGTM with one request.

Thank you for lgtm. this CL becomes pretty better!

>
https://codereview.chromium.org/169933002/diff/210001/Source/platform/graphic...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/169933002/diff/210001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:87: inline bool
operator==(const
> blink::WebExternalTextureMailbox& a, const blink::WebExternalTextureMailbox&
b)
> This really belongs in public/platform/WebExternalTextureMailbox.h, similarly
to
> how operator== is defined for WebString. Could you please move it there?
You'll
> need to add an OWNER, perhaps abarth or dglazkov (jamesr's out right now).

abarth@, could you review public/platform/WebExternalTextureMailbox.h ?
I add operator==() as well as operator!=(). operator!=() is not used but I
believe it's c++ idiom that operator!=() should be defined if operator==() is
defined.

[Ken Russell (switch to Gerrit), 2014-04-22 19:58:36]

Please hold off landing this for the moment. A memory management bug was just
found in the recent DrawingBuffer and context ownership refactoring  (see
crbug.com/365747 ) and landing this now will only compound the problem.

[dshwang, 2014-04-24 09:06:26]

abarth@, could you review public/platform/WebExternalTextureMailbox.h ?
A memory management bug is resolved.

[abarth-chromium, 2014-04-24 14:59:05]

public/ rslgtm

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
File public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
public/platform/WebExternalTextureMailbox.h:49: return !memcmp(a.name, b.name,
sizeof(a.name));
We don't need to compare the syncPoint as well?

[dshwang, 2014-04-24 15:03:23]

Thank you for review.

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
File public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
public/platform/WebExternalTextureMailbox.h:49: return !memcmp(a.name, b.name,
sizeof(a.name));
On 2014/04/24 14:59:06, abarth wrote:
> We don't need to compare the syncPoint as well?

we don't need syncPoint comparison. The main purpose of operator==() is to
compare webgl's mailbox to compositor's mailbox. compositor usually changes sync
point.

[danakj, 2014-04-24 15:10:01]

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
File public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
public/platform/WebExternalTextureMailbox.h:49: return !memcmp(a.name, b.name,
sizeof(a.name));
On 2014/04/24 15:03:23, dshwang wrote:
> On 2014/04/24 14:59:06, abarth wrote:
> > We don't need to compare the syncPoint as well?
> 
> we don't need syncPoint comparison. The main purpose of operator==() is to
> compare webgl's mailbox to compositor's mailbox. compositor usually changes
sync
> point.

If you're not comparing every field, might I suggest a method instead of
operator== such as NameEquals()?

[dshwang, 2014-04-24 15:19:10]

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
File public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/169933002/diff/250001/public/platform/WebExte...
public/platform/WebExternalTextureMailbox.h:49: return !memcmp(a.name, b.name,
sizeof(a.name));
nameEquals() looks better. It's why I wait while don't click commit.
I'll update soon. :)

[dshwang, 2014-04-24 15:43:33]

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

[commit-bot: I haz the power, 2014-04-24 15:44:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/169933002/29...

[commit-bot: I haz the power, 2014-04-24 16:52:28]

Change committed as 172520