From 96a5fa9d782561b30a32dbe3ca2b5f41e5992330 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 16 Feb 2022 22:26:31 +0000 Subject: [PATCH] upload: Fix resizing non-animated images. 5dab6e9d31be began honoring the list of disposals for every frame. Unfortunately, passing a list of disposals for a non-animated image raises an exception: ``` File "zerver/lib/upload.py", line 212, in resize_emoji image_data = resize_gif(im, size) File "zerver/lib/upload.py", line 165, in resize_gif frames[0].save( File "[...]/PIL/Image.py", line 2212, in save save_handler(self, fp, filename) File "[...]/PIL/GifImagePlugin.py", line 605, in _save _write_single_frame(im, fp, palette) File "[...]/PIL/GifImagePlugin.py", line 506, in _write_single_frame _write_local_header(fp, im, (0, 0), flags) File "[...]/PIL/GifImagePlugin.py", line 647, in _write_local_header disposal = int(im.encoderinfo.get("disposal", 0)) TypeError: int() argument must be a string, a bytes-like object or a number, not 'list' ``` `check_add_realm_emoji` calls this as: ``` try: is_animated = upload_emoji_image(image_file, emoji_file_name, a uthor) emoji_uploaded_successfully = True finally: if not emoji_uploaded_successfully: realm_emoji.delete() return None # ... ``` This is equivalent to dropping _all_ exceptions silently. As such, Zulip has silently rejected all non-animated images larger than 64x64 since 5dab6e9d31be. Adjust to only pass a single disposal if there are no additional frames. Add a test for non-animated images, which requires also fixing the incidental bug that all GIF images were being recorded as animated, regardless of if they had more than 1 frame or not. --- zerver/lib/upload.py | 7 +++++-- zerver/tests/images/still_large_img.gif | Bin 0 -> 2841 bytes zerver/tests/test_realm_emoji.py | 12 ++++++++++++ zerver/tests/test_upload.py | 8 ++++++++ 4 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 zerver/tests/images/still_large_img.gif diff --git a/zerver/lib/upload.py b/zerver/lib/upload.py index d07cbef0dd..cacae7f6d9 100644 --- a/zerver/lib/upload.py +++ b/zerver/lib/upload.py @@ -169,7 +169,7 @@ def resize_gif(im: GifImageFile, size: int = DEFAULT_EMOJI_SIZE) -> bytes: format="GIF", append_images=frames[1:], duration=duration_info, - disposal=disposals, + disposal=disposals if len(frames) > 1 else disposals[0], loop=loop, ) return out.getvalue() @@ -211,7 +211,10 @@ def resize_emoji( if should_resize: image_data = resize_gif(im, size) - return image_data, True, still_image_data + if im.n_frames > 1: + return image_data, True, still_image_data + else: + return image_data, False, None else: # Note that this is essentially duplicated in the # still_image code path, above. diff --git a/zerver/tests/images/still_large_img.gif b/zerver/tests/images/still_large_img.gif new file mode 100644 index 0000000000000000000000000000000000000000..de3bf7d3bea13100955622c0d29fab4cdb38e877 GIT binary patch literal 2841 zcmeH`XFnSX1H}m?rCL?jMO(K+xhO@AwkRb{jG$(1f>1&2Rn*?phz7BDL}Jz!s+5Mt zh`sl0P@7oKz0W6jKEU%oZ_n?%IOlgDD&S|&VKlTfv`1)~e^69ZR8mp`fk3ZczXpTB z5C}v?MMYIrRZUF|3Wchxt7~XzXliP{dGkg~OG{f@TSrGnPft%@U*EvMz|hdp$jHdV z#KhFp)XdDx+}zy4!U6_^Sz20JSy{p1a9dkjJ3BiB0%31&@9600gML= z?(Xj4;o<4&>E-3+?d|R3Csi~>CxtTy9w6wIewzhu#`t{qlZ|&{v z9UUE=ot<4>T|^?WySw}6&z~d`slUH}U|@huCJzn{4h;>BjEszqj*gFyPfkuwO-;?t z&d$%zFDxu9EiElCFR!hwt*@_dY;64c^@~EGY;JCDZEbCDZ}05v?CtIC@9!TR92_1V z9vvMWA0MBboKUIM)6>(lv$ON_^NWj%%gf7u7yUo~a|8EJ=xP5o-@jy5afI2z)nT@F zR`%CzVUCV4IZ+V_$$v%j_`iAoJcb5%Nka>~qg;{G76QB_X!4^Xw>=E-Ks7_TGOr_o z^C@ELM`eCj{5>U(FUnN~#Ha^4SQBDZVNWQ(Rd>coRS_xunNIH%QOBk?TOnkIqVnY( zG*=lZ2!HpbWDu#AuSz=kg4IjXsYaZ8*Ob1EHg1o8eO!YZkF**@$n}iiCh8O?yWz}r zTRjcl>l4$=;^pQ9KRWp{HO`8;b_Ql4saS*G3th3d+ryyGtE?+OKW!q&>Q}FlGKxR) zu)J{BCSzLBV02^6#;~qwPYcuJ3T4vNzOI8biGBZ>5F&p*{kGnvxZ^pG`HaMK@JfHW zOav$4#lX5K-hMSoZs%}YqKWyEUUSCwXm>4FL$j}?VDIp^Fk8IlUymu&W395@KJ&o! zi*px-dx1$T&+b_=aIYBzGi`mp7vd>*JZH_Ok*Xi|hgoE97|bzsUZ2MjXc@^?_Ie>g zE3+OJEm(%pjS;D0u!>b6aax*4kt20JNq2VUN4%Ut0d8rr+btz3^OY|UFrQbV``yG^ISYKHsBI-I?Fe8mVD zGPb^&9XN7TnXzhmnL8th54)CU#-z3O9#3dnD@X=#hubF<6@d#gJO%l)@RdVo)+`hP zk@_T*$pmpXV`HN<7I&F8fyJ!vY~YKqxd>%EA@Emu7NSDvZOJ$jr4niO!rlcF7D%a% zeISZ+Eit9ttO>dh#e>Vlf;a0t$w77yydBL}gBw_U3tV;{xYcCC)`(XsGi2T-z$Pl* zgUcMFw_8ocE4MAq0S*;>Z)K?_ZH$+tlx8Lp2e|!`59h2@J+3C!oO2zTX6-6rzN_^yM0%Nd@ho+NuaPs&`#xtFhJ1kpFj#O&VDJn(A2x^b6d z-=SB;A2*<3JDT3AJ&O?gtiQ(>gfeXER{m*%e3a1y%SL0JO&PmD6NO0*8RK097Oz6r zK7UsCce9rSOv}#!f(dYj%JPm^N3qV!0|?ZrpLZ93;N(7IcN9WS=z0zuPL2m^+-(i`| zz?2BTUHzNewhL==H_ml>y10nl6vF_578%m(bFk=du6GCr0DOIy(`R*$UCBW*r!@83 z>{Kf$Q}ffqBl^~~yOZYUR0#W77*XqN{x4eiuDm#2IEc<<4ti~^aP#ip!ECZBbnUzi z4DdI6>`O^hw$Qfet3ke?@wt?J(bgM95bY3ARiNY!!=Iguw4F?I8oFbJv*WCCY&0FH z%LfCBtVu*~c~Cq9PjP$NZ)Y`+V7oYO=Bw17n0O+X~bn@axj&1o>q8 z9Ns@tv#-mFq)s;5_+vQv;S&kz`#X7Q3TYtsZ~WHFyoEC3PnDjiAY=DHH^k+?Uv~Qi zGosxa`CooFG zw6M|WS6?Hn9c`rrX{|&u4k-qsO|)cdqL)rFcHkZApNCH$OL2P#vF3Cv@yVPJq10DQ z2-%s3GJ6${jjr%&96W6p(^GOTo+{>m5qrx8v02%QA>%ALD$~a54SlKu79X#EcgOUG zu3yXA%R|ZaEen4{F^2(6QNWuMLVnLB90B=j^LL5{zyhoV4Q^>F5fBBG6!D|@X4a{* zk$mHcgi+F-9mU#67ReE>Lc$@!*cyj#t3Sm-CYKG3^?Pc;l+SLY@sGPkH7#cc9+HBT zF~O=auIs#2B?tX*q*`?EMLB)`v;{!h_t8j8EvBV-6KzmEX_9rb{yQ$ur3V99c^MZ; z7Dh8vp7sL#yt3yXZiD_Rp8;a8w@4F~{e35XT!1A*=vpZ9xBO<4Up}J&0BB_X13V)t A761SM literal 0 HcmV?d00001 diff --git a/zerver/tests/test_realm_emoji.py b/zerver/tests/test_realm_emoji.py index a8f2cb82b8..a56a5e8c6a 100644 --- a/zerver/tests/test_realm_emoji.py +++ b/zerver/tests/test_realm_emoji.py @@ -271,6 +271,18 @@ class RealmEmojiTest(ZulipTestCase): result = self.client_post("/json/realm/emoji/my_emoji", {"f1": fp1, "f2": fp2}) self.assert_json_error(result, "You must upload exactly one file.") + def test_emoji_upload_success(self) -> None: + self.login("iago") + with get_test_image_file("img.gif") as fp: + result = self.client_post("/json/realm/emoji/my_emoji", {"file": fp}) + self.assert_json_success(result) + + def test_emoji_upload_resize_success(self) -> None: + self.login("iago") + with get_test_image_file("still_large_img.gif") as fp: + result = self.client_post("/json/realm/emoji/my_emoji", {"file": fp}) + self.assert_json_success(result) + def test_emoji_upload_file_size_error(self) -> None: self.login("iago") with get_test_image_file("img.png") as fp: diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 1ffe6cf0ca..1ee21ee14f 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -1343,6 +1343,14 @@ class EmojiTest(UploadSerializeMixin, ZulipTestCase): still_image = Image.open(io.BytesIO(still_img_data)) self.assertEqual((50, 50), still_image.size) + # Test a non-animated image which does need to be resized + still_large_img_data = read_test_image_file("still_large_img.gif") + resized_img_data, is_animated, no_still_data = resize_emoji(still_large_img_data, size=50) + im = Image.open(io.BytesIO(resized_img_data)) + self.assertEqual((50, 50), im.size) + self.assertFalse(is_animated) + assert no_still_data is None + def tearDown(self) -> None: destroy_uploads() super().tearDown()