[webkit-reviews] review denied: [Bug 101023] Coordinated Graphics: Add CoordinatedImageBacking to manage an image resource smartly. : [Attachment 173542] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 101023: Coordinated Graphics: Add CoordinatedImageBacking to manage an
image resource smartly.
https://bugs.webkit.org/show_bug.cgi?id=101023

Attachment 173542: Patch
https://bugs.webkit.org/attachment.cgi?id=173542&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=173542&action=review


I don't think this patch is fully baked, and it has a lot of implications that
are not addressed.
Let's try this at smaller steps.

> Source/WebKit2/ChangeLog:13
> +	   1. We can remove a texture if an image is off the viewport.

This is a tricky optimization that requires a lot more thinking.

> Source/WebKit2/ChangeLog:14
> +	   2. We can update a texture if an image is updated. (i.e. gif
animations)

Can we maybe start with a small patch that does only this? Maybe with a test
for directly composited animated GIFs?

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:233
> +    static const IntSize tileSize(512, 512);
> +    IntRect rect(IntPoint::zero(), size);
> +
> +    for (float y = 0; y < size.height(); y += tileSize.height()) {

What's the point in tiling this?

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:423
> +void
LayerTreeRenderer::notifyInvisibleImageBacking(CoordinatedImageBackingID
imageID)
>  {
> -    if (!imageID) {
> +    ASSERT(m_imageBackings.contains(imageID));
> +    ImageBackingMap::iterator it = m_imageBackings.find(imageID);
> +    RefPtr<CoordinatedImageBackingStore> backingStore = it->value;
> +    backingStore->removeAllTiles();
> +    m_imageBackingStoresWithPendingBuffers.add(backingStore);
> +}

So for the leaves demo, every leave will regenerate the backing texture when it
enters the viewport, and destroy it when it exits?

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:363
> +	   if (!newNativeImagePtr)
> +	       return;

Shouldn't this be an ASSERT?

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:581
> +    return
tiledBackingStoreVisibleRect().intersects(tiledBackingStoreContentsRect());

This is incorrect, it should use contentsRect() and not
tiledBackingStoreContentsRect().
Also, this means that if the user scrolls close to the border of an image, we
will release and recreate the texture over and over again...

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.c
pp:42
> +    return reinterpret_cast<CoordinatedImageBackingID>(image);

Add a comment about why this is safe.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.c
pp:116
> +    releaseSurfaceIfNeeded();
> +
> +    bool previousIsVisible = m_isVisible;
> +    notifyInvisibleImageBackingInNeeded();
> +    bool chagedToVisible = !previousIsVisible && m_isVisible;
> +
> +    bool needToUpdate = chagedToVisible || (m_isVisible && m_didInvalidate);

> +    if (!needToUpdate) {
> +	   m_didInvalidate = false;
> +	   return;
> +    }
> +
> +    m_surface = ShareableSurface::create(m_image->size(),
m_image->currentFrameHasAlpha() ? ShareableBitmap::SupportsAlpha :
ShareableBitmap::NoFlags, ShareableSurface::SupportsGraphicsSurface);
> +    m_handle = adoptPtr(new ShareableSurface::Handle());
> +
> +    if (!m_surface->createHandle(*m_handle)) {
> +	   releaseSurfaceIfNeeded();
> +	   m_didInvalidate = false;
> +	   return;
> +    }
> +
> +    IntRect rect(IntPoint::zero(), m_image->size());
> +    OwnPtr<GraphicsContext> context =
m_surface->createGraphicsContext(rect);
> +    context->drawImage(m_image.get(), ColorSpaceDeviceRGB, rect, rect);
> +
> +    m_client->updateImageBacking(id(), *m_handle);
> +    m_didInvalidate = false;

This is essentially TBS for images, except it's binary.
Either way, this looks like an optimization that can be added at a later patch.
One thing at a time please :)