Bug 70656 - [chromium] Make accelerated drawing work with threaded compositing
Summary: [chromium] Make accelerated drawing work with threaded compositing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-21 15:35 PDT by Alok Priyadarshi
Modified: 2011-10-24 13:47 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (14.45 KB, patch)
2011-10-24 09:14 PDT, Alok Priyadarshi
jamesr: review+
jamesr: commit-queue-
Details | Formatted Diff | Diff
proposed patch (14.45 KB, patch)
2011-10-24 10:43 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-10-21 15:35:43 PDT
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.
Comment 1 Alok Priyadarshi 2011-10-24 09:14:41 PDT
Created attachment 112195 [details]
proposed patch
Comment 2 James Robinson 2011-10-24 10:23:10 PDT
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?
Comment 3 Nat Duca 2011-10-24 10:37:19 PDT
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...
Comment 4 Alok Priyadarshi 2011-10-24 10:43:05 PDT
Created attachment 112211 [details]
proposed patch

Initialize() -> initialize()
Comment 5 Alok Priyadarshi 2011-10-24 10:48:51 PDT
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.
Comment 6 Nat Duca 2011-10-24 11:01:24 PDT
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.
Comment 7 Alok Priyadarshi 2011-10-24 11:12:50 PDT
(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.
Comment 8 James Robinson 2011-10-24 11:29:30 PDT
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.
Comment 9 Brian Salomon 2011-10-24 11:50:54 PDT
(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.
Comment 10 WebKit Review Bot 2011-10-24 13:47:44 PDT
Comment on attachment 112211 [details]
proposed patch

Clearing flags on attachment: 112211

Committed r98277: <http://trac.webkit.org/changeset/98277>
Comment 11 WebKit Review Bot 2011-10-24 13:47:50 PDT
All reviewed patches have been landed.  Closing bug.