Bug 100797 – Coordinated Graphics: Manage the lifecycle of shareable surfaces precisely.

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

[Dongseong Hwang]

This patch makes UpdateAtlas manage the lifecycle of shareable surfaces
containing the updates in the way how CoordinatedTile manages the lifecycle of
tiles. Currently, UI Process creates the shareable surface when receiving an
UpdateTileForLayer message, but there is no exact point to remove the shareable
surface. Now, we introduce new two messages to handle the lifecycle:
CreateUpdateAtlas and RemoveUpdateAtlas.

This patch gives us two benefits.
1. Reduce file and mmap operations. Web Process does not need to duplicate a
file handle every tile update. UI Proces does not need to create a
ShareableSurface every UpdateTileForLayer message.
2. Save memory. We can remove a ShareableSurface in UI Process when UpdateAtlas
in Web Process is removed.

[Dongseong Hwang]

Created attachment 171553 [details]
Patch

[Dongseong Hwang]

Since Bug 91609, LayerTreeCoordinatorProxy has not keep a shareable surface if a shareable surface backed ShareableBitmap, not GraphicsSurface.
Bug 91609 said "Caching of ShareableSurfaces breaks tiling" if a shareable surface backed ShareableBitmap.
However, I can not understand fully.

This patch makes LayerTreeCoordinatorProxy keep a ShareableSurfaces regardless of the backed implementation of a shareable surface. I'm afraid that this patch causes the bug that Bug 91609 fixed. 
I need advice :)

[Noam Rosenthal]

(In reply to comment #2)
> Since Bug 91609, LayerTreeCoordinatorProxy has not keep a shareable surface if a shareable surface backed ShareableBitmap, not GraphicsSurface.
> Bug 91609 said "Caching of ShareableSurfaces breaks tiling" if a shareable surface backed ShareableBitmap.
> However, I can not understand fully.
> 
> This patch makes LayerTreeCoordinatorProxy keep a ShareableSurfaces regardless of the backed implementation of a shareable surface. I'm afraid that this patch causes the bug that Bug 91609 fixed. 
> I need advice :)

I don't fully understand myself bug 91609 and why tiling was broken. Zeno, Kenneth, Jocelyn?

[Zeno Albisser]

(In reply to comment #3)
> (In reply to comment #2)
> > Since Bug 91609, LayerTreeCoordinatorProxy has not keep a shareable surface if a shareable surface backed ShareableBitmap, not GraphicsSurface.
> > Bug 91609 said "Caching of ShareableSurfaces breaks tiling" if a shareable surface backed ShareableBitmap.
> > However, I can not understand fully.
> > 
> > This patch makes LayerTreeCoordinatorProxy keep a ShareableSurfaces regardless of the backed implementation of a shareable surface. I'm afraid that this patch causes the bug that Bug 91609 fixed. 
> > I need advice :)
> 
> I don't fully understand myself bug 91609 and why tiling was broken. Zeno, Kenneth, Jocelyn?

If i remember correctly, this was the following issue:
Once a GraphicsSurface has been created, the instance will be reused "endlessly", unless the surface changes size or the page is reloaded. So in other words, the instance remains the same, but the texture bound to the surface changes.

Now with the ShareableSurface we have two cases:
If it is backed by a GraphicsSurface, we can cache it.
If it is backed by a BitmapTexture, we actually have to reload the bitmap, because the bitmap is actually transferred through IPC, and therefore it is not sufficient to just redraw a texture.

[Dongseong Hwang]

(In reply to comment #4)
> (In reply to comment #3)
> > I don't fully understand myself bug 91609 and why tiling was broken. Zeno, Kenneth, Jocelyn?
> 
> If i remember correctly, this was the following issue:
> Once a GraphicsSurface has been created, the instance will be reused "endlessly", unless the surface changes size or the page is reloaded. So in other words, the instance remains the same, but the texture bound to the surface changes.
> 
> Now with the ShareableSurface we have two cases:
> If it is backed by a GraphicsSurface, we can cache it.
> If it is backed by a BitmapTexture, we actually have to reload the bitmap, because the bitmap is actually transferred through IPC, and therefore it is not sufficient to just redraw a texture.

Thanks for explanation.

As I understand your explanation, if ShareableSurface is backed by a ShareableBitmap (not BitmapTexture), UI Process can read the image data while Web Process is writing.
I don't know exact implementation in the past, but currently it is impossible.

I briefly summarize current implementation.
1. CoordinatedTile draws on ShareableSurface in Web Process.
2. Web Process send an update message to UI Process.
3. UI Process updates BitmapTexture using ShareableSurface.

NOTE
a) Before #3 is completed, #1 can not start again because UI Process sends a RenderNextFrame message after #3 is completed.
b) BitmapTexture can not be backed by ShareableSurface. BitmapTexture is just updated using ShareableSurface.


I put ShareableSurface::copyToTexture below to clarify "NOTE b)". ShareableSurface::copyToTexture just update BitmapTexture using itself regardless of it backed by GraphicsSurface or ShareableBitmap.

#if USE(TEXTURE_MAPPER)
void ShareableSurface::copyToTexture(PassRefPtr<WebCore::BitmapTexture> passTexture, const IntRect& target, const IntPoint& sourceOffset)
{
    RefPtr<BitmapTexture> texture(passTexture);

#if USE(GRAPHICS_SURFACE)
    if (isBackedByGraphicsSurface()) {
        RefPtr<BitmapTextureGL> textureGL = toBitmapTextureGL(texture.get());
        if (textureGL) {
            uint32_t textureID = textureGL->id();
            uint32_t textureTarget = textureGL->textureTarget();
            m_graphicsSurface->copyToGLTexture(textureTarget, textureID, target, sourceOffset);
            return;
        }

        RefPtr<Image> image = m_graphicsSurface->createReadOnlyImage(IntRect(sourceOffset, target.size()));
        if (image)
            texture->updateContents(image.get(), target, IntPoint::zero(), BitmapTexture::UpdateCanModifyOriginalImageData);
    }
#endif

    ASSERT(m_bitmap);
    RefPtr<Image> image = m_bitmap->createImage();
    texture->updateContents(image.get(), target, sourceOffset, BitmapTexture::UpdateCanModifyOriginalImageData);
    return;
}
#endif // USE(TEXTURE_MAPPER)


In my understanding, we can cache ShareableSurface in both cases. Could you feedback again? :)

