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: Linking to ImageMagick by default Date: Sat, 29 Dec 2018 08:56:29 +0200 Message-ID: <83pntk6cte.fsf@gnu.org> References: <20181205223901.GA5543@breton.holly.idiocy.org> <20181208183810.GA2465@breton.holly.idiocy.org> <20181210220944.GA4793@breton.holly.idiocy.org> <20181219160308.GA43504@breton.holly.idiocy.org> <83efadcw3j.fsf@gnu.org> <20181227131105.GA18792@breton.holly.idiocy.org> <834lay6pe4.fsf@gnu.org> <20181228212115.GA59461@breton.holly.idiocy.org> 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 1546066528 24203 195.159.176.226 (29 Dec 2018 06:55:28 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 29 Dec 2018 06:55:28 +0000 (UTC) Cc: emacs-devel@gnu.org To: Alan Third Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Dec 29 07:55:23 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 1gd8Wo-0006BX-Kz for ged-emacs-devel@m.gmane.org; Sat, 29 Dec 2018 07:55:22 +0100 Original-Received: from localhost ([127.0.0.1]:35014 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gd8Yu-0005eg-18 for ged-emacs-devel@m.gmane.org; Sat, 29 Dec 2018 01:57:32 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:43443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gd8YI-0005c7-Uy for emacs-devel@gnu.org; Sat, 29 Dec 2018 01:57:27 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gd8YD-0002dK-Pj for emacs-devel@gnu.org; Sat, 29 Dec 2018 01:56:54 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:40010) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gd8YD-0002be-Gi; Sat, 29 Dec 2018 01:56:49 -0500 Original-Received: from [176.228.60.248] (port=2091 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1gd8YD-0006Wd-1a; Sat, 29 Dec 2018 01:56:49 -0500 In-reply-to: <20181228212115.GA59461@breton.holly.idiocy.org> (message from Alan Third on Fri, 28 Dec 2018 21:21:15 +0000) 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:232029 Archived-At: > Date: Fri, 28 Dec 2018 21:21:15 +0000 > From: Alan Third > Cc: emacs-devel@gnu.org > > The docstring for image-flush looks like this: > > doc: /* Flush the image with specification SPEC on frame FRAME. > This removes the image from the Emacs image cache. If SPEC specifies > an image file, the next redisplay of this image will read from the > current contents of that file. > > FRAME nil or omitted means use the selected frame. > FRAME t means refresh the image on all frames. */) > > Should it mention that it will garbage the frame and therefore cause a > redisplay? I'm okay with saying that this causes a complete redisplay of FRAME (or all frames if FRAME is t), but the bit about making the frames garbaged is an implementation detail, so I don't think it belongs to the doc string. > > > +struct image_spec > > > +{ > > > + ptrdiff_t image_id; > > > + int width; > > > + int height; > > > + > > > + /* Relief to draw around the image. */ > > > + int relief; > > > + > > > + /* Optional margins around the image. This includes the relief. */ > > > + int hmargin, vmargin; > > > + > > > + /* Percent of image height used as ascent. A value of > > > + CENTERED_IMAGE_ASCENT means draw the image centered on the > > > + line. */ > > > + int ascent; > > > +#define DEFAULT_IMAGE_ASCENT 50 > > > +#define CENTERED_IMAGE_ASCENT -1 > > > +}; > > > > I'm not sure I understand the need for introducing this new struct. > > For starters, it repeats some of the fields we have already in other > > structures. Can you explain why you needed this? Why not store the > > results of calculating the size (in calc_image_spec) in the original > > structure returned by IMAGE_FROM_ID, for example? > > > > One reason to avoid this new struct is that it makes struct glyph, > > struct glyph_string, and struct it larger, which will make the display > > code slower, especially if the larger structs cause spilling of CPU > > caches. The display engine shuffles these structures to and fro in > > many places, so they should be as small as possible. > > I added this struct to separate the image pixel data from the size > information. The idea is to avoid loading and holding multiple copies > of the same image in RAM when we want to display at different sizes. I think with my suggestion you will still hold only one copy: the last one whose size was calculated. AFAIU, the same happens with your implementation, you just hold the last size separately. > I considered making the reference in struct it and others a pointer to > struct image_spec because it is noticeably larger than what it’s > replacing, but I’m unsure of the lifetime and when I would have to > free it and so on. No, this is not a good idea, so you were right not to do that. > > > + /* FIXME: Surely there's a better way to do this? */ > > > + if (!EQ (property, QCwidth) > > > + && !EQ (property, QCheight) > > > + && !EQ (property, QCmax_width) > > > + && !EQ (property, QCmax_height) > > > + && !EQ (property, QCscale) > > > + && !EQ (property, QCmargin) > > > + && !EQ (property, QCascent) > > > + && !EQ (property, QCrelief)) > > > + cache_spec = Fcons (property, Fcons (value, cache_spec)); > > > > Why did you think there was something wrong with this code? > > I just thought there might be a neater way to do it. To be honest, I > think when I wrote that message the code was full of XCARs which I > subsequently got rid of. But if you’re happy with it then I’m happy > with it. I see nothing wrong with a series of equality checks, we do that elsewhere, albeit not in a loop. > > One possible alternative would be not to cons a new image spec without > > these attributes, but compare specs ignoring these attributes instead > > of using EQ. Did you consider this alternative? > > It might be more efficient. I’m not sure which option would be better. To my eyes, consing a list that will be used only once is less elegant, and will also cause more GC. So maybe try the alternative, and see if it works well. > > > > +/* Compute the desired size of an image with native size WIDTH x HEIGHT. > > > + Use SPEC to deduce the size. Store the desired size into > > > + *D_WIDTH x *D_HEIGHT. Store -1 x -1 if the native size is OK. */ > > > +static void > > > +compute_image_size (size_t width, size_t height, > > > + Lisp_Object spec, > > > + int *d_width, int *d_height) > > > +{ > > > + Lisp_Object value; > > > + int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1; > > > + double scale = 1; > > > + > > > + value = image_spec_value (spec, QCscale, NULL); > > > + if (NUMBERP (value)) > > > + scale = XFLOATINT (value); > > > + > > > + value = image_spec_value (spec, QCmax_width, NULL); > > > + if (FIXNATP (value)) > > > + max_width = min (XFIXNAT (value), INT_MAX); > > > + > > > + value = image_spec_value (spec, QCmax_height, NULL); > > > + if (FIXNATP (value)) > > > + max_height = min (XFIXNAT (value), INT_MAX); > > > + > > > + /* If width and/or height is set in the display spec assume we want > > > + to scale to those values. If either h or w is unspecified, the > > > + unspecified should be calculated from the specified to preserve > > > + aspect ratio. */ > > > + value = image_spec_value (spec, QCwidth, NULL); > > > + if (FIXNATP (value)) > > > + { > > > + desired_width = min (XFIXNAT (value) * scale, INT_MAX); > > > + /* :width overrides :max-width. */ > > > + max_width = -1; > > > + } > > > + > > > + value = image_spec_value (spec, QCheight, NULL); > > > + if (FIXNATP (value)) > > > + { > > > + desired_height = min (XFIXNAT (value) * scale, INT_MAX); > > > + /* :height overrides :max-height. */ > > > + max_height = -1; > > > + } > > > + > > > + /* If we have both width/height set explicitly, we skip past all the > > > + aspect ratio-preserving computations below. */ > > > + if (desired_width != -1 && desired_height != -1) > > > + goto out; > > > > This avoids the unnecessary calculations too late, IMO. Since I > > expect most images to not require resizing, I suggest to make that > > frequent case as fast as it is today, which means avoid any > > unnecessary lookups in the image spec. As a minimum, you don't need > > to look up :max-width and :max-height. Bonus points if you avoid > > calling this function altogether when the image needs no resizing. > > I’m not sure how to check whether the image needs resizing without > doing the lookups for :width, :height, :max-width, etc. Maybe I'm misreading the code, but it looks to me you only need :scale, :width, and :height. Am I missing something? > At the moment it’s avoided simply by not using :type imagemagick. If the caller might know whether resizing is needed, perhaps we should pass an extra flag argument to that effect. > It should be possible to avoid all this for fringe bitmaps, though. > The image IDs for them are negative numbers, so there’s no > point doing the calculations if id < 0. Definitely. Thanks.