From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Alan Third Newsgroups: gmane.emacs.devel Subject: Re: Linking to ImageMagick by default Date: Fri, 28 Dec 2018 21:21:15 +0000 Message-ID: <20181228212115.GA59461@breton.holly.idiocy.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> 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 1546032588 9293 195.159.176.226 (28 Dec 2018 21:29:48 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 28 Dec 2018 21:29:48 +0000 (UTC) User-Agent: Mutt/1.10.1 (2018-07-13) Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Dec 28 22:29:44 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 1gczhP-0002IG-MC for ged-emacs-devel@m.gmane.org; Fri, 28 Dec 2018 22:29:43 +0100 Original-Received: from localhost ([127.0.0.1]:33251 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gczjW-0007Ev-Fg for ged-emacs-devel@m.gmane.org; Fri, 28 Dec 2018 16:31:54 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:36444) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gczjL-00074J-Sk for emacs-devel@gnu.org; Fri, 28 Dec 2018 16:31:45 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gczZU-0005WI-6b for emacs-devel@gnu.org; Fri, 28 Dec 2018 16:21:33 -0500 Original-Received: from mail-wr1-x433.google.com ([2a00:1450:4864:20::433]:43658) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gczZR-00058B-Nl; Fri, 28 Dec 2018 16:21:30 -0500 Original-Received: by mail-wr1-x433.google.com with SMTP id r10so21932026wrs.10; Fri, 28 Dec 2018 13:21:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=rI+0RDqgSxDvteZAtTRPpgpjEC6FXxY2XeifK9DD39Y=; b=uphcDOFyK3S54ulyrgRPls08DzbNNLcM6OhCcZHCGJ+OW68YLj07Qmwc/sBztNknGp ZdqsOv1ftHpFRw9Sh8oPPOB9PFmdtKGzx37793TJF8hUiH6DVYkqvsS+SiXNGsVBazTi +RZDCnE9ymBAYvp5ritAv9QVQTkRl+tKkoiiZZQW9nwyvO6s9XjviN+elWedT4ZJ3GoT OYguvHQCsXKFhTUn/HDkuS8vA8fqUybgXI2WwCNMCaWDVyxqJq80Id3zc6qn5JNkGBf2 wJ6kd06/vMgvHbxkQNmKDzDljeWySp8ozpgMPd4LloOmGJxieaf1meK3PtCwb5u4QmUK VMmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=rI+0RDqgSxDvteZAtTRPpgpjEC6FXxY2XeifK9DD39Y=; b=EDmLokmzpGgVEyGBdCvdYQ0UPnBHV/gbD5ZkoBokmzSdcTJ4SYIczXxniH21jqH18H A5LAapS/Oi4k4bvnTrLHhQpXqbdXBm2Fq4hrld71gPhhHti3releQjO7/s1AavFqDV6j 4vHDfxdXYewT/KJrZbO3ZblRO8dLb3tJIgbBb7/fRhKhudgs1N5eKqS1adMeDdQGq+qA 3AN6EtxDkf+nz0D+QzhfbqMLP89Q29dskMP18sWma+xmURP4jyQAaH/+7RLgMP97SOXN SIFD+cQgW1ymgm4SZc9QFU2fxxw5V89w5M/IEL7GZNYGM9D1Pbnml0sYUDwaxPIC5cRi kJSA== X-Gm-Message-State: AJcUukcGftU86fjl54LhBLYNbm8akIIv0HN18PosajAz+fNoJfxu5KnB DKiyRm3SCeIhpJ9dRRqL2JWnZIUZObI= X-Google-Smtp-Source: ALg8bN5fHnzPVWP4/OoSMaoXjdTRY3Md6xz3zTzrytRMTRnvEon0aALJxc3d10VC4ueFn4uEQDdFjw== X-Received: by 2002:adf:e7d0:: with SMTP id e16mr27728890wrn.142.1546032078284; Fri, 28 Dec 2018 13:21:18 -0800 (PST) Original-Received: from breton.holly.idiocy.org (ip6-2001-08b0-03f8-8129-6003-371f-a7f7-f110.holly.idiocy.org. [2001:8b0:3f8:8129:6003:371f:a7f7:f110]) by smtp.gmail.com with ESMTPSA id y13sm36651759wme.2.2018.12.28.13.21.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Dec 2018 13:21:17 -0800 (PST) Content-Disposition: inline In-Reply-To: <834lay6pe4.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:4864:20::433 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:232023 Archived-At: On Fri, Dec 28, 2018 at 10:12:35AM +0200, Eli Zaretskii wrote: > > Date: Thu, 27 Dec 2018 13:11:05 +0000 > > From: Alan Third > > Cc: emacs-devel@gnu.org > > > > I think that during redisplay something must be looking at the image > > and deciding it hasn’t changed, but I can’t find anything like that. > > My first suspicion is that redisplay doesn't even look at the image, > because nothing tells it that it should. IOW, it's a side effect of > redisplay optimizations that should be disabled in this case. Yes, I finally worked this out today. Like you say, the redisplay is a side‐effect, but of flushing an image from the cache. I’ve added a (force-window-update) to image--change-size and that’s fixed it. I can’t believe I spent so long on that. 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? > > +/* Struct for use by display functions. Contains the desired size of > > + the image, derived from the display properties. */ > > + > > +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 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. > > @@ -5213,17 +5210,8 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p > > } > > > > #ifdef HAVE_WINDOW_SYSTEM > > - if (it.what == IT_IMAGE) > > - { > > - /* Note that this ignores images that are fringe bitmaps, > > - because their image ID is zero, and so IMAGE_OPT_FROM_ID will > > - return NULL. This is okay, since fringe bitmaps are not > > - displayed in the text area, and so are never the object we > > - are interested in. */ > > - img = IMAGE_OPT_FROM_ID (it.f, it.image_id); > > - if (img && !NILP (img->spec)) > > - *object = img->spec; > > - } > > + if (it.what == IT_IMAGE && !NILP (it.object)) > > + *object = it.object; > > #endif > > This doesn't look right. The original code puts in *OBJECT the Lisp > spec of the image, whereas you put there the object being iterated by > struct it, which is a buffer or a Lisp string. Please make sure you > test the functions that call buffer_posn_from_coords, as part of your > testing, such as (AFAIR) posn-at-x-y, and verify that the info they > return is unchanged, when the coordinates are on an image. You’re right. Thanks. > > + /* 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. > 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. > > +/* 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. At the moment it’s avoided simply by not using :type imagemagick. 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. -- Alan Third