From ef28576673d3080df2807842f8415cabe09db095 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Sun, 5 Sep 2021 05:43:59 -0500
Subject: [PATCH] Fix #3400: Smarter thumbnail generation for videos

---
 app/logical/ffmpeg.rb              |  25 +++++++++++++++++++++++++
 app/logical/media_file/image.rb    |  12 ++++++++----
 app/logical/media_file/ugoira.rb   |   7 +++----
 app/logical/media_file/video.rb    |   4 +---
 config/initializers/inflections.rb |   1 +
 test/files/valid_ugoira.zip        | Bin 6663 -> 0 bytes
 test/unit/media_file_test.rb       |  16 ++++++++++++----
 test/unit/upload_service_test.rb   |   2 --
 8 files changed, 50 insertions(+), 17 deletions(-)
 create mode 100644 app/logical/ffmpeg.rb
 delete mode 100644 test/files/valid_ugoira.zip

diff --git a/app/logical/ffmpeg.rb b/app/logical/ffmpeg.rb
new file mode 100644
index 000000000..24f30045c
--- /dev/null
+++ b/app/logical/ffmpeg.rb
@@ -0,0 +1,25 @@
+class FFmpeg
+  attr_reader :file
+
+  # Operate on a file with FFmpeg.
+  # @param file [File, String] a webm, mp4, gif, or apng file
+  def initialize(file)
+    @file = file.is_a?(String) ? File.open(file) : file
+  end
+
+  # Generate a .jpg preview image for a video or animation. Generates
+  # thumbnails intelligently by avoiding blank frames.
+  #
+  # @return [MediaFile] the preview image
+  def smart_video_preview
+    vp = Tempfile.new(["video-preview", ".jpg"], binmode: true)
+
+    # https://ffmpeg.org/ffmpeg.html#Main-options
+    # https://ffmpeg.org/ffmpeg-filters.html#thumbnail
+    ffmpeg_out, status = Open3.capture2e("ffmpeg -i #{file.path} -vf thumbnail=300 -frames:v 1 -y #{vp.path}")
+    raise "ffmpeg failed: #{ffmpeg_out}" if !status.success?
+    Rails.logger.debug(ffmpeg_out)
+
+    MediaFile.open(vp)
+  end
+end
diff --git a/app/logical/media_file/image.rb b/app/logical/media_file/image.rb
index e65508602..dab6a5dab 100644
--- a/app/logical/media_file/image.rb
+++ b/app/logical/media_file/image.rb
@@ -38,11 +38,15 @@ class MediaFile::Image < MediaFile
   # @see https://github.com/jcupitt/libvips/wiki/HOWTO----Image-shrinking
   # @see http://jcupitt.github.io/libvips/API/current/Using-vipsthumbnail.md.html
   def preview(width, height)
-    output_file = Tempfile.new(["image-preview", ".jpg"])
-    resized_image = image.thumbnail_image(width, height: height, **THUMBNAIL_OPTIONS)
-    resized_image.jpegsave(output_file.path, **JPEG_OPTIONS)
+    if is_animated?
+      FFmpeg.new(file).smart_video_preview
+    else
+      output_file = Tempfile.new(["image-preview", ".jpg"])
+      resized_image = image.thumbnail_image(width, height: height, **THUMBNAIL_OPTIONS)
+      resized_image.jpegsave(output_file.path, **JPEG_OPTIONS)
 
-    MediaFile::Image.new(output_file)
+      MediaFile::Image.new(output_file)
+    end
   end
 
   def crop(width, height)
diff --git a/app/logical/media_file/ugoira.rb b/app/logical/media_file/ugoira.rb
index 19270f754..aefc19bf5 100644
--- a/app/logical/media_file/ugoira.rb
+++ b/app/logical/media_file/ugoira.rb
@@ -36,6 +36,7 @@ class MediaFile::Ugoira < MediaFile
   # XXX should take width and height and resize image
   def convert
     raise NotImplementedError, "can't convert ugoira to webm: ffmpeg or mkvmerge not installed" unless self.class.videos_enabled?
+    raise RuntimeError, "can't convert ugoira to webm: no ugoira frame data was provided" unless frame_data.present?
 
     Dir.mktmpdir("ugoira-#{md5}") do |tmpdir|
       output_file = Tempfile.new(["ugoira-conversion", ".webm"], binmode: true)
@@ -87,10 +88,8 @@ class MediaFile::Ugoira < MediaFile
   end
 
   def preview_frame
