[webkit-reviews] review denied: [Bug 56749] Enable skia gpu rendering for content layers : [Attachment 92630] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 56749: Enable skia gpu rendering for content layers
https://bugs.webkit.org/show_bug.cgi?id=56749

Attachment 92630: proposed patch
https://bugs.webkit.org/attachment.cgi?id=92630&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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