Bug 101023 – Coordinated Graphics: Refactor code related to directly composited images.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

Currently, Coordinated Graphics has several problems in terms of handling a composited image.
1. We can not send an image to UI Process using GraphicsSurface, because GraphicsSurface has the size limitation.(GL_MAX_TEXTURE_SIZE)
2. CoordinatedGraphicsLayer, LayerTreeCoordinator, LayerTreeCoordinatorProxy and LayerTreeRenderer are tightly coupled to share an image using ShareableBitmap.
3. LayerTreeCoordinator needs platform dependent code to copy an image to ShareableBitmap.
4. We must allocate big continuous memory when an image is big.
5. Even if a composited image is off the viewport, TextureMapperLayer can not remove an image texture.

If we use TiledBackingStore to handle a composited image, we can get following benefits.
1. We can use GraphicsSurface to send an image to UI Process.
2. We can remove huge complex code to handle IPC for an image and just reuse existing TiledBackingStore.
3. LayerTreeCoordinator does not need platform dependent code.
4. We can send a big image to UI Process using many small tiles.
5. When a composited image is off the viewport, TextureMapperLayer can remove an image texture to save texture memory.

[Dongseong Hwang]

Created attachment 171995 [details]
Patch

[Dongseong Hwang]

I wanted to remove the usage of ShareableBitmap in LayerTreeCoordinator to implement Threaded Coordinated Graphics.
Fortunately, TBS can replace to ShareableBitmap and TBS is better in all aspects.

This patch does not logically dependent on Bug 100819, but physically dependent on Bug 100819, because Bug 100819 is a huge change of Coordinated Graphics.

[Noam Rosenthal]

Comment on attachment 171995 [details]
Patch

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

This will break a really important feature for directly composited images, which is that they create only one texture per image (e.g. in the leaves demo we have only 3 leave textures).

I think a better solution would be if we had an actual additional CoordinatedGraphicsLayer for every composited image, and then the visible rect for that image would be the union of the visible rects of all contents rects of the layers that contain that image. 
I can prototype that if you're busy, you seem to be doing a lot lately :)

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:419
> +    else if (m_compositedImageBackingStore)
> +        m_compositedImageBackingStore->invalidate(IntRect(rect));

This needs to be adjusted to the layer's contentsRect.

[Noam Rosenthal]

Also there is no point in having a contentScale for the image, since it doesn't have antialiasing.

[Noam Rosenthal]

Actually, looking at this again, it can be much simpler:
1. if the image is small, we use the current mechanism.
2. if the image is big, we make the layer appear as if it draws content, and paint the composited image during tiledBackingStorePaint. We assume that huge images don't normally appear on many layers. 

What say you?

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 171995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171995&action=review
> 
> This will break a really important feature for directly composited images, which is that they create only one texture per image (e.g. in the leaves demo we have only 3 leave textures).
OMG, you are right!

> I think a better solution would be if we had an actual additional CoordinatedGraphicsLayer for every composited image, and then the visible rect for that image would be the union of the visible rects of all contents rects of the layers that contain that image. 
> I can prototype that if you're busy, you seem to be doing a lot lately :)
I think you have a good idea. You can do it :)

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:419
> > +    else if (m_compositedImageBackingStore)
> > +        m_compositedImageBackingStore->invalidate(IntRect(rect));
> 
> This needs to be adjusted to the layer's contentsRect.

I want to remove tiles off the viewport and when entering on viewport, I want to adjust visible rect.
After we have the union of the visible rects of all contents rects of the layers, we need more elaborate adjusting visible rect.

(In reply to comment #4)
> Also there is no point in having a contentScale for the image, since it doesn't have antialiasing.

m_compositedImageBackingStore always have 1.f scales image, and TextureMapperLayer will scale the texture. So CoordinatedGraphicsLayer::adjustContentsScale() does not do anything to m_compositedImageBackingStore.

(In reply to comment #5)
> Actually, looking at this again, it can be much simpler:
> 1. if the image is small, we use the current mechanism.
> 2. if the image is big, we make the layer appear as if it draws content, and paint the composited image during tiledBackingStorePaint. We assume that huge images don't normally appear on many layers. 
> 
> What say you?

That's good idea.

How about caching TBS, not ShareableBitmap. TBS always have 1.f scaled image. We can remove and create tiles using the union of the visible rects of all contents rects of the layers.
If we manage TBS like ShareableBitmap, we only have 3 leaves TBS in webkit leaves.

How do you think about it? :)

[Noam Rosenthal]

Created attachment 172236 [details]
WIP patch (not for review)

This patch should show the direction; It's not fully tested yet so not for review.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Target.pri', u'Source/WebCo..." exit_code: 1
Source/WebCore/platform/graphics/AtomicImage.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/AtomicImage.h:43:  The parameter name "image" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/AtomicImage.h:77:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 9 files


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

[Dongseong Hwang]

(In reply to comment #7)
> Created an attachment (id=172236) [details]
> WIP patch (not for review)
> 
> This patch should show the direction; It's not fully tested yet so not for review.

Great! Your patch will make Coordinated Graphics pretty better than current implementation.

However, my gut tells me that we use TBS in all cases. I think we can create only 3 TBS of leaves in webkit leaves demo.

If we use TBS, we can get great benefits for saving memory.
If there are many small directly composited images, we want to release the textures when those image are not visible on viewport.
For example, if there is a html5 game using css3 animation (maybe made using Sencha animation) and the game needs many enemies, weapons, objects and etc., current Coordinated Graphics keeps many small image textures until the RenderLayers are destroyed.

If we use TBS, we can easily release invisible textures.

I can implement it. I'll show you few hours later here. :)

[Dongseong Hwang]

Created attachment 172260 [details]
WIP TBS patch (not for review) 

This patch should show the direction too.

This patch can not draw webkit leaves demo yet. I'll fix ASAP but it will be tomorrow due to personal errands.

I need your feedback too :)

[Noam Rosenthal]

(In reply to comment #9)
patch will make Coordinated Graphics pretty better than current implementation.
> 
> However, my gut tells me that we use TBS in all cases. I think we can create only 3 TBS of leaves in webkit leaves demo.

I understand why your got tells you this, but let me explain an alternative :)
TBS is great for huge images, but for small/medium images I'd rather we decode the image directly into a GraphicsSurface, and then display the surface as a texture in the UI process. If we go the TBS route we can't do that.

[Dongseong Hwang]

(In reply to comment #11)
> (In reply to comment #9)
> I understand why your got tells you this, but let me explain an alternative :)
> TBS is great for huge images, but for small/medium images I'd rather we decode the image directly into a GraphicsSurface, and then display the surface as a texture in the UI process. If we go the TBS route we can't do that.

Wow, you think everything in advance! It is great to decode the image directly into a GraphicsSurface. I agree we can not do this with the TBS.

I think CoordinatedGraphicsLayer should not know this low level abstraction. I want CoordinatedGraphicsLayer to just use CoordinatedCompositedImage (the name is subject to change) as the previous patch did and then CoordinatedCompositedImage decides to use TBS or decode the image to GraphicsSurface. We can get the implementation as merging your previous patch, especially 1024 check in CoordinatedGraphicsLayer into my previous patch. When we add the direct decoding feature, we need to change only CoordinatedCompositedImage. In addition, CoordinatedCompositedImage can remove unused small images off the viewport in the way how CoordinatedCoompositedImage removes the TBS.

I'll update WIP patch to demonstrate my suggestion. :)

[Dongseong Hwang]

Created attachment 172477 [details]
1. WIP TBS patch (not for review)

[Dongseong Hwang]

Created attachment 172478 [details]
2. WIP directly decoding into GraphicsSurface (not for review) 

This patch is not yet completed. The purpose of this patch shows the direction.

In the future, CoordinatedCompositedImage uses GraphicsSurface when Image is small.
We can implement like this patch.

[Dongseong Hwang]

Created attachment 172716 [details]
Patch

[Dongseong Hwang]

Created attachment 172717 [details]
Patch

[Dongseong Hwang]

(In reply to comment #16)
> Created an attachment (id=172717) [details]
> Patch

I forgot CMakelist for efl.

[Dongseong Hwang]

Comment on attachment 172717 [details]
Patch

I complete the patch. I have thoroughly tested this implementation, especially in css3/filters/filter-animation-multi-hw.html, http://www.dorothybrowser.com/test/webkitTest/css3/leaves/ and http://www.satine.org/research/webkit/snowleopard/snowstack.html.

I think we can implement directly image decoding into a GraphicsSurface after this patch. We will change CoordinatedCompositedImage to add direct decoding. If you don't land this patch, we will change LayerTreeCoordinator to add direct decoding. Both cases are the same in that we implement direct decoding.
I rather prefer landing this patch because code related to the composited image gets together into CoordinatedCompositedImage class.

I need your feedback :)


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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedCompositedImage.cpp:43
> +    return reinterpret_cast<CoordinatedCompositedImageID>(image);

I don't fully understand why the current implementation uses a unique key like QPixmap::cacheKey().
Image commonly belongs to CachedImage and WebCore manages single CachedImage per unique URI.
So I don't use AtomicImage that noamr introduced. Could you explain if I'm wrong?

[Dongseong Hwang]

Created attachment 172743 [details]
Patch

[Noam Rosenthal]

Comment on attachment 172717 [details]
Patch

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

Good progress, see comments

> Source/WebCore/ChangeLog:12
> +
> +        Fix a potential bug in GraphicsLayerTextureMapper.
> +        m_compositedNativeImagePtr must be initialied. Otherwise the member's
> +        undefined value can cause undefined behaviour.
> +
> +        In addition, fix some nits in TiledBackingStore and TextureMapperLayer.

Can you do this in a different patch/bug?

> Source/WebKit2/ChangeLog:26
> +

Tests?

> Source/WebKit2/ChangeLog:28
> +        In addition, this patch fixes the bug that Coordinated Graphics only renders the
> +        first frame of a gif animation image.

Is this an isolated fix that can be done in a different patch?

> Source/WebKit2/ChangeLog:30
> +        * Shared/CoordinatedGraphics/CoordinatedCompositedImageID.h: Added.

I think we can define those types in WebLayerTreeInfo.h

> Source/WebKit2/ChangeLog:107
> +        (CoordinatedCompositedImageClient):
> +        (CoordinatedCompositedImageGraphicsLayerClient):
> +        (CoordinatedCompositedImage):

How about CoordinatedImageBacking instead of CoordinatedCompositedImage?

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.cpp:223
> +    // See LayerTreeRenderer::purgeGLResources()
> +    m_surfaces.clear();

Is this related?

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.messages.in:36
> +    CreateTiledCompositedImage(uintptr_t imageID)
> +    DestroyTiledCompositedImage(uintptr_t imageID)
> +    CreateTileForCompositedImage(uintptr_t imageID, int tileID, WebCore::IntRect targetRect, WebKit::SurfaceUpdateInfo updateInfo)
> +    UpdateTileForCompositedImage(uintptr_t imageID, int tileID, WebCore::IntRect targetRect, WebKit::SurfaceUpdateInfo updateInfo)
> +    RemoveTileForCompositedImage(uintptr_t imageID, int tileID)

CreateTiledImageBacking
DestroyTiledImageBacking
CreateTileForImageBacking
UpdateTileForImageBacking
RemoveTileForImageBacking

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:358
> +    removeReleasedImagesIfNeeded();

Doesn't sound right to call that here. Shouldn't we call it at the end of syncRemoteContent ?

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:360
> +    if (InvalidCoordinatedCompositedImageID == imageID) {

if (imageID == InvalidCoordinatedCompositedImageID) you write, more comprehensive the code will be :)

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:369
> +CoordinatedBackingStore* LayerTreeRenderer::getBackingStore(WebLayerID id)

Shouldn't this be a PassRefPtr?

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedCompositedImage.cpp:43
>> +    return reinterpret_cast<CoordinatedCompositedImageID>(image);
> 
> I don't fully understand why the current implementation uses a unique key like QPixmap::cacheKey().
> Image commonly belongs to CachedImage and WebCore manages single CachedImage per unique URI.
> So I don't use AtomicImage that noamr introduced. Could you explain if I'm wrong?

Is there absolutely no code that can change an Image* after it's created? if so your solution would work but it feels a bit risky.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedCompositedImage.cpp:51
> +class CompositedTiledBackingStore : public TiledBackingStoreClient, public CoordinatedTileClient {

Should be TiledImageBacking

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedCompositedImage.h:46
> +class CoordinatedCompositedImageClient {

Make it a nested class, CoordinatedImageBacking::Coordinator

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedCompositedImage.h:59
> +class CoordinatedCompositedImageGraphicsLayerClient {

I think it's ok for CoordinatedImageBacking to access CoordinatedGraphicsLayer directly.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:479
> +        bool imageInstanceReplaced = m_coordinatedCompositedImage && m_coordinatedCompositedImage->id() != CoordinatedCompositedImage::getCoordinatedCompositedImageID(m_compositedImage.get());

Add (): m_coordinatedCompositedImage && (...)

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:491
> +    } else
> +        releaseCompositedImageIfNeeded();

Put this first, and use early return

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:560
> +IntRect CoordinatedGraphicsLayer::compositedImageVisibleRect()

Do you ever call adjustVisibleRect()?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:564
> +    FloatSize ratioOfimageOverLayerSize(m_compositedImage->width() / size().width(), m_compositedImage->height() / size().height());

This is wrong - it should work with contentsRect(), not with the layer size.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:54
> +    // This method is the antithesis of CoordinatedCompositedImageClient::releaseCompositedImage(). See CoordinatedCompositedImageClient.
> +    virtual PassRefPtr<CoordinatedCompositedImage> adoptCompositedImage(WebCore::Image*) = 0;

adopt is not right here - should be createImageBacking(Image*), and no need for the comment.

[Rafael Brandao]

Comment on attachment 172743 [details]
Patch

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

Just a few comments.

> Source/WebCore/ChangeLog:8
> +        Fix a potential bug in GraphicsLayerTextureMapper.

Is this potential bug testable somehow?

> Source/WebCore/ChangeLog:12
> +        In addition, fix some nits in TiledBackingStore and TextureMapperLayer.

You could remove this line from ChangeLog and explain them inline.

> Source/WebCore/ChangeLog:16
> +        * platform/graphics/TiledBackingStore.cpp:

For example, you could do explain a little what nit fix you did here for TiledBackingStore.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:53
> +    // This method is the antithesis of CoordinatedCompositedImageClient::releaseCompositedImage(). See CoordinatedCompositedImageClient.

I think "See CoordinatedCompositedImageClient." could be removed from this sentence.

[Dongseong Hwang]

Thanks for reviews!

I'll fix it tomorrow because of other tasks in my job.
please wait :)

[Noam Rosenthal]

Comment on attachment 172743 [details]
Patch

See comments for previous patches...

[Dongseong Hwang]

Created attachment 173536 [details]
Patch

[Dongseong Hwang]

I agree on TBS is overkill because Coordinated Graphics does not draw big images anymore after Bug 101827, which is awesome!

I believe adding CoordinatedImageBacking is still right architecturally.
Adding CoordinatedImageBacking lightens LayerTreeCoordinator's various roles.

And we can get benefits as Changelog said.

When we implement direct image decoding into a GraphicsSurface, we need to change only the CoordinatedImageBacking.

Although this patch changes and adds code hugely for a side feature, a directly composited image, I think changed code is more readable :)

[Dongseong Hwang]

Created attachment 173542 [details]
Patch

[Noam Rosenthal]

Comment on attachment 173542 [details]
Patch

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.cpp:42
> +    return reinterpret_cast<CoordinatedImageBackingID>(image);

Add a comment about why this is safe.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp: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 :)

[Noam Rosenthal]

Comment on attachment 173542 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:58
> +    // FIXME: We would need to decode a small image directly into a GraphicsSurface.
> +    // http://webkit.org/b/101426

Hmm... point is that once we use GraphicsSurfaces, we don't need to create/destroy the backing texture, which would make this patch unnecessary.

[Dongseong Hwang]

Comment on attachment 173542 [details]
Patch

Thanks for your review :)

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

