GNOME Bugzilla – Bug 685915
Screenshot API is broken on big-endian
Last modified: 2012-11-12 18:10:00 UTC
The glReadPixels path through shell_screen_grabber_grab() looks like this on ppc64: http://ajax.fedorapeople.org/ppc.png I'm reasonably sure this is an endianness bug in that path, as the cogl_read_pixels() path in the same function works fine (verified with a simple #if G_BYTE_ORDER == G_LITTLE_ENDIAN around the readpixels path).
Created attachment 226207 [details] [review] Remove shell-screen-grabber The screen grabber was a workaround for an extremely slow path in Mesa when reading back pixel data from the frame buffer. It was using pixel buffer objects by directly calling into GL to hit a fast blit path in Intel's driver. This should no longer be necessary with the latest Mesa because the normal read pixels path now has a fast path to just memcpy the data. Using PBOs in that case just adds an extra indirection because the data is read into an intermediate buffer and then copied back out again. We want to be able to remove the dependency on linking against libGL directly from Gnome Shell because that will not work if Cogl is actually using GLES. Also libGL includes GLX which means gnome-shell ends up with a hard dependency on Xlib which hinders the goal of getting Gnome Shell to be a Wayland compositor. https://bugs.freedesktop.org/show_bug.cgi?id=46631 --- We should just delete this code path ... the driver has been fixed a while ago, Neil had a patch for this for a while (http://git.gnome.org/browse/gnome-shell/commit/?h=wip/wayland&id=5248c8bcbcc1) Rebased version is attached.
Review of attachment 226207 [details] [review]: I'd like Owen to review this, but it looks fine to me.
Testing on Intel Ironlake, Mesa 9.0-0.3.fc18.x86_64 display is 1440x900. Added timing to the screenshot function. Full screenshot before this patch: 35.541 msec 139.103 MB/sec After this patch: 685.23 msec 7.21487 MB/sec Tested with both cogl-1.10 and cogl-1.12. (1.10 is in the GNOME jhbuild moduleset, but that may be accidental.) I don't think it need to be said that the numbers above are not OK.
Created attachment 226273 [details] [review] cogl: Enable PBO path for all mesa versions when using intel There seem to be other cases where the slow path is hit even with mesa 9.0.
Created attachment 226286 [details] [review] shell-screen-grabber.c: Fix PBO path for big-endian machines Here's a quick-fix patch for GNOME Shell that I think will work; not sure if we want this or the combination of the two other patches.
Review of attachment 226286 [details] [review]: ::: src/shell-screen-grabber.c @@ +179,3 @@ glReadPixels (x, y, width, height, GL_BGRA, GL_UNSIGNED_BYTE, 0); +#else + glReadPixels (x, y, width, height, GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, 0); I would prefer if the glReadPixels call wasn't duplicated, but this is OK.
(In reply to comment #5) > Created an attachment (id=226286) [details] [review] > shell-screen-grabber.c: Fix PBO path for big-endian machines > > Here's a quick-fix patch for GNOME Shell that I think will work; not > sure if we want this or the combination of the two other patches. Well given that the cogl people discourage the use of direct GL we should probably use the two other patches.
(In reply to comment #7) > Well given that the cogl people discourage the use of direct GL we should > probably use the two other patches. The issue is that the other solution has not been tested as heavily. It's possible that it will wreck performance on some other driver.
Did we gain any new insight on this ?
(In reply to comment #9) > Did we gain any new insight on this ? We know what's broken ... we have two ways to fix it ... we have not decided which fix we want.
(In reply to comment #8) > (In reply to comment #7) > > Well given that the cogl people discourage the use of direct GL we should > > probably use the two other patches. > > The issue is that the other solution has not been tested as heavily. It's > possible that it will wreck performance on some other driver. How about doing the safer fix in 3.6.x and the other one in master?
can we come to a decision ?
(In reply to comment #12) > can we come to a decision ? Yes just commited the patches as proposed in comment 11