Bug 70656 – [chromium] Make accelerated drawing work with threaded compositing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

Accelerated drawing is incompatible with threaded compositing due to accessing the compositor context in the wrong thread. The compositor thread can only be used in LayerTextureUpdater::updateTextureRect().

LayerTextureUpdaterSkPicture, the subclass of LayerTextureUpdater used in the accelerated drawing path caches the compositor thread and uses it in the destructor to delete a bunch of GPU resources.

[Alok Priyadarshi]

Created attachment 112195 [details]
proposed patch

[James Robinson]

Comment on attachment 112195 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112195&action=review

R=me

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:57
> +    SkCanvas* Initialize(GraphicsContext3D*, TextureAllocator*, ManagedTexture*);

lowercase 'initialize' please

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:79
> +SkCanvas* FrameBuffer::Initialize(GraphicsContext3D* context, TextureAllocator* allocator, ManagedTexture* texture)

the function name should be lowercase

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:203
> +    FrameBuffer buffer;
> +    SkCanvas* canvas = buffer.Initialize(context, allocator, texture);

This means we'll create a new FBO every time we upload a tile, right? That sounds like a good plan to start but we'll probably want to cache this somehow in the future, correct?

[Nat Duca]

Maybe this makes a good case for making an FBO allocator inside one of the texture managers? E.g "give me an FBO of such and such size." As part of such a patch, we could make CCRenderSurfaces use them too, which might clean up some of the mess there...

[Alok Priyadarshi]

Created attachment 112211 [details]
proposed patch

Initialize() -> initialize()

[Alok Priyadarshi]

Comment on attachment 112195 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112195&action=review

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:57
>> +    SkCanvas* Initialize(GraphicsContext3D*, TextureAllocator*, ManagedTexture*);
> 
> lowercase 'initialize' please

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:79
>> +SkCanvas* FrameBuffer::Initialize(GraphicsContext3D* context, TextureAllocator* allocator, ManagedTexture* texture)
> 
> the function name should be lowercase

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:203
>> +    SkCanvas* canvas = buffer.Initialize(context, allocator, texture);
> 
> This means we'll create a new FBO every time we upload a tile, right? That sounds like a good plan to start but we'll probably want to cache this somehow in the future, correct?

I think FBO should rather be managed by skia. It manages/reuses the stencil buffer already. We should just be able to specify a texture and skia should create the rest.

Anyhow I doubt caching the FBO will help performance. We are not allocating the color and stencil buffer for each tile upload, just an FBO, which should not be expensive. I will revisit this if it shows up in profiling.

[Nat Duca]

At least on older GPUs, recreating FBOs makes drivers cry. They try to hand out frame buffers from an area called "tiled memory" but that is a fixed resource. The more you thrash FBOs the more you create allocation pressure on the underlying tiled allocator.

[Alok Priyadarshi]

(In reply to comment #6)
> At least on older GPUs, recreating FBOs makes drivers cry. They try to hand out frame buffers from an area called "tiled memory" but that is a fixed resource. The more you thrash FBOs the more you create allocation pressure on the underlying tiled allocator.

I am surprised to hear this. AFAIK an FBO is just a logical container of buffers. So it should not need any buffer memory.

Anyways I will talk to bsalomon@ to check if it makes sense for skia to manage FBO IDs. Otherwise we should think about an FBO allocator as you suggest.

[James Robinson]

I think it's fine to start out correct but possibly slow for this code. We can tune later and without this patch it's just impossible to maintain this path.

[Brian Salomon]

(In reply to comment #7)
> (In reply to comment #6)
> > At least on older GPUs, recreating FBOs makes drivers cry. They try to hand out frame buffers from an area called "tiled memory" but that is a fixed resource. The more you thrash FBOs the more you create allocation pressure on the underlying tiled allocator.
> 
> I am surprised to hear this. AFAIK an FBO is just a logical container of buffers. So it should not need any buffer memory.
> 
> Anyways I will talk to bsalomon@ to check if it makes sense for skia to manage FBO IDs. Otherwise we should think about an FBO allocator as you suggest.

I think it makes sense to give skia a texture id and internally it can decide how to wrap it in a fbo. Skia may want to attach a stencil (or not) and should hide all that from the outside world. I agree with James, though, that this work can be deferred until after the patch lands.

[WebKit Review Bot]

Comment on attachment 112211 [details]
proposed patch

Clearing flags on attachment: 112211

Committed r98277: <http://trac.webkit.org/changeset/98277>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.