From: Eli Zaretskii <eliz@gnu.org>
To: "Juan José García-Ripoll" <juanjose.garciaripoll@gmail.com>
Cc: emacs-devel@gnu.org
Subject: Re: Interest in nt_load_image?
Date: Mon, 30 Mar 2020 16:46:02 +0300 [thread overview]
Message-ID: <838sji3qhx.fsf@gnu.org> (raw)
In-Reply-To: <86imimwa4t.fsf@csic.es> (juanjose.garciaripoll@gmail.com)
> From: Juan José García-Ripoll
> <juanjose.garciaripoll@gmail.com>
> 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 */.
next prev parent reply other threads:[~2020-03-30 13:46 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-29 19:33 Interest in nt_load_image? Juan José García-Ripoll
2020-03-29 21:55 ` Juan José García-Ripoll
2020-03-30 2:21 ` Eli Zaretskii
2020-03-30 7:54 ` Juan José García-Ripoll
2020-03-30 13:46 ` Eli Zaretskii [this message]
2020-03-30 14:36 ` Juan José García-Ripoll
2020-03-30 14:44 ` Juan José García-Ripoll
2020-03-30 15:50 ` Eli Zaretskii
2020-03-30 16:15 ` Juan José García-Ripoll
2020-03-30 16:37 ` Eli Zaretskii
2020-03-30 17:16 ` Stefan Monnier
2020-03-30 18:32 ` Eli Zaretskii
2020-03-30 18:41 ` Stefan Monnier
2020-03-30 18:57 ` Eli Zaretskii
2020-03-30 19:39 ` Stefan Monnier
2020-03-31 14:19 ` Eli Zaretskii
2020-03-30 18:06 ` Juan José García-Ripoll
2020-03-30 18:47 ` Eli Zaretskii
2020-03-30 18:53 ` Eli Zaretskii
2020-03-30 23:21 ` Juanma Barranquero
2020-03-31 8:01 ` Juan José García-Ripoll
2020-03-31 13:14 ` Juanma Barranquero
2020-03-31 14:50 ` Stefan Monnier
2020-03-31 15:25 ` Juanma Barranquero
2020-03-31 16:54 ` Stefan Monnier
2020-03-31 15:13 ` Juan José García-Ripoll
2020-03-31 15:28 ` Juanma Barranquero
2020-03-31 13:31 ` Eli Zaretskii
2020-03-30 15:14 ` Stefan Monnier
2020-03-30 15:57 ` Eli Zaretskii
2020-03-30 17:10 ` Stefan Monnier
2020-03-30 18:23 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=838sji3qhx.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=emacs-devel@gnu.org \
--cc=juanjose.garciaripoll@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).