New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Large photos (e.g. from a smart phone camera) cannot be opened at all on Signal 2.29.1 #3860

Closed
jmhertlein opened this Issue Sep 2, 2018 · 29 comments

Comments

Projects
None yet
@jmhertlein

jmhertlein commented Sep 2, 2018

  • I have searched open and closed issues for duplicates
  • I am submitting a bug report for existing functionality that does not work as intended
  • This isn't a feature request or a discussion topic

Bug description

After the recent update that changes it so signal doesn't display image previews for large-dimension images, images are no longer viewable in any way. Tapping on an image just brings up the "share" dialog.

Steps to reproduce

  • send an image, or receive an image. In this case, my SO is sending photos taken on a Samsung Note 8's primary camera to my iphone.
  • You'll see it just says "JPG" and a file size, instead of showing the thumbnail/preview
  • Tapping on it does not open it, just pops up the "select an app" picker.

Actual result: There is no clear way to view images in the app anymore.

Expected result: I should be able to easily view these images.

Device info

Device: iPhone 8S

iOS version: 11.4.1

Signal version: 2.29.1

@czekaj

This comment has been minimized.

Show comment
Hide comment
@czekaj

czekaj Sep 2, 2018

I have anecdotal evidence of the same thing happening when I send a photo taken with my Pixel 2 camera app or an mp4 video I downloaded. I use Signal 4.24.8 for Android on my Pixel 2 to send the photo/video. The other party is on iOS with Signal 2.29.1 and they can't see it.

Smaller photos, like ones saved by WhatsApp (which resizes and compresses photos) or photos taken directly by Signal app are received without a problem on Signal 2.29.1 on iOS.

czekaj commented Sep 2, 2018

I have anecdotal evidence of the same thing happening when I send a photo taken with my Pixel 2 camera app or an mp4 video I downloaded. I use Signal 4.24.8 for Android on my Pixel 2 to send the photo/video. The other party is on iOS with Signal 2.29.1 and they can't see it.

Smaller photos, like ones saved by WhatsApp (which resizes and compresses photos) or photos taken directly by Signal app are received without a problem on Signal 2.29.1 on iOS.

@vaxus

This comment has been minimized.

Show comment
Hide comment
@vaxus

vaxus Sep 2, 2018

I am seeing the same issue as well.

Device: iPhone 7 Plus

iOS Version: 11.4.1

Signal Version: 2.29.1.1

See attached screenshot.
chat jpeg issue

vaxus commented Sep 2, 2018

I am seeing the same issue as well.

Device: iPhone 7 Plus

iOS Version: 11.4.1

Signal Version: 2.29.1.1

See attached screenshot.
chat jpeg issue

@nzbart

This comment has been minimized.

Show comment
Hide comment
@nzbart

nzbart Sep 3, 2018

This is affecting me, also. Is it only affecting a small group of people, or is it a really inconvenient bug that's affecting everyone?

nzbart commented Sep 3, 2018

This is affecting me, also. Is it only affecting a small group of people, or is it a really inconvenient bug that's affecting everyone?

@czekaj

This comment has been minimized.

Show comment
Hide comment
@czekaj

czekaj Sep 3, 2018

Has anyone experienced this issue iOS -> iOS?

So far we have at least two examples of Android -> iOS (arrow is from sender to receiver) which might indicate special format of Android photos/videos.

czekaj commented Sep 3, 2018

Has anyone experienced this issue iOS -> iOS?

So far we have at least two examples of Android -> iOS (arrow is from sender to receiver) which might indicate special format of Android photos/videos.

@nzbart

This comment has been minimized.

Show comment
Hide comment
@nzbart

nzbart Sep 3, 2018

Good point - my scenario is Android -> iOS as well.

nzbart commented Sep 3, 2018

Good point - my scenario is Android -> iOS as well.

@lostwithouthope

This comment has been minimized.

Show comment
Hide comment
@lostwithouthope

lostwithouthope Sep 3, 2018

