Adjust DrawingBuffer's visibility tracking behavior with lost context

Adjust DrawingBuffer's visibility tracking behavior with lost context

This patch adjusts DrawingBuffer's behavior when a context is lost in
two ways:

1. Always tell DrawingBuffer whether it is visible or not regardless of
   whether the WebGL context is lost. This is needed because visibility
   notifications may arrive from the browser while the context is lost
   and the compositor may also attempt to composite the DrawingBuffer
   during this time.

2. Don't check the current visibility in
   DrawingBuffer::prepareMailbox() if the DrawingBuffer is being
   destroyed. This is because the DrawingBuffer may become hidden right
   before the context is forcibly lost (using WEBGL_lose_context), and
   after that point WebGLRenderingContextBase no longer has a reference
   to the DrawingBuffer to update its visibility state.

BUG=411372
TEST=Added in https://codereview.chromium.org/560023002

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

[Sami, 2014-09-10 14:54:00]

skyostil@chromium.org changed reviewers:
+ kbr@chromium.org

[danakj, 2014-09-10 15:11:49]

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

[danakj, 2014-09-10 15:11:49]

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

https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
!m_context->isContextLost())
Can you explain why you don't free mailboxes when the context is lost?

[Sami, 2014-09-10 15:12:36]

On 2014/09/10 15:11:49, danakj wrote:
>
https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
> !m_context->isContextLost())
> Can you explain why you don't free mailboxes when the context is lost?

I'm just keeping that part of the existing behavior. I'm not sure if I should be
freeing them or not. Any ideas?

[danakj, 2014-09-10 15:16:42]

On Wed, Sep 10, 2014 at 11:12 AM, <skyostil@chromium.org> wrote:

> On 2014/09/10 15:11:49, danakj wrote:
>
> https://codereview.chromium.org/556303002/diff/1/Source/
> platform/graphics/gpu/DrawingBuffer.cpp
>
>> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
>>
>
>
> https://codereview.chromium.org/556303002/diff/1/Source/
> platform/graphics/gpu/DrawingBuffer.cpp#newcode197
>
>> Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
>> !m_context->isContextLost())
>> Can you explain why you don't free mailboxes when the context is lost?
>>
>
> I'm just keeping that part of the existing behavior. I'm not sure if I
> should be
> freeing them or not. Any ideas?


Oh.. ok maybe someone else can comment on why it was like that then. I
can't imagine what you gain from holding onto a mailbox from a lost context.

Destroying the active mailbox when the context is not lost OTOH may (i
think will) cause the browser to show black briefly for the WebGL when you
tab-switch back.

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

[Sami, 2014-09-10 15:28:47]

On 2014/09/10 15:16:42, danakj wrote:
> Oh.. ok maybe someone else can comment on why it was like that then. I
> can't imagine what you gain from holding onto a mailbox from a lost context.
> 
> Destroying the active mailbox when the context is not lost OTOH may (i
> think will) cause the browser to show black briefly for the WebGL when you
> tab-switch back.

Right. I agree with you that that check is pretty suspicious and in general
altering behavior for a lost context is a bug factory because the context can be
lost pretty much at any point.

[Sami, 2014-09-10 15:30:25]

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

[Sami, 2014-09-10 15:30:48]

+dongseong.hwang@intel.com who added mailbox recycling (and appears to be on
vacation)

[danakj, 2014-09-10 15:31:51]

danakj@chromium.org changed reviewers:
+ junov@chromium.org

[danakj, 2014-09-10 15:31:52]

Perhaps +junov who reviewed stuff :)

[Justin Novosad, 2014-09-10 17:09:50]

This looks fine to me. Too bad that the test isn't in the same repo as the fix.
That's life I guess.

lgtm

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

https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
!m_context->isContextLost())
On 2014/09/10 15:11:49, danakj wrote:
> Can you explain why you don't free mailboxes when the context is lost?

Had a similar issue with 2D canvas. It's because you can't delete resources on a
lost context. But it is moot anyways since lost contexts just drop everything
they own (or ref) anyways.

[danakj, 2014-09-10 17:12:12]

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

https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
!m_context->isContextLost())
On 2014/09/10 17:09:50, junov wrote:
> On 2014/09/10 15:11:49, danakj wrote:
> > Can you explain why you don't free mailboxes when the context is lost?
> 
> Had a similar issue with 2D canvas. It's because you can't delete resources on
a
> lost context. But it is moot anyways since lost contexts just drop everything
> they own (or ref) anyways.

