Re: Interest in nt_load_image?

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <address@hidden>
> Date: Mon, 30 Mar 2020 09:54:26 +0200
> 
> > Please point to the relevant APIs, to make the discussion more
> > practical.
> 
> GDI+ is an evolution of GDI that supports arbitrary plug-ins for image 
> formats,
> both bitmaps and vector type. It is a bit more modern than GDI, from what I
> get, but, just as GDI, it is not the modern standard for Windows 2D
> displays. Indeed, it is old enough that it is also supported by Windows XP.
> 
> https://docs.microsoft.com/en-us/windows/win32/gdiplus/-gdiplus-gdi-start
> 
> GDI+ has a flat C interface that allows loading images, querying properties,
> displaying them and converting them to older GDI formats.
> 
> https://docs.microsoft.com/en-us/windows/win32/gdiplus/-gdiplus-flatapi-flat

Thanks.  Interesting, I didn't know GDI+ had a C API.

Did you verify using these APIs will allow us to keep using the code
which processes images at display time?  Will image scaling and
rotation still work as it does now?  What about masking?

> - At configuration time, it works just as the NextStep (NS) system, disabling
>   the use of libpng, libjpeg, libtiff and libgif when the build system is
>   Mingw.

AFAIU, we will also be able to support EXIF without ImageMagick.

> - In images such as PNG, GIF or TIFF, it currently does not use a bitmask for
>   display. Instead, it relies on GDI+'s convertion to HBITMAP, which allows
>   alpha blending with any background color of choice.

I don't know enough to understand what that means, but if this doesn't
lose functionality, it's OK.

> - In the C code, it replaces the load_jpeg, load_gif, etc, with a generic
>   w32_load_image() function in src/w32image.c. This function is heavily
>   inspired by ns_load_image() in src/nsimage.c.

There's a complication we must consider in this regard, see below.

> The only thing that is missing is a place to call GdipShutdown(). I do not 
> know
> how to add an exit handler for Emacs' C core.

I think you want to do it in w32.c:term_ntproc.

> I have also verified that it is possible to convert all *.xpm icons to *.png
> format and thus eliminate the need to include libXpm-noX.dll. Plus, the size 
> of
> the icons is reduced by 50%

We aren't going to get rid of XPM icons in the distribution any time
soon (because of other platforms), so I don't see an urgent need to do
this.

> --- a/src/image.c
> +++ b/src/image.c
> @@ -18,6 +18,12 @@ Copyright (C) 1989, 1992-2020 Free Software Foundation, 
> Inc.
>  along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
>  
>  #include <config.h>
> +#ifdef HAVE_GDIPLUS
> +#undef HAVE_JPEG
> +#undef HAVE_PNG
> +#undef HAVE_GIF
> +#undef HAVE_TIFF
> +#endif

We cannot do this at compile time, because we still try supporting
ancient Windows versions where GDI+ is not available.  Moreover, I
think it's good to let users have the ability to decide dynamically
which image display capability they want.  It certainly makes sense to
do that initially, while the GDI+ way is being tested in all kinds of
real-life use cases.

So all the compile-time tests will have to be rewritten as run-time
tests, and we should provide variables to force Emacs use/not use
GDI+, perhaps at image format granularity, and expose those variables
to Lisp, so users could control that.

A few other minor comments about the code below:

> +#elif defined HAVE_GDIPLUS
>  
> -#endif /* HAVE_NS */
> +static bool
> +png_load (struct frame *f, struct image *img)
> +{
> +  return w32_load_image (f, img,
> +                         image_spec_value (img->spec, QCfile, NULL),
> +                         image_spec_value (img->spec, QCdata, NULL));
> +}
> +
> +#define init_png_functions init_w32_image_load_functions

And this stuff will have to learn to coexist with the current code
which loads PNG etc. images using the respective libraries.

> +static int
> +gdiplus_initialized_p()

Style: we use ANSI C99 declarations, so please say

  static int
  gdiplus_initialized_p (void)

(Btw, it looks like this function should return a 'bool', not 'int'.

Also, please leave a space between the end of the function/macro name
and the opening parentheses.

> +  static int gdip_initialized = 0;

No need to initialize a static variable to a zero value.

> +  if (gdip_initialized < 0)
> +    {
> +      return 0;
> +    }
> +  else if (gdip_initialized)
> +    {
> +      return 1;
> +    }

Style: we don't use braces when a block has only one line.

> +      status = GdiplusStartup(&token, &input, &output);
                               ^^
Style: space missing there.

> +static float
> +w32_frame_delay(GpBitmap *pBitmap, int frame)

A nit: why 'float' and not 'double'?  'float' causes a minor
inefficiency, so unless there's a good reason, I'd prefer 'double'.

> +   // Assume that the image has a property item of type 
> PropertyItemEquipMake.
> +   // Get the size of that property item.

Please use C-style comments /* like this */, not C++-style comments.

> +      fprintf(stderr, "FrameCount: %d\n", (int)frameCount);
> +      fprintf(stderr, "     index: %d\n", frame);

Are these left-overs from the debugging stage?

> +static ARGB
> +w32_image_bg_color(struct frame *f, struct image *img)
> +{
> +  /* png_color_16 *image_bg; */
        ^^^^^^^^^^^^^^^^^^^^^^^
And this?

> +  if (STRINGP (specified_bg)
> +      ? FRAME_TERMINAL (f)->defined_color_hook (f,
> +                                                SSDATA (specified_bg),
> +                                                &color,
> +                                                false,
> +                                                false)
> +      : (FRAME_TERMINAL (f)->query_frame_background_color (f, &color),
> +         true))

Do we really need to go through the hook mechanism here?  The frame
type is known in advance, right?

> +  if (STRINGP (spec_file))
> +    {
> +      filename_to_utf16 (SSDATA (spec_file) , filename);
> +      status = GdipCreateBitmapFromFile (filename, &pBitmap);

What to do here if w32-unicode-filenames is nil?

> +  else if (STRINGP (spec_data))
> +    {
> +      IStream *pStream = SHCreateMemStream ((BYTE *) SSDATA (spec_data),
> +                                            SBYTES (spec_data));

Are we sure spec_data is a unibyte string here?  Do we need an
assertion or maybe a test and a conversion?

> +      /* In multiframe pictures, select the first one */

Style: comments should end with a period and 2 spaces before */.