png images in tool-bar / alpha mask

png images in tool-bar / alpha mask

[David Reitter]

I'm trying to use PNG images with alpha information in the tool-bar  
and find that the mask / alpha is ignored:

(find-image (list  (list :type 'png :file "undo.png")))

i.e. I'm not setting a ":mask 'heuristic" property here.  The  
background just appears white.

This is with the new toolkit toolbars (under Carbon), but I remember  
having such problems with the non-toolkit toolbar as well.
`image-make-p' generally returns nil for PNGs.

It seems that "image_load_quartz2d" is used to load the file (in  
image.c).  This already determines some background colors and draws it  
into a new graphics context, with the background color present.  And  
then it doesn't actually store the original image data, but creates an  
XCreatePixmap.  (A mask is handled separately.)

Would the solution be to also store the original image data  
(CGImageRef) in the "struct image", and then to simply use that when  
defining the toolbar, if it is present and if the toolkit supports it?

As is probably obvious, I'm not familiar with how this is generally  
handled in Emacs, but if someone could provide a bit of guidance, I  
could try to make it work.

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 1 Apr 2008 18:14:45 +0100, David Reitter <david.reitter@gmail.com> said:

> Would the solution be to also store the original image data
> (CGImageRef) in the "struct image", and then to simply use that when
> defining the toolbar, if it is present and if the toolkit supports
> it?

That wouldn't work for the case that some postprocessing is necessary
for the loaded image.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

On 1 Apr 2008, at 22:41, YAMAMOTO Mitsuharu wrote:
>>
>> Would the solution be to also store the original image data
>> (CGImageRef) in the "struct image", and then to simply use that when
>> defining the toolbar, if it is present and if the toolkit supports
>> it?
>
> That wouldn't work for the case that some postprocessing is necessary
> for the loaded image.

I think that would be okay.  For high-quality images, such processing  
is best done manually or with a dedicated tool chain  
(ImageMagick...).  One could remove the reference to the original  
image as soon as any kind of processing is done in order to maintain  
compatibility.

I'm working on a patch to see if this is doable.  I'll get back to you  
once I've tested it.

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 2 Apr 2008 00:10:09 +0100, David Reitter <david.reitter@gmail.com> said:

>>> Would the solution be to also store the original image data
>>> (CGImageRef) in the "struct image", and then to simply use that
>>> when defining the toolbar, if it is present and if the toolkit
>>> supports it?
>> 
>> That wouldn't work for the case that some postprocessing is
>> necessary for the loaded image.

> I think that would be okay.  For high-quality images, such
> processing is best done manually or with a dedicated tool chain
> (ImageMagick...).  One could remove the reference to the original
> image as soon as any kind of processing is done in order to maintain
> compatibility.

Then, that sounds feasible.

> I'm working on a patch to see if this is doable.  I'll get back to
> you once I've tested it.

OK, I'll wait a month.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

On 2 Apr 2008, at 13:39, YAMAMOTO Mitsuharu wrote:
>
>> I'm working on a patch to see if this is doable.  I'll get back to
>> you once I've tested it.
>
> OK, I'll wait a month.

OK, how about the patches below?

The change to image.c is actually very short.
The patch to tool-bar.el is intended to load PNG files where they are  
available, and to automatically load files named <basename>_dis.<ext>  
and <basename>_sel.<ext> to automatically find the disabled (greyed  
out) and selected (darkened) variant where provided.

I find that the algorithm that is applied now (by OS X at least) to my  
PNG graphics works perfectly, so I'm not even providing the _dis and  
_sel files any longer.  However, this was beneficial when the non-tool- 
kit toolbar was used to avoid the default manipulation of the images,  
so I'm leaving it in for others.




Index: image.c
===================================================================
RCS file: /sources/emacs/emacs/src/image.c,v
retrieving revision 1.65.2.13
diff -c -r1.65.2.13 image.c
*** image.c	28 Mar 2008 14:57:32 -0000	1.65.2.13
--- image.c	3 Apr 2008 15:01:11 -0000
***************
*** 2767,2773 ****
       }
     CGContextDrawImage (context, rectangle, image);
     QDEndCGContext (ximg, &context);
!   CGImageRelease (image);

     /* Maybe fill in the background field while we have ximg handy. */
     if (NILP (image_spec_value (img->spec, QCbackground, NULL)))
--- 2767,2774 ----
       }
     CGContextDrawImage (context, rectangle, image);
     QDEndCGContext (ximg, &context);
!
!   img->data.ptr_val = image; /* retain original data */

     /* Maybe fill in the background field while we have ximg handy. */
     if (NILP (image_spec_value (img->spec, QCbackground, NULL)))
Index: tool-bar.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/tool-bar.el,v
retrieving revision 1.7.2.2
diff -c -r1.7.2.2 tool-bar.el
*** tool-bar.el	7 Jan 2008 02:44:11 -0000	1.7.2.2
--- tool-bar.el	3 Apr 2008 15:04:53 -0000
***************
*** 90,95 ****
--- 90,165 ----
   		'(menu-item "tool bar" ignore
   			    :filter (lambda (ignore) tool-bar-map)))

+ (defun tool-bar-set-file-extension (image-spec-list extension)
+   "Set new file extensions for all :file properties
+ Replace any extensions of :file properties in elements of
+ IMAGE-SPEC-LIST. An extension may start with a period . or an
+ underscore. EXTENSION and the original file name extension (starting
+ with a period) are added to the file name.
+
+ E.g. foo_dis.xpm becomes foo_sel.xpm if EXTENSION is '_sel'."
+   (mapcar
+    (lambda (spec)
+      (let ((f (plist-get spec :file))
+ 	    )
+         (if (null f)
+ 	    spec
+ 	  ;; need to replace previous extensions, including those
+ 	  ;; starting with _ -
+ 	  (plist-put spec :file (concat (replace-regexp-in-string "[\.\_].*$"
+ 								  "" f)
+ 					extension
+ 					(file-name-extension f t)))
+ 	  )))
+    image-spec-list))
+
+ (defun tool-bar-get-image-spec (icon)
+   (let* ((fg (face-attribute 'tool-bar :foreground))
+ 	 (bg (face-attribute 'tool-bar :background))
+ 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
+ 			(if (eq bg 'unspecified) nil (list :background bg))))
+ 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
+ 	 (xpm-lo-spec (if (> (display-color-cells) 256)
+ 			  nil
+ 			(list :type 'xpm :file
+                               (concat "low-color/" icon ".xpm"))))
+ 	 (png-spec (list :type 'png
+ 			   :file (concat icon ".png") :background "grey") )
+ 	 (pbm-spec (append (list :type 'pbm :file
+                                  (concat icon ".pbm")) colors))
+ 	 (xbm-spec (append (list :type 'xbm :file
+                                  (concat icon ".xbm")) colors))
+ 	 (image (find-image
+ 		(if (display-color-p)
+ 		    (list png-spec xpm-lo-spec xpm-spec pbm-spec xbm-spec)
+ 		  (list pbm-spec xbm-spec xpm-lo-spec xpm-spec))))
+ 	 (image-sel (find-image
+ 		     (if (display-color-p)
+ 			 (tool-bar-set-file-extension
+ 			  (list png-spec xpm-lo-spec xpm-spec pbm-spec xbm-spec)
+ 			  "_sel")
+ 		       nil)))
+ 	 (image-dis (find-image
+ 		     (if (display-color-p)
+ 			 (tool-bar-set-file-extension
+ 			  (list png-spec xpm-lo-spec xpm-spec pbm-spec xbm-spec)
+ 			  "_dis")
+ 		       nil)))
+ 	 (images (when image ;; image may be nil if not found.
+ 		    (unless (image-mask-p image)
+  		     (setq image (append image '(:mask heuristic))))
+ 		   (if (and image-sel image-dis)
+ 		       (progn		
+ 			 (unless (image-mask-p image-sel)
+ 			   (setq image-sel (append image-sel
+ 						   '(:mask heuristic))))
+ 			 (unless (image-mask-p image-dis)
+ 			   (setq image-dis (append image-dis
+ 						   '(:mask heuristic))))
+ 			 (vector image-sel image image-dis image-dis))
+ 		     image))))
+     (cons image images)))
+
   ;;;###autoload
   (defun tool-bar-add-item (icon def key &rest props)
     "Add an item to the tool bar.
***************
*** 119,147 ****
   function will first try to use low-color/ICON.xpm if display-color- 
cells
   is less or equal to 256, then ICON.xpm, then ICON.pbm, and finally
   ICON.xbm, using `find-image'."
!   (let* ((fg (face-attribute 'tool-bar :foreground))
! 	 (bg (face-attribute 'tool-bar :background))
! 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
! 			(if (eq bg 'unspecified) nil (list :background bg))))
! 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
! 	 (xpm-lo-spec (if (> (display-color-cells) 256)
! 			  nil
! 			(list :type 'xpm :file
!                               (concat "low-color/" icon ".xpm"))))
! 	 (pbm-spec (append (list :type 'pbm :file
!                                  (concat icon ".pbm")) colors))
! 	 (xbm-spec (append (list :type 'xbm :file
!                                  (concat icon ".xbm")) colors))
! 	 (image (find-image
! 		(if (display-color-p)
! 		    (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
! 		  (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))))
!
       (when (and (display-images-p) image)
-       (unless (image-mask-p image)
- 	(setq image (append image '(:mask heuristic))))
         (define-key-after map (vector key)
! 	`(menu-item ,(symbol-name key) ,def :image ,image ,@props)))))

   ;;;###autoload
   (defun tool-bar-add-item-from-menu (command icon &optional map  
&rest props)
--- 189,200 ----
   function will first try to use low-color/ICON.xpm if display-color- 
cells
   is less or equal to 256, then ICON.xpm, then ICON.pbm, and finally
   ICON.xbm, using `find-image'."
!   (let* ((is (tool-bar-get-image-spec icon))
! 	 (image (car is))
! 	 (images (cdr is)))
       (when (and (display-images-p) image)
         (define-key-after map (vector key)
! 	`(menu-item ,(symbol-name key) ,def :image ,images ,@props)))))

   ;;;###autoload
   (defun tool-bar-add-item-from-menu (command icon &optional map  
&rest props)
***************
*** 174,196 ****
       (setq from-map global-map))
     (let* ((menu-bar-map (lookup-key from-map [menu-bar]))
   	 (keys (where-is-internal command menu-bar-map))
! 	 (fg (face-attribute 'tool-bar :foreground))
! 	 (bg (face-attribute 'tool-bar :background))
! 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
! 			(if (eq bg 'unspecified) nil (list :background bg))))
! 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
! 	 (xpm-lo-spec (if (> (display-color-cells) 256)
! 			  nil
! 			(list :type 'xpm :file
!                               (concat "low-color/" icon ".xpm"))))
! 	 (pbm-spec (append (list :type 'pbm :file
!                                  (concat icon ".pbm")) colors))
! 	 (xbm-spec (append (list :type 'xbm :file
!                                  (concat icon ".xbm")) colors))
! 	 (spec (if (display-color-p)
! 		   (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
! 		 (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))
! 	 (image (find-image spec))
   	 submap key)
       (when (and (display-images-p) image)
         ;; We'll pick up the last valid entry in the list of keys if
--- 227,235 ----
       (setq from-map global-map))
     (let* ((menu-bar-map (lookup-key from-map [menu-bar]))
   	 (keys (where-is-internal command menu-bar-map))
! 	 (is (tool-bar-get-image-spec icon))
! 	 (image (car is))
! 	 (images (cdr is))
   	 submap key)
       (when (and (display-images-p) image)
         ;; We'll pick up the last valid entry in the list of keys if
***************
*** 210,221 ****
   			key kk)))))
         (when (and (symbolp submap) (boundp submap))
   	(setq submap (eval submap)))
-       (unless (image-mask-p image)
- 	(setq image (append image '(:mask heuristic))))
         (let ((defn (assq key (cdr submap))))
   	(if (eq (cadr defn) 'menu-item)
   	    (define-key-after in-map (vector key)
! 	      (append (cdr defn) (list :image image) props))
   	  (setq defn (cdr defn))
   	  (define-key-after in-map (vector key)
   	    (let ((rest (cdr defn)))
--- 249,258 ----
   			key kk)))))
         (when (and (symbolp submap) (boundp submap))
   	(setq submap (eval submap)))
         (let ((defn (assq key (cdr submap))))
   	(if (eq (cadr defn) 'menu-item)
   	    (define-key-after in-map (vector key)
! 	      (append (cdr defn) (list :image images) props))
   	  (setq defn (cdr defn))
   	  (define-key-after in-map (vector key)
   	    (let ((rest (cdr defn)))

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 3 Apr 2008 16:09:06 +0100, David Reitter <david.reitter@gmail.com> said:

>>> I'm working on a patch to see if this is doable.  I'll get back to
>>> you once I've tested it.
>> 
>> OK, I'll wait a month.

> OK, how about the patches below?

> The change to image.c is actually very short.

It seems to completely ignore postprocessing for PNG images.  Didn't
you say that "One could remove the reference to the original image as
soon as any kind of processing is done in order to maintain
compatibility."?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On 3 Apr 2008, at 22:15, YAMAMOTO Mitsuharu wrote:
>
> It seems to completely ignore postprocessing for PNG images.  Didn't
> you say that "One could remove the reference to the original image as
> soon as any kind of processing is done in order to maintain
> compatibility."?

prepare_image_for_display sets data.ptr_val in the image structure as  
well.
What happens when the image is manipulated after it's been first drawn?

I assumed that data.ptr_val would be invalidated at that point, as is  
done in x_clear_image_1.

Are you concerned about postprocess_image?
One would have to invalidate the pointer and free the image when  
anything is done to the image there (x_build_heuristic_mask is called,  
or QCconversion is given).

Any other places where image manipulation can take place?



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 3 Apr 2008 22:42:47 +0100, David Reitter <david.reitter@gmail.com> said:

>> It seems to completely ignore postprocessing for PNG images.
>> Didn't you say that "One could remove the reference to the original
>> image as soon as any kind of processing is done in order to
>> maintain compatibility."?

> prepare_image_for_display sets data.ptr_val in the image structure
> as well.  What happens when the image is manipulated after it's been
> first drawn?

prepare_image_for_display is never called before all the
postprocessing for the image is completed.

				     YAMAMOTO Mitusharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

On 3 Apr 2008, at 22:55, YAMAMOTO Mitsuharu wrote:
>
> prepare_image_for_display is never called before all the
> postprocessing for the image is completed.

OK, so postprocessing seems to be the only processing that can be done  
to an image.


Index: image.c
===================================================================
RCS file: /sources/emacs/emacs/src/image.c,v
retrieving revision 1.65.2.13
diff -c -r1.65.2.13 image.c
*** image.c	28 Mar 2008 14:57:32 -0000	1.65.2.13
--- image.c	3 Apr 2008 22:38:10 -0000
***************
*** 1549,1555 ****
       }

   #if defined (MAC_OS) && USE_CG_DRAWING
!   if (img->data.ptr_val)
       {
         CGImageRelease (img->data.ptr_val);
         img->data.ptr_val = NULL;
--- 1549,1555 ----
       }

   #if defined (MAC_OS) && USE_CG_DRAWING
!   if (img->data.ptr_val != NULL)
       {
         CGImageRelease (img->data.ptr_val);
         img->data.ptr_val = NULL;
***************
*** 1850,1861 ****

         mask = image_spec_value (spec, QCheuristic_mask, NULL);
         if (!NILP (mask))
! 	x_build_heuristic_mask (f, img, mask);
         else
   	{
   	  int found_p;

   	  mask = image_spec_value (spec, QCmask, &found_p);

   	  if (EQ (mask, Qheuristic))
   	    x_build_heuristic_mask (f, img, Qt);
--- 1850,1877 ----

         mask = image_spec_value (spec, QCheuristic_mask, NULL);
         if (!NILP (mask))
! 	{
! #if defined (MAC_OS) && USE_CG_DRAWING
! 	  if (img->data.ptr_val != NULL)
! 	    {
! 	      CGImageRelease (img->data.ptr_val);
! 	      img->data.ptr_val = NULL;
! 	    }
! #endif
! 	  x_build_heuristic_mask (f, img, mask);
! 	}
         else
   	{
   	  int found_p;

   	  mask = image_spec_value (spec, QCmask, &found_p);
+ #if defined (MAC_OS) && USE_CG_DRAWING
+ 	  if (!NILP(mask) && img->data.ptr_val != NULL)
+ 	    {
+ 	      CGImageRelease (img->data.ptr_val);
+ 	      img->data.ptr_val = NULL;
+ 	    }
+ #endif

   	  if (EQ (mask, Qheuristic))
   	    x_build_heuristic_mask (f, img, Qt);
***************
*** 2756,2761 ****
--- 2772,2782 ----
         UNGCPRO;
         return 0;
       }
+
+ #ifdef USE_CG_DRAWING
+   img->data.ptr_val = image; /* retain original data */
+ #endif
+
     rectangle = CGRectMake (0, 0, width, height);
     QDBeginCGContext (ximg, &context);
     if (png_p)
***************
*** 2767,2773 ****
       }
     CGContextDrawImage (context, rectangle, image);
     QDEndCGContext (ximg, &context);
-   CGImageRelease (image);

     /* Maybe fill in the background field while we have ximg handy. */
     if (NILP (image_spec_value (img->spec, QCbackground, NULL)))
--- 2788,2793 ----

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 3 Apr 2008 23:42:46 +0100, David Reitter <david.reitter@gmail.com> said:

>    	  mask = image_spec_value (spec, QCmask, &found_p);
> + #if defined (MAC_OS) && USE_CG_DRAWING
> + 	  if (!NILP(mask) && img->data.ptr_val != NULL)
> + 	    {
> + 	      CGImageRelease (img->data.ptr_val);
> + 	      img->data.ptr_val = NULL;
> + 	    }
> + #endif

`:mask nil' means removing the mask.

> + #ifdef USE_CG_DRAWING
> +   img->data.ptr_val = image; /* retain original data */
> + #endif

Should be #if instead of #ifdef.

>      CGContextDrawImage (context, rectangle, image);
>      QDEndCGContext (ximg, &context);
> -   CGImageRelease (image);

Need to release it if !USE_CG_DRAWING.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

On 4 Apr 2008, at 00:12, YAMAMOTO Mitsuharu wrote:
> `:mask nil' means removing the mask.
> Should be #if instead of #ifdef.
> Need to release it if !USE_CG_DRAWING.


All good points. Thanks.
Also, image-mask-p needed to be changed to recognize the alpha channel.



2008-04-04  David Reitter <david.reitter@gmail.com>

	* tool-bar.el (tool-bar-set-file-extension): add function
	(tool-bar-get-image-spec): load PNG if available and load image
	files named foo_sel.bar and foo_dis.bar to define selected and
  	disabled state images.
	(tool-bar-local-item-from-menu): take images and masks from icon,
	do not create extra mask.


2008-04-04  David Reitter <david.reitter@gmail.com>

	* image.c (image_load_quartz2d) [USE_CG_DRAWING]: retain
	platform image data
	(postprocess_image) [USE_CG_DRAWING]: release data when
	making modifications to the image
	(Fimage_mask_p) [USE_CG_DRAWING]: check for mask in data




Index: lisp/tool-bar.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/tool-bar.el,v
retrieving revision 1.7.2.2
diff -c -r1.7.2.2 tool-bar.el
*** lisp/tool-bar.el	7 Jan 2008 02:44:11 -0000	1.7.2.2
--- lisp/tool-bar.el	4 Apr 2008 11:22:19 -0000
***************
*** 90,95 ****
--- 90,166 ----
   		'(menu-item "tool bar" ignore
   			    :filter (lambda (ignore) tool-bar-map)))

+ (defun tool-bar-set-file-extension (image-spec-list extension)
+   "Set new file extensions for all :file properties
+ Replace any extensions of :file properties in elements of
+ IMAGE-SPEC-LIST. An extension may start with a period . or an
+ underscore. EXTENSION and the original file name extension (starting
+ with a period) are added to the file name.
+
+ E.g. foo_dis.xpm becomes foo_sel.xpm if EXTENSION is '_sel'."
+   (mapcar
+    (lambda (spec)
+      (let ((f (plist-get spec :file))
+ 	    )
+         (if (null f)
+ 	    spec
+ 	  ;; need to replace previous extensions, including those
+ 	  ;; starting with _ -
+ 	  (plist-put spec :file (concat (replace-regexp-in-string "[\.\_].*$"
+ 								  "" f)
+ 					extension
+ 					(file-name-extension f t)))
+ 	  )))
+    image-spec-list))
+
+ (defun tool-bar-get-image-spec (icon)
+   (let* ((fg (face-attribute 'tool-bar :foreground))
+ 	 (bg (face-attribute 'tool-bar :background))
+ 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
+ 			(if (eq bg 'unspecified) nil (list :background bg))))
+ 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
+ 	 (xpm-lo-spec (if (> (display-color-cells) 256)
+ 			  nil
+ 			(list :type 'xpm :file
+                               (concat "low-color/" icon ".xpm"))))
+ 	 (png-spec (if (image-type-available-p 'png)
+ 		       (list :type 'png :file (concat icon ".png")
+ 			     :background "grey")))
+ 	 (pbm-spec (append (list :type 'pbm :file
+                                  (concat icon ".pbm")) colors))
+ 	 (xbm-spec (append (list :type 'xbm :file
+                                  (concat icon ".xbm")) colors))
+ 	 (image (find-image
+ 		(if (display-color-p)
+ 		    (list png-spec xpm-lo-spec xpm-spec pbm-spec xbm-spec)
+ 		  (list pbm-spec xbm-spec xpm-lo-spec xpm-spec))))
+ 	 (image-sel (find-image
+ 		     (if (display-color-p)
+ 			 (tool-bar-set-file-extension
+ 			  (list png-spec xpm-lo-spec xpm-spec pbm-spec xbm-spec)
+ 			  "_sel")
+ 		       nil)))
+ 	 (image-dis (find-image
+ 		     (if (display-color-p)
+ 			 (tool-bar-set-file-extension
+ 			  (list png-spec xpm-lo-spec xpm-spec pbm-spec xbm-spec)
+ 			  "_dis")
+ 		       nil)))
+ 	 (images (when image ;; image may be nil if not found.
+ 		    (unless (image-mask-p image)
+  		     (setq image (append image '(:mask heuristic))))
+ 		   (if (and image-sel image-dis)
+ 		       (progn		
+ 			 (unless (image-mask-p image-sel)
+ 			   (setq image-sel (append image-sel
+ 						   '(:mask heuristic))))
+ 			 (unless (image-mask-p image-dis)
+ 			   (setq image-dis (append image-dis
+ 						   '(:mask heuristic))))
+ 			 (vector image-sel image image-dis image-dis))
+ 		     image))))
+     (cons image images)))
+
   ;;;###autoload
   (defun tool-bar-add-item (icon def key &rest props)
     "Add an item to the tool bar.
***************
*** 119,147 ****
   function will first try to use low-color/ICON.xpm if display-color- 
cells
   is less or equal to 256, then ICON.xpm, then ICON.pbm, and finally
   ICON.xbm, using `find-image'."
!   (let* ((fg (face-attribute 'tool-bar :foreground))
! 	 (bg (face-attribute 'tool-bar :background))
! 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
! 			(if (eq bg 'unspecified) nil (list :background bg))))
! 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
! 	 (xpm-lo-spec (if (> (display-color-cells) 256)
! 			  nil
! 			(list :type 'xpm :file
!                               (concat "low-color/" icon ".xpm"))))
! 	 (pbm-spec (append (list :type 'pbm :file
!                                  (concat icon ".pbm")) colors))
! 	 (xbm-spec (append (list :type 'xbm :file
!                                  (concat icon ".xbm")) colors))
! 	 (image (find-image
! 		(if (display-color-p)
! 		    (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
! 		  (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))))
!
       (when (and (display-images-p) image)
-       (unless (image-mask-p image)
- 	(setq image (append image '(:mask heuristic))))
         (define-key-after map (vector key)
! 	`(menu-item ,(symbol-name key) ,def :image ,image ,@props)))))

   ;;;###autoload
   (defun tool-bar-add-item-from-menu (command icon &optional map  
&rest props)
--- 190,201 ----
   function will first try to use low-color/ICON.xpm if display-color- 
cells
   is less or equal to 256, then ICON.xpm, then ICON.pbm, and finally
   ICON.xbm, using `find-image'."
!   (let* ((is (tool-bar-get-image-spec icon))
! 	 (image (car is))
! 	 (images (cdr is)))
       (when (and (display-images-p) image)
         (define-key-after map (vector key)
! 	`(menu-item ,(symbol-name key) ,def :image ,images ,@props)))))

   ;;;###autoload
   (defun tool-bar-add-item-from-menu (command icon &optional map  
&rest props)
***************
*** 174,196 ****
       (setq from-map global-map))
     (let* ((menu-bar-map (lookup-key from-map [menu-bar]))
   	 (keys (where-is-internal command menu-bar-map))
! 	 (fg (face-attribute 'tool-bar :foreground))
! 	 (bg (face-attribute 'tool-bar :background))
! 	 (colors (nconc (if (eq fg 'unspecified) nil (list :foreground fg))
! 			(if (eq bg 'unspecified) nil (list :background bg))))
! 	 (xpm-spec (list :type 'xpm :file (concat icon ".xpm")))
! 	 (xpm-lo-spec (if (> (display-color-cells) 256)
! 			  nil
! 			(list :type 'xpm :file
!                               (concat "low-color/" icon ".xpm"))))
! 	 (pbm-spec (append (list :type 'pbm :file
!                                  (concat icon ".pbm")) colors))
! 	 (xbm-spec (append (list :type 'xbm :file
!                                  (concat icon ".xbm")) colors))
! 	 (spec (if (display-color-p)
! 		   (list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
! 		 (list pbm-spec xbm-spec xpm-lo-spec xpm-spec)))
! 	 (image (find-image spec))
   	 submap key)
       (when (and (display-images-p) image)
         ;; We'll pick up the last valid entry in the list of keys if
--- 228,236 ----
       (setq from-map global-map))
     (let* ((menu-bar-map (lookup-key from-map [menu-bar]))
   	 (keys (where-is-internal command menu-bar-map))
! 	 (is (tool-bar-get-image-spec icon))
! 	 (image (car is))
! 	 (images (cdr is))
   	 submap key)
       (when (and (display-images-p) image)
         ;; We'll pick up the last valid entry in the list of keys if
***************
*** 210,221 ****
   			key kk)))))
         (when (and (symbolp submap) (boundp submap))
   	(setq submap (eval submap)))
-       (unless (image-mask-p image)
- 	(setq image (append image '(:mask heuristic))))
         (let ((defn (assq key (cdr submap))))
   	(if (eq (cadr defn) 'menu-item)
   	    (define-key-after in-map (vector key)
! 	      (append (cdr defn) (list :image image) props))
   	  (setq defn (cdr defn))
   	  (define-key-after in-map (vector key)
   	    (let ((rest (cdr defn)))
--- 250,259 ----
   			key kk)))))
         (when (and (symbolp submap) (boundp submap))
   	(setq submap (eval submap)))
         (let ((defn (assq key (cdr submap))))
   	(if (eq (cadr defn) 'menu-item)
   	    (define-key-after in-map (vector key)
! 	      (append (cdr defn) (list :image images) props))
   	  (setq defn (cdr defn))
   	  (define-key-after in-map (vector key)
   	    (let ((rest (cdr defn)))
Index: src/image.c
===================================================================
RCS file: /sources/emacs/emacs/src/image.c,v
retrieving revision 1.65.2.13
diff -c -r1.65.2.13 image.c
*** src/image.c	28 Mar 2008 14:57:32 -0000	1.65.2.13
--- src/image.c	4 Apr 2008 11:14:01 -0000
***************
*** 1135,1140 ****
--- 1135,1146 ----
         struct image *img = IMAGE_FROM_ID (f, id);
         if (img->mask)
   	mask = Qt;
+ #if USE_CG_DRAWING
+       /* Mask may be in an Alpha channel in the image data */
+       if (img->data.ptr_val != NULL &&
+ 	  CGImageGetAlphaInfo(img->data.ptr_val) != kCGImageAlphaNone)
+ 	mask = Qt;
+ #endif
       }
     else
       error ("Invalid image specification");
***************
*** 1549,1555 ****
       }

   #if defined (MAC_OS) && USE_CG_DRAWING
!   if (img->data.ptr_val)
       {
         CGImageRelease (img->data.ptr_val);
         img->data.ptr_val = NULL;
--- 1555,1561 ----
       }

   #if defined (MAC_OS) && USE_CG_DRAWING
!   if (img->data.ptr_val != NULL)
       {
         CGImageRelease (img->data.ptr_val);
         img->data.ptr_val = NULL;
***************
*** 1850,1861 ****

         mask = image_spec_value (spec, QCheuristic_mask, NULL);
         if (!NILP (mask))
! 	x_build_heuristic_mask (f, img, mask);
         else
   	{
   	  int found_p;

   	  mask = image_spec_value (spec, QCmask, &found_p);

   	  if (EQ (mask, Qheuristic))
   	    x_build_heuristic_mask (f, img, Qt);
--- 1856,1883 ----

         mask = image_spec_value (spec, QCheuristic_mask, NULL);
         if (!NILP (mask))
! 	{
! #if defined (MAC_OS) && USE_CG_DRAWING
! 	  if (img->data.ptr_val != NULL)
! 	    {
! 	      CGImageRelease (img->data.ptr_val);
! 	      img->data.ptr_val = NULL;
! 	    }
! #endif
! 	  x_build_heuristic_mask (f, img, mask);
! 	}
         else
   	{
   	  int found_p;

   	  mask = image_spec_value (spec, QCmask, &found_p);
+ #if defined (MAC_OS) && USE_CG_DRAWING
+ 	  if (found_p && img->data.ptr_val != NULL)
+ 	    {
+ 	      CGImageRelease (img->data.ptr_val);
+ 	      img->data.ptr_val = NULL;
+ 	    }
+ #endif

   	  if (EQ (mask, Qheuristic))
   	    x_build_heuristic_mask (f, img, Qt);
***************
*** 2767,2773 ****
--- 2789,2800 ----
       }
     CGContextDrawImage (context, rectangle, image);
     QDEndCGContext (ximg, &context);
+
+ #if USE_CG_DRAWING
+   img->data.ptr_val = image; /* retain original data */
+ #else
     CGImageRelease (image);
+ #endif

     /* Maybe fill in the background field while we have ximg handy. */
     if (NILP (image_spec_value (img->spec, QCbackground, NULL)))

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Fri, 4 Apr 2008 12:24:58 +0100, David Reitter <david.reitter@gmail.com> said:

> Also, image-mask-p needed to be changed to recognize the alpha
> channel.

And the invalidation only takes care of the `mask' spec, but not for
`conversion'.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

On 4 Apr 2008, at 13:20, YAMAMOTO Mitsuharu wrote:
>
> And the invalidation only takes care of the `mask' spec, but not for
> `conversion'.

Index: image.c
===================================================================
RCS file: /sources/emacs/emacs/src/image.c,v
retrieving revision 1.65.2.13
diff -c -r1.65.2.13 image.c
*** image.c	28 Mar 2008 14:57:32 -0000	1.65.2.13
--- image.c	4 Apr 2008 15:05:11 -0000
***************
*** 1135,1140 ****
--- 1135,1146 ----
        struct image *img = IMAGE_FROM_ID (f, id);
        if (img->mask)
  	mask = Qt;
+ #if USE_CG_DRAWING
+       /* Mask may be in an Alpha channel in the image data */
+       if (img->data.ptr_val != NULL &&
+ 	  CGImageGetAlphaInfo(img->data.ptr_val) != kCGImageAlphaNone)
+ 	mask = Qt;
+ #endif
      }
    else
      error ("Invalid image specification");
***************
*** 1549,1555 ****
      }

  #if defined (MAC_OS) && USE_CG_DRAWING
!   if (img->data.ptr_val)
      {
        CGImageRelease (img->data.ptr_val);
        img->data.ptr_val = NULL;
--- 1555,1561 ----
      }

  #if defined (MAC_OS) && USE_CG_DRAWING
!   if (img->data.ptr_val != NULL)
      {
        CGImageRelease (img->data.ptr_val);
        img->data.ptr_val = NULL;
***************
*** 1835,1840 ****
--- 1841,1848 ----
      {
        Lisp_Object conversion, spec;
        Lisp_Object mask;
+       int release = 0;
+       int found_p = 0;

        spec = img->spec;

***************
*** 1850,1862 ****

        mask = image_spec_value (spec, QCheuristic_mask, NULL);
        if (!NILP (mask))
! 	x_build_heuristic_mask (f, img, mask);
        else
  	{
- 	  int found_p;
-
  	  mask = image_spec_value (spec, QCmask, &found_p);
!
  	  if (EQ (mask, Qheuristic))
  	    x_build_heuristic_mask (f, img, Qt);
  	  else if (CONSP (mask)
--- 1858,1872 ----

        mask = image_spec_value (spec, QCheuristic_mask, NULL);
        if (!NILP (mask))
! 	{
! 	  release = 1;
! 	  x_build_heuristic_mask (f, img, mask);
! 	}
        else
  	{
  	  mask = image_spec_value (spec, QCmask, &found_p);
! 	  if (found_p)
! 	    release = 1;
  	  if (EQ (mask, Qheuristic))
  	    x_build_heuristic_mask (f, img, Qt);
  	  else if (CONSP (mask)
***************
*** 1876,1882 ****


        /* Should we apply an image transformation algorithm?  */
!       conversion = image_spec_value (spec, QCconversion, NULL);
        if (EQ (conversion, Qdisabled))
  	x_disable_image (f, img);
        else if (EQ (conversion, Qlaplace))
--- 1886,1895 ----


        /* Should we apply an image transformation algorithm?  */
!       found_p = 0;
!       conversion = image_spec_value (spec, QCconversion, &found_p);
!       if (found_p)
! 	release = 1;
        if (EQ (conversion, Qdisabled))
  	x_disable_image (f, img);
        else if (EQ (conversion, Qlaplace))
***************
*** 1893,1898 ****
--- 1906,1918 ----
  			      Fplist_get (tem, QCmatrix),
  			      Fplist_get (tem, QCcolor_adjustment));
  	}
+ #if defined (MAC_OS) && USE_CG_DRAWING
+       if (release && img->data.ptr_val != NULL)
+ 	{
+ 	  CGImageRelease (img->data.ptr_val);
+ 	  img->data.ptr_val = NULL;
+ 	}
+ #endif
      }
  }

***************
*** 2767,2773 ****
--- 2787,2798 ----
      }
    CGContextDrawImage (context, rectangle, image);
    QDEndCGContext (ximg, &context);
+
+ #if USE_CG_DRAWING
+   img->data.ptr_val = image; /* retain original data */
+ #else
    CGImageRelease (image);
+ #endif

    /* Maybe fill in the background field while we have ximg handy. */
    if (NILP (image_spec_value (img->spec, QCbackground, NULL)))

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Sat, 5 Apr 2008 11:58:15 +0100, David Reitter <david.reitter@gmail.com> said:

> On 4 Apr 2008, at 13:20, YAMAMOTO Mitsuharu wrote:
>> 
>> And the invalidation only takes care of the `mask' spec, but not for
>> `conversion'.

> Index: image.c
> ===================================================================
> RCS file: /sources/emacs/emacs/src/image.c,v
> retrieving revision 1.65.2.13
> diff -c -r1.65.2.13 image.c
> *** image.c	28 Mar 2008 14:57:32 -0000	1.65.2.13
> --- image.c	4 Apr 2008 15:05:11 -0000
> ***************
> *** 1135,1140 ****
> --- 1135,1146 ----
>         struct image *img = IMAGE_FROM_ID (f, id);
>         if (img->mask)
>   	mask = Qt;
> + #if USE_CG_DRAWING
> +       /* Mask may be in an Alpha channel in the image data */
> +       if (img->data.ptr_val != NULL &&
> + 	  CGImageGetAlphaInfo(img->data.ptr_val) != kCGImageAlphaNone)
> + 	mask = Qt;
> + #endif
>       }
>     else
>       error ("Invalid image specification");

This is not the only place where NULL-check of img->mask is used for
determining the existence of the image mask.  At least, xdisp.c and
macterm.c also assume that.  And add BLOCK_INPUT around
CGImageGetAlphaInfo just in case.


> ***************
> *** 1835,1840 ****
> --- 1841,1848 ----
>       {
>         Lisp_Object conversion, spec;
>         Lisp_Object mask;
> +       int release = 0;
> +       int found_p = 0;

>         spec = img->spec;

> ***************
> *** 1850,1862 ****

>         mask = image_spec_value (spec, QCheuristic_mask, NULL);
>         if (!NILP (mask))
> ! 	x_build_heuristic_mask (f, img, mask);
>         else
>   	{
> - 	  int found_p;
> -
>   	  mask = image_spec_value (spec, QCmask, &found_p);
> !
>   	  if (EQ (mask, Qheuristic))
>   	    x_build_heuristic_mask (f, img, Qt);
>   	  else if (CONSP (mask)
> --- 1858,1872 ----

>         mask = image_spec_value (spec, QCheuristic_mask, NULL);
>         if (!NILP (mask))
> ! 	{
> ! 	  release = 1;
> ! 	  x_build_heuristic_mask (f, img, mask);
> ! 	}
>         else
>   	{
>   	  mask = image_spec_value (spec, QCmask, &found_p);
> ! 	  if (found_p)
> ! 	    release = 1;
>   	  if (EQ (mask, Qheuristic))
>   	    x_build_heuristic_mask (f, img, Qt);
>   	  else if (CONSP (mask)
> ***************
> *** 1876,1882 ****


>         /* Should we apply an image transformation algorithm?  */
> !       conversion = image_spec_value (spec, QCconversion, NULL);
>         if (EQ (conversion, Qdisabled))
>   	x_disable_image (f, img);
>         else if (EQ (conversion, Qlaplace))
> --- 1886,1895 ----


>         /* Should we apply an image transformation algorithm?  */
> !       found_p = 0;
> !       conversion = image_spec_value (spec, QCconversion, &found_p);
> !       if (found_p)
> ! 	release = 1;
>         if (EQ (conversion, Qdisabled))
>   	x_disable_image (f, img);
>         else if (EQ (conversion, Qlaplace))
> ***************
> *** 1893,1898 ****
> --- 1906,1918 ----
>   			      Fplist_get (tem, QCmatrix),
>   			      Fplist_get (tem, QCcolor_adjustment));
>   	}
> + #if defined (MAC_OS) && USE_CG_DRAWING
> +       if (release && img->data.ptr_val != NULL)
> + 	{
> + 	  CGImageRelease (img->data.ptr_val);
> + 	  img->data.ptr_val = NULL;
> + 	}
> + #endif
>       }
>   }

I would add an assignment to some variable around each call to
x_build_heuristic_mask etc. so as not to be confused with bogus
mask/conversion specs.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: png images in tool-bar / alpha mask

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

On 6 Apr 2008, at 03:52, YAMAMOTO Mitsuharu wrote:

> This is not the only place where NULL-check of img->mask is used for
> determining the existence of the image mask.  At least, xdisp.c and
> macterm.c also assume that.

Yes, but doesn't that happen in order to check whether there is a  
bitmap mask that will be used for something?

x_draw_image_foreground actually uses that mask to draw the image, and  
my understanding is that we want to use the original (platform  
specific) image data for that, including its alpha channel.  As we  
can't do that (yet) on X or W32, we don't set the platform specific  
image data in the first place there.
In macterm.c, we want to call mac_copy_area rather than  
mac_copy_area_with_mask, right?

In x_draw_image_foreground (macterm.c), it is always safe to draw a  
rectangle around given that we don't know what the alpha channel  
specifies, but one could go through Fimage_mask_pp to get the same  
semantics.

As for image.c: I don't understand all of the code.  In general, it  
appears safe to assume we don't have a mask on the C level.

The alternative would be to acquire a bitmap mask from the PNG when  
loading it. Maybe that would be cleaner. 
   

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

Re: png images in tool-bar / alpha mask

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 6 Apr 2008 14:42:08 +0100, David Reitter <david.reitter@gmail.com> said:

>> This is not the only place where NULL-check of img->mask is used
>> for determining the existence of the image mask.  At least, xdisp.c
>> and macterm.c also assume that.

> Yes, but doesn't that happen in order to check whether there is a
> bitmap mask that will be used for something?

Did you seriously test your own patch?  You'll see the previous
contents unerased under the alpha channel part because filling with
the background color is done only when the image is considered to have
a mask.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp