Bug 70656

Summary: [chromium] Make accelerated drawing work with threaded compositing
Product: WebKit Reporter: Alok Priyadarshi <alokp>
Component: Layout and RenderingAssignee: Alok Priyadarshi <alokp>
Status: RESOLVED FIXED    
Severity: Normal CC: bsalomon, enne, jamesr, nduca, senorblanco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
proposed patch
jamesr: review+, jamesr: commit-queue-
proposed patch none

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.