[Zeno Albisser]

(In reply to comment #5)
Okay i probably mixed up BitmapTexture  and ShareableBitmap. - Sorry for the confusion.
But my point is the same anyway:
A ShareableSurface is either backed by a GraphicsSurface or by a ShareableBitmap.
Where you can simply reuse the GraphicsSurface, because it is actually double buffered, and the frontBuffer texture can simply be "re-bound" and be drawn.
In case of the ShareableSurface being backed by a ShareableBitmap you would actually need to reload the bitmap data from shared memory (Which is certainly done, when you recreate a new instance). This is why i believe, we cannot actually reuse the instance on the UIProcess side.

And that was what caused the bug that i fixed back then.

[Dongseong Hwang]

(In reply to comment #6)
> (In reply to comment #5)
> Okay i probably mixed up BitmapTexture  and ShareableBitmap. - Sorry for the confusion.
> But my point is the same anyway:
> A ShareableSurface is either backed by a GraphicsSurface or by a ShareableBitmap.
> Where you can simply reuse the GraphicsSurface, because it is actually double buffered, and the frontBuffer texture can simply be "re-bound" and be drawn.
> In case of the ShareableSurface being backed by a ShareableBitmap you would actually need to reload the bitmap data from shared memory (Which is certainly done, when you recreate a new instance). This is why i believe, we cannot actually reuse the instance on the UIProcess side.
> 
> And that was what caused the bug that i fixed back then.

Thank you for explanation!

I understand. I'll reproduce this behavior. If reproduced, I'll find the way how we flush or update brand new shared memory in UI Process gracefully.

[Dongseong Hwang]

Hi, Zeno.

Could you say your environment in which you experienced this bug?

I can not reproduce this bug. I use Ubuntu 12.04.

I think animations/timing-functions.html is one of the best test cases for this bug, because Web Process makes progress various animations.
However, I can not reproduce. I have tested google, yahoo, youtube and nytimes also.

[Dongseong Hwang]

We clarified the problem: If ShareableSurface is backed by ShareableBitmap, UI Process can read stale updates.

I surveyed whether the problem is possible.

In my conclusion, it is impossible at least on linux. I don't know the case of Mac and Win.

ShareableBitmap draws and reads a image using SharedMemory. SharedMemoryUnix uses shm_open and mmap to share memory between processes.
SharedMemory calls mmap with MAP_SHARED flag.
  mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fileDescriptor, 0);

The following document said "The MAP_SHARED flag specifies that modifications made to the mapped file region are immediately visible to other processes which are mapped to the same region and also use the MAP_SHARED flag."
http://h30097.www3.hp.com/docs/base_doc/DOCUMENTATION/V50_HTML/ARH9TATE/SHMCHPXX.HTM

I means UI Process can not read stale updates.

So I think we can cache ShareableSurface in LayerTreeCoordinatorProxy. It can reduce redundant mmap calls.
If there are a problem in mac and win, I think it is the problem that SharedMemory[Port] should fix.
SharedMemory[Port] can solve this problem gracefully. For example,

void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder& encoder) const
{
    ADD CODE TO FLUSH UPDATES HERE!!!!
    encoder << static_cast<uint64_t>(m_size);
    encoder << CoreIPC::MachPort(m_port, MACH_MSG_TYPE_MOVE_SEND);
    m_port = MACH_PORT_NULL;
}

bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& handle)
{
    ASSERT(!handle.m_port);
    ASSERT(!handle.m_size);

    uint64_t size;
    if (!decoder->decodeUInt64(size))
        return false;

    CoreIPC::MachPort machPort;
    if (!decoder->decode(machPort))
        return false;
    
    handle.m_size = size;
    handle.m_port = machPort.port();

    ADD CODE TO SYNCH UPDATES HERE!!!!
    return true;
}