Same issue here wenn picture is sent from Android to iOS, but not in the other direction.

lostwithouthope commented Sep 3, 2018

Same issue here wenn picture is sent from Android to iOS, but not in the other direction.

@zynexiz

This comment has been minimized.

Show comment
Hide comment
@zynexiz

zynexiz Sep 3, 2018

My friend has an iOS device, and I'm on Android 8 (Sony firmware) and we both have this problem both ways. He can download it to show the image, in my case I don't even see that a image is sent.

A smaller image size works though, it seems it's just big images that fail.

zynexiz commented Sep 3, 2018

My friend has an iOS device, and I'm on Android 8 (Sony firmware) and we both have this problem both ways. He can download it to show the image, in my case I don't even see that a image is sent.

A smaller image size works though, it seems it's just big images that fail.

@czekaj

This comment has been minimized.

Show comment
Hide comment
@czekaj

czekaj Sep 3, 2018

I guess we'll need @michaelkirk-signal / @michaelkirk to get involved since he made the changes related to image and video validation in 2.29.1.

czekaj commented Sep 3, 2018

I guess we'll need @michaelkirk-signal / @michaelkirk to get involved since he made the changes related to image and video validation in 2.29.1.

@skbdba192

This comment has been minimized.

Show comment
Hide comment
@skbdba192

skbdba192 Sep 3, 2018

Can confirm that iOS>iOS is having the same problem, as well as iOS>Android and back, for me at least. I am running iOS 12 pub beta 10, and signal 2.29.1.1

skbdba192 commented Sep 3, 2018

Can confirm that iOS>iOS is having the same problem, as well as iOS>Android and back, for me at least. I am running iOS 12 pub beta 10, and signal 2.29.1.1

@KienHui

This comment has been minimized.

Show comment
Hide comment
@KienHui

KienHui Sep 4, 2018

I'm just here to boost the signal on this issue. I've got the same issue (as does my family members on iOS).

KienHui commented Sep 4, 2018

I'm just here to boost the signal on this issue. I've got the same issue (as does my family members on iOS).

@KienHui

This comment has been minimized.

Show comment
Hide comment
@KienHui

KienHui Sep 4, 2018

So, looking through at the commit, I think the cause is in https://github.com/signalapp/Signal-iOS/blob/e715bf9ea2f51e2d0c4549d7b4fa68e442e4d026/SignalServiceKit/src/Util/NSData%2BImage.m , around line 138. It effectively restricts the image to being 4 megapixel in resolution. However, we're still sending images larger than that. If CGFloat kMaxDimension = 2 * 1024; is changed to CGFloat kMaxDimension = 4 * 1024; Then most people's phones will be perfectly fine (until resolution increases for most people again). Or at least, it's there for pictures not showing up.

KienHui commented Sep 4, 2018

So, looking through at the commit, I think the cause is in https://github.com/signalapp/Signal-iOS/blob/e715bf9ea2f51e2d0c4549d7b4fa68e442e4d026/SignalServiceKit/src/Util/NSData%2BImage.m , around line 138. It effectively restricts the image to being 4 megapixel in resolution. However, we're still sending images larger than that. If CGFloat kMaxDimension = 2 * 1024; is changed to CGFloat kMaxDimension = 4 * 1024; Then most people's phones will be perfectly fine (until resolution increases for most people again). Or at least, it's there for pictures not showing up.

@jmhertlein

This comment has been minimized.

Show comment
Hide comment
@jmhertlein

jmhertlein Sep 4, 2018

Independent of the threshold that an image is considered "too large", I think it's also a UX bug that images like this cannot be opened seamlessly in the image viewer. The main bug that the patch in question was fixing was a DoS attack bug, right? From creating the thumbnail automatically being able to exhaust memory with a maliciously-crafted image? (Like a pure-white PNG that's 1B pixels by 1B pixels).

If that's the case, I don't see why using the built-in seamless image viewer isn't an option, since the user tapping the icon would only result in one crash, and not a complete inability to view the conversation. And it's the same crash that would otherwise happen in any other image viewer the user does use to open the image.

I think this issue would be much lower priority if the original patch had preserved the ability to still use the built-in image viewer.

jmhertlein commented Sep 4, 2018

Independent of the threshold that an image is considered "too large", I think it's also a UX bug that images like this cannot be opened seamlessly in the image viewer. The main bug that the patch in question was fixing was a DoS attack bug, right? From creating the thumbnail automatically being able to exhaust memory with a maliciously-crafted image? (Like a pure-white PNG that's 1B pixels by 1B pixels).

If that's the case, I don't see why using the built-in seamless image viewer isn't an option, since the user tapping the icon would only result in one crash, and not a complete inability to view the conversation. And it's the same crash that would otherwise happen in any other image viewer the user does use to open the image.

I think this issue would be much lower priority if the original patch had preserved the ability to still use the built-in image viewer.

@jmhertlein

This comment has been minimized.

Show comment
Hide comment
@jmhertlein

jmhertlein Sep 4, 2018

I realize that the patch is probably just making it act like a file upload instead of an image upload, and that it's probably additional work to make the image-upload be able to not show the thumbnail, but if it fixes the UX bug, I think it is worth it.

jmhertlein commented Sep 4, 2018

I realize that the patch is probably just making it act like a file upload instead of an image upload, and that it's probably additional work to make the image-upload be able to not show the thumbnail, but if it fixes the UX bug, I think it is worth it.

@michaelkirk-signal

This comment has been minimized.

Show comment
Hide comment
@michaelkirk-signal

michaelkirk-signal Sep 4, 2018

Contributor

Hey all, thanks for the feedback. We're aware of this issue and working on fixing it.

Contributor

michaelkirk-signal commented Sep 4, 2018

Hey all, thanks for the feedback. We're aware of this issue and working on fixing it.

@mossmann

This comment has been minimized.

Show comment
Hide comment
@mossmann

mossmann Sep 7, 2018

A little workaround I found for viewing "large" images within Signal: Copy and paste the image into an outgoing message (which you need not send). It isn't a great workaround, but maybe it will help someone until this issue is addressed.

mossmann commented Sep 7, 2018

A little workaround I found for viewing "large" images within Signal: Copy and paste the image into an outgoing message (which you need not send). It isn't a great workaround, but maybe it will help someone until this issue is addressed.

@greveritt

This comment has been minimized.

Show comment
Hide comment
@greveritt

greveritt Sep 7, 2018

My friend with an iPhone said he saved the file and opened it in another program. He said that did it.

greveritt commented Sep 7, 2018

My friend with an iPhone said he saved the file and opened it in another program. He said that did it.

@mossmann

This comment has been minimized.

Show comment
Hide comment
@mossmann

mossmann Sep 7, 2018

Yes, exporting a large image works, but that exposes the image to another app, which could be undesirable for viewing private images.

mossmann commented Sep 7, 2018

Yes, exporting a large image works, but that exposes the image to another app, which could be undesirable for viewing private images.

@jmhertlein

This comment has been minimized.

Show comment
Hide comment
@jmhertlein

jmhertlein Sep 7, 2018

It's also terrible UX, which IMO is a security concern, since it might drive users to a less-secure but more-convenient alternative.

jmhertlein commented Sep 7, 2018

It's also terrible UX, which IMO is a security concern, since it might drive users to a less-secure but more-convenient alternative.

@michaelkirk-signal

This comment has been minimized.

Show comment
Hide comment
@michaelkirk-signal

michaelkirk-signal Sep 7, 2018

Contributor

This is fixed in the current beta. If you'd like to help test before we get to production, send a message to support@signal.org with the subject "sign me up for the iOS beta"

Contributor

michaelkirk-signal commented Sep 7, 2018

This is fixed in the current beta. If you'd like to help test before we get to production, send a message to support@signal.org with the subject "sign me up for the iOS beta"

@wormeyman

This comment has been minimized.

Show comment
Hide comment
@wormeyman

wormeyman Sep 7, 2018

I don't see this issue taged in any commits what ended up being the fix?

wormeyman commented Sep 7, 2018

I don't see this issue taged in any commits what ended up being the fix?

@zynexiz

This comment has been minimized.

Show comment
Hide comment
@zynexiz

zynexiz Sep 7, 2018

Got the update today, and as far as I can tell it's working.

zynexiz commented Sep 7, 2018

Got the update today, and as far as I can tell it's working.

@karlstr

This comment has been minimized.

Show comment
Hide comment
@karlstr

karlstr Sep 8, 2018

When will this be in production?

karlstr commented Sep 8, 2018

When will this be in production?

@michaelkirk-signal

This comment has been minimized.

Show comment
Hide comment
@michaelkirk-signal

michaelkirk-signal Sep 9, 2018

Contributor

This fix has shipped with the 2.29.2 release - in the app store as of a few minutes ago.

Contributor

michaelkirk-signal commented Sep 9, 2018

This fix has shipped with the 2.29.2 release - in the app store as of a few minutes ago.

@czekaj

This comment has been minimized.

Show comment
Hide comment
@czekaj

czekaj Sep 9, 2018

Just tested it and it seems to work for large photos but large videos still don't work 😢
Should we open another issue?

czekaj commented Sep 9, 2018

Just tested it and it seems to work for large photos but large videos still don't work 😢
Should we open another issue?

@mschenchang

This comment has been minimized.

Show comment
Hide comment
@mschenchang

mschenchang Sep 10, 2018

But this problem still exist for my phone: iphone 6s, latest version of signal. The photo I sent is fine, I can see them, but not the photo other people sent to me, it's written always JPG file as shown above, no matter how big how small it is.

mschenchang commented Sep 10, 2018

But this problem still exist for my phone: iphone 6s, latest version of signal. The photo I sent is fine, I can see them, but not the photo other people sent to me, it's written always JPG file as shown above, no matter how big how small it is.

@michaelkirk-signal

This comment has been minimized.

Show comment
Hide comment
@michaelkirk-signal

michaelkirk-signal Sep 10, 2018

Contributor

@czekaj - what was the source of the videos? Do you know their resolution? Can you provide an example?

Contributor

michaelkirk-signal commented Sep 10, 2018

@czekaj - what was the source of the videos? Do you know their resolution? Can you provide an example?

@michaelkirk-signal

This comment has been minimized.

Show comment
Hide comment
@michaelkirk-signal

michaelkirk-signal Sep 10, 2018

Contributor

@mschenchang - What version, exactly, are you running? Can you submit a debug log?

Contributor

michaelkirk-signal commented Sep 10, 2018

@mschenchang - What version, exactly, are you running? Can you submit a debug log?

@anniejalota

This comment has been minimized.

Show comment
Hide comment
@anniejalota

anniejalota Sep 10, 2018

Thanks for the fix! My app didn't auto-update but just updated the app to 2.29.2 and seemed to have resolved the issue.

anniejalota commented Sep 10, 2018

Thanks for the fix! My app didn't auto-update but just updated the app to 2.29.2 and seemed to have resolved the issue.

@mro mro referenced this issue Sep 10, 2018

Closed

Large dimension images don't display inline #3878

3 of 3 tasks complete
@LKBorg

This comment has been minimized.

Show comment
Hide comment
@LKBorg

LKBorg Sep 14, 2018

Damn iOS AppStore didn't showed me the update! ^^

So, I filed a bug report, since this was the latest (.1) update. Yeeez.
Thanks to your "[ ] checked if already exist in the forum" tick mark, I found this! ^^

Thanks for the fast bugfix! :)

LKBorg commented Sep 14, 2018

Damn iOS AppStore didn't showed me the update! ^^

So, I filed a bug report, since this was the latest (.1) update. Yeeez.
Thanks to your "[ ] checked if already exist in the forum" tick mark, I found this! ^^

Thanks for the fast bugfix! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment