WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
100797
Coordinated Graphics: Manage the lifecycle of shareable surfaces precisely.
https://bugs.webkit.org/show_bug.cgi?id=100797
Summary
Coordinated Graphics: Manage the lifecycle of shareable surfaces precisely.
Dongseong Hwang
Reported
2012-10-30 17:07:17 PDT
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.
Attachments
Patch
(24.23 KB, patch)
2012-10-30 17:09 PDT
,
Dongseong Hwang
no flags
Details
Formatted Diff
Diff
Patch
(24.25 KB, patch)
2012-10-31 22:57 PDT
,
Dongseong Hwang
no flags
Details
Formatted Diff
Diff
Patch
(24.05 KB, patch)
2012-11-01 17:36 PDT
,
Dongseong Hwang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Dongseong Hwang
Comment 1
2012-10-30 17:09:37 PDT
Created
attachment 171553
[details]
Patch
Dongseong Hwang
Comment 2
2012-10-30 17:15:13 PDT
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
Comment 3
2012-10-30 17:27:23 PDT
(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
Comment 4
2012-10-31 03:14:06 PDT
(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
Comment 5
2012-10-31 04:25:12 PDT
(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
Comment 6
2012-10-31 08:53:59 PDT
(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
Comment 7
2012-10-31 15:18:18 PDT
(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
Comment 8
2012-10-31 20:16:18 PDT
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
Comment 9
2012-10-31 22:50:28 PDT
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
Comment 10
2012-10-31 22:57:14 PDT
Created
attachment 171771
[details]
Patch
Dongseong Hwang
Comment 11
2012-10-31 22:57:34 PDT
(In reply to
comment #10
)
> Created an attachment (id=171771) [details] > Patch
Rebased to upstream.
Zeno Albisser
Comment 12
2012-11-01 03:23:15 PDT
(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 13
2012-11-01 08:24:10 PDT
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
Comment 14
2012-11-01 17:36:10 PDT
Created
attachment 171958
[details]
Patch
Dongseong Hwang
Comment 15
2012-11-01 17:36:47 PDT
(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
Comment 16
2012-11-01 17:37:38 PDT
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 17
2012-11-02 00:23:20 PDT
Comment on
attachment 171958
[details]
Patch Clearing flags on attachment: 171958 Committed
r133270
: <
http://trac.webkit.org/changeset/133270
>
WebKit Review Bot
Comment 18
2012-11-02 00:23:25 PDT
All reviewed patches have been landed. Closing bug.
Dongseong Hwang
Comment 19
2012-11-02 00:33:05 PDT
Thank you for review, noam. :)
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug