bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[YAMAMOTO Mitsuharu]

Steps to Reproduce:

1. Build Emacs with the ImageMagick support.

2. $ emacs -Q &

3. M-x eww RET wikipedia RET

Result:

Probably you'll observe animated favicons in the search results by
DuckDuckGo.  I think these animations are unexpected and unwanted.
See also bug#18333.

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

In GNU Emacs 24.3.93.1 (x86_64-apple-darwin10.8.0, GTK+ Version 3.12.2)
 of 2014-08-27 on yamamoto-mitsuharu-no-iMac.local
Windowing system distributor `The X.Org Foundation', version 11.0.10402000
Configured using:
 `configure CPPFLAGS=-I/opt/local/include LDFLAGS=-L/opt/local/lib'

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 27 Aug 2014 09:57:29 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> Steps to Reproduce:
> 1. Build Emacs with the ImageMagick support.

> 2. $ emacs -Q &

> 3. M-x eww RET wikipedia RET

> Result:

> Probably you'll observe animated favicons in the search results by
> DuckDuckGo.  I think these animations are unexpected and unwanted.
> See also bug#18333.

Currently, all the image data containing multiple image frames seems
to be treated as animations.

In shr-put-image (lisp/net/shr.el), we have

	  (when (and shr-image-animate
                     (cond ((fboundp 'image-multi-frame-p)
		       ;; Only animate multi-frame things that specify a
		       ;; delay; eg animated gifs as opposed to
		       ;; multi-page tiffs.  FIXME?
                            (cdr (image-multi-frame-p image)))
                           ((fboundp 'image-animated-p)
                            (image-animated-p image))))
            (image-animate image nil 60)))

And in lisp/image.el,

(defun image-multi-frame-p (image)
  "Return non-nil if IMAGE contains more than one frame.
The actual return value is a cons (NIMAGES . DELAY), where NIMAGES is
the number of frames (or sub-images) in the image and DELAY is the delay
in seconds that the image specifies between each frame.  DELAY may be nil,
in which case you might want to use `image-default-frame-delay'."
  (when (fboundp 'image-metadata)
    (let* ((metadata (image-metadata image))
	   (images (plist-get metadata 'count))
	   (delay (plist-get metadata 'delay)))
      (when (and images (> images 1))
	(if (or (not (numberp delay)) (< delay 0))
	    (setq delay image-default-frame-delay))
	(cons images delay)))))

Which should be fixed?  Should image-multi-frame-p, a new function in
24.4, stop using image-default-frame-delay as a default value for
DELAY?  Or should each caller of image-multi-frame-p temporarily
let-bind image-default-frame-delay to nil if it doesn't want to treat
images that are missing DELAY value as animations?

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

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Glenn Morris]

YAMAMOTO Mitsuharu wrote:

> Should image-multi-frame-p, a new function in 24.4, stop using
> image-default-frame-delay as a default value for DELAY? Or should each
> caller of image-multi-frame-p temporarily let-bind
> image-default-frame-delay to nil if it doesn't want to treat images
> that are missing DELAY value as animations?

I think I was intending it to work the first way, not the second way
(which sounds a bit yucky?). But I don't seem to have implemented it
right. No idea what I was thinking...

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Glenn Morris]

>> Should image-multi-frame-p, a new function in 24.4, stop using
>> image-default-frame-delay as a default value for DELAY? Or should each
>> caller of image-multi-frame-p temporarily let-bind
>> image-default-frame-delay to nil if it doesn't want to treat images
>> that are missing DELAY value as animations?
>
> I think I was intending it to work the first way, not the second way
> (which sounds a bit yucky?). But I don't seem to have implemented it
> right. No idea what I was thinking...

I think perhaps I meant to do something like the following, which I
applied to emacs-24.

However, AFAICS now nothing is animated in eww.
Perhaps this is related to shr's preference for ImageMagick and the fact
that that backend still does not return a delay
(http://debbugs.gnu.org/10747).

In theory ImageMagick should be used only when rescaling is needed, but
it seems to be used all (?) the time (because in shr-put-image, size is
often nil?).

--- a/lisp/image.el	2014-02-25 21:59:14 +0000
+++ b/lisp/image.el	2014-09-14 23:59:57 +0000
@@ -637,8 +637,8 @@
 	   (images (plist-get metadata 'count))
 	   (delay (plist-get metadata 'delay)))
       (when (and images (> images 1))
-	(if (or (not (numberp delay)) (< delay 0))
-	    (setq delay image-default-frame-delay))
+	(and delay (or (not (numberp delay)) (< delay 0))
+	     (setq delay image-default-frame-delay))
 	(cons images delay)))))
 

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Juri Linkov]

> I think perhaps I meant to do something like the following, which I
> applied to emacs-24.
>
> However, AFAICS now nothing is animated in eww.
> Perhaps this is related to shr's preference for ImageMagick and the fact
> that that backend still does not return a delay
> (http://debbugs.gnu.org/10747).

I guess we need to install the patch from bug#10747 to emacs-24 as well,
so an animated image could be detected in imagemagick by its delay,
since non-zero delays is one way to detect an animated format according to
http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=11988

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Lars Magne Ingebrigtsen]

Juri Linkov <juri@jurta.org> writes:

>> I think perhaps I meant to do something like the following, which I
>> applied to emacs-24.
>>
>> However, AFAICS now nothing is animated in eww.
>> Perhaps this is related to shr's preference for ImageMagick and the fact
>> that that backend still does not return a delay
>> (http://debbugs.gnu.org/10747).
>
> I guess we need to install the patch from bug#10747 to emacs-24 as well,
> so an animated image could be detected in imagemagick by its delay,
> since non-zero delays is one way to detect an animated format according to
> http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=11988

I think that sounds like the right solution.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

>> I guess we need to install the patch from bug#10747 to emacs-24 as well,
>> so an animated image could be detected in imagemagick by its delay,
>> since non-zero delays is one way to detect an animated format according to
>> http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=11988
>
> I think that sounds like the right solution.

Especially since no images I see are animated any more, and 9gag gets a
lot more boring to read.  Please install.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Juri Linkov]

>>> I guess we need to install the patch from bug#10747 to emacs-24 as well,
>>> so an animated image could be detected in imagemagick by its delay,
>>> since non-zero delays is one way to detect an animated format according to
>>> http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=11988
>>
>> I think that sounds like the right solution.
>
> Especially since no images I see are animated any more, and 9gag gets a
> lot more boring to read.  Please install.

The patch from bug#10747 is installed now to emacs-24,
so I'm closing bug#18334.

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Glenn Morris]

Lars Magne Ingebrigtsen wrote:

> Especially since no images I see are animated any more

(I did say that would happen...)

You seem to end up using ImageMagick to render all images, even when no
resizing is needed. Did you intend that?

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Lars Magne Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Lars Magne Ingebrigtsen wrote:
>
>> Especially since no images I see are animated any more
>
> (I did say that would happen...)

Could you do a merge with trunk?  >"?

> You seem to end up using ImageMagick to render all images, even when no
> resizing is needed. Did you intend that?

Uhm, let's see if I can remember...

First of all, `image-size' used to transmit the entire picture via X, so
I wanted to avoid doing that.  And then I implemented `max-width/height'
both to avoid doing the size computation everywhere, but also to help
with the `image-size' thing.

Then `image-size' was fixed so that it could be used again, but I was
thinking that first decoding a picture with libjpeg (for instance) to
determine the size, and then with ImageMagick to display it, would
probably be slower (due to caching at various levels) and more
error-prone (all libraries have bugs), so I just left it with
ImageMagick.

And now I want to implement resizing in all the decoders, so that we
don't have to use ImageMagick much at all.

See?  Makes sense.  >"?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Glenn Morris]

Lars Magne Ingebrigtsen wrote:

> Could you do a merge with trunk?  >"?

Not till the end of the month, no. But anyone else should feel free.

bug#18334: 24.3.93; ImageMagick: eww shows favicon animations in search results

[Stefan Monnier]

> Could you do a merge with trunk?  >"?

Not sure what you mean by that.  But I just sync'd the trunk with the
emacs-24 branch.  In case you wonder how to do that.
Well, you can do "bzr merge .../emacs-24" of course, but in order to
handle a few details and avoid some spurious conflicts, you can instead
use
- M-x load RET .../trunk/admin/bzrmerge.el RET
- M-x cd RET .../trunk RET
- Mx- bzrmerge RET .../emacs-24 RET
which will guess at some commits that don't need to be merged (e.g. because
its commit message says that it's a backport) and ask you for
confirmation that these should indeed be skipped.


        Stefan