Re: GDI+ take 3

[Eli Zaretskii <eliz@gnu.org>]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Tue, 21 Apr 2020 08:44:13 +0200
> 
> > That makes some assumptions I felt uneasy about.  First, it assumes
> > that the native_image entry will never have an init method; this could
> > become wrong at some future point, at which time someone will have to
> > remember to adapt initialize_image_type to handle that (since
> > native_image doesn't really have a library to load).
> 
> I do not think this is right. The current implementation is
> 
> - Something wants to load an image and invokes lookup_image_type()
> - lookup_image_type() calls image_can_use_native_api
>   + within image_can_use_native_api, the type of image is verified
>      - if it is right, it _initializes_ gdi+ (= init method)
>      - otherwise returns false
>   + when image_can_use_native_api returns false, lookup_image_type
>      loops over image type
>      - for each image type it calls initialize_image_type()
>        * initialize_image_type aclls image_can_use_native_api again!
>        * it then invokes the initialization method
> 
> So, in the current architecture, image_can_use_native_api() is called
> redundantly.

Yes, but it's very fast.  And also, is the above flow the only one?
Can we ever call initialize_image_type directly?  Doesn't that happen
when we first time call image-type-available-p, for example?  Such a
call can be issued by Lisp which tries to see if a certain type of
images can be displayed, and if not, use some fallback.

> > Second, what happens if the TYPE argument specifies an image type the
> > native_image cannot handle (e.g., SVG), and the corresponding optional
> > image library is not linked in or is unavailable at run time?  With
> > your patch, we would return true, right?
> 
> No. It would call the initialization method.

I think we may be miscommunicating.  If Emacs is built without
librsvg, the HAVE_RSVG macro is not defined, and the SVG part of the
image_types[] array and init_svg_functions are not compiled into
Emacs.  So there's no initialization method to call for SVG images,
and initialize_image_type should return false.

> > You are right.  However, I believe I already fixed that, albeit a bit
> > differently, as part of commit 423089d (after bumping into the same
> > problem during testing my recent changes).  We can use your change
> > instead, if you think it is better for some reason.
> 
> I am not sure. I pulled latest emacs and w32_frame_delay() currently has
> this
> 
> 	  delay = decode_delay (propertyItem, frame);
> 	  if (delay <= 0)
> 	    {
> 	      /* In GIF files, unfortunately, delay is only specified
> 		 for the first frame.  */
> 	      delay = decode_delay (propertyItem, 0);
> 	    }
> 
> This code is wrong. It originates in my wrong decoding from
> PropertyItem. I was testing the GDI+ library with various GIF files and
> they returned 0 for the delay at later frames, but it was because I
> misunderstood how Property Item works. It should read
> 
> 	  delay = decode_delay (propertyItem, frame);

OK, I'll remove that part.

> Now, in both cases (before and with this change) the output of this
> function is used here (this is the code in master).
> 
>           if (delay >= 0)
>             metadata = Fcons (Qdelay, Fcons (make_float (delay), metadata));
> 
> If delay == 0, it should not be returned. But I suppose it is just
> nitpicking.

I thought a delay of zero was a valid value.  But if it isn't, you
are, of course, right, and we should change the inequality to use >.