I think we can go with this patch. I need to listen your opinions!!

[Dongseong Hwang]

Created attachment 171771 [details]
Patch

[Dongseong Hwang]

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

Rebased to upstream.

[Zeno Albisser]

(In reply to comment #11)
> I think we can go with this patch. I need to listen your opinions!!

I have been looking at the bug 91609 again.
And i think the real problem was, that previously we used a compound key generated from layerID and tileID as a key for the surface map. Therefore even if the content changed in between the cached version was used. In your case you changed that to use an atlasID that you generate in the UpdateAtlas ctor.
So I would assume that this is alright. Also it seems you tested it for exactly that case. :-)

[Noam Rosenthal]

Comment on attachment 171771 [details]
Patch

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

Some nitpicks, otherwise good!

> Source/WebKit2/ChangeLog:19
> +        file handle every tile update. UI Proces does not need to create a

Proces -> Process

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.cpp:82
> +#ifndef NDEBUG
> +    SurfaceMap::iterator it = m_surfaces.find(atlasID);
> +    ASSERT(it == m_surfaces.end());
> +#endif

ASSERT(!m_surfaces.contains(atlasID))

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.cpp:91
> +#ifndef NDEBUG
> +    SurfaceMap::iterator it = m_surfaces.find(atlasID);
> +    ASSERT(it != m_surfaces.end());
> +#endif

ditto

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:39
> +    static int id = 0;

id -> nextID

[Dongseong Hwang]

Created attachment 171958 [details]
Patch

[Dongseong Hwang]

(In reply to comment #12)
> (In reply to comment #11)
> > I think we can go with this patch. I need to listen your opinions!!
> 
> I have been looking at the bug 91609 again.
> And i think the real problem was, that previously we used a compound key generated from layerID and tileID as a key for the surface map. Therefore even if the content changed in between the cached version was used. In your case you changed that to use an atlasID that you generate in the UpdateAtlas ctor.
> So I would assume that this is alright. Also it seems you tested it for exactly that case. :-)

Thanks for investigation. I understand now :)

[Dongseong Hwang]

Thanks for your review!

(In reply to comment #13)
> (From update of attachment 171771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171771&action=review
> 
> Some nitpicks, otherwise good!
> 
> > Source/WebKit2/ChangeLog:19
> > +        file handle every tile update. UI Proces does not need to create a
> 
> Proces -> Process

Done.

> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.cpp:82
> > +#ifndef NDEBUG
> > +    SurfaceMap::iterator it = m_surfaces.find(atlasID);
> > +    ASSERT(it == m_surfaces.end());
> > +#endif
> 
> ASSERT(!m_surfaces.contains(atlasID))

Done.

> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.cpp:91
> > +#ifndef NDEBUG
> > +    SurfaceMap::iterator it = m_surfaces.find(atlasID);
> > +    ASSERT(it != m_surfaces.end());
> > +#endif
> 
> ditto

ditto

> 
> > Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:39
> > +    static int id = 0;
> 
> id -> nextID

Done.

[WebKit Review Bot]

Comment on attachment 171958 [details]
Patch

Clearing flags on attachment: 171958

Committed r133270: <http://trac.webkit.org/changeset/133270>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dongseong Hwang]

Thank you for review, noam. :)