Re: GDI+ take 3

[Juan José García-Ripoll]

Eli Zaretskii <address@hidden> writes:
>> From: Juan José García-Ripoll
>>  <address@hidden>
>> Date: Sun, 19 Apr 2020 22:28:24 +0200
>> 
>> Two minor changes attached. First, the introduction of the Qnative_image
>> type makes one call to image_can_use_native_api irrelevant.
>
> 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. The second call can be eliminated because it will always
return false. Moreover, image_can_use_native_api() always must implement
an init method, because failed initialization is a cause for not
supporting the API.

> 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.

>> Regarding this, I think it is a bad idea to introduce Qnative_image,
>> because that is not an image format and users cannot recognize the
>> actual image type from the image descriptor.
>
> I'm not sure I understand the issue.  When you say "users", what do
> you mean in this context?  If you mean users like me and you, then how
> and where would we see Qnative_image?

By "users" I mean anyone using the image library, from image.el,
subcompoments or third-party libraries that use that. I assumed that the
internal type in image.c was also the type returned by functions such as
IMAGE-TYPE and the like. I am not sure any more.

> What would you propose to do instead?

To separate the notion of image type from the notion of image
handler. The former is a unique identifier for the type of image or
format, the latter is which library handles it. But, as I said above, if
img->type.type is not exposed, it does not matter.

>> The second fix makes w32image.c behave like nsimage.c, in that a delay=0
>> is regarded as non-existant, thus returning nil. This makes animations
>> default to the minimum animation delay, which currently is 0.01, and
>> more useful than a delay of 0.
>
> 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);

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.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es