Bug 100797

Created attachment 171553 [details] Patch

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 :)

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

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

(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? :)

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

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

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.

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

Created attachment 171771 [details] Patch

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

(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. :-)

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

Created attachment 171958 [details] Patch

(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 :)

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.

Comment on attachment 171958 [details] Patch Clearing flags on attachment: 171958 Committed r133270: <http://trac.webkit.org/changeset/133270>

All reviewed patches have been landed. Closing bug.

Thank you for review, noam. :)