From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: x-export-frames for non-Cairo builds Date: Fri, 02 Feb 2018 12:30:41 +0200 Message-ID: <83shajd6pa.fsf@gnu.org> References: <83372tm491.fsf@gnu.org> <83607ola9h.fsf@gnu.org> <13e61094-02dd-d7ba-c48e-25f1187f0e9b@gmail.com> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1517588199 22571 195.159.176.226 (2 Feb 2018 16:16:39 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 2 Feb 2018 16:16:39 +0000 (UTC) Cc: emacs-devel@gnu.org To: =?utf-8?Q?Cl=C3=A9ment?= Pit-Claudel Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Feb 02 17:16:34 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ehe0r-0005IY-H8 for ged-emacs-devel@m.gmane.org; Fri, 02 Feb 2018 17:16:29 +0100 Original-Received: from localhost ([::1]:38671 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehe2s-0007yB-DC for ged-emacs-devel@m.gmane.org; Fri, 02 Feb 2018 11:18:34 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:47804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehdBp-0001eN-T6 for emacs-devel@gnu.org; Fri, 02 Feb 2018 10:24:51 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ehdAl-0000M3-Jp for emacs-devel@gnu.org; Fri, 02 Feb 2018 10:23:45 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:47378) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehdAl-000607-8p; Fri, 02 Feb 2018 10:22:39 -0500 Original-Received: from [176.228.60.248] (port=3734 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1ehYcU-0005UL-Ks; Fri, 02 Feb 2018 05:30:59 -0500 In-reply-to: <13e61094-02dd-d7ba-c48e-25f1187f0e9b@gmail.com> (message from =?utf-8?Q?Cl=C3=A9ment?= Pit-Claudel on Fri, 26 Jan 2018 18:13:28 -0500) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:222405 Archived-At: > From: Clément Pit-Claudel > Date: Fri, 26 Jan 2018 18:13:28 -0500 > > Understood, thanks. Here's a first draft of the patch. I have a few questions: Thanks, and apologies for a long delay in responding. > * Is there a way to wrap 'attributes: noreturn' in a preprocessor directive? I used an auxilliary C function instead because of that. I don't understand why you needed that attribute. Can you elaborate? In general, the auxiliary function is short enough to make the function redundant: just put its body inside Fexport_frame. > * Is it OK to reuse `frame' after calling `decode_window_system_frame (frame)'? Yes. That function doesn't change its argument, it only extracts a C pointer from it. > * What would be the proper way to save the output of Fx_export_frame (there's a FIXME at that point in the code)? I could either use a_write on the actual string data, or add further branches to x_cr_export_frame to write to disk directly. Just open a file and write the data into it. > * x_cr_export_frame does a number of things that I don't understand too well: > specbind (Qredisplay_dont_pause, Qt); > redisplay_preserve_echo_area (31); > block_input (); > record_unwind_protect (x_cr_destroy, make_save_ptr (cr)); > x_clear_area (f, 0, 0, width, height); > expose_frame (f, 0, 0, width, height); > Do I need any of these in x_gtk3_export_frame? These measures make sure the frame is fully visible and completely redrawn, before you export it, so I think you do need them. The call to record_unwind_protect is needed because otherwise the data allocated by cairo_create will not be released if the function signals an error, or the user types C-g. If none of the objects created inside x_gtk3_export_frame need to be released (the code indicates they don't, but maybe you forgot something), then you don't need a similar call to record_unwind_protect. > * The docs of gdk_pixbuf_get_from_window say that "If the window you’re obtaining data from is partially obscured by other windows, then the contents of the pixbuf areas corresponding to the obscured regions are undefined". Is there a way I can check for that? I think the above measures eliminate this problem by calling expose_frame. > I made it possible for the function to return a string instead of writing to disk to save time (I'm hoping to make Emacs screencasts). One issue is that the cast to an Emacs string is still quite slow (quick benchmarks: I did 200 screenshots with xwd, with my new function saving a png and then a bmp into a string, and with my new function saving a png and then a bmp to disk: 1.82s, 5.1s, 1.84s, 5.2s, 0.7s[!]). Is there a trick I could use to make image capture faster? Could I store the GdkPixbuf directly into an Emacs image and save it later? Sorry, I don't know enough about GTK programming to answer that. > On 2018-01-26 09:45, Stefan Monnier wrote: > > And which code to use should be based on dispatching on the frame type > > (so it can work even if we have several different kinds of frame types: > > I'm not sure I understood this right. Does the patch below do what you had in mind? I believe so. Some more comments below. > > Cheers, > Clément. > > [2:text/x-patch Hide Save:export-frame.patch (6kB)] > > diff --git a/src/frame.c b/src/frame.c > index 9b56080..f0c0d34 100644 > --- a/src/frame.c > +++ b/src/frame.c > @@ -3508,6 +3508,67 @@ bottom edge of FRAME's display. */) > > return Qt; > } > + > +#ifdef HAVE_WINDOW_SYSTEM > +static > +#if ! (defined HAVE_GTK3 || defined USE_CAIRO) > +_Noreturn > +#endif > +Lisp_Object > +export_frame (Lisp_Object frame, Lisp_Object type, Lisp_Object fname) > +{ > + struct frame *f = decode_window_system_frame (frame); > + > + if (!FRAME_VISIBLE_P (f)) > + error ("Frames to be exported must be visible"); > + > + if (FRAME_OBSCURED_P (f)) > + error ("Frames to be exported must not be obscured"); > + > + if (!NILP(fname)) > + CHECK_STRING(fname); The Cairo code in x-export-frames also does these checks, so why not call x_cr_export_frame directly? > +#ifdef USE_CAIRO > + // FIXME: save output when FNAME is non-nil > + /* Question: Is it OK to reuse FRAME here? */ > + return Fx_export_frames(frame, type); > +#elif HAVE_GTK3 > + return x_gtk3_export_frame(f, type, fname); > +#endif Here and elsewhere please make sure you leave a blank between the name of a function/macro and the opening parenthesis. > + /* Fall through */ > + case output_initial: > + case output_termcap: > + case output_w32: > + case output_msdos_raw: > + case output_ns: > + default: > + error ("Unsupported toolkit"); I think implementation for text-mode frames should not be too hard: you could reuse some of the code in dump-frame-glyph-matrix and its subroutines. The result should be a text file, of course. Also "Unsupported toolkit" is slightly misleading, it would be better to say something like "This command does not yet support frames of type %s" with a suitable string replacing %s. > +DEFUN ("export-frame", Fexport_frame, > + Sexport_frame, 0, 3, 0, > + doc: /* Capture a screenshot of FRAME in TYPE format. > +Available formats depend on the graphic toolkit in use. This last sentence should be moved to after the description of TYPE. (And I would call the argument FORMAT instead.) > +Currently, this function only works with Cairo and GTK3. > + > +FRAME must be a live, visible, and unobscured frame. It defaults to Partially obscured frames are okay, right? If so, the doc string should tell that. > +If FNAME is non-nil, save the resulting capture under FNAME; if > +FNAME is nil, return the captured data as a unibyte string. */) This never says explicitly that FNAME is a file name. Either tell that or call the argument FILE. Also, "save in file FNAME" is better; "under" is a strange way of saying this.