From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Interest in nt_load_image? Date: Mon, 30 Mar 2020 16:46:02 +0300 Message-ID: <838sji3qhx.fsf@gnu.org> References: <86369r0xcv.fsf@csic.es> <83k1324m60.fsf@gnu.org> <86imimwa4t.fsf@csic.es> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="64341"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Juan =?utf-8?Q?Jos=C3=A9_Garc=C3=ADa-Ripoll?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Mar 30 15:46:33 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jIukK-000GcE-KB for ged-emacs-devel@m.gmane-mx.org; Mon, 30 Mar 2020 15:46:32 +0200 Original-Received: from localhost ([::1]:50224 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jIukJ-00052h-K9 for ged-emacs-devel@m.gmane-mx.org; Mon, 30 Mar 2020 09:46:31 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:50665) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jIuji-0004cI-Le for emacs-devel@gnu.org; Mon, 30 Mar 2020 09:45:56 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:44469) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1jIuji-0006oG-H7; Mon, 30 Mar 2020 09:45:54 -0400 Original-Received: from [176.228.60.248] (port=2613 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jIujh-00048T-Hr; Mon, 30 Mar 2020 09:45:54 -0400 In-Reply-To: <86imimwa4t.fsf@csic.es> (juanjose.garciaripoll@gmail.com) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 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-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:246005 Archived-At: > From: Juan José García-Ripoll > > 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 . */ > > #include > +#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 */.