GNOME Bugzilla – Bug 652952
Add screenshot interface
Last modified: 2012-02-14 20:26:11 UTC
See patch.
Created attachment 190212 [details] [review] Add screenshot interface Add a method to shell_global to allow screenshoting arbitary parts of the screen and save the result into a specified png image. Also add a method to shellDBus to expose to applications like gnome-screenshot.
Created attachment 190213 [details] [review] Add screenshot interface Add a method to shell_global to allow screenshoting arbitary parts of the screen and save the result into a specified png image. Also add a method to shellDBus to expose to applications like gnome-screenshot. --- Whitespace fix.
gnome-screenshot has three modes we want to support: - get_whole_desktop(), just does what it says, it takes an image of the whole display. If you have multiple screens, all of them are grabbed, considering their geometry and relative positioning, like this [1] (don't look at the red lines there, I took this screenshot for some other bug a while ago). - get_focused_window(), takes an image of the currently focused window. There should be an option to enable/disable WM borders. - get_area(), takes an image of an user-defined rectangle, no options involved. I think we want to keep the configuration/save parts separate from the shell still in gnome-screenshot, so it probably makes sense for those methods either to save to a path directly (gnome-screenshot could pass a temporary filename there in case it wants to e.g. save to clipboard directly), as passing a giant PNG over DBus could be really expensive. [1] http://people.gnome.org/~cosimoc/multihead-shell.png
(In reply to comment #3) > gnome-screenshot has three modes we want to support: > > - get_whole_desktop(), just does what it says, it takes an image of the whole > display. If you have multiple screens, all of them are grabbed, considering > their geometry and relative positioning, like this [1] (don't look at the red > lines there, I took this screenshot for some other bug a while ago). > > - get_focused_window(), takes an image of the currently focused window. There > should be an option to enable/disable WM borders. > > - get_area(), takes an image of an user-defined rectangle, no options involved. > > I think we want to keep the configuration/save parts separate from the shell > still in gnome-screenshot, so it probably makes sense for those methods either > to save to a path directly (gnome-screenshot could pass a temporary filename > there in case it wants to e.g. save to clipboard directly), as passing a giant > PNG over DBus could be really expensive. The current patch works this way, ie. you pass an image and it saves the result there. DBus isn't a protocol for large binary data so that's fine. As for the other modes will add them.
Review of attachment 190213 [details] [review]: Need to add grab_display() and grab_window() methods.
Created attachment 190318 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) --- For some reason the window method sometimes produces garbage, need to investigate.
Created attachment 190465 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) --- *) Fixed corruption issues *) Fix litte vs. big endian bug
Comment on attachment 190465 [details] [review] Add screenshot interface >+ * indicating whether the operation was successfull or not. "successful" (likewise in other funcs) >+ * Takes a screenshot of the focused window (optionally ommitting the frame) "omitting" >+ Screenshot : function (filename) { >+ return global.screenshot_area (0, 0, global.screen_width, global.screen_height, filename); does this do the right thing with dead areas in multi-monitor setups? >+shell_global_screenshot_area (ShellGlobal *global, >+ int x, align the parameters >+ cogl_read_pixels (x, y, width, height, COGL_READ_PIXELS_COLOR_BUFFER, COGL_PIXEL_FORMAT_BGRA_8888_PRE, data); you check byte order in screenshow_window, but not in screenshot_area... ?
(In reply to comment #8) > (From update of attachment 190465 [details] [review]) > >+ * indicating whether the operation was successfull or not. > > "successful" (likewise in other funcs) OK > >+ * Takes a screenshot of the focused window (optionally ommitting the frame) > > "omitting" OK > >+ Screenshot : function (filename) { > >+ return global.screenshot_area (0, 0, global.screen_width, global.screen_height, filename); > > does this do the right thing with dead areas in multi-monitor setups? I suppose so but I can't test right now due to libtool + rpath spoiling my day. > >+shell_global_screenshot_area (ShellGlobal *global, > >+ int x, > > align the parameters OK > >+ cogl_read_pixels (x, y, width, height, COGL_READ_PIXELS_COLOR_BUFFER, COGL_PIXEL_FORMAT_BGRA_8888_PRE, data); > > you check byte order in screenshow_window, but not in screenshot_area... ? Oops...
(In reply to comment #8) > >+ Screenshot : function (filename) { > >+ return global.screenshot_area (0, 0, global.screen_width, global.screen_height, filename); > > does this do the right thing with dead areas in multi-monitor setups? No.
Created attachment 190811 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) --- *) Fix multiscreen bug *) Fix byte order in !ScreenShotWindow *) Fix spelling / style
Created attachment 190833 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) --- *) Whitespace fix
Created attachment 190875 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) --- *) Fix build ...
Comment on attachment 190833 [details] [review] Add screenshot interface >+ cairo_region_destroy screen_region); >+ >+ cr = cairo_create (image); >+ >+ for (i = 0; i < cairo_region_num_rectangles (screen_region); i++) that needs to be "stage_region". But even then the generated PNG file is wrong. (screenshot_window() works, screenshot() doesn't).
Created attachment 190882 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) -- s/screen_region/stage_region/
Review of attachment 190882 [details] [review]: Marking needs-work based on IRC discussion with Robert.
Created attachment 192507 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) --- Read the backbuffer directly after paint asynchrounsly to avoid reading it while it is in an undefined state.
Created attachment 192508 [details] [review] Add screenshot interface Adds methods to shell_global to allow taking screenshots save the result into a specified png image. It exposes three methods via shellDBus applications like gnome-screenshot: *) Screenshot (screenshots the whole screen) *) ScreenshotWindow (screenshots the focused window) *) ScreenshotArea (screenshots a specific area) -- *) Cleanups
Comment on attachment 192508 [details] [review] Add screenshot interface Note that this patch is currently based on top of the unredirect patch, so make sure you don't accidentally commit that with it. Graphically it seems to work. It's still annoyingly slow. (With my 1400x1050 laptop screen, the shell freezes for .5s when taking the screenshot. With a 1920x1200 external monitor attached as well, it takes 1.5s.) It looks like about 1/3 of that time is cogl_read_pixels(), and 2/3 is cairo_surface_write_to_png(). At least the latter part could be moved to another thread. Not sure if we can do anything about cogl_read_pixels(). (How does the recorder avoid being slow?)
(In reply to comment #19) > (From update of attachment 192508 [details] [review]) > Note that this patch is currently based on top of the unredirect patch, so make > sure you don't accidentally commit that with it. Oops ... forgot to clean my tree. > Graphically it seems to work. It's still annoyingly slow. (With my 1400x1050 > laptop screen, the shell freezes for .5s when taking the screenshot. With a > 1920x1200 external monitor attached as well, it takes 1.5s.) It looks like > about 1/3 of that time is cogl_read_pixels(), and 2/3 is > cairo_surface_write_to_png(). At least the latter part could be moved to > another thread. Not sure if we can do anything about cogl_read_pixels(). (How > does the recorder avoid being slow?) The recorder avoids being slow by ..... being slow. I have been testing using my nvidia system where glReadPixel does not suck like that. Will test on my INTEL system later but really reading a *single* frame back shouldn't take that long ... not sure about how to make it faster ... could try using PBO but not sure that would really help. As for cairo_surface_write_to_png() yeah pushing that to a thread would definitely help (we shouldn't really do any I/O in the main thread).
Comment on attachment 192508 [details] [review] Add screenshot interface Attachment 192508 [details] pushed as b7fd78b - Add screenshot interface Pushed based on IRC discussion, bug is still open for the speed issues.
taking off the blocker, since the patch got committed
bug 653590 adds an async read_pixels API
i noticed that when grabbing the whole screen the white effect is missing the fade and is ugly and i believe slower, compared with alt+print screen effect, with a fullscreen window, 1280x800
(In reply to comment #24) > i noticed that when grabbing the whole screen the white effect is missing the > fade and is ugly and i believe slower, compared with alt+print screen effect, > with a fullscreen window, 1280x800 Not sure what you are talking about ... nothing in this bug has anything to do with a fade effect.
(In reply to comment #25) > (In reply to comment #24) > > i noticed that when grabbing the whole screen the white effect is missing the > > fade and is ugly and i believe slower, compared with alt+print screen effect, > > with a fullscreen window, 1280x800 > > Not sure what you are talking about ... nothing in this bug has anything to do > with a fade effect. Follow up on that; IRC conversation: <ioni> drago01, in the screenshot support i mean that when print screen is used the effect has a solid white screen and with alt+printscreen the effect is with a white that is transparent and it looks like a while that fades in to white and then fades out to transparent <ioni> but definitely print screen effect is ugly than with alt+printscreen <ioni> or just slow and that's why is ugly? <drago01> ioni: that has nothing to do with that bug <drago01> ioni: that is done by gnome-screenshot <drago01> ioni: not by the shell <drago01> ioni: the bug is about a dbus interface to let the shell dump a screenshot to a file <ioni> the effect is done by gnome-screenshot? <drago01> yes <ioni> ok <ioni> drago01, so right now in 3.1.90 who is doing the screenshot? <ioni> gnome-shell or gnome-screenshot? <drago01> currently still gome-screenshot see https://bugzilla.gnome.org/show_bug.cgi?id=657330 <bebot> Bug 657330: normal, Normal, ---, gnome-utils-maint, UNCONFIRMED, use the gnome-shell api for taking screenshots when available <ioni> ok, now i understand And he filed a new bug 658172
What about window alpha? Semi-translucent windows, etc? What about taking area screenshot without root window? Cf. this example. Click it, observe window borders are smooth, opacity i preserved and no root window. http://screenshots.debian.net/package/octave3.2
Created attachment 205793 [details] [review] Screenshot: Move filesystem I/O to a thread Writting the screenshot to a file can take a relativly long time in which we block the compositor,so do that part in a separate thread to avoid the hang.
Review of attachment 205793 [details] [review]: Since the implementation is getting quite complicated, can we move this from ShellGlobal into a new file? I'm fine with committing this patch, and then moving it. ::: src/shell-global.c @@ +2199,3 @@ window_rect->height); + screenshot_data->image = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, window_rect->width, window_rect->height); Keep the line wrapping the same as it was.
(In reply to comment #29) > Review of attachment 205793 [details] [review]: > > Since the implementation is getting quite complicated, can we move this from > ShellGlobal into a new file? I'm fine with committing this patch, and then > moving it. Yeah makes sense. > ::: src/shell-global.c > @@ +2199,3 @@ > window_rect->height); > > + screenshot_data->image = cairo_image_surface_create > (CAIRO_FORMAT_ARGB32, window_rect->width, window_rect->height); > > Keep the line wrapping the same as it was. OK.
Comment on attachment 205793 [details] [review] Screenshot: Move filesystem I/O to a thread Attachment 205793 [details] pushed as 882fe48 - Screenshot: Move filesystem I/O to a thread Leaving the bug open for the async read pixels and moving the screenshot stuff into a new file.
(In reply to comment #19) > Graphically it seems to work. It's still annoyingly slow. (With my 1400x1050 > laptop screen, the shell freezes for .5s when taking the screenshot. With a > 1920x1200 external monitor attached as well, it takes 1.5s.) It looks like > about 1/3 of that time is cogl_read_pixels(), and 2/3 is > cairo_surface_write_to_png(). Is it still slow for you? Now that the pixel grabbing should be faster with Owen's ScreenGrabber and the cairo_surface_write_to_png() has been moved to a thread?
Owen's patches fixed the slowdown; we have it split out of shell-global now so nothing left here.