aura: Attach lost context callback to the shared main thread context

aura: Attach lost context callback to the shared main thread context

r225643 broke lost context recovery on Aura with the thread (i.e. Chrome OS).
The lost context callback needs to be attached to the shared main thread
context, since that's what creates the resources - the offscreen one may not
even exist / be used.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226610

[jamesr, 2013-10-02 05:37:51]

Doh. lgtm

Guess we don't have a way to test this?
On Oct 1, 2013 9:00 PM, <piman@chromium.org> wrote:

> Reviewers: jamesr,
>
> Description:
> aura: Attach lost context callback to the shared main thread context
>
> r225643 broke lost context recovery on Aura with the thread (i.e. Chrome
> OS).
> The lost context callback needs to be attached to the shared main thread
> context, since that's what creates the resources - the offscreen one may
> not
> even exist / be used.
>
> BUG=none
>
> Please review this at
https://codereview.chromium.**org/25367003/<https://codereview.chromium.org/2...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files (+8, -13 lines):
>   M content/browser/aura/gpu_**process_transport_factory.cc
>
>
> Index: content/browser/aura/gpu_**process_transport_factory.cc
> diff --git a/content/browser/aura/gpu_**process_transport_factory.cc
> b/content/browser/aura/gpu_**process_transport_factory.cc
> index e9080f03efafec531bda87582a838b**5e27f0066a..**
> 3f6707498e33c8fc76279101228fd5**aa6321367a 100644
> --- a/content/browser/aura/gpu_**process_transport_factory.cc
> +++ b/content/browser/aura/gpu_**process_transport_factory.cc
> @@ -407,16 +407,6 @@ GpuProcessTransportFactory::**
> OffscreenCompositorContextProv**ider() {
>        GpuProcessTransportFactory::**CreateOffscreenCommandBufferCo**
> ntext(),
>        "Compositor-Offscreen");
>
> -  if (!offscreen_compositor_**contexts_.get())
> -    return NULL;
> -
> -  if (!ui::Compositor::**WasInitializedWithThread()) {
> -    offscreen_compositor_contexts_**->SetLostContextCallback(base:**
> :Bind(
> -        &GpuProcessTransportFactory::
> -            OnLostMainThreadSharedContextI**nsideCallback,
> -        callback_factory_.GetWeakPtr()**));
> -  }
> -
>    return offscreen_compositor_contexts_**;
>  }
>
> @@ -440,9 +430,14 @@ GpuProcessTransportFactory::**
> SharedMainThreadContextProvide**r() {
>              OffscreenCompositorContextProv**ider().get());
>    }
>
> -  if (shared_main_thread_contexts_ &&
> -      !shared_main_thread_contexts_-**>BindToCurrentThread())
> -    shared_main_thread_contexts_ = NULL;
> +  if (shared_main_thread_contexts_) {
> +    shared_main_thread_contexts_->**SetLostContextCallback(base::**Bind(
> +        &GpuProcessTransportFactory::
> +            OnLostMainThreadSharedContextI**nsideCallback,
> +        callback_factory_.GetWeakPtr()**));
> +    if (!shared_main_thread_contexts_**->BindToCurrentThread())
> +      shared_main_thread_contexts_ = NULL;
> +  }
>    return shared_main_thread_contexts_;
>  }
>
>
>
>

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

[commit-bot: I haz the power, 2013-10-02 20:37:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/25367003/10001

[piman, 2013-10-02 20:46:31]

Added test.

[commit-bot: I haz the power, 2013-10-02 23:21:13]

Change committed as 226610

[vignatti (out of this project), 2013-10-10 12:34:02]

On 2013/10/02 23:21:13, I haz the power (commit-bot) wrote:
> Change committed as 226610

there's problem I think with this CL, that I'm seeing with Chrome on
Ozone-Wayland:
https://github.com/01org/ozone-wayland/issues/106

My implementation doesn't provide the offscreen implementation. So in the snip
that has been changed, SetLostContextCallback() is performed and then
BindToCurrentThread() returns false (falls in makeContextCurrent()). That now
means that ContextProviderCommandBuffer::lost_context_callback_ will be set but
never scheduled to run. Next when another context is being created (a new tab is
opened in Chrome), SetLostContextCallback() will fail because
lost_context_callback_ is dirty:

[783:783:1010/150304:FATAL:context_provider_command_buffer.cc(299)] Check
failed: lost_context_callback_.is_null() || lost_context_callback.is_null(). 
 [0x7fca2b8db41c] base::debug::StackTrace::StackTrace()
 [0x7fca2b9166f3] logging::LogMessage::~LogMessage()
 [0x7fca30169413]
content::ContextProviderCommandBuffer::SetLostContextCallback()
 [0x7fca2f0bab0a]
content::GpuProcessTransportFactory::SharedMainThreadContextProvider()
 [0x7fca2f0ba2ea]
content::GpuProcessTransportFactory::CreateSharedSurfaceHandle()
 [0x7fca2ee6d2cf] content::RenderWidgetHostViewAura::GetCompositingSurface()
 [0x7fca2ee4f981] content::RenderWidgetHostImpl::GetCompositingSurface()
 [0x7fca2ee30eaf] content::RenderViewHostImpl::CreateRenderView()
 [0x7fca2eedfba3] content::WebContentsImpl::CreateRenderViewForRenderManager()
 [0x7fca2f09a77e] content::RenderViewHostManager::InitRenderView()

I think a method like ContextProviderCommandBuffer::UnsetLostContextCallback()
could be the trick to clean that pointer. @piman?

[vignatti (out of this project), 2013-10-10 12:58:53]

On 2013/10/10 12:34:02, vignatti wrote:
> On 2013/10/02 23:21:13, I haz the power (commit-bot) wrote:
> > Change committed as 226610
> 
> there's problem I think with this CL, that I'm seeing with Chrome on
> Ozone-Wayland:
> https://github.com/01org/ozone-wayland/issues/106
> 
> My implementation doesn't provide the offscreen implementation. So in the snip
> that has been changed, SetLostContextCallback() is performed and then
> BindToCurrentThread() returns false (falls in makeContextCurrent()). That now
> means that ContextProviderCommandBuffer::lost_context_callback_ will be set
but
> never scheduled to run. Next when another context is being created (a new tab
is
> opened in Chrome), SetLostContextCallback() will fail because
> lost_context_callback_ is dirty:
> 
> [783:783:1010/150304:FATAL:context_provider_command_buffer.cc(299)] Check
> failed: lost_context_callback_.is_null() || lost_context_callback.is_null(). 
>  [0x7fca2b8db41c] base::debug::StackTrace::StackTrace()
>  [0x7fca2b9166f3] logging::LogMessage::~LogMessage()
>  [0x7fca30169413]
> content::ContextProviderCommandBuffer::SetLostContextCallback()
>  [0x7fca2f0bab0a]
> content::GpuProcessTransportFactory::SharedMainThreadContextProvider()
>  [0x7fca2f0ba2ea]
> content::GpuProcessTransportFactory::CreateSharedSurfaceHandle()
>  [0x7fca2ee6d2cf] content::RenderWidgetHostViewAura::GetCompositingSurface()
>  [0x7fca2ee4f981] content::RenderWidgetHostImpl::GetCompositingSurface()
>  [0x7fca2ee30eaf] content::RenderViewHostImpl::CreateRenderView()
>  [0x7fca2eedfba3] content::WebContentsImpl::CreateRenderViewForRenderManager()
>  [0x7fca2f09a77e] content::RenderViewHostManager::InitRenderView()
> 
> I think a method like ContextProviderCommandBuffer::UnsetLostContextCallback()
> could be the trick to clean that pointer. @piman?

something like this works in my implementation:
https://gist.github.com/tiagovignatti/6917897

[piman, 2013-10-10 20:11:58]

On 2013/10/10 12:34:02, vignatti wrote:
> On 2013/10/02 23:21:13, I haz the power (commit-bot) wrote:
> > Change committed as 226610
> 
> there's problem I think with this CL, that I'm seeing with Chrome on
> Ozone-Wayland:
> https://github.com/01org/ozone-wayland/issues/106
> 
> My implementation doesn't provide the offscreen implementation.

I'm confused. You need this context for proper behavior. It's used to manage the
textures that come from the renderer.

> So in the snip
> that has been changed, SetLostContextCallback() is performed and then
> BindToCurrentThread() returns false (falls in makeContextCurrent()). That now
> means that ContextProviderCommandBuffer::lost_context_callback_ will be set
but
> never scheduled to run.

I'm more confused. If BindToCurrentThread fails, we reset
shared_main_thread_contexts_, so the next time around we would recreate it.

> Next when another context is being created (a new tab is
> opened in Chrome), SetLostContextCallback() will fail because
> lost_context_callback_ is dirty:
> 
> [783:783:1010/150304:FATAL:context_provider_command_buffer.cc(299)] Check
> failed: lost_context_callback_.is_null() || lost_context_callback.is_null(). 
>  [0x7fca2b8db41c] base::debug::StackTrace::StackTrace()
>  [0x7fca2b9166f3] logging::LogMessage::~LogMessage()
>  [0x7fca30169413]
> content::ContextProviderCommandBuffer::SetLostContextCallback()
>  [0x7fca2f0bab0a]
> content::GpuProcessTransportFactory::SharedMainThreadContextProvider()
>  [0x7fca2f0ba2ea]
> content::GpuProcessTransportFactory::CreateSharedSurfaceHandle()
>  [0x7fca2ee6d2cf] content::RenderWidgetHostViewAura::GetCompositingSurface()
>  [0x7fca2ee4f981] content::RenderWidgetHostImpl::GetCompositingSurface()
>  [0x7fca2ee30eaf] content::RenderViewHostImpl::CreateRenderView()
>  [0x7fca2eedfba3] content::WebContentsImpl::CreateRenderViewForRenderManager()
>  [0x7fca2f09a77e] content::RenderViewHostManager::InitRenderView()
> 
> I think a method like ContextProviderCommandBuffer::UnsetLostContextCallback()
> could be the trick to clean that pointer. @piman?

[danakj1, 2013-10-10 20:13:36]

On Thu, Oct 10, 2013 at 4:11 PM, <piman@chromium.org> wrote:

> On 2013/10/10 12:34:02, vignatti wrote:
>
>> On 2013/10/02 23:21:13, I haz the power (commit-bot) wrote:
>> > Change committed as 226610
>>
>
>  there's problem I think with this CL, that I'm seeing with Chrome on
>> Ozone-Wayland:
>>
https://github.com/01org/**ozone-wayland/issues/106<https://github.com/01org/...
>>
>
>  My implementation doesn't provide the offscreen implementation.
>>
>
> I'm confused. You need this context for proper behavior. It's used to
> manage the
> textures that come from the renderer.
>
>
>  So in the snip
>> that has been changed, SetLostContextCallback() is performed and then
>> BindToCurrentThread() returns false (falls in makeContextCurrent()). That
>> now
>> means that ContextProviderCommandBuffer::**lost_context_callback_ will
>> be set
>>
> but
>
>> never scheduled to run.
>>
>
> I'm more confused. If BindToCurrentThread fails, we reset
> shared_main_thread_contexts_, so the next time around we would recreate it.


I believe it is re-using the offscreen compositor context, which will now
have a LostContextHandler set on it.


>
>
>  Next when another context is being created (a new tab is
>> opened in Chrome), SetLostContextCallback() will fail because
>> lost_context_callback_ is dirty:
>>
>
>  [783:783:1010/150304:FATAL:**context_provider_command_**buffer.cc(299)]
>> Check
>> failed: lost_context_callback_.is_**null() ||
>> lost_context_callback.is_null(**).
>>   [0x7fca2b8db41c] base::debug::StackTrace::**StackTrace()
>>   [0x7fca2b9166f3] logging::LogMessage::~**LogMessage()
>>   [0x7fca30169413]
>> content::**ContextProviderCommandBuffer::**SetLostContextCallback()
>>   [0x7fca2f0bab0a]
>> content::**GpuProcessTransportFactory::**SharedMainThreadContextProvide**
>> r()
>>   [0x7fca2f0ba2ea]
>> content::**GpuProcessTransportFactory::**CreateSharedSurfaceHandle()
>>   [0x7fca2ee6d2cf] content::**RenderWidgetHostViewAura::**
>> GetCompositingSurface()
>>   [0x7fca2ee4f981] content::RenderWidgetHostImpl:**
>> :GetCompositingSurface()
>>   [0x7fca2ee30eaf] content::RenderViewHostImpl::**CreateRenderView()
>>   [0x7fca2eedfba3] content::WebContentsImpl::**
>> CreateRenderViewForRenderManag**er()
>>   [0x7fca2f09a77e] content::**RenderViewHostManager::**InitRenderView()
>>
>
>  I think a method like ContextProviderCommandBuffer::**
>> UnsetLostContextCallback()
>> could be the trick to clean that pointer. @piman?
>>
>
>
>
https://codereview.chromium.**org/25367003/<https://codereview.chromium.org/2...
>

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

[piman, 2013-10-10 20:20:28]

On Thu, Oct 10, 2013 at 1:13 PM, Dana Jansens <danakj@google.com> wrote:

> On Thu, Oct 10, 2013 at 4:11 PM, <piman@chromium.org> wrote:
>
>> On 2013/10/10 12:34:02, vignatti wrote:
>>
>>> On 2013/10/02 23:21:13, I haz the power (commit-bot) wrote:
>>> > Change committed as 226610
>>>
>>
>>  there's problem I think with this CL, that I'm seeing with Chrome on
>>> Ozone-Wayland:
>>>
https://github.com/01org/**ozone-wayland/issues/106<https://github.com/01org/...
>>>
>>
>>  My implementation doesn't provide the offscreen implementation.
>>>
>>
>> I'm confused. You need this context for proper behavior. It's used to
>> manage the
>> textures that come from the renderer.
>>
>>
>>  So in the snip
>>> that has been changed, SetLostContextCallback() is performed and then
>>> BindToCurrentThread() returns false (falls in makeContextCurrent()).
>>> That now
>>> means that ContextProviderCommandBuffer::**lost_context_callback_ will
>>> be set
>>>
>> but
>>
>>> never scheduled to run.
>>>
>>
>> I'm more confused. If BindToCurrentThread fails, we reset
>> shared_main_thread_contexts_, so the next time around we would recreate
>> it.
>
>
> I believe it is re-using the offscreen compositor context, which will now
> have a LostContextHandler set on it.
>

Oh, I see. I suppose in the single-threaded case we could reset both. But
the point above stands.

>
>
>>
>>
>>  Next when another context is being created (a new tab is
>>> opened in Chrome), SetLostContextCallback() will fail because
>>> lost_context_callback_ is dirty:
>>>
>>
>>  [783:783:1010/150304:FATAL:**context_provider_command_**buffer.cc(299)]
>>> Check
>>> failed: lost_context_callback_.is_**null() ||
>>> lost_context_callback.is_null(**).
>>>   [0x7fca2b8db41c] base::debug::StackTrace::**StackTrace()
>>>   [0x7fca2b9166f3] logging::LogMessage::~**LogMessage()
>>>   [0x7fca30169413]
>>> content::**ContextProviderCommandBuffer::**SetLostContextCallback()
>>>   [0x7fca2f0bab0a]
>>> content::**GpuProcessTransportFactory::**SharedMainThreadContextProvide*
>>> *r()
>>>   [0x7fca2f0ba2ea]
>>> content::**GpuProcessTransportFactory::**CreateSharedSurfaceHandle()
>>>   [0x7fca2ee6d2cf] content::**RenderWidgetHostViewAura::**
>>> GetCompositingSurface()
>>>   [0x7fca2ee4f981] content::RenderWidgetHostImpl:**
>>> :GetCompositingSurface()
>>>   [0x7fca2ee30eaf] content::RenderViewHostImpl::**CreateRenderView()
>>>   [0x7fca2eedfba3] content::WebContentsImpl::**
>>> CreateRenderViewForRenderManag**er()
>>>   [0x7fca2f09a77e] content::**RenderViewHostManager::**InitRenderView()
>>>
>>
>>  I think a method like ContextProviderCommandBuffer::**
>>> UnsetLostContextCallback()
>>> could be the trick to clean that pointer. @piman?
>>>
>>
>>
>>
https://codereview.chromium.**org/25367003/<https://codereview.chromium.org/2...
>>
>
>

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