>> 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.

Yes, I agree.

In this patch, when all clients of one image is not visible, CoordinatedImageBacking send a message to UI Process to remove textures.
I think we will find better policy.

>> 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?

I can. I'll separate out. However, as I mentioned in Bug 93458, creating a layout test is hard, but I can make a manual test like ManualTests/drag-image.html. Do you think a good idea for testing?

>> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:233
>> +    for (float y = 0; y < size.height(); y += tileSize.height()) {
> 
> What's the point in tiling this?

We do not make tiling as expecting to always receive small image enough to store it in single texture.
I'll change to make one tile always.

BTW, do you think it is ok to add CoordinatedImageBackingStore reusing CoordinatedBackingStore?

>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:423
>> +}
> 
> So for the leaves demo, every leave will regenerate the backing texture when it enters the viewport, and destroy it when it exits?

No, when ten layers share an image, the first leave generates the baking texture when it enters the viewport. and the last leave destroys the texture when it exits lastly. If there is at least one leave in the viewport, the texture never be destroyed.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:363
>> +            return;
> 
> Shouldn't this be an ASSERT?

You are right!

>> 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...

Yes, right. However, if the user scrolls close to the border of the coverRect of TBS, we release and recreate the texture of a tile over and over again too.
I think we need more elaborate caching mechanism like MemoryCache. MemoryCache has live capacity. MemoryCache does not remove any resource until the live capacity is full. If the capacity is full, MemoryCache removes resource by the LRU order.
We can add the caching mechanism in the future.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:42
>> +    return reinterpret_cast<CoordinatedImageBackingID>(image);
> 
> Add a comment about why this is safe.

// CoordinatedImageBacking keeps a RefPtr<Image> member, so the same Image pointer can not refer two different instances until CoordinatedImageBacking releases the member.
// When CoordinatedImageBacking releases the member, Coordinated Graphics destroys all data structures related to this image, so even if a different image is allocated to the same pointer, it is safe.

How about the above comment?

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:58
>> +    // http://webkit.org/b/101426
> 
> Hmm... point is that once we use GraphicsSurfaces, we don't need to create/destroy the backing texture, which would make this patch unnecessary.

I think even if we use GraphicsSurfaces, we should destroy the backing texture. Otherwise, If web app receives images using AJAX continuously, we keep textures until WebKit crashes due to out of video memory.
We need memory management mechanism like MemoryCache as I mentioned above. When we introduce something like MemoryCache, this class is still useful.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:96
> +    bool needToUpdate = chagedToVisible || (m_isVisible && m_didInvalidate);

We upload a texture again when invalidate() is called or the image becomes visible at this frame.
"the image becomes visible" means all clients were invisible in the previous frame and then at least one client is visible in this frame.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:116
>> +    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 :)

m_didInvalidate does not exist for optimization. m_didInvalidate is needed for gif animation.
When gif animation is progressed, clients (a.k.a CoordinatedGraphicsLayer) calls invalidate().
If you want, I can add supporting gif animation in another patch.

[Noam Rosenthal]

Comment on attachment 173542 [details]
Patch

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

>>> 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?
> 
> I can. I'll separate out. However, as I mentioned in Bug 93458, creating a layout test is hard, but I can make a manual test like ManualTests/drag-image.html. Do you think a good idea for testing?

That would work.

>>> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:233
>>> +    for (float y = 0; y < size.height(); y += tileSize.height()) {
>> 
>> What's the point in tiling this?
> 
> We do not make tiling as expecting to always receive small image enough to store it in single texture.
> I'll change to make one tile always.
> 
> BTW, do you think it is ok to add CoordinatedImageBackingStore reusing CoordinatedBackingStore?

Yes, we should assert if this image is too large.
I don't see why we couldn't reuse CoordinatedBackingStore.

>>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:423
>>> +}
>> 
>> So for the leaves demo, every leave will regenerate the backing texture when it enters the viewport, and destroy it when it exits?
> 
> No, when ten layers share an image, the first leave generates the baking texture when it enters the viewport. and the last leave destroys the texture when it exits lastly. If there is at least one leave in the viewport, the texture never be destroyed.

So, if we had an animation where all the instances of the same image come in and out of the viewport, the backing texture for that image would be destroyed/recreated continuously.

>>> 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...
> 
> Yes, right. However, if the user scrolls close to the border of the coverRect of TBS, we release and recreate the texture of a tile over and over again too.
> I think we need more elaborate caching mechanism like MemoryCache. MemoryCache has live capacity. MemoryCache does not remove any resource until the live capacity is full. If the capacity is full, MemoryCache removes resource by the LRU order.
> We can add the caching mechanism in the future.

coverRect at least gives us some buffer, so images stay in memory for a bit longer.
I agree that something like a MemoryCache would be a better policy here.

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:42
>>> +    return reinterpret_cast<CoordinatedImageBackingID>(image);
>> 
>> Add a comment about why this is safe.
> 
> // CoordinatedImageBacking keeps a RefPtr<Image> member, so the same Image pointer can not refer two different instances until CoordinatedImageBacking releases the member.
> // When CoordinatedImageBacking releases the member, Coordinated Graphics destroys all data structures related to this image, so even if a different image is allocated to the same pointer, it is safe.
> 
> How about the above comment?

The first part is sufficient.

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:58
>>> +    // http://webkit.org/b/101426
>> 
>> Hmm... point is that once we use GraphicsSurfaces, we don't need to create/destroy the backing texture, which would make this patch unnecessary.
> 
> I think even if we use GraphicsSurfaces, we should destroy the backing texture. Otherwise, If web app receives images using AJAX continuously, we keep textures until WebKit crashes due to out of video memory.
> We need memory management mechanism like MemoryCache as I mentioned above. When we introduce something like MemoryCache, this class is still useful.

Doesn't webkit already cache images using a memory cache? In the end the Image is owned by CachedImage. We should find a way to release the texture when the CachedImage is destroyed.

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:116
>>> +    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 :)
> 
> m_didInvalidate does not exist for optimization. m_didInvalidate is needed for gif animation.
> When gif animation is progressed, clients (a.k.a CoordinatedGraphicsLayer) calls invalidate().
> If you want, I can add supporting gif animation in another patch.

Sorry, comment was supposed to be on the whole function.

[Dongseong Hwang]

Comment on attachment 173542 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:233
>>>> +    for (float y = 0; y < size.height(); y += tileSize.height()) {
>>> 
>>> What's the point in tiling this?
>> 
>> We do not make tiling as expecting to always receive small image enough to store it in single texture.
>> I'll change to make one tile always.
>> 
>> BTW, do you think it is ok to add CoordinatedImageBackingStore reusing CoordinatedBackingStore?
> 
> Yes, we should assert if this image is too large.
> I don't see why we couldn't reuse CoordinatedBackingStore.

Assert is proper I think.

Without CoordinatedImageBackingStore, we have two solutions to reuse CoordinatedBackingStore.
1. LayerTreeRenderer generates tile id and calls CoordinatedBackingStore::createTile and CoordinatedBackingStore::removeTile at the proper time.
2. Add CoordinatedBackingStore::updateContents and updateContents() creates and removes a tile at the proper time.

In #1, I don't want LayerTreeRenderer to concern something like Vector<int /* tileID */>. LayerTreeRenderer has many roles already.
In #2, CoordinatedBackingStore::updateContents is not essential for CoordinatedBackingStore's role. If I add CoordinatedBackingStore::updateContents, I'm afraid other developers are confused which they can call CoordinatedBackingStore::updateContents and CoordinatedBackingStore::createTile to the same instance.
So, I made the easy-to-use class: CoordinatedImageBackingStore.

>>>> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:423
>>>> +}
>>> 
>>> So for the leaves demo, every leave will regenerate the backing texture when it enters the viewport, and destroy it when it exits?
>> 
>> No, when ten layers share an image, the first leave generates the baking texture when it enters the viewport. and the last leave destroys the texture when it exits lastly. If there is at least one leave in the viewport, the texture never be destroyed.
> 
> So, if we had an animation where all the instances of the same image come in and out of the viewport, the backing texture for that image would be destroyed/recreated continuously.

Please see the below comment.

>>>> 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...
>> 
>> Yes, right. However, if the user scrolls close to the border of the coverRect of TBS, we release and recreate the texture of a tile over and over again too.
>> I think we need more elaborate caching mechanism like MemoryCache. MemoryCache has live capacity. MemoryCache does not remove any resource until the live capacity is full. If the capacity is full, MemoryCache removes resource by the LRU order.
>> We can add the caching mechanism in the future.
> 
> coverRect at least gives us some buffer, so images stay in memory for a bit longer.
> I agree that something like a MemoryCache would be a better policy here.

There is a solution!

Timer can absorb shock of deletion like UpdateAtlas. I suggest we will remove an image 3 seconds (subject to change) after an image is invisible. If the image is visible again, we stop the timer.
It can solve this problem as well as the above problem (an animation where all the instances of the same image come in and out of the viewport)

>>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:58
>>>> +    // http://webkit.org/b/101426
>>> 
>>> Hmm... point is that once we use GraphicsSurfaces, we don't need to create/destroy the backing texture, which would make this patch unnecessary.
>> 
>> I think even if we use GraphicsSurfaces, we should destroy the backing texture. Otherwise, If web app receives images using AJAX continuously, we keep textures until WebKit crashes due to out of video memory.
>> We need memory management mechanism like MemoryCache as I mentioned above. When we introduce something like MemoryCache, this class is still useful.
> 
> Doesn't webkit already cache images using a memory cache? In the end the Image is owned by CachedImage. We should find a way to release the texture when the CachedImage is destroyed.

