signalapp / Signal-Android

A private messenger for Android.
https://signal.org
GNU Affero General Public License v3.0
25.59k stars 6.14k forks source link

Lower compression for sent pictures #672

Closed ghost closed 8 years ago

ghost commented 10 years ago

It would be nice if pictures which are sent over the data channel aren't so highly compressed.

billie80 commented 8 years ago

Scaling to a max edge of 1280 is probably the reason why panoramic images do not look good. Imagine an image with a resolution of 12800x1000. Scaling this leaves only 100 px in height. On my device I get approx. 7650x1150 px in portrait mode and 6000x500 px in landscape mode. So why not scaling to the min edge instead or only then if the aspect ratio reaches a certain value?

Edit: This does not make sense. A solution would be scaling to the min edge when the image reaches a certain aspect ratio but obviously then using a lower resolution than 1280, else the previous method would increase panoramic images in size.

Also 420 KB seems a little low for today's camera resolutions. If I am right about the memory restrictions the file size of the resulting image (due to compression) does not matter that much but the resolution, because it goes to memory uncompressed.

moxie0 commented 8 years ago

@billie80 JPG compression is lossy, so there is a correlation between file size and memory consumption.

billie80 commented 8 years ago

Hm, when I have an uncompressed bitmap file, disk size is equal to memory size. When I compress it as jpg disk size decreases while the memory size remains the same. So memory size only depends on the resolution (number of pixels) and color information, or am I wrong?

jeremymasters commented 8 years ago

@moxie0 So I had another look at my whatsapp scenario. If I take a picture with the normal phone's camera app, whatsapp sends it how it is. if i take a picture with Whatsapp's built-in photo capture, it does reduce the HxW to about 75% of normal camera dimensions but I don't know if that's an arbitrary integer value or a percentage under the hood.

What target value for filesize are you guys looking to hit, 1MB? Feel free to refer me to code, btw. I haven't programmed any Android but I do code so maybe I can sort through it a bit and not bother you so much with baby questions.

UNIcodeX commented 8 years ago

I did some testing last night between Signal and Textra. I had someone send me the exact same picture through both apps. The picture, as received in Textra from Textra (regular MMS) was 1121x1992 resolution and 555 KB in size. The picture, as received in Signal from Signal, was 405x720 resolution and 66.63 KB in size. The picture as received through Textra is roughly 2.7x better quality, and it's still just over half a MB. The original picture's resolution is 5312x2988, but even when compressed to 1121x1992 it was much better visual quality than the 405x720 from Signal.

Textra's settings have an option to limit MMS pictures for 1,000 KB. It states that this constraint is so that the provider does not reject the MMS message.

Digging around in the code, I traced a possible cause for this issue to the following file, which contains code for defining constraints on the images. Currently it is set to 1024x768, or even lower still if the device returns a state of being low on memory: /src/org/thoughtcrime/securesms/mms/MmsMediaConstraints.java

public class MmsMediaConstraints extends MediaConstraints {
  private static final int MAX_IMAGE_DIMEN_LOWMEM = 768;
  private static final int MAX_IMAGE_DIMEN        = 1024;
  public  static final int MAX_MESSAGE_SIZE       = 280 * 1024;

  @Override
  public int getImageMaxWidth(Context context) {
    return Util.isLowMemory(context) ? MAX_IMAGE_DIMEN_LOWMEM : MAX_IMAGE_DIMEN;
  }

Two possible avenues for improving the quality of images: A) Increase the resolution of the constraint - This is the easiest and quickest to realize. B) Modify the constraint to be calculated on file size, rather than resolution - Perhaps a long term goal after implementing option A.

dkasak commented 8 years ago

Currently it is set to 1024x768, or even lower still if the device returns a state of being low on memory: /src/org/thoughtcrime/securesms/mms/MmsMediaConstraints.java

I'd like to note that I encounter this case regularly with almost all the people I converse over Signal. I've seen it happen in conversations where both participants had relatively new phones (like Samsung Galaxy S5 or S6) which have plenty of RAM, so the "low on memory" state seems to either be triggered erroneously or it is a misnomer and the constraints are too stringent.

UNIcodeX commented 8 years ago

Good point dkasak. I failed to mention that the devices used in my testing were both LG G4 Android phones. At the time, free RAM would have been around 500 MB. I'd have to test again to get an accurate number for free RAM.

jeremymasters commented 8 years ago

I don't think it's free memory they are checking. It's what the device itself is classified as. Like if you hit the debug log in your android app, it should have a memclass. Not sure exactly what that's based on, but unless it's lowmem, I'd consider 3MB a good limit instead of the pixel count.