-    tempfile = Tempfile.new("ugoira-preview", binmode: true)
-    zipfile.entries.first.extract(tempfile.path) { true } #  'true' means overwrite the existing tempfile.
-    MediaFile.open(tempfile)
+    FFmpeg.new(convert).smart_video_preview
   end
 
-  memoize :zipfile, :preview_frame, :dimensions
+  memoize :zipfile, :preview_frame, :dimensions, :convert
 end
diff --git a/app/logical/media_file/video.rb b/app/logical/media_file/video.rb
index d540483d5..13afbd7fb 100644
--- a/app/logical/media_file/video.rb
+++ b/app/logical/media_file/video.rb
@@ -32,9 +32,7 @@ class MediaFile::Video < MediaFile
   end
 
   def preview_frame
-    vp = Tempfile.new(["video-preview", ".jpg"], binmode: true)
-    video.screenshot(vp.path, seek_time: 0)
-    MediaFile.open(vp.path)
+    FFmpeg.new(file).smart_video_preview
   end
 
   memoize :video, :preview_frame, :dimensions, :duration, :has_audio?
diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb
index 09961dda5..bb36bfc5f 100644
--- a/config/initializers/inflections.rb
+++ b/config/initializers/inflections.rb
@@ -5,6 +5,7 @@
 # locales as you wish. All of these examples are active by default:
 ActiveSupport::Inflector.inflections(:en) do |inflect|
   inflect.uncountable "general"
+  inflect.acronym "FFmpeg"
 #   inflect.plural /^(ox)$/i, '\1en'
 #   inflect.singular /^(ox)en/i, '\1'
 #   inflect.irregular 'person', 'people'
