Bug 56749 – Enable skia gpu rendering for content layers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

SKIA can now do hardware accelerated rendering. Enable it for content layers.

[Alok Priyadarshi]

Created attachment 86331 [details]
proposed patch

The attached patch enables skia to render directly into the texture used by the compositor. It is rather crude at this point but conveys the approach I plan on taking.

[Brian Salomon]

Alok, the Gr side of this looks correct to me. Obviously there are the shared-context issues to work out but this looks like a good start so that we can begin to experiment and find the bugs.

[Alok Priyadarshi]

Created attachment 86375 [details]
proposed patch

I think it is ready to submit. It is hacky at this point but good enough for skia guys to play with. I will enable skia-gpu for root layer next, after which I should have a better idea on how to refactor stuff to hide the ugliness.

[WebKit Review Bot]

Attachment 86375 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:55:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:67:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:68:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 86375 [details]
proposed patch

Please fix the style issues.

Why doesn't the fbo live on the PlatformCanvas or something further down?  It seems odd to put that on the ContentLayerChromium.  I feel like we keep creating this same abstraction multiple times in multiple places.

Additionally, tying this behavior to ENABLE(SKIA_GPU) will make it harder to evaluate using skia for just canvas 2d vs for everything.  Have you figured out how we can test these two configurations independently?

[Brian Salomon]

> Additionally, tying this behavior to ENABLE(SKIA_GPU) will make it harder to evaluate using skia for just canvas 2d vs for everything.  Have you figured out how we can test these two configurations independently?

+1. I hadn't thought of this but James is right but enabling gpu skia for C2D and layers should be testable independently.

[Vangelis Kokkevis]

(In reply to comment #6)
> > Additionally, tying this behavior to ENABLE(SKIA_GPU) will make it harder to evaluate using skia for just canvas 2d vs for everything.  Have you figured out how we can test these two configurations independently?
> 
> +1. I hadn't thought of this but James is right but enabling gpu skia for C2D and layers should be testable independently.

I would recommend making it a runtime flag option.  The "--enable-composite-to-texture" is a good example of a chrome-only flag that gets propagated through the webkit settings.

[Alok Priyadarshi]

Created attachment 86476 [details]
proposed patch

- Fixed style issues
- Added command-line flag --enable-accelerated-content-layers. Please note that this will only come into effect if SKIA_GPU compile-time flag is enabled. I will send out the changes on chromium side shortly.

[Alok Priyadarshi]

(In reply to comment #5)
> (From update of attachment 86375 [details])
> Please fix the style issues.
> 
> Why doesn't the fbo live on the PlatformCanvas or something further down?  It seems odd to put that on the ContentLayerChromium.  I feel like we keep creating this same abstraction multiple times in multiple places.
> 

Yes fbo on ContentLayerChromium is wrong! This is a quick hack for skia guys to play with. I would prefer to do the refactoring once I have looked at root-layer and have a better idea on how to do it. At present there are three potential candidates for refactoring - skia::PlatformCanvas, WebCore::PlatformCanvas, and WebCore::PlatformContextSkia. Mike is also looking at skia::PlatformCanvas.

[Alok Priyadarshi]

Created attachment 86530 [details]
proposed patch

Reused an existing flag acceleratedDrawingEnabled.

[Alok Priyadarshi]

Comment on attachment 86530 [details]
proposed patch

Please review.

[James Robinson]

Comment on attachment 86530 [details]
proposed patch

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

Overall looks good, but some things need addressing.

Please remember to add this flag to DumpRenderTree as well as chrome (see --enable-accelerated-compositing as one example) so we can run the layout tests in this configuration.

> Source/WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=56749
> +
> +        * platform/graphics/chromium/ContentLayerChromium.cpp:

You should try to explain a patch in the ChangeLog.  For example, on this patch it'd be really nice to understand which macros this behavior is tied to, how it works at a high level, and what bits are unfinished (for example keeping m_fbo on the ContentLayerChromium is bad, so explain why it's done that way and what the plan to improve it is).

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:344
> +        GraphicsContext3D* context = layerRendererContext();
> +        m_contentsTexture = LayerTexture::create(context, layerRenderer()->textureManager());

not sure why you are putting 'context' in a local here - it doesn't seem to buy much.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:68
> +    bool isContentsTextureValid(const IntSize&) const;
> +    bool reserveContentsTexture(const IntSize&);

should these go in the private section? the current subclass (ImageLayerChromium) isn't calling these functions from what I can tell.

[Vangelis Kokkevis]

Comment on attachment 86530 [details]
proposed patch

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

I don't think this is quite ready to check in yet.  I'm changing to r- as I think there are some issues that need to be addressed even for a prototype implementation.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:54
> +    if (!gGR) {

This suffers from the same issue that the global GrContext used by Canvas2D does. There will be one global GrContext per Render process, however there could be multiple GC3D's used (one per tab).  The easy way to trigger the bug is to run chrome in --single-process mode and open two composited tabs.  A quick workaround here would be to have one GrContext per LayerRenderChromium.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:94
> +        layerRendererContext()->deleteFramebuffer(m_fbo);

need to also set m_fbo to 0

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:215
> +        context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_fbo);

I'm surprised that this works at all. Doesn't binding the new framebuffer interfere with the framebuffer the compositor is using?  Same for the viewport...  This method is currently getting called while the compositor is in the middle of rendering which means that changes to the bound framebuffer and/or viewport will be unexpected.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2429
> +    m_layerRenderer->setAcceleratedDrawingEnabled(m_page->settings()->acceleratedDrawingEnabled());

m_page->settings()   ->  settings()

[Alok Priyadarshi]

Comment on attachment 86530 [details]
proposed patch

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

>> Source/WebCore/ChangeLog:8

> 
> You should try to explain a patch in the ChangeLog.  For example, on this patch it'd be really nice to understand which macros this behavior is tied to, how it works at a high level, and what bits are unfinished (for example keeping m_fbo on the ContentLayerChromium is bad, so explain why it's done that way and what the plan to improve it is).

DONE

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:54
>> +    if (!gGR) {
> 
> This suffers from the same issue that the global GrContext used by Canvas2D does. There will be one global GrContext per Render process, however there could be multiple GC3D's used (one per tab).  The easy way to trigger the bug is to run chrome in --single-process mode and open two composited tabs.  A quick workaround here would be to have one GrContext per LayerRenderChromium.

Moved to LayerRenderChromium with a FIXME comment.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:94
>> +        layerRendererContext()->deleteFramebuffer(m_fbo);
> 
> need to also set m_fbo to 0

DONE

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:215
>> +        context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_fbo);
> 
> I'm surprised that this works at all. Doesn't binding the new framebuffer interfere with the framebuffer the compositor is using?  Same for the viewport...  This method is currently getting called while the compositor is in the middle of rendering which means that changes to the bound framebuffer and/or viewport will be unexpected.