UNIcodeX commented 8 years ago

I agree. 3MB, with today's internet speeds, is fairly small. A user defined file size constraint for attachments could even be implemented in the settings. Not to exceed a hard coded ceiling value. The constraint could be (would have to be) smaller for an MMS, than it would for going through Signal servers, so as to keep from being rejected, but I think people are able to understand the limitations of SMS/MMS.

The question is... Will the developer make the change(s)?

jeremymasters commented 8 years ago

I think they are open for changes, but with the caveat of:

The answer is not more options, and there are no power users: https://github.com/WhisperSystems/Signal-Android/blob/master/CONTRIBUTING.md#development-ideology

UNIcodeX commented 8 years ago

Fine with me. Just set more sane upper limits, with automatic fall back on different modes of communication.

billie80 commented 8 years ago

I did some quick tests with panoramas.

Portrait example: Original properties: Resolution: 1168x7648 File size: 1926 KB Orientation: 90° Properties after sending with Signal: Resolution: 195x29 File size: 2 KB Orientation: 0°

Resolution and file size are way lower than the push constraints

Landscape example: Original properties: Resolution: 5904x480 File size: 572 KB Orientation: 0° Properties after sending with Signal: Resolution: 1279x104 File size: 17 KB Orientation: 0°

The resolution is slightly lower and the file size is way lower than the push constraints.

UNIcodeX commented 8 years ago

In a cursory look at the code last night, I'm not sure where it is determined that the picture actually NEEDS to be resized. I only saw references to a check on whether the image CAN be resized. But I didn't dig too terribly deep.

haffenloher commented 8 years ago

I opened two PRs trying to improve the situation within the current file size and memory restrictions: #5765 and #5772. Would be great if you could test these with different pictures in normal and low-mem configurations in order to determine 1) if the results are actually better 2) if the whole thing is still as robust as before, i.e. if it still manages to produce a result conforming to the restrictions every time, or if it has to give up on some images after 5 iterations.

billie80 commented 8 years ago

@haffenloher thanks for working on this. Using pixel count is a great idea. This also means I got something right ;-) I would gladly test this but this is only possible on a rooted phone if I am right. If this change would make it into a beta version I could apply for the beta channel to test your changes. Is it also possible to increase the max. file size? However I guess this is up to @moxie. With this the initial compression value could probably be increased as well.

UNIcodeX commented 8 years ago

I've tried the latest beta and the picture quality is vastly improved. Excellent work!

May I ask, however , that the quality be increased to 2 or 3 Megapixels? Images sent via Textra over regular MMS are around 2 MP.

Dyras commented 8 years ago

@UNIcodeX It doesn't seem like the latest beta has anything affecting the picture quality though...

Personally I think the resolution should be increased to at least Full HD. Maybe even Quad HD to cover the high-end phones.

billie80 commented 8 years ago

@UNIcodeX I think nothing has changed in the code which has to do with picture quality. Did you use the same pictures as input to test this? A quick test with my two panoramas gave the same results with low resolution and quality. @haffenloher just opened two pull requests. This does not mean it will get into Signal immediately, it first has to be accepted and then merged. To get it merged someone has to test the proposed changes and verify that they improve the situation.

darmbrust commented 8 years ago

Glad to see the resolution is improving, but what I'd really like to see (and what I had assumed this would do in the first place, since it sent over the data connection) is just give me an option to send the file exactly as it is. There is currently choices for Images, Audio, Video. Just give us an option for "Files". Add an option to the attachment area for "File". If we pick a picture via the file option, instead of the image option, we just send the image exactly as is with no processing / preview / etc. The bonus is it now works for any file type.... and I can finally send full resolution images without having to revert to e-mail or something like that. Probably should be written up as an enhancement request, if there is any chance something like this could be added.

Ideally, the send image option would just work like I had initially expected, and just send the images without mucking with them... but obviously that is complicated by all of the other things described above. Giving us the ability to push them through as files, however, bypasses a lot of those issues, and gives the user customizability over what is downloaded (and when) with the existing UI design.

UNIcodeX commented 8 years ago

@billie80 I am aware that it doesn't necessarily get pushed into the main branch immediately.... However, I am signed up for beta, and have tested with the exact same picture as was tested with before. This time yielded better image quality. I wouldn't report better quality unless I had actually tested it...

For your informational pleasure: Before latest Beta update - 405x720 @ 0.292 MP before latest beta - copy

After latest Beta update - 720x1279 @ 0.920 MP after latest beta - copy