I'm not sure why you can't delete things? You can DeleteTexture and it "works"
just does nothing, but doesn't generate an error or anything, and lets you avoid
branches. Is that what you mean or something else?

[Justin Novosad, 2014-09-10 17:16:24]

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

https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
!m_context->isContextLost())
On 2014/09/10 17:09:50, junov wrote:
> On 2014/09/10 15:11:49, danakj wrote:
> > Can you explain why you don't free mailboxes when the context is lost?
> 
> Had a similar issue with 2D canvas. It's because you can't delete resources on
a
> lost context. But it is moot anyways since lost contexts just drop everything
> they own (or ref) anyways.

It's the deleting of mailboxes that was causing trouble.  Perhaps that is no
longer a problem today?

I think leaving invalid mailboxes in m_recycledMailboxQueue is fine since they
will never get used anyways since the DrawingBuffer is permanently disabled once
it's context is lost.  Code might be less fragile if we cleared
m_recycledMailboxQueue (without) deleting.  So perhaps instead of doing this,
you could just add an early exit in deleteMailbox if the context is lost?  Just
a suggestion. Feel free to disregard if you disagree.

[danakj, 2014-09-10 17:18:12]

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

https://codereview.chromium.org/556303002/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:197: if (m_isHidden &&
!m_context->isContextLost())
On 2014/09/10 17:16:24, junov wrote:
> On 2014/09/10 17:09:50, junov wrote:
> > On 2014/09/10 15:11:49, danakj wrote:
> > > Can you explain why you don't free mailboxes when the context is lost?
> > 
> > Had a similar issue with 2D canvas. It's because you can't delete resources
on
> a
> > lost context. But it is moot anyways since lost contexts just drop
everything
> > they own (or ref) anyways.
> 
> It's the deleting of mailboxes that was causing trouble.  Perhaps that is no
> longer a problem today?
> 
> I think leaving invalid mailboxes in m_recycledMailboxQueue is fine since they
> will never get used anyways since the DrawingBuffer is permanently disabled
once
> it's context is lost.  Code might be less fragile if we cleared
> m_recycledMailboxQueue (without) deleting.  So perhaps instead of doing this,
> you could just add an early exit in deleteMailbox if the context is lost? 
Just
> a suggestion. Feel free to disregard if you disagree.

As long as we don't use them anything is okay, but I'm super curious about the
claim deleting mailboxes on a lost context causing trouble. That would be
problematic, what kind of trouble?

[Justin Novosad, 2014-09-10 17:20:20]

Looking at the code, I don't really understand why there is a problem with the
delete... I just vaguely remember encountering the same issue in the past. 
Sami, can you explain why the delete needs to be skipped.  I suppose there was
some kind of problem?

[Sami, 2014-09-10 17:24:23]

On 2014/09/10 17:20:20, junov wrote:
> Looking at the code, I don't really understand why there is a problem with the
> delete... I just vaguely remember encountering the same issue in the past. 
> Sami, can you explain why the delete needs to be skipped.  I suppose there was
> some kind of problem?

See message #5, I'm just keeping the existing behavior without really knowing
why :) I'm happy to remove the guard if that sounds okay.

[Sami, 2014-09-10 17:44:22]

I removed the guard, ran the context_lost tests and didn't see any problems.

[Justin Novosad, 2014-09-10 17:55:25]

On 2014/09/10 17:44:22, Sami wrote:
> I removed the guard, ran the context_lost tests and didn't see any problems.

Sweet!  re-lgtm

[danakj, 2014-09-10 18:22:24]

On 2014/09/10 17:44:22, Sami wrote:
> I removed the guard, ran the context_lost tests and didn't see any problems.

Awesome, thanks, that'll make understanding this code easier :) LGTM2

[Ken Russell (switch to Gerrit), 2014-09-10 22:45:14]

Sami, thanks very much for fixing this. LGTM

[Ken Russell (switch to Gerrit), 2014-09-10 22:45:17]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:46:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/556303002/20001

[commit-bot: I haz the power, 2014-09-10 23:26:39]

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

[commit-bot: I haz the power, 2014-09-10 23:26:41]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Sami, 2014-09-11 09:49:35]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-11 09:50:41]

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

[commit-bot: I haz the power, 2014-09-11 09:51:27]

Committed patchset #2 (id:20001) as 181808