It works because the compositor works in two stages - update and draw. This function is called in the update phase and hence does not mess with the draw phase of the compositor. Anyways I have added code to save and restore state.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:344
>> +        m_contentsTexture = LayerTexture::create(context, layerRenderer()->textureManager());
> 
> not sure why you are putting 'context' in a local here - it doesn't seem to buy much.

DONE

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:68
>> +    bool reserveContentsTexture(const IntSize&);
> 
> should these go in the private section? the current subclass (ImageLayerChromium) isn't calling these functions from what I can tell.

DONE

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2429
>> +    m_layerRenderer->setAcceleratedDrawingEnabled(m_page->settings()->acceleratedDrawingEnabled());
> 
> m_page->settings()   ->  settings()

This is actually correct. settings() returns WebSettings which does not give access to WebCore::Settings.

[Alok Priyadarshi]

Created attachment 86686 [details]
proposed patch

PTAL

[Vangelis Kokkevis]

Comment on attachment 86686 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:89
> +            context->getIntegerv(WebCore::GraphicsContext3D::FRAMEBUFFER_BINDING,

I'm afraid that the get calls would really kill the performance of this path.  I now see why changing the fbo and viewport doesn't affect the compositor so you'll be probably better off with the code you had before... Sorry... :(

Long term, if we want to get skia mixing calls with the compositor, we'll need to implement a resetContext call for the compositor that will allow it to return to a state it expects to be in.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2429
> +    m_layerRenderer->setAcceleratedDrawingEnabled(m_page->settings()->acceleratedDrawingEnabled());

I see now why you need to go through m_page.

[Alok Priyadarshi]

Created attachment 86827 [details]
proposed patch

Ok I removed state caching. I was also able to get rid of SkGpuDevice::Current3DApiRenderTarget() which makes glGet calls.

[James Robinson]

Comment on attachment 86827 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:91
> +#if USE(SKIA) && ENABLE(SKIA_GPU)
> +    // FIXME: Find a better place to move this. What we really need is a
> +    // graphics context that can draw into a texture whether accelerated
> +    // or not.
> +    Platform3DObject m_fbo;

This comment doesn't make sense - what would it mean to draw into a texture if not accelerated?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:152
> +        m_skiaContext->setTextureCacheLimits(512, 50 * 1024 * 1024);

magic numbers are bad, give these a descriptive name at least to start.

[Alok Priyadarshi]

Comment on attachment 86827 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:91
>> +    Platform3DObject m_fbo;
> 
> This comment doesn't make sense - what would it mean to draw into a texture if not accelerated?

The same thing we do now - render into a bitmap and upload it to the texture. I have not given much thought to how the two paths will be hidden from ContentLayerChromium. But it seems like we need some sort of a graphics context with texture as the render target. If the context is not accelerated the graphics context will do the copy underneath.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:152
>> +        m_skiaContext->setTextureCacheLimits(512, 50 * 1024 * 1024);
> 
> magic numbers are bad, give these a descriptive name at least to start.

I copied it from PlatformContextSkia.
Brian: Are these required for my use case? and if yes what do these numbers mean?

[Alok Priyadarshi]

Created attachment 86840 [details]
proposed patch

Mike helped me add comments and var names for those magic numbers.

[WebKit Review Bot]

Attachment 86840 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:152:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alok Priyadarshi]

Created attachment 86845 [details]
proposed patch

Fixed style.

[Brian Salomon]

For some reason I don't have permission to comment on the patch diff. Alok, to answer your Q about the magic numbers:

They give the size of Ganesh's texture and glyph caches. I think Mike set the values in PlatformContextSkia. My suggestion: make a local descriptive name here using the current values. Mike has another patch out to associate the GrContext with a SGC3D. Once that is in we can set these to some reasonable values in a well-defined place.

[James Robinson]

I think you should try to use the GrContext and GL context from the SharedGraphicsContext3D that Mike made work with http://trac.webkit.org/changeset/81915.  I'm going to remove the ability for arbitrary objects to use the compositor context at arbitrary times soon and this patch will just add to the problems.

[Alok Priyadarshi]

(In reply to comment #24)
> I think you should try to use the GrContext and GL context from the SharedGraphicsContext3D that Mike made work with http://trac.webkit.org/changeset/81915.  I'm going to remove the ability for arbitrary objects to use the compositor context at arbitrary times soon and this patch will just add to the problems.

When doing accelerated drawing I think we are wasting vram by rendering to a texture. We might be better off rendering the content layers directly to the window or root fbo, in which case we should just use the compositor context to do the drawing. A separate context or backing texture may be justified for image layers and canvas, but not for content and root layers.

[James Robinson]

(In reply to comment #25)
> (In reply to comment #24)
> > I think you should try to use the GrContext and GL context from the SharedGraphicsContext3D that Mike made work with http://trac.webkit.org/changeset/81915.  I'm going to remove the ability for arbitrary objects to use the compositor context at arbitrary times soon and this patch will just add to the problems.
> 
> When doing accelerated drawing I think we are wasting vram by rendering to a texture. We might be better off rendering the content layers directly to the window or root fbo, in which case we should just use the compositor context to do the drawing. A separate context or backing texture may be justified for image layers and canvas, but not for content and root layers.

Possibly some day, but for now we can't use the compositor context for content draws directly.  To do that we'd need to marshall the draw calls in some form.

In the longer term we'd need to do some careful testing to make sure we can handle animated transforms on complex content layers efficiently without rendering to an intermediate surface.

[James Robinson]

Comment on attachment 86845 [details]
proposed patch

R- to clear it from the queue.  I'm also pretty sure this doesn't apply to ToT - we've changed a lot in this area lately.

[Alok Priyadarshi]

Created attachment 92141 [details]
proposed patch

Now we also accelerate root layer. One caveat is that the gpu path will not render native UI controls on windows.
Related CL on the chromium side: http://codereview.chromium.org/6914029/

[James Robinson]

(In reply to comment #28)
> Created an attachment (id=92141) [details]
> proposed patch
> 
> Now we also accelerate root layer. One caveat is that the gpu path will not render native UI controls on windows.

Well that's pretty bad - what's the fix?  It's not going to be very useful to users if native controls are missing.

> Related CL on the chromium side: http://codereview.chromium.org/6914029/

[Alok Priyadarshi]

> > Now we also accelerate root layer. One caveat is that the gpu path will not render native UI controls on windows.
> 
> Well that's pretty bad - what's the fix?  It's not going to be very useful to users if native controls are missing.
> 

The fix has to be done on the chromium side. Please see TODO in the chromium CL - http://codereview.chromium.org/6914029/

A simple fix would be to render the control to a bitmap and copy that bitmap to skia canvas.

[Vangelis Kokkevis]

Comment on attachment 92141 [details]
proposed patch

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

This looks really good.  Just a couple of small comments.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:88
> +    m_pixels = pixels;

Should these two lines be moved to the base class implementation of beginUpload?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:260
> +    GLC(m_context, m_context->clearColor(0, 1, 0, 1));

I would recommend putting this in a DEBUG build guard.  We've seen platforms where the clears are really slow.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:537
> +        m_pictureUploader = new TilePictureUploader(m_tileSize, m_layerRenderer->context(), m_layerRenderer->skiaContext());

Remove the braces or you'll get a style error

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:49
> +// Updates texture of a given tile.

This comment should be omitted as it's not particularly enlightening.

[Vangelis Kokkevis]

Also, definitely get Enne to have a look as it touches a lot of tiler code.

[James Robinson]

Comment on attachment 92141 [details]
proposed patch

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

Patch seems a bit stale - which is expected given all the churn in LayerRendererChromium and friends lately.  Can you confirm that the patch still applies and compiles with the new OwnPtr<> strictness enabled?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:333
> +    // Update compositor resources for root layer.
> +    m_rootLayerContentTiler->uploadCanvas();

this is an odd place to upload the root layer's canvas - looks like this patch is against a slightly older rev.  In ToT we upload the root layer tiler's contents before calling updateLayers().  See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp#L208

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.cpp:118
> +PlatformCanvas::Painter::Painter(PlatformCanvas* canvas, PlatformCanvas::Painter::TextOption option) : m_canvas(canvas)

nit: normally we put the initializers on their own lines with a 4-space indent, even if there's just one

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.cpp:132
> +    m_skiaContext = new PlatformContextSkia(skiaCanvas);

can you verify if this compiles on ToT?  The rules for OwnPtr<> initialization recently were tightened and I think you need an adoptPtr() here and possibly in other places as well

[Adrienne Walker]

Comment on attachment 92141 [details]
proposed patch

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

I am kind of frustrated at the large amount of #ifdefs that are creeping into LayerTilerChromium here where it previously had none.  That's usually a sign to me that a better abstraction is needed or (minimally) code should be more data-driven.  The The TileTextureInterface class is a great abstraction to handle most of the differences between the Skia GPU path and the other paths.  However, I think there a number of additional places where codepaths can be merged or behavioral changes should be passed in as constructor flags rather than as in-class #ifdefs.  I apologize in advance for all the refactoring nits that I'm about to make.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-160
> -void LayerRendererChromium::useShader(unsigned programId)
> -{
> -    if (programId != m_currentShader) {
> -        GLC(m_context.get(), m_context->useProgram(programId));
> -        m_currentShader = programId;
> -    }
> -}
> -

I don't understand this change.  Does the driver always handle checking if we're changing to same program or are we checking this value somewhere outside this patch?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:51
> +class TileTextureInterface {

Can this and its subclasses get moved to their own file(s)? There's a fair amount of code, #includes, and #ifdefs to support them.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:64
> +    void beginUpload(const uint8_t* pixels, const IntRect& paintRect);
> +    void endUpload();
> +    virtual void upload(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect);

This is a weird mix of virtual and non-virtual functions.  This whole patch would feel cleaner to me if LayerTilerChromium had exactly one uploader member variable and these functions were all virtual.  I think that would clean up a large number of #ifdefs and duplicate codepaths.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:96
> +void TilePixelUploader::upload(LayerTexture* texture, const IntRect& sourceRect, const IntRect& destRect)

All of the code in this function is probably going to conflict with https://bugs.webkit.org/show_bug.cgi?id=60008.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:306
>      m_tileSize = size;

The uploader caches the tile size.  I suspect you need to modify it in this function too.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:491
> +#if USE(SKIA)
> +    PlatformCanvas::Backing canvasBacking = layerRenderer()->acceleratedDrawingEnabled() ?
> +            PlatformCanvas::PictureBacking : PlatformCanvas::BitmapBacking;
> +    m_canvas.setBacking(canvasBacking);
> +#endif

There are a number of places where layerRenderer()->acceleratedDrawingEnabled() is used to mean "use skia".  Can this backing parameter be passed in via the constructor to be set on the PlatformCanvas or maybe the PlatformCanvas be created elsewhere with this parameter and passed into the constructor?

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:537
>> +        m_pictureUploader = new TilePictureUploader(m_tileSize, m_layerRenderer->context(), m_layerRenderer->skiaContext());
> 
> Remove the braces or you'll get a style error

Is there a reason to create this lazily? It seems cleaner to create it in the constructor (if possible) so there's never a worry that it's invalid.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:597
> +#if USE(SKIA)
> +            // SKIA coordinate system origin is top-left while that in OpenGL is bottom-left.
> +            // So the texture rendered by skia is upside-down for OpenGL. Note that this is
> +            // not a problem for the bitmap path because the pixels get flipped when uploading
> +            // them using glTexImage2D. We need to fix this only for the accelerated path.
> +            // The following transforms flips the texture coordinates such that the texture
> +            // is upright for OpenGL.
> +            if (layerRenderer()->acceleratedDrawingEnabled()) {
> +                texTranslateY += 1.0;
> +                texScaleY *= -1.0;
> +            }
> +#endif

Would you mind adding a member variable to flip the final texture and then pass in a flag to the constructor to specify whether we should do this? I'd prefer that over another #ifdef wart.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:80
> -    void updateFromPixels(const IntRect& contentRect, const IntRect& paintRect, const uint8_t* pixels);
> +    void uploadPixels(const IntRect& contentRect, const IntRect& paintRect, const uint8_t* pixels);
> +
> +#if USE(SKIA)
> +    // Reserve and upload tile textures from an externally painted picture.
> +    void uploadPicture(const IntRect& contentRect, const IntRect& paintRect, const SkPicture*);
> +#endif

See above comments.  I think these can be the same function.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:179
> +    OwnPtr<TilePixelUploader> m_pixelUploader;
> +#if USE(SKIA)
> +    OwnPtr<TilePictureUploader> m_pictureUploader;
> +#endif

See above note.  I think there should be one "OwnPtr<TileTextureInterface> m_uploader;" member for this class.

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.cpp:80
> +void PlatformCanvas::reset()
> +{
>  #if USE(SKIA)
> -    m_skiaCanvas = skia::CreateBitmapCanvas(size.width(), size.height(), false);
> +    m_skiaCanvas.clear();
> +    m_skiaPicture.clear();

I like the changes to PlatformCanvas to defer allocations until painting rather than doing it at resize.  Thanks for that.

[Alok Priyadarshi]

Comment on attachment 92141 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-160
>> -
> 
> I don't understand this change.  Does the driver always handle checking if we're changing to same program or are we checking this value somewhere outside this patch?

The motivation behind removing the cached shader is that skia does not use LayerRendererChromium interface. It directly uses the GL interface. So if we have to do caching, we will do it at the command-buffer or even lower level. But I think the driver should do the right thing. I have not seen any regressions.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:333
>> +    m_rootLayerContentTiler->uploadCanvas();
> 
> this is an odd place to upload the root layer's canvas - looks like this patch is against a slightly older rev.  In ToT we upload the root layer tiler's contents before calling updateLayers().  See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp#L208

updateRootLayerContents() does not upload the canvas. It just paints the canvas. I have moved uploadCanvas from drawRootLayer(). See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp#L172. We do not want to be uploading canvas in the middle of the draw to keep GL state consistent.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:51
>> +class TileTextureInterface {
> 
> Can this and its subclasses get moved to their own file(s)? There's a fair amount of code, #includes, and #ifdefs to support them.

Will do.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:64
>> +    virtual void upload(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect);
> 
> This is a weird mix of virtual and non-virtual functions.  This whole patch would feel cleaner to me if LayerTilerChromium had exactly one uploader member variable and these functions were all virtual.  I think that would clean up a large number of #ifdefs and duplicate codepaths.

beginUpload is not virtual because it takes different arguments. TilePixelUploader takes pixels while TilePictureUploader takes picture. With the same approach of two uploaders, I could cleanup a couple of ifdefs. But there will always be a few mainly because the accelerated path is not implemented for CG. Another approach that pushes the upload responsibility to PlatformCanvas would cleanup all the ifdefs. What do you think?

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:88
>> +    m_pixels = pixels;
> 
> Should these two lines be moved to the base class implementation of beginUpload?

The arguments are different - pixels and picture.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:260
>> +    GLC(m_context, m_context->clearColor(0, 1, 0, 1));
> 
> I would recommend putting this in a DEBUG build guard.  We've seen platforms where the clears are really slow.

will do.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:306
>>      m_tileSize = size;
> 
> The uploader caches the tile size.  I suspect you need to modify it in this function too.

LayerTilerChromium::reset() deletes the uploader, so it is already handled.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:491
>> +#endif
> 
> There are a number of places where layerRenderer()->acceleratedDrawingEnabled() is used to mean "use skia".  Can this backing parameter be passed in via the constructor to be set on the PlatformCanvas or maybe the PlatformCanvas be created elsewhere with this parameter and passed into the constructor?

Yes passing it in the constructor is a good idea. In which case we should pass that flag to LayerRendererChromium constructor too and remove LayerRendererChromium::setAcceleratedDrawingEnabled() function.

>>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:537
>>> +        m_pictureUploader = new TilePictureUploader(m_tileSize, m_layerRenderer->context(), m_layerRenderer->skiaContext());
>> 
>> Remove the braces or you'll get a style error
> 
> Is there a reason to create this lazily? It seems cleaner to create it in the constructor (if possible) so there's never a worry that it's invalid.

I was creating it lazily because ImageLayerChromium would not use the picture-uploader, only pixel-uploader.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:597
>> +#endif
> 
> Would you mind adding a member variable to flip the final texture and then pass in a flag to the constructor to specify whether we should do this? I'd prefer that over another #ifdef wart.

yes.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:49
>> +// Updates texture of a given tile.
> 
> This comment should be omitted as it's not particularly enlightening.

will do.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:80
>> +#endif
> 
> See above comments.  I think these can be the same function.

Sorry. I did not understand - which can be same function?

>> Source/WebCore/platform/graphics/chromium/PlatformCanvas.cpp:118
>> +PlatformCanvas::Painter::Painter(PlatformCanvas* canvas, PlatformCanvas::Painter::TextOption option) : m_canvas(canvas)
> 
> nit: normally we put the initializers on their own lines with a 4-space indent, even if there's just one

will do.

>> Source/WebCore/platform/graphics/chromium/PlatformCanvas.cpp:132
>> +    m_skiaContext = new PlatformContextSkia(skiaCanvas);
> 
> can you verify if this compiles on ToT?  The rules for OwnPtr<> initialization recently were tightened and I think you need an adoptPtr() here and possibly in other places as well

It does compile fine.

[Alok Priyadarshi]

Created attachment 92630 [details]
proposed patch

Adrienne: Removed most of ifdefs from LayerTilerChromium. The ones remaining are because the CG path is not implemented yet. There are two uploaders in LayerTilerChromium because it also needs to support ImageLayerChromium that directly uses uploadPixels. I could not think of a clean way for ImageLayerChromium to express which uploader it wants to use.

Addressed other comments from Vangelis and James.

[James Robinson]

It really sucks that ImageLayerChromium subclasses ContentLayerChromium.  That made sense when they shared 99% of their implementations, but now they don't.

I haven't looked at the patch closely yet, but maybe it'd be better to divorce them before doing this.

[James Robinson]

(In reply to comment #35)
> (From update of attachment 92141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92141&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-160
> >> -
> > 
> > I don't understand this change.  Does the driver always handle checking if we're changing to same program or are we checking this value somewhere outside this patch?
> 
> The motivation behind removing the cached shader is that skia does not use LayerRendererChromium interface. It directly uses the GL interface. So if we have to do caching, we will do it at the command-buffer or even lower level. But I think the driver should do the right thing. I have not seen any regressions.

I'm missing why this is changing - your code changes the program switching from happening on the LayerRendererChromium to happening on the GraphicsContext3D, but both are completely invisible to skia.  What is the benefit?

[Vangelis Kokkevis]

Comment on attachment 92630 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:-252
> -            else

The reason why we do the reserve operation here is that we don't want the texture manager to reclaim the texture between update and upload.  If that were to happen, we wouldn't have enough pixels to upload.  So I think the reserve needs to stay here.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:267
> +#if USE(SKIA)

Does this really need to be inside an #if USE(SKIA) ?  I think it would be better to get rid of the fence and rely on the fact that acceleratedDrawingEnabled won't be set on CG platforms (for now)

[Alok Priyadarshi]

Comment on attachment 92141 [details]
proposed patch

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

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-160
>>>> -
>>> 
>>> I don't understand this change.  Does the driver always handle checking if we're changing to same program or are we checking this value somewhere outside this patch?
>> 
>> The motivation behind removing the cached shader is that skia does not use LayerRendererChromium interface. It directly uses the GL interface. So if we have to do caching, we will do it at the command-buffer or even lower level. But I think the driver should do the right thing. I have not seen any regressions.
> 
> I'm missing why this is changing - your code changes the program switching from happening on the LayerRendererChromium to happening on the GraphicsContext3D, but both are completely invisible to skia.  What is the benefit?

GraphicsContext3D::useProgram() does not cache programId. The main thing I wanted to remove from here is this - "if (programId != m_currentShader)". SKIA was setting a different shader without LayerRendererChromium knowing about it. As I said before IF we need to do caching, it should be done at a common point used by both compositor and skia.

[Alok Priyadarshi]

Comment on attachment 92630 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:-252
>> -            else
> 
> The reason why we do the reserve operation here is that we don't want the texture manager to reclaim the texture between update and upload.  If that were to happen, we wouldn't have enough pixels to upload.  So I think the reserve needs to stay here.

Hmm. I guess I could leave it here for this patch. But it will have to be moved before compositor can be moved to a different thread. AFAIK update will run in the webkit thread which will not have access to the compositor context. LayerTexture::reserve makes GL calls.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:267
>> +#if USE(SKIA)
> 
> Does this really need to be inside an #if USE(SKIA) ?  I think it would be better to get rid of the fence and rely on the fact that acceleratedDrawingEnabled won't be set on CG platforms (for now)

No it does not need to be inside an #if guard. This is just ignoring the accelerated flag. The flag either needs to be ignored here or at WebView level. I thought it would be better to ignore it here because this class is closer to the uploader. But I do not have strong opinion about doing it either way.

[Alok Priyadarshi]

(In reply to comment #37)
> It really sucks that ImageLayerChromium subclasses ContentLayerChromium.  That made sense when they shared 99% of their implementations, but now they don't.
> 
> I haven't looked at the patch closely yet, but maybe it'd be better to divorce them before doing this.

ImageLayerChromium would still use the tiler, right? I think it should. In which case we would also need to refactor LayerTilerChromium into two classes. The one used by ImageLayerChromium will not have an internal canvas or a update member function.

I would let Adrienne make this call.

[James Robinson]

Comment on attachment 92630 [details]
proposed patch

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

Adrienne and I spent some time whiteboarding where this patch is going and where we want to take this code in general.  Unfortunately this isn't the cleanest design to start with, but I think that we can make progress without stepping on each other's toes too much.

Conceptually it seems that with this patch there are three types of tiled layers:

Image layers: these have an image as a source and update textures by uploading regions of the decoded image via texSubImage2D

Content layers in the non-accelerated case: these are backed by a GraphicsLayerClient and update textures by first painting into a PlatformCanvas and then uploading regions of the software buffer via texSubImage2D

Content layers in the accelerated case: these are backed by a GraphicsLayerClient and update textures by first painting into a SkPicture and then by drawPicture()ing into textures

At a high level, then, it seems to make more sense to leave PlatformCanvas alone and have the abstraction be at the TileTextureInterface/TextureUpdater level.  Image and non-accelerated content layers can own a TilePixelUploader (I'd suggest the name PlatformCanvasTextureUpdater) which owns a PlatformCanvas.  Accelerated content layers can own a TilePaintUploader (possibly renamed to SkiaTextureUpdater) which owns an SkPicture.  Then LayerTilerChromium only needs to interface with the updater.

If it makes things easier I think it'd be OK to leave the uploader/updater owned by the LayerTilerChromium for now, and we can move it later.  We definitely don't want to have both SkPictures and PlatformCanvas coexist on a given layer.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:76
> +    void uploadPixels(const IntRect& contentRect, const IntRect& paintRect, const uint8_t* pixels);
> +
> +    // Reserve and upload tile textures from an externally painted picture.
> +    void uploadPicture(const IntRect& contentRect, const IntRect& paintRect, const PlatformCanvas::Picture*);

it doesn't make much sense to have both of these functions exist on the same class since you'll never mix calls - for a given LayerTilerChromium you will either always uploadPixels() or uploadPicture().

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:118
> +        bool m_topDown; // True if texture is top-down.

Is this a property of each tile?  I think all tiles within a tiler will have the same value for this, right?

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:70
> +    typedef int Picture;

this doesn't make much sense - what does 'int' mean as a Picture?

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:158
> +    GLC(m_context.get(), m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, 0));

why do you need to change the binding here? this seems unnecessary

> Source/WebCore/platform/graphics/chromium/TilePictureUploader.cpp:106
> +    m_context->viewport(0, 0, m_tileSize.width(), m_tileSize.height());
> +    clearFrameBuffer();
> +
> +    // Notify SKIA to sync its internal GL state.
> +    m_skiaContext->resetContext();
> +    // Offset from source rectangle to this destination rectangle.
> +    IntPoint offset(sourceRect.x() - m_paintRect.x(), sourceRect.y() - m_paintRect.y());
> +    m_canvas->save();
> +    m_canvas->translate(-offset.x(), -offset.y());
> +    m_canvas->drawPicture(const_cast<SkPicture&>(*m_picture));
> +    m_canvas->restore();
> +    // Flush SKIA context so that all the rendered stuff appears on the texture.
> +    m_skiaContext->flush(GrContext::kForceCurrentRenderTarget_FlushBit);
> +
> +    // Unbind texture.
> +    m_context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, 0, 0);

this doesn't really seem like "uploading" - i think the function needs a better name.  update()? draw()?

should you set a clip before calling drawPicture()? it seems that without that skia will have to issue draw calls for all content in the layer, not just the content that intersects sourceRect

> Source/WebCore/platform/graphics/chromium/TilePictureUploader.h:49
> +class TilePictureUploader : public TileTextureInterface {

this should have 'skia' somewhere in the name since this implementation is skia-specific

> Source/WebCore/platform/graphics/chromium/TilePictureUploader.h:79
> +// FIXME: Implement CG path.
> +class TilePictureUploader : public TileTextureInterface {

this class doesn't appear to be instantiated in the CG path, so do you need to provide an empty implementation?  it'd be better if this file only implemented the skia version and was named appropriately

> Source/WebCore/platform/graphics/chromium/TilePixelUploader.h:56
> +    GraphicsContext3D* m_context;

i think it'd be better if rather than stashing a pointer to a GraphicsContext3D the upload() call took a GC3D as a parameter.

> Source/WebCore/platform/graphics/chromium/TileTextureInterface.h:37
> +class TileTextureInterface {

this name is not very descriptive. what about TextureUpdater?

> Source/WebCore/platform/graphics/chromium/TileTextureInterface.h:49
> +    // Uploads resources into layer-texture.
> +    virtual void upload(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;

the ganesh implementation does more than just texSubImage2D, so i think something like 'updateTextureRect' would be a better name for this

[James Robinson]

(In reply to comment #40)
> (From update of attachment 92141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92141&action=review
> 
> >>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-160
> >>>> -
> >>> 
> >>> I don't understand this change.  Does the driver always handle checking if we're changing to same program or are we checking this value somewhere outside this patch?
> >> 
> >> The motivation behind removing the cached shader is that skia does not use LayerRendererChromium interface. It directly uses the GL interface. So if we have to do caching, we will do it at the command-buffer or even lower level. But I think the driver should do the right thing. I have not seen any regressions.
> > 
> > I'm missing why this is changing - your code changes the program switching from happening on the LayerRendererChromium to happening on the GraphicsContext3D, but both are completely invisible to skia.  What is the benefit?
> 
> GraphicsContext3D::useProgram() does not cache programId. The main thing I wanted to remove from here is this - "if (programId != m_currentShader)". SKIA was setting a different shader without LayerRendererChromium knowing about it. As I said before IF we need to do caching, it should be done at a common point used by both compositor and skia.

That's sad, it'll make the compositor slower.  The command buffer service side doesn't appear to have a fastpath for setting the program to the same value so we're going to issue a _lot_ of redundant useProgram() calls with this patch.

Is adding caching difficult?  Can you profile the effect of not having it (independently of everything else in this patch) to see if we still need it?

[James Robinson]

(In reply to comment #42)
> (In reply to comment #37)
> > It really sucks that ImageLayerChromium subclasses ContentLayerChromium.  That made sense when they shared 99% of their implementations, but now they don't.
> > 
> > I haven't looked at the patch closely yet, but maybe it'd be better to divorce them before doing this.
> 
> ImageLayerChromium would still use the tiler, right? I think it should. In which case we would also need to refactor LayerTilerChromium into two classes. The one used by ImageLayerChromium will not have an internal canvas or a update member function.
> 
> I would let Adrienne make this call.

LayerTilerChromium could still be reused by both, it just needs to be less tied to the actual mechanism by which the textures are updated.  You don't need to do this refactoring now but it'll happen soon.

[Alok Priyadarshi]

Comment on attachment 92630 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:76
>> +    void uploadPicture(const IntRect& contentRect, const IntRect& paintRect, const PlatformCanvas::Picture*);
> 
> it doesn't make much sense to have both of these functions exist on the same class since you'll never mix calls - for a given LayerTilerChromium you will either always uploadPixels() or uploadPicture().

This will go away with refactoring.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:118
>> +        bool m_topDown; // True if texture is top-down.
> 
> Is this a property of each tile?  I think all tiles within a tiler will have the same value for this, right?

This too.

>> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:70
>> +    typedef int Picture;
> 
> this doesn't make much sense - what does 'int' mean as a Picture?

it means it is not implemented yet. int is just a placeholder. It should have a FIXME.

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:158
>> +    GLC(m_context.get(), m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, 0));
> 
> why do you need to change the binding here? this seems unnecessary

The texture created here is used as an FBO attachment for accelerated path, in which case you need to unbind the texture. For unaccelerated path we always bind the texture before updating it, so I think it is better for this function to not leave the texture bound.

>> Source/WebCore/platform/graphics/chromium/TilePictureUploader.cpp:106
>> +    m_context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, 0, 0);
> 
> this doesn't really seem like "uploading" - i think the function needs a better name.  update()? draw()?
> 
> should you set a clip before calling drawPicture()? it seems that without that skia will have to issue draw calls for all content in the layer, not just the content that intersects sourceRect

I like update. It can be renamed with the refactoring you suggested.

about clip: you may be right. I will look into it.

>> Source/WebCore/platform/graphics/chromium/TilePictureUploader.h:49
>> +class TilePictureUploader : public TileTextureInterface {
> 
> this should have 'skia' somewhere in the name since this implementation is skia-specific

yes for now we could add 'skia' somewhere.

>> Source/WebCore/platform/graphics/chromium/TilePictureUploader.h:79
>> +class TilePictureUploader : public TileTextureInterface {
> 
> this class doesn't appear to be instantiated in the CG path, so do you need to provide an empty implementation?  it'd be better if this file only implemented the skia version and was named appropriately

If we do not provide an empty implementation, a member OwnPtr<TilePictureUploader> member variable will complain. But this will go away with refactoring.

>> Source/WebCore/platform/graphics/chromium/TilePixelUploader.h:56
>> +    GraphicsContext3D* m_context;
> 
> i think it'd be better if rather than stashing a pointer to a GraphicsContext3D the upload() call took a GC3D as a parameter.

TilePictureUploader constructor needs GraphicsContext3D. You also do not want GraphicsContext3D changing with every upload call where you need to recreate FBOs.

>> Source/WebCore/platform/graphics/chromium/TileTextureInterface.h:37
>> +class TileTextureInterface {
> 
> this name is not very descriptive. what about TextureUpdater?

TextureUpdater is fine. I wanted to keep it consistent with TilePaintInterface.

>> Source/WebCore/platform/graphics/chromium/TileTextureInterface.h:49
>> +    virtual void upload(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;
> 
> the ganesh implementation does more than just texSubImage2D, so i think something like 'updateTextureRect' would be a better name for this

I like updateTextureRect.

[Alok Priyadarshi]

(In reply to comment #43)
> (From update of attachment 92630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92630&action=review
> 
> Adrienne and I spent some time whiteboarding where this patch is going and where we want to take this code in general.  Unfortunately this isn't the cleanest design to start with, but I think that we can make progress without stepping on each other's toes too much.
> 
> Conceptually it seems that with this patch there are three types of tiled layers:
> 
> Image layers: these have an image as a source and update textures by uploading regions of the decoded image via texSubImage2D
> 
> Content layers in the non-accelerated case: these are backed by a GraphicsLayerClient and update textures by first painting into a PlatformCanvas and then uploading regions of the software buffer via texSubImage2D
> 
> Content layers in the accelerated case: these are backed by a GraphicsLayerClient and update textures by first painting into a SkPicture and then by drawPicture()ing into textures
> 

There are another two cases - root layer (accelerated and non-accelerated). The root layers are handled in the same way as content layers. So whatever abstraction/code we add at the content-layer level, also needs to be added to LayerRendererChromium for root layer. Is there a particular reason why the root layer is not a content layer?

> At a high level, then, it seems to make more sense to leave PlatformCanvas alone and have the abstraction be at the TileTextureInterface/TextureUpdater level.  Image and non-accelerated content layers can own a TilePixelUploader (I'd suggest the name PlatformCanvasTextureUpdater) which owns a PlatformCanvas.  Accelerated content layers can own a TilePaintUploader (possibly renamed to SkiaTextureUpdater) which owns an SkPicture.  Then LayerTilerChromium only needs to interface with the updater.
>

I like the idea of moving the canvas to TextureUpdater. But image layer does not use PlatformCanvas. It only needs a way to upload pixels it already has. The current implementation of TilePixelUploader serves this purpose perfectly well. So I think it will be best to leave TilePixelUploader and TilePictureUploader the way I have implemented. We could extend PlatformCanvas to also upload its backing. PlatformCanvas will take an extra bool accelerated flag and choose the backing, and use TilePixelUploader or TilePictureUploader depending on the backing.

I could sketch this in a design doc and discuss it in a VC.

> If it makes things easier I think it'd be OK to leave the uploader/updater owned by the LayerTilerChromium for now, and we can move it later.  We definitely don't want to have both SkPictures and PlatformCanvas coexist on a given layer.
>

[Adrienne Walker]

(In reply to comment #47)

> Is there a particular reason why the root layer is not a content layer?

There's no technical reason.  It's a change we've wanted to do because having the root layer not be a content layer creates some special case warts in LayerRendererChromium. We just haven't had the time to do that yet.

> I like the idea of moving the canvas to TextureUpdater. But image layer does not use PlatformCanvas. It only needs a way to upload pixels it already has. The current implementation of TilePixelUploader serves this purpose perfectly well. So I think it will be best to leave TilePixelUploader and TilePictureUploader the way I have implemented. We could extend PlatformCanvas to also upload its backing. PlatformCanvas will take an extra bool accelerated flag and choose the backing, and use TilePixelUploader or TilePictureUploader depending on the backing.
> 
> I could sketch this in a design doc and discuss it in a VC.

I think the ownership model that you describe is backwards.  PlatformCanvas and SkPicture feel like implementation details and TilePaintUploader feels like the abstracted interface to draw into from a backing and then update tiles from.

You bring up an excellent point that ImageLayerChromium doesn't really use the PlatformCanvas.  That says to me that we just need three texture updaters for each of these use cases with a different backing.

SkiaTextureUpdater -> owns an SkPicture
ContentTextureUpdater -> owns a PlatformCanvas
ImageTextureUpdater -> owns a decoded image

In this path, the LayerTilerChromium does not need multiple texture updating paths for images, canvases, and pictures.  It merely needs one that takes a base texture updater pointer.  In the future, we can then move the texture updaters to the layers themselves and LayerTilerChromium won't need any #ifdefs or multiple paths at all.

[Alok Priyadarshi]

> I think the ownership model that you describe is backwards.  PlatformCanvas and SkPicture feel like implementation details and TilePaintUploader feels like the abstracted interface to draw into from a backing and then update tiles from.
>
> You bring up an excellent point that ImageLayerChromium doesn't really use the PlatformCanvas.  That says to me that we just need three texture updaters for each of these use cases with a different backing.
> 
> SkiaTextureUpdater -> owns an SkPicture
> ContentTextureUpdater -> owns a PlatformCanvas
> ImageTextureUpdater -> owns a decoded image
> 

I think there is potential for code sharing between ContentTextureUpdater and ImageTextureUpdater. They both need to upload pixels. This common code could be TilePixelUploader.

> In this path, the LayerTilerChromium does not need multiple texture updating paths for images, canvases, and pictures.  It merely needs one that takes a base texture updater pointer.  In the future, we can then move the texture updaters to the layers themselves and LayerTilerChromium won't need any #ifdefs or multiple paths at all.

Completely agree on a base texture updater pointer. I think we should move forward with the things we all agree on:
- Move PlatformCanvas out of LayerTilerChromium
- Create an abstract TileTextureUpdater to be used by LayerTilerChromium
- ImageLayerChromium does not subclass ContentLayerChromium

Should I file a separate bug? Anyone volunteering to do this refactor?

[Adrienne Walker]

(In reply to comment #49)
> > I think the ownership model that you describe is backwards.  PlatformCanvas and SkPicture feel like implementation details and TilePaintUploader feels like the abstracted interface to draw into from a backing and then update tiles from.
> >
> > You bring up an excellent point that ImageLayerChromium doesn't really use the PlatformCanvas.  That says to me that we just need three texture updaters for each of these use cases with a different backing.
> > 
> > SkiaTextureUpdater -> owns an SkPicture
> > ContentTextureUpdater -> owns a PlatformCanvas
> > ImageTextureUpdater -> owns a decoded image
> 
> I think there is potential for code sharing between ContentTextureUpdater and ImageTextureUpdater. They both need to upload pixels. This common code could be TilePixelUploader.

That seems quite reasonable, as there's already this shared path in LayerTilerChromium.  They would both need to share the temporary pixel buffer for uploading too.

> > In this path, the LayerTilerChromium does not need multiple texture updating paths for images, canvases, and pictures.  It merely needs one that takes a base texture updater pointer.  In the future, we can then move the texture updaters to the layers themselves and LayerTilerChromium won't need any #ifdefs or multiple paths at all.
> 
> Completely agree on a base texture updater pointer. I think we should move forward with the things we all agree on:
> - Move PlatformCanvas out of LayerTilerChromium
> - Create an abstract TileTextureUpdater to be used by LayerTilerChromium
> - ImageLayerChromium does not subclass ContentLayerChromium
> 
> Should I file a separate bug? Anyone volunteering to do this refactor?

In terms of making this patch landable sooner, I would say that ImageLayerChromium deriving from ContentLayerChromium is a separate piece of work and can be its own bug.  LayerTilerChromium can own all the texture updaters in the short term.

I would also split out the change to LayerRendererChromium::useShader into a separate patch.  It doesn't seem relevant to this change and you could address James's concerns above independently without adding more friction to this discussion.

As for creating an abstract TileTextureUpdater, I'm neutral on whether that needs to be done in this patch or as part of some later ContentLayerChromium/ImageLayerChromium refactoring.  If it seems easier to go that route, I think it'd be better.

However, I think moving PlatformCanvas out of LayerTilerChromium and into the pixel updater and moving SkPicture out of PlatformCanvas and into its picture updater seems like it needs to happen before this can land.  It doesn't make any sense to me to make large changes to PlatformCanvas only to take them all back out again.

[Alok Priyadarshi]

Created attachment 94280 [details]
proposed patch

[WebKit Review Bot]

Attachment 94280 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:288:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:132:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alok Priyadarshi]

Created attachment 94434 [details]
Fixed style errors

[James Robinson]

Comment on attachment 94434 [details]
Fixed style errors

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

Please use DrawingBuffer for the LayerTextureUpdaterSkPicture, or let us know why it won't work for this use case.  Otherwise this is looking pretty close!

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:76
> +    static PassRefPtr<LayerRendererChromium> create(PassRefPtr<GraphicsContext3D>, PassOwnPtr<LayerPainterChromium> contentPaint, bool acceleratedDrawing);

this is a somewhat awkward variable name - it would read better with "acceleratedDrawingEnabled" or "accelerateDrawing".

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:149
> +    IntPoint offset(sourceRect.x() - contentRect().x(), sourceRect.y() - contentRect().y());
> +    m_canvas->save();
> +    m_canvas->translate(-offset.x(), -offset.y());
> +    m_canvas->drawPicture(m_picture);
> +    m_canvas->restore();

should you set a clip here? it seems that this code will generate a display list for the entire layer's contents for each tile, which would be fairly wasteful

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:196
> +    context()->bindRenderbuffer(GraphicsContext3D::RENDERBUFFER, m_stencilBuffer);
> +    context()->renderbufferStorage(GraphicsContext3D::RENDERBUFFER, GraphicsContext3D::STENCIL_INDEX8, m_bufferSize.width(), m_bufferSize.height());
> +    context()->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::RENDERBUFFER, m_stencilBuffer);

this section all feels redundant with DrawingBuffer.  DrawingBuffer supports creating+managing a frame buffer with a stencil attachment, managing the lifetime, etc.  Why aren't you using one here?

[Mike Reed]

Comment on attachment 94434 [details]
Fixed style errors

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

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:149
>> +    m_canvas->restore();
> 
> should you set a clip here? it seems that this code will generate a display list for the entire layer's contents for each tile, which would be fairly wasteful

The canvas is always clipped to its device, which is the texture/tile in question in this case, so no need for an extra clipRect.

Just for clarity, we are not generating a displaylist at this point: we already have one. We are in playback mode now (playing m_picture into m_canvas).

[James Robinson]

(In reply to comment #55)
> (From update of attachment 94434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94434&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:149
> >> +    m_canvas->restore();
> > 
> > should you set a clip here? it seems that this code will generate a display list for the entire layer's contents for each tile, which would be fairly wasteful
> 
> The canvas is always clipped to its device, which is the texture/tile in question in this case, so no need for an extra clipRect.
> 
> Just for clarity, we are not generating a displaylist at this point: we already have one. We are in playback mode now (playing m_picture into m_canvas).

Thanks for the clarification - I misread that part.

[Alok Priyadarshi]

Comment on attachment 94434 [details]
Fixed style errors

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:76
>> +    static PassRefPtr<LayerRendererChromium> create(PassRefPtr<GraphicsContext3D>, PassOwnPtr<LayerPainterChromium> contentPaint, bool acceleratedDrawing);
> 
> this is a somewhat awkward variable name - it would read better with "acceleratedDrawingEnabled" or "accelerateDrawing".

changed to accelerateDrawing

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:196
>> +    context()->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::RENDERBUFFER, m_stencilBuffer);
> 
> this section all feels redundant with DrawingBuffer.  DrawingBuffer supports creating+managing a frame buffer with a stencil attachment, managing the lifetime, etc.  Why aren't you using one here?

Two main reasons:
1. DrawingBuffer has its own color buffer. In our case, the color buffer is provided by LayerTexture.
2. It seems heavily tied to canvas 2d - has PlatformLayer for example.

It may be good to refactor it eventually.

[Alok Priyadarshi]

Created attachment 94472 [details]
proposed patch

Renamed acceleratedDrawing -> accelerateDrawing

[James Robinson]

(In reply to comment #57)
> >> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:196
> >> +    context()->framebufferRenderbuffer(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::RENDERBUFFER, m_stencilBuffer);
> > 
> > this section all feels redundant with DrawingBuffer.  DrawingBuffer supports creating+managing a frame buffer with a stencil attachment, managing the lifetime, etc.  Why aren't you using one here?
> 
> Two main reasons:
> 1. DrawingBuffer has its own color buffer. In our case, the color buffer is provided by LayerTexture.
> 2. It seems heavily tied to canvas 2d - has PlatformLayer for example.
> 
> It may be good to refactor it eventually.

It makes me sad to reinvent infrastructure rather than modifying the existing infrastructure to cover new use cases.  We're just going to end up with more and more near-copies of the same code to do nearly the same thing :(

[James Robinson]

Comment on attachment 94472 [details]
proposed patch

R=me

[Alok Priyadarshi]

Committed r87167: <http://trac.webkit.org/changeset/87167>

[James Robinson]

With this patch and --enable-accelerated-drawing ASSERT(context()->checkFramebufferStatus(GraphicsContext3D::FRAMEBUFFER) == GraphicsContext3D::FRAMEBUFFER_COMPLETE); on line 137 of LayerTextureUpdaterCanvas.cpp fails on every composited page I visit (for example, try the poster circle example or any of the compositing layout tests).

[Alok Priyadarshi]

(In reply to comment #62)
> With this patch and --enable-accelerated-drawing ASSERT(context()->checkFramebufferStatus(GraphicsContext3D::FRAMEBUFFER) == GraphicsContext3D::FRAMEBUFFER_COMPLETE); on line 137 of LayerTextureUpdaterCanvas.cpp fails on every composited page I visit (for example, try the poster circle example or any of the compositing layout tests).

I just ran into it on Linux. I can also repro on windows with --use-gl=desktop. Works fine with ANGLE. Investigating.

[Alok Priyadarshi]

(In reply to comment #63)
> (In reply to comment #62)
> > With this patch and --enable-accelerated-drawing ASSERT(context()->checkFramebufferStatus(GraphicsContext3D::FRAMEBUFFER) == GraphicsContext3D::FRAMEBUFFER_COMPLETE); on line 137 of LayerTextureUpdaterCanvas.cpp fails on every composited page I visit (for example, try the poster circle example or any of the compositing layout tests).
> 

We are hitting this assert on desktop GL because it does not support an FBO without a depth buffer or separate stencil and depth buffers. It insists on using a packed depth and stencil buffer, even though SKIA does not use the depth buffer.

SKIA has extensive code to check for various FBO configs that I am reluctant to copy-paste here. DrawingBuffer has some code to check for a couple of configs, but not as robust as that in SKIA. It seems it will be better to let skia create the frame-buffer. I am looking at ways SKIA API can be changed for this case.