@darmbrust Excellent idea, which mirrors my own thoughts a bit. Improved resolution of in-line images as well as the option to send any file type. I could think of a few situations where someone may need to securely send documents, as well as full resolution images so as to capture every detail of the information being shared.

billie80 commented 8 years ago

@UNIcodeX strange thing is that I am now registered for beta versions too. I did not receive an update so it seems latest beta is equal to latest official version. Using this version I don't see improvements for the pictures (panoramas) I tested. The dimensions of your picture are just one pixel below the maximum allowed size of 1280 so maybe this is the result of random noise ;-)

UNIcodeX commented 8 years ago

@billie80 Negative, Ghost Rider; the original picture size is 5312x2988 @ 16MP

haffenloher commented 8 years ago

@UNIcodeX you maybe got a ROM update which changed ro.config.low_ram from true to false, so your phone no longer reports being a low-RAM device (giving you a max edge of 1280 instead of 768). Whatever it is, there was no change in Signal.

UNIcodeX commented 8 years ago

I understand what you're saying. All I'm saying is that I was having the issue, I received an update for Signal beta, tested, quality was better.

I have received no known updates to my ROM. Nothing has advised me of an update, or even a compatability patch.

Maybe it's a fluke.. Just trying to help.

At any rate, I think we all agree that the image quality should be as high as possible and that an option should be added full for file transfer.

UNIcodeX commented 8 years ago

I just tested on another phone on my account. Same provider, same Model, same build, kernel numbers, etc.... The only difference is that this phone is not running the Signal beta. The image quality is still low, like the "Before latest Beta update" example above. 405x720 @ 0.292 MP

So, it seems something, somewhere, is different

moxie0 commented 8 years ago

in 3.21.0

ghost commented 7 years ago

I get the 2048 limit but why we don't use a common size?

We used to 1280 that is realy nice on phones so why don't scale up to 1920?

High-definition television (HDTV): HD (1280 × 720 progressive scan) Full HD (1920 × 1080 progressive scan) https://en.wikipedia.org/wiki/Display_resolution#Current_standards

jeremymasters commented 7 years ago

Because 2048 is higher resolution, resulting in pictures that are higher quality. Moxie is giving the best resolution possible. Not sure why you'd want to reduce that just to feel good about having a HD number. Many screens are higher resolution than 1920 now.

ghost commented 7 years ago

a standard is not an aesthetics thing or a feeling about numbers i'd go with the common one since the other one is maked up and not usefull

a random resolution is not bad itself but maybe a common one will work better

Dyras commented 7 years ago

I don't see what the big deal is. Most phones are low memory devices, so it's not like this change will actually start to matter until people start replacing their phones in 2-3 years. By that time higher resolutions will be more popular, which means 2048 instead of 1920 will actually make a difference.

Personally I wish the picture resolution was even higher, like Quad HD (2560x1440). However, there currently seems to be limitations in Android to prevent this. Is there a way around this? Probably, but I've never seen Signal as a WhatsApp competitor. I've always seen it as a testbed for the latest encryption technology, which is why we still don't have the new diversity emoji crap. I honestly wish they would add that so that my friends would stop bitching about it, but as long as I can send pictures of my cat to my mom without the pictures looking like crap, I'm happy. The larger the picture, the happier I get and since 2048 seems to be the maximum, I guess I'll have to settle for that.

ghost commented 7 years ago

if using the standard is useless ignore my comments please

speaking of quad hd while hd is not even there on common devices is a bit out of picture if bigger is always better the same apply: go with nearest common size i find that "bigger is always better" a bit missleading since we don't size the real world like that it's about losing 6% of the new bigger picture in the far future to make sense in the close future when that thing will happen we will need a lot more size to get the quality than 6%

my points over. i stop to add noise

fractalf commented 7 years ago

Thanks a lot to everyone involved in making this happen! 👍

UNIcodeX commented 7 years ago

looks like this issue is back again. I cleared out all running apps and took a picture and sent it through signal. The issue was fixed and pictures sent in this manner were at 2.4 MP, but now it's sending at 750 KP. What gives??

carstn commented 7 years ago

@UNIcodeX Diagnosing the issue will be most productive if you post

If possible, posting the actual files helps most. I'd suggest opening a new issue with this info.

UNIcodeX commented 7 years ago

@carstn looks like when using the camera icon in the text box, the resolution is capped at a little higher than 700 KP, but when you use a picture that was taken from another camera app, the resolution is better (2.4MP). Seems that the resolution is also better when using the paperclip icon to select an attachment type and then selecting camera.

haffenloher commented 7 years ago

@UNIcodeX if you're taking a landscape picture in portrait mode, you're only using a cropped portion of your camera's image. To use the full resolution, either expand the quick camera preview using the bottom right button or rotate your phone to landscape.

FeuRenard commented 7 years ago

https://github.com/WhisperSystems/Signal-Android/commit/d2be49af42eeebc888816904b1f765090be175ac brings further improvements, when you look at the file PushMediaConstraints.

private static final int MAX_IMAGE_DIMEN = 4096;

public int getImageMaxSize() { return 6 * MB; }

moxie0 commented 7 years ago

Correct, the limits should now be such that images taken on the device using the camera app are not modified at all before transmission (no scaling, no compression). We have a subsampling image view so that when those images are viewed on lower end devices with a max texture size of 2048 or whatever, it's still rendered.

There might be a transition period where users running the new version of the app send images to users on old devices running an old version of the app, and they are too large to be viewed.

jeremymasters commented 7 years ago

Awesome-r.

ka223 commented 7 years ago

@moxie0 But this is only by pictures taken with the build in signal camera? I am using another camera app. :(

If we resize the images to 1000x750 with a jpeg quality "40", we have a much better quality by almost the same size (or lower). For example the picture from zofrex. https://www.dropbox.com/s/rt6jbzsb6wzbl54/60WWNVA_40.jpg?dl=0

moxie0 commented 7 years ago

@ka223 the limits apply to all images, not just those taken within signal

ka223 commented 7 years ago

@moxie0 hm, but the pictures I received over signal are not better. They are more compressed and downscaled to 408x306 with ~31KB. Original file 3264x2448 with 682KB !

If I look back to my chat history, pictures up to the 15.01.2017 are scaled with 576x768 size 71kb.

Something changed, but in the wrong direction. The pictures are sent from an android 4.1 device.

UNIcodeX commented 7 years ago

the pictures signal takes arent great quality compared to the default camera app. i just use the default app and send that. they look much better that way.

On Feb 2, 2017 5:45 PM, "Andre" notifications@github.com wrote:

@moxie0 https://github.com/moxie0 hm, but the pictures I received over signal are not better. They are more compressed and downscaled to 408x306 with ~31KB.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WhisperSystems/Signal-Android/issues/672#issuecomment-277121475, or mute the thread https://github.com/notifications/unsubscribe-auth/AH2n-sOeytSXd2UYu4IuefFK3xX2jeeUks5rYmqogaJpZM4BkrZo .

ka223 commented 7 years ago

@moxie0 Can someone verify, that the pictures are currently so low by design?

Or is it a new bug to report? For example the original picture and the arrived one: https://www.dropbox.com/sh/ihl0xbc7qjiamn9/AAATpk5dlD3LcIKAsRVWwyUKa?dl=0

Dyras commented 7 years ago

@ka223 What device are you using since you're only on 4.1?

moxie0 commented 7 years ago

@ka223 On devices that report themselves as "low memory" we still scale to a max edge of 768.

ka223 commented 7 years ago

@Dyras: @moxie0 : Thanks. I am on CM13. The picture with 408x306 is send from an Alcatel 997d with Android 4.1. Pictures in the log from last year are 768x576 ..., then it is a bug? :/

Pictures from another contact with an android 4.4 device are downscaled to 640x480, also not the "max edge of 768".

moxie0 commented 7 years ago

@ka223 A debug log will show the memclass at the top. If it is indeed a low memclass device, then it's not a bug.

Dyras commented 7 years ago

@moxie0 @ka223 Alcatel 997d only has 1 gig RAM, which means this is not a bug http://www.gsmarena.com/alcatel_ot_997d-5110.php

There's no way around this at the moment, at least not any obvious easy to implement way. I'd suggest you try to find someone on Reddit or something who is willing to find a way to compress/downsize a picture without making low memory devices crap themselves. There's an $87.76 bounty for pull requests so there's got to be someone out there willing to do it.

ka223 commented 7 years ago

@Dyras @moxie0 Only ... 1GB is too low to send pictures downscaled to ~80kb? Maybe I can compare this with a very low ram device. ;)

The CM 13 moto g2 device (memclass 96) has 1GB Ram and the send pictures are in the original size.

moto g2: memclass 96 Ram 1GB send pictures with 3264x2448 874kb (original size)

Alcatel 997d memclass 64 Ram 1GB send pictures with 408x306 ~31kb

Ace memclass 32 (low-mem) Ram 256 MB send pictures with 640x480 ~71kb receive pictures with 3264x2448

However this is calculated with the memclass, but both (g2 and the 997d) have 1gb ram and the 256mb ram device can send pictures in higher quality than the 1gb ram device, although the ace has a lower memclass.