diff --git a/test/files/valid_ugoira.zip b/test/files/valid_ugoira.zip
deleted file mode 100644
index 463bed1c80444b39af01e33d6bf82878495ae99d..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 6663
zcmWIWW@Zs#0D;H5U0qYWJu1`L7#Kj9i-Cc`01Wi93ex}I`2T=Gz{}0kje(Jok->w(
zgMs1yZ3brsW+o;OVrFJ$VP<Aw<zQuHVPWNBXJ_N!;o{-p=Hlk&<r5O%<rC!N<`xhW
z5EK>>6&2;-7ncwdkq{CQ6#*H-$jrjR%EHRY%E~Fi%grl7GWdUhL6C#NhQWrJQILU2
zkdaxC@&6G9X$D3HCT2!Pknf<Fjh%&+gPHUH5e6Lr21XV}7A8g}R#u246C*PVD;op5
zpb&?!h^V5NlCq(ZimI{6#Q#SaBpDeQP)%ZBVrF3j*`h4S$iT$J$ifIUhJk^Bk%?K5
zg;mio(2-3@#HjH9Ee0NDMg}H9W<dsfhF{AUWJs|!!ilE+r^@w}j=xWJXx#F%%~y2Z
zUfryxms!exim1Mrq$5>owQ9xswBzrzk82)U5&X~oJ?}c#ozlAkww>3glDYGA#ieIk
z6Fpv=`fU#kmwfg_BVy7@Ghac8O?|7BzW!%Wn4KiP&T(hSjBUB@J-6&XEWP72*~n!1
z8R`CCL5Z$zQ#vNruCq>-krr`%vufoj{SbwzFThSyv|iYn68SeM=xUmg!Ku!kJxAx9
z>R9=;R7k`jqNwAF(j@&9WwlsUr$1d67g;pQ-_Y$|eM8OItEOt#zWHJscBPfN)>cjW
zCBd~r&{Ou!oYhO-g!goYT-m))P*bo#WZl6Z;hEd^xx3!g{`Gc$>d|8{jUNndC9Y`?
zH#ud0OQk+&Vu7+LS91SvSxJvuCKt>PvL}E8?C|Vdwp~6PpPspQD^;JClJ>V!2#R$(
zdo_1U=#!JunTKD^Ss!+?J~8w{m}U4B3qIqVkdM#KpUex3-rDPOTw8wL%^PNXk_Rju
z<Rv?mbY@ED@42;&^G)&M`uQR0XZ`H7(@tBhO-ieuc(10sI%2kuOXakE>J$8uL~i`?
z`L%yrdbw7V+p5D!UYinT<lL<kkPSQPd+Z_mmR1*;%E(IPEe}hxWA3~Dn%sUpcha1S
zt!cB~=DM8wo0We2{MzH&=KRTi&@sDat(VWHe$}skm7eXpev`{~LXD=w_l))5zu3(U
zt;{?hJ(vH-bf<^mt6r~8&E3Mw>~JB_??T8=$sLudW)B~IJU5wbGs~JI?-w7RWTx^Y
z)$QF}IbWHCqc?rr`5&pAnN+;_kfMNI)+?`9T^U<GD7dx;i&-4$XR=nh&9h0VNtZce
zu8TmRAggeT%j4egs&Lh`>Agq3&6qDNx1@8%+{2edW32A|XAn{rtp0fW!iDxc`@9PO
z{qOdFkNok+&Rn>@w`<G&`P-Dt*H`&n-};{+^H$e+Lmua2yJf-)3eUvt7d@i!Z@IJ@
zPu*PBj;Wg#Z+pEt%p~k@+l!NH*XH`T{MwW+*dO@KbR(Nu{Sy|cGpBCx)=!Mq?=cj7
z+7t5qqnpo*z9t2$+sDk_sHYV8fBMzv{$ssX*D6`@1sPI&B_F@a?LGUax8LrOt6JH=
zs2uNZ)@LWP6ID3ghI%J9S2@Sd?c%jrFYS2uO5x^QH;Y9q*FQYte7yhr3fDw$q2s^Y
zUujF!9lgDC^{-Bi#oPE!PPLTIeN_12&EwvMb6N`}m@>?+|IY8oH@}>}=1HCAG7I1R
zK04m6?GmRaOH1UL9^{!S#2>TJ)U2g4VOF?oIFs;G<Mn|##-(~G6Q4$(sZx#6*q*!n
z&8auxf1|FmZcSac=?Sx=LV)0=o}kvi1*)YBHW)|e9&60HT+yf9eJ^aahM&jolPTBs
z=zY_3mCoDer18}u?(W6V(BM@CT?@@FnDDY@?%XZvJO7F4&l2?~$+Me-PHc>w6u$4b
z%UYhcCn}M-TV7`xDOy}oJYHB4Xl^B(R(nA){O!9q$L_GhcY0T^KXIr3b)o9pzJ$kO
zI=&8pnmjrS8v>&g)f+>1raceeo7Km-!$Y#v{C9I;gY%KH>uFDt)B_^~wC3L7KG7U4
zxono(OwkA<wG$QXZPynnXC$egJ`@|!T;KoZ>XNs|m!9AJr)j^&NsUCG^~=)MO~`)W
z$f<Q|&kyExC+eR}GJYK{_=P`GV{y`wl!W5cBC}=wMQmU5SW7&EQ_gTWiCl6r-}$&S
zD(6YK)%z79R~$S}FW~6XXX;$A6`a&fK<T~y|4r2DXk+3XAzyX|1`vi<M}~SsxH@8H
z1l699`i6mlft8J&1yUUu!Kx!hHWqfUJToH`qB>F(6%$u7G;$1|a#ccEMb*TK%9YaJ
z;0xd=024*gLIqjNCT+5*oaV!QSIlPf4o1zu)fd+*M|@2Vy`!{K;;YNkz;%yX`K5lg
zPspzme{?Qj`cJj{x1#P}I<>}>*H7QV?%V~R%om(38^e|_Q%IY+ULkn@LaCm2-!?j5
zn-QY3j5W}Gdd%f}k(yHvPxK6my09fS`fy0fWW~_C6JOunrL<^Ksh?<w)km9Un=OxK
zZu0VsH=FiIAV=Zk*Bx~aL^~Y!uf8aAFX~C0OvlReR|QLW{598{D~!sSG%5V-(j6_*
z97eTy@;g|Q1oB^faW|i~Q@mYCtN5!+EXy`frOQ)azD?ogH``rDJByX_oTnuPZJf2>
zjzuQh%%y#9mW;3YH{=?|7|EVmQv6r(ACq(xznV?a%1{2hz5O>-ZPXX;P1!!%S?*Lx
zhJV<fg$ES(P2>yP*z5eY-p|radtzTgYS=%O+1ek@=Y2c7!;ha|r?da)@|o6(O_tjI
z77vmlPq#HCPnh(#<N5ZLY}cnZS**U^&bIn--?{DCHF-xiN_;K+5Yu|&kK-M&Nj4z{
zZ|x*nDz#P|?>O$hd4BSRj;Be*clW#U?LW9@_s_RG<S#tFe`|~QpQu@B_O4<qzi;!*
zil{E%vPGuM!(K03y`)c((`wP;;1##OWEG$Kv`Xj}TXSFT7O9s8%BN?_>2yAexNszC
zYu}_0=~IiPv_5@TIO6`*FMP$F{fk3OB>O_2@-g?^wwh;i^QZ2q)4B<ZlBc%MGrz&x
zQvJX#Sn^||>RRqQALkqn+-TeK!v6d!<7dm|MK`C;4clmJf8xEJ_1-nU%am+R*{M%(
zIik7o$IP$wIp^MmZ0QYcJ2EweogrUyqC}{4=o1;W>X@t99I2d>EIQsTx^8~4Zpp*1
zw^NqO=|(Qw9UW-AKWgU3&#w#LF1MHZp`><SH)yiX;*+)YDKXb?{Z@3}lzwQ+;j*r}
zf6dE7<6QhBt?a+;Uc4jp)vi}JgKmM^R|boas?YnL&5;|m4Xs~%e6sE6k)9RSi?&Z+
z$nb4ifb!;VTMtfpRAwA?qH%M0P<TkN>+(h>9jzG~ZwuLQJj^&Cuyw_?&SsZa*OUVV
zr8*BO6&_o?S9{WC?_-auef&H7gIvz|J^a!d&bNL4kq(Z%Ki<9&j=bG}d7kj=U+e$9
zwwG@|_9*}3#<1C+za4S=`rh-_xBYLe6C<A~c2rzX?s~^(*?s-dj#l|wg`3inzr48M
zvUaLWs?OD3k-FnkH)UtLt<R3XqHjF!-r8^6Nxb0-YtN}X4c(+^BvZFvT7LcW@EZa1
zVh(@!8dSApQ^pLvGY{9k5h^Xxp1w&xggdTrL59uC$FHQmn}0I1`Q0fp^X8}PO)(K_
z($lsHIrM3*Okh5%5Zf)<WHj?pdeK6*kE@fn3h3=GH}IGJ&v0*{)uCys{lA>AgfV<m
z-92mnGnM7;(%fnxPbAJflAYV(xcI)JM?-Rp5Tx?Fx&2y^-IvRD0_S}$c)PYszI*z1
zW1C~avy)*v=k4^Z+T)wAP%x!6h*kYm(yZ>p_8^~0HmfV!I>WhiZyX0zp8s?+rA#??
zot|*e$T_gX+^uBUD$c&34wF5v^A@Govn<_lLR0;~yBEb8>r$M*3f>KsbJ+N=(>Am>
zBxT~1`kkU(*Y~dDTL0i)?4NIU_+J#he;dUc?r<S(_Uw%zH~%@uPj|BEDK5xzx)7c+
z^|{5IMLCTpJT@Iu>Q!?oWa;?vBu;d_x;yXtca2{A<(@u~*qUQ|;?}M|Jrn<?cvdIR
z5D-(C)f2YMK`c1IN@LD-$*#rIK9!oU4d|(qX}lg>zr$35#l}6iEYD5pSJyFzReBHN
zTyk>~IXo1$hVgW6;(7ML^u-e?+qLy-*}E1u)yv;p?RmT3dw%*)*1Et+0f%PB`$+9_
zyMCbQq?V}RjYHEM|9MZa+o~ih`MPK;+tuPbA9g*vUD@?6G+?#So2>a-YXv8HJiL2A
z<iLg%*G)Nklw&%cWZ#>+<9WMQz37FY{|txCg!E4exg~}Q?sqt_@?747!#(GJbqF3d
zY~q+T$8@oD)G<@RwOL9^8K$w!4$uN$9=*~n$TqT+WMg0eVR)r$G*l~HDF$%I4Y>~p
zu5>|NKrL{c%gVsW!o<W3u5%GxKw%LMQAH&&LkS~e)1d!H7$m?QK(Hbh0ji$=A7M}y
zWMpJu1a*I565#%y5DTjzo3MzYV_+iA{$G=tS#Y?q=Eh^UrrvwCPb8buaL-Ab8x!)k
zo&5HA)i;gx%Q;r9y!@%7%chA)buHj3j;(RQr}dZDUz}TaE^pq;$e57i?;DJMMFifR
z$k!xWT2xk7ae93(i^$a(CuOxK96e;8<Z5KB7vMbkW&KOlw`=atJ$By6N1c7;#G<n`
zv&23|=>G8%;-4$~;-ZWKdq9_!i}_r)nc{wqi};y#@m|-w?bEHWcd8xRn>2|t;k&-9
zTHWbWc;i7p_@Wc*ORv6Jv1IQY{ge7K>Ku(%TNA{0tX;k7&3A2$Gs&~HE*#w58**EB
z(u7qO-9p0bHw8i}f&(8+pBVZuKo4Ag8|vi0QvccV<dn9;g0~vMmIk^@T&7IBrZ3*z
zvs;dTmc_(9Q@^+>ey#YW?fdgf;XCVfU&Hg?{;jj!9X?$<Wqamvx%k~VA9c2!OqqZ8
z(X;uUD_1Um=g*zSx^G6?!$96G*7Dnq6+hf@B6^XF`Hfi~rk5Cb>(iE&$*cama%Gl^
z`0UaJZ$fVIfm#e~jSC(X&D#62Q}={nUyzoNoln<%&A<A2mQO8w)1Tk|W$T+iabBOf
z{pwSX&F@w0e!KtrL(7$O=A?zE8W(?)=eWD0>_0=@@z>{G9hw%(@g&zNF<0^Egwtzw
z=y(_?<$tZcC;Z8NXHi$zid`H^Wv>g2d@tWR&ZaW!PkM3IKHD^BxpULJFTB>B*zx{U
zN@Vy<^;xezRqnX`ghh=_&%*wVc+{~^tKOI#JLGETJk8|v5_QS;tOX~_Cxx}T^aXCR
zDG}0(jsK=1|H4EtB6QoM^IJ>z>=r%x;&tFuwI@L-n{wu<SMxKh-5D<{P_<G&@70PG
zomVSE9d>6(u{GPRJF~I()1vLGj&F))i<XzKG~Kr$#aiTKNL<g$CPB9ey1RJK^zHQ&
zJe2sv?018A!tPA>ZS$@4436K-NGVa<_#%Nf+^NvI%2INn$j7fwLJovV2eB41#1uX*
zF!o<|B;D+Q@$!8i<&_uhKB4|6XWPW*N;^wbF8iLU-5y;rHz#t6(Ia=2;tg)?qKt<H
z*n4zKt<97BqYhiGJ%3K=qHBe$6lYOlq)}2!OYWSN6XRwjW#68vspZtNh=oa^iRV8<
z!iw!|zFUre;(63}>R@Z8YCu}q1~<*h1>8Ny^PJp;<OFtvP4SpKN$AN=?sdAp`tpmq
znQFhb`RdNQ`&L)FPe}gBl*zl?GnY+XrNvd6Q_nQ#)1FnJto3;r^SK*Qw<D74{jDdm
z{x(ZKS|YP=`GnJkH`KdqD&ATbIZC!&bG|E?;<(ZIA*-OEfNK7?=Vj&d<xUyydG}fT
z(1+jGV%OHK7f5_9ZQAmrklkX_l-?~YY3mj(Kd85MJyS+jA46`asX>O;!fRp9{jt%U
z6MDsD+*+4KWU#VGGl;kd3c9$6_z4OMnn<qyv2&Zie})$u=Rf$n|JgroyD;-l`#-<`
z_C4bJ>))kc-d^7M_3_Rkv0c1dRvg=yty86PrtyWC+`RJ<DP_iL`~?pBn)!D5n-3{z
z{G9Ps&!a*$^g>ahwX<tO)ZWBMdyT1z*Fu}P_iVa$b&9B>YDiYhbO-N={&!h9g{Qba
zm?{FvpWu-qDYj<x3S?QV@39|D3=AL)uRx54Y6T+6fKl?Zv9Pj(>JLp&{lUb<!~pHF
zF)%POfX5I8h1i8fM8%XO6b+%(hX~3jB)FpZe}q9q5V>B0h@y-Ye)R?Q7m+ZF-_!-^
zr7>%^eX8@iWiIuc`-n+#>Bhjrl9t?CdcJBp)Xr=@-fk|(`BS`3w&^C4Mn*<PmTyM3
zt7VtXc%qZ|O?Hz;s-o2e@2<sZah{LFg{<8<K3?$HIrTq7p~1g~=Fp|7d%Ekjb*jr&
zuYT}xE%%f%gW0-srX1Bzd?06O`1NM{>dD{Uy*qd2Zv6Bz?i3afkhv`Ua8KXfsmm(X
ztG^d52u<U3XUsdRGeu3u$6fN)&amudYJYbYZF-{Pr{JK(qj3KZYttv*Ia^PD<9*CN
z>4d{a(=}|*r!F$}zIMRKInJ6l#-k$X;iE#OJP+?lPjpLjO!wBahPD3r=A5>jKkui{
zlF~y*{!E@+p7qgHxHLU6Z)5)x-ShgVcjQhG5D*Y3nt$kdkLG^{*EN6nZ~xrax<5Pm
z$NJCjf7WcM{`L26?c81Ec6-Y8oYpfw_Ho;~I%Vst>1LcOQf%kF+$1gM_VnO89@bae
zO`hA>7#sJmIli*U`JnYK9$)n%7P`#ZX3moJ8C#Pg;+S~u-JE*r(4i+P?=D7f*5EAC
z{k~zNanYGS!kb$+F(Cnd^!&l-^svQ;k%0k(;rYX4sOAq*Q2qcXT{r>C8zKUjN&5%`
znb}}f0^Drk81|qrXJis#76GfrHnjnkU|?X_(g<Q1fB`5D0=!Z6p^rg<RD<xA#(p;F
z1PORV3SA$1Ga94^gts)Z@Im!~8`bFg&`VE{9uVHr=qdu$2QEv|^`WOKkRA};(wHg^
a)dx;v0p6@^AaPa(Rt5=128NpwARYkENt9dw

diff --git a/test/unit/media_file_test.rb b/test/unit/media_file_test.rb
index 4c77cf5b3..b45d055e9 100644
--- a/test/unit/media_file_test.rb
+++ b/test/unit/media_file_test.rb
@@ -33,7 +33,8 @@ class MediaFileTest < ActiveSupport::TestCase
 
     should "determine the correct dimensions for a ugoira file" do
       skip unless MediaFile.videos_enabled?
-      assert_equal([60, 60], MediaFile.open("test/files/valid_ugoira.zip").dimensions)
+      frame_data = JSON.parse(File.read("test/files/ugoira.json"))
+      assert_equal([60, 60], MediaFile.open("test/files/ugoira.zip", frame_data: frame_data).dimensions)
     end
 
     should "determine the correct dimensions for a flash file" do
@@ -57,7 +58,8 @@ class MediaFileTest < ActiveSupport::TestCase
       assert_equal([512, 512], mf.dimensions)
       assert_equal([512, 512], mf.dimensions)
 
-      mf = MediaFile.open("test/files/valid_ugoira.zip")
+      frame_data = JSON.parse(File.read("test/files/ugoira.json"))
+      mf = MediaFile.open("test/files/ugoira.zip", frame_data: frame_data)
       assert_equal([60, 60], mf.dimensions)
       assert_equal([60, 60], mf.dimensions)
     end
@@ -92,7 +94,7 @@ class MediaFileTest < ActiveSupport::TestCase
     end
 
     should "determine the correct extension for a ugoira file" do
-      assert_equal(:zip, MediaFile.open("test/files/valid_ugoira.zip").file_ext)
+      assert_equal(:zip, MediaFile.open("test/files/ugoira.zip").file_ext)
     end
 
     should "determine the correct extension for a flash file" do
@@ -113,12 +115,18 @@ class MediaFileTest < ActiveSupport::TestCase
   end
 
   context "#preview" do
-    should "generate a preview image" do
+    should "generate a preview image for a static image" do
       assert_equal([150, 101], MediaFile.open("test/files/test.jpg").preview(150, 150).dimensions)
       assert_equal([113, 150], MediaFile.open("test/files/test.png").preview(150, 150).dimensions)
       assert_equal([150, 150], MediaFile.open("test/files/test.gif").preview(150, 150).dimensions)
     end
 
+    should "generate a preview image for an animated image" do
+      skip unless MediaFile.videos_enabled?
+      assert_equal([86, 52], MediaFile.open("test/files/test-animated-86x52.gif").preview(150, 150).dimensions)
+      assert_equal([150, 150], MediaFile.open("test/files/apng/normal_apng.png").preview(150, 150).dimensions)
+    end
+
     should "generate a preview image for a video" do
       skip unless MediaFile.videos_enabled?
       assert_equal([150, 150], MediaFile.open("test/files/test-512x512.webm").preview(150, 150).dimensions)
diff --git a/test/unit/upload_service_test.rb b/test/unit/upload_service_test.rb
index 3b9de7c70..89c8859b2 100644
--- a/test/unit/upload_service_test.rb
+++ b/test/unit/upload_service_test.rb
@@ -60,8 +60,6 @@ class UploadServiceTest < ActiveSupport::TestCase
 
           assert_not_nil(@upload.context["ugoira"])
           assert_operator(File.size(file.path), :>, 0)
-
-          file.close
         end
       end
     end