Closed
Bug 787853
Opened 12 years ago
Closed 12 years ago
Fix "failed to create pixmap" assertion in the layout/scrolling/image-1.html reftest
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 2 obsolete files)
3.98 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
In the layout/reftests/scrolling/image-1.html reftest, there's a failing assertion. The problem is that the surface passed to GLXLibrary::CreatePixmap isn't a Xlib surface, but an Image surface. The patch that I've written creates a new Xlib surface when the surface passed to CreatePixmap is an Image surface and copies the contents from that old surface to the new one.
Attachment #657734 -
Flags: feedback?(karlt)
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: nobody → mar.castelluccio
Attachment #657734 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #657734 -
Flags: feedback?(karlt)
Attachment #657736 -
Flags: feedback?(karlt)
Comment 2•12 years ago
|
||
Comment on attachment 657736 [details] [diff] [review] Patch For efficiency we want to keep graphics paths as short as possible. A key place where GL can outperform RENDER/Xlib is in transferring cpu memory surfaces to gpu memory. GL can go directly to the kernel, while data for a Pixmap must first go to the X server, via a pipe or XShm. So, if cairoImage->mSurface is not an Xlib surface, or CreatePixmap() fails for some other reason, then we want to take the UploadSurfaceToTexture() path in AllocateTexturesCairo().
Attachment #657736 -
Flags: feedback?(karlt) → feedback-
Comment 3•12 years ago
|
||
I expect moving CreatePixmap to AllocateTexturesCairo should be fine. And while doing that, moving to gfxXlibSurface::GetGLXPixmap like CanvasLayerOGL might be the right way to go (bug 655017); that would save having to explicitly destroy the GLXPixmap. The BindTexImage also logically belongs in the same function as the UploadSurfaceToTexture because each of these methods need only be called when the contents of the cairo surface change. No explicit ReleaseTexImage would be required as I expect the texture is destroyed with the CairoOGLBackendData. http://www.opengl.org/registry/specs/EXT/texture_from_pixmap.txt says for glXBindTexImageEXT: "If the texture target for the drawable is GLX_TEXTURE_2D_EXT or GLX_TEXTURE_RECTANGLE_EXT, then buffer defines a 2D texture for the current 2D or rectangle texture object respectively." I wonder whether that means the BindTexImage call should be after a BindTexture call - I don't really know, but cogl seems to ensure that happens. Some of the unlanded changes in bug 680817 comment 7 moved BindTexImage in this way but didn't add a BindTexture call. This may or may not have been related to the problems with the old NVIDIA driver.
Comment 4•12 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #3) > Some of the unlanded changes in bug 680817 comment 7 moved BindTexImage in > this way but didn't add a BindTexture call. Oh, I expect there was a BindTexture in MakeTexture. Similarly for AllocateTexturesCairo, there is already a BindTexture in SetClamping.
Assignee | ||
Comment 5•12 years ago
|
||
However, the reftest is failing (the image-1.html?ref page doesn't show the div with the yellow background).
Attachment #657736 -
Attachment is obsolete: true
Attachment #657883 -
Flags: review?(karlt)
Comment 6•12 years ago
|
||
Comment on attachment 657883 [details] [diff] [review] Patch v2 If you feel like doing so, now would be a good time to change the defined(MOZ_WIDGET_GTK2) to defined(MOZ_X11), in the patch and around the #includes. It will need to change for GTK3 anyway. mozilla/X11Util.h can be removed too. >+ if (aImage->mSurface->GetType() == gfxASurface::SurfaceTypeXlib) { >+ gfxXlibSurface *xsurf = static_cast<gfxXlibSurface*>(static_cast<gfxASurface*>(aImage->mSurface)); >+ GLXPixmap pixmap = xsurf->GetGLXPixmap(); >+ if (pixmap) { >+ if (aImage->mSurface->GetContentType() == gfxASurface::CONTENT_COLOR_ALPHA) { Just need to wrap some lines to stay within 80 columns.
Attachment #657883 -
Flags: review?(karlt) → review+
Comment 7•12 years ago
|
||
>+ gfxXlibSurface *xsurf = static_cast<gfxXlibSurface*>(static_cast<gfxASurface*>(aImage->mSurface));
Sorry; missed this before.
Use static_cast<gfxXlibSurface*>(aImage->mSurface.get()) if static_cast<gfxXlibSurface*>(aImage->mSurface) doesn't work.
Assignee | ||
Comment 8•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=179806578d56 There's a crash that I can't reproduce and some expected reftest failures (but less than I expected). The reftest failures are the same on R and Ru, probably Ru isn't actually unaccelerated. Talos tests are all green.
Assignee | ||
Comment 9•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6b3e1d2f36aa
Assignee | ||
Comment 10•12 years ago
|
||
Forget the last comment, I've cancelled that build because I forgot to add the layers-enabling patch to the queue. This is a debug try build: https://tbpl.mozilla.org/?tree=Try&rev=e402f92dbadf (to narrow the crashes down) This is another opt try build with layers acceleration disabled: https://tbpl.mozilla.org/?tree=Try&rev=859651c91075 (to compare talos results)
Assignee | ||
Comment 11•12 years ago
|
||
The debug build didn't crash on mochitest-5. The comparison of talos results shows two main regressions: ts_paint 11.99% t_paint 6.28%
Comment 12•12 years ago
|
||
Compare Talos is not being helpful at the moment. Posted a table of results in Bug 680817 comment 12.
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f86b28171a2
Flags: in-testsuite+
Comment 14•12 years ago
|
||
Push backed out for intermittent but extremely frequent (80%+) Linux aborts: https://tbpl.mozilla.org/php/getParsedLog.php?id=15269811&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/4e414b3635ed
Comment 15•12 years ago
|
||
Also in reftests: https://tbpl.mozilla.org/php/getParsedLog.php?id=15267378&tree=Mozilla-Inbound
Comment 16•12 years ago
|
||
And M3: https://tbpl.mozilla.org/php/getParsedLog.php?id=15270080&tree=Mozilla-Inbound
Assignee | ||
Comment 17•12 years ago
|
||
That's strange, this didn't happen on try builds. And the code changed is just in gfx/layers/opengl/ImageLayerOGL.cpp that shouldn't be used with basic layers, should it?
Comment 18•12 years ago
|
||
Yes, I doubt it was this patch. I'll look into bug 777946.
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c659fdb19f9 https://tbpl.mozilla.org/?tree=Try&rev=de35e29dcb7d
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9c659fdb19f9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•