Yes, you are right. We don't need something like memory cache anymore.
When CachedImage removes decoded data, this class will be notified to by CachedImage or BitmapImage in the future.

>>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:116
>>>> +    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 :)
>> 
>> m_didInvalidate does not exist for optimization. m_didInvalidate is needed for gif animation.
>> When gif animation is progressed, clients (a.k.a CoordinatedGraphicsLayer) calls invalidate().
>> If you want, I can add supporting gif animation in another patch.
> 
> Sorry, comment was supposed to be on the whole function.

Ok. I'll!

[Dongseong Hwang]

Created attachment 174380 [details]
Patch

[Dongseong Hwang]

Created attachment 174389 [details]
Patch

[Noam Rosenthal]

Comment on attachment 174389 [details]
Patch

I'd prefer it if we did the class refactoring first, and then add the "invisible" optimization as a second step.

[Dongseong Hwang]

(In reply to comment #34)
> (From update of attachment 174389 [details])
> I'd prefer it if we did the class refactoring first, and then add the "invisible" optimization as a second step.

Ok, sounds reasonable. I'll do that.

[Dongseong Hwang]

Created attachment 174561 [details]
Patch

[Dongseong Hwang]

(In reply to comment #36)
> Created an attachment (id=174561) [details]
> Patch

Removed invisible optimization as you mentioned.

[Noam Rosenthal]

Comment on attachment 174561 [details]
Patch

LGTM!

[Dongseong Hwang]

(In reply to comment #38)
> (From update of attachment 174561 [details])
> LGTM!

Thanks!!

[Dongseong Hwang]

(In reply to comment #39)
> (In reply to comment #38)
> > (From update of attachment 174561 [details] [details])
> > LGTM!
> 
> Thanks!!

This bug depend on Bug 102313. Could you take a look at Bug 102313?

[Noam Rosenthal]

Comment on attachment 174561 [details]
Patch

Clearing cq flag until dependent patch is r+ed.

[Dongseong Hwang]

(In reply to comment #41)
> (From update of attachment 174561 [details])
> Clearing cq flag until dependent patch is r+ed.

Could you cq+ again?

I was confused. This patch does not depend on any bug.

[WebKit Review Bot]

Comment on attachment 174561 [details]
Patch

Clearing flags on attachment: 174561

Committed r134873: <http://trac.webkit.org/changeset/134873>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 174561 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:52
> +    ASSERT(textureMapper->maxTextureSize() >= std::max(m_tileRect.size().width(), m_tileRect.size().height()));

It broke the debug build:

/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp: In member function 'void WebKit::CoordinatedBackingStoreTile::swapBuffers(WebCore::TextureMapper*)':
[1;31m/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:52: error: no match for 'operator>=' in 'textureMapper->WebCore::TextureMapper::maxTextureSize() >= std::max [with _Tp = int](((const int&)((const int*)(&((WebKit::CoordinatedBackingStoreTile*)this)->WebKit::CoordinatedBackingStoreTile::m_tileRect.WebCore::IntRect::size().WebCore::IntSize::width()))), ((const int&)((const int*)(&((WebKit::CoordinatedBackingStoreTile*)this)->WebKit::CoordinatedBackingStoreTile::m_tileRect.WebCore::IntRect::size().WebCore::IntSize::height()))))'[0m
/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/platform/LayoutUnit.h:345: note: candidates are: bool WebCore::operator>=(const WebCore::LayoutUnit&, const WebCore::LayoutUnit&)
/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/platform/LayoutUnit.h:350: note:                 bool WebCore::operator>=(const WebCore::LayoutUnit&, int)
/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/platform/LayoutUnit.h:355: note:                 bool WebCore::operator>=(float, const WebCore::LayoutUnit&)
/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/platform/LayoutUnit.h:360: note:                 bool WebCore::operator>=(const WebCore::LayoutUnit&, float)
/home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/platform/LayoutUnit.h:365: note:                 bool WebCore::operator>=(int, const WebCore::LayoutUnit&)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qstring.h:1309: note:                 bool operator>=(const QStringRef&, const QStringRef&)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qstring.h:1038: note:                 bool operator>=(const char*, QLatin1String)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qstring.h:984: note:                 bool operator>=(QLatin1String, QLatin1String)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qstring.h:1336: note:                 bool operator>=(const char*, const QStringRef&)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qstring.h:1025: note:                 bool operator>=(const char*, const QString&)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qbytearray.h:560: note:                 bool operator>=(const char*, const QByteArray&)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qbytearray.h:558: note:                 bool operator>=(const QByteArray&, const char*)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qbytearray.h:556: note:                 bool operator>=(const QByteArray&, const QByteArray&)
/usr/local/Trolltech/Qt5/Qt-5.0.0-r39/include/QtCore/qchar.h:390: note:                 bool operator>=(QChar, QChar)

[Csaba Osztrogonác]

And it broke the Qt Mac build too: 

In file included from /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:30:
In file included from /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:25:
In file included from /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:24:
In file included from /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.h:33:
In file included from /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Shared/WebLayerTreeInfo.h:25:
In file included from /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:29:
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:39:11: error: member reference base type 'const unsigned long' is not a structure or union
        t.encode(encoder);
        ~ ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.h:92:27: note: in instantiation of member function 'CoreIPC::ArgumentCoder<unsigned long>::encode' requested here
        ArgumentCoder<T>::encode(*this, t);
                          ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/Arguments.h:62:17: note: in instantiation of function template specialization 'CoreIPC::ArgumentEncoder::encode<unsigned long>' requested here
        encoder.encode(argument1);
                ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:39:11: note: in instantiation of member function 'CoreIPC::Arguments1<const unsigned long &>::encode' requested here
        t.encode(encoder);
          ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.h:92:27: note: in instantiation of member function 'CoreIPC::ArgumentCoder<Messages::LayerTreeCoordinatorProxy::CreateImageBacking>::encode' requested here
        ArgumentCoder<T>::encode(*this, t);
                          ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/MessageSender.h:45:18: note: in instantiation of function template specialization 'CoreIPC::ArgumentEncoder::encode<Messages::LayerTreeCoordinatorProxy::CreateImageBacking>' requested here
        encoder->encode(message);
                 ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/MessageSender.h:38:16: note: in instantiation of function template specialization 'CoreIPC::MessageSender<WebKit::WebPage>::send<Messages::LayerTreeCoordinatorProxy::CreateImageBacking>' requested here
        return send(message, static_cast<T*>(this)->destinationID());
               ^
/Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:486:16: note: in instantiation of function template specialization 'CoreIPC::MessageSender<WebKit::WebPage>::send<Messages::LayerTreeCoordinatorProxy::CreateImageBacking>' requested here
    m_webPage->send(Messages::LayerTreeCoordinatorProxy::CreateImageBacking(imageID));
               ^
1 error generated.

[KyungTae Kim]

I fixed the Debug mode build error with https://bugs.webkit.org/show_bug.cgi?id=102465.
Please take a look at it and fix it if it's not a correct fix.

[Dongseong Hwang]

(In reply to comment #47)
> I fixed the Debug mode build error with https://bugs.webkit.org/show_bug.cgi?id=102465.
> Please take a look at it and fix it if it's not a correct fix.

You are right! Thanks!

[Dongseong Hwang]

(In reply to comment #46)
> And it broke the Qt Mac build too: 


Thanks for reporting!

[Dongseong Hwang]

(In reply to comment #46)
> And it broke the Qt Mac build too: 
> /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:39:11: error: member reference base type 'const unsigned long' is not a structure or union
>         t.encode(encoder);
>         ~ ^
> /Users/indt/Development/build-slaves/indt-mountainlion/qt-mountainlion-release/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:486:16: note: in instantiation of function template specialization 'CoreIPC::MessageSender<WebKit::WebPage>::send<Messages::LayerTreeCoordinatorProxy::CreateImageBacking>' requested here
>     m_webPage->send(Messages::LayerTreeCoordinatorProxy::CreateImageBacking(imageID));
>                ^
> 1 error generated.

I can not fix it, because I can not understand what is problem.

CreateImageBacking(uintptr_t imageID) message is pretty similar to SetRootCompositingLayer(uint32_t id) message. We define CoordinatedImageBackingID as uintptr_t, while WebLayerID is defined as uint32_t.
What is different is that CreateImageBacking uses uintptr_t instead of uint32_t.

I want to dig into more, but I don't have mac. I feel very sorry that I can not fix my mistake. Thank someone who fix it in advance.

[Csaba Osztrogonác]

I filed a new bug report to fix the Mac build - https://bugs.webkit.org/show_bug.cgi?id=102475