873758 - [MMS] Image rescaling follow up: Apply image attachment rescaling ability in composer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Steve Chung [:steveck]]

This is a follow up bug for bug 840038. We land image blob rescaling function previously but message composer didn't apply it. Now we need this function for attaching the oversize image instead of rejection.

Comment 2

[Corey Frang [:gnarf]]

Need to verify the spec here... please help.

* This magic number "300k" defines what exactly (it is now loaded from a setting on the phone)?
  * The maximum number of bytes we should have in attachments? Text & Images? (blob.size?)
* We MUST resize images to make them fit?
  * So then, one image = 300k? two images, resize both to 150k? three -> 100k? etc?
  * What is the limit of images?
* The wireframes mention a "too large to attach" screen, but I don't find a "would you like to resize the attachments" screen.

Comment 3

[Corey Frang [:gnarf]]

Steve, would you mind if I finish out this bug?  I'll use you to review!

Comment 4

[Steve Chung [:steveck]]

Attachment: Patch for image attachment resizing

Comment 5

[Steve Chung [:steveck]]

Sorry for the late reply... It takes time for settling down the unit test.
we definitely need for info from UX and product, but I'll explain about the patch briefly in the comment:

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #2)
 
> * This magic number "300k" defines what exactly (it is now loaded from a
> setting on the phone)?
The magic number comes from image rescaling user story... It's different from the message size limitation. Need more info about the magic number from product.
>   * The maximum number of bytes we should have in attachments? Text &
> Images? (blob.size?)
I think the limitation here is only for the image attachment.
> * We MUST resize images to make them fit?
>   * So then, one image = 300k? two images, resize both to 150k? three ->
> 100k? etc?
>   * What is the limit of images?
If my memory is correct, the limit of the image number should be 5 (in a single mms message). This patch only make sure that user could attach at least one image in message. How about setting the image size limitation to (max message size / max image number)? Need UX/product input for this.
> * The wireframes mention a "too large to attach" screen, but I don't find a
> "would you like to resize the attachments" screen.
Both Android and iOS doesn't popup the warning message before resizing, and it seems annoying because image size limitation might be small if we could allow user to append 5 or more images.

Comment 6

[ayman maat :maat]

(In reply to Steve Chung from comment #5)
> Sorry for the late reply... It takes time for settling down the unit test.
> we definitely need for info from UX and product, but I'll explain about the
> patch briefly in the comment:
> 
> (In reply to Corey Frang [:gnarf] [:gnarf37] from comment #2)
>  
> > * This magic number "300k" defines what exactly (it is now loaded from a
> > setting on the phone)?
> The magic number comes from image rescaling user story... It's different
> from the message size limitation. Need more info about the magic number from
> product.
> >   * The maximum number of bytes we should have in attachments? Text &
> > Images? (blob.size?)
> I think the limitation here is only for the image attachment.
> > * We MUST resize images to make them fit?
> >   * So then, one image = 300k? two images, resize both to 150k? three ->
> > 100k? etc?
> >   * What is the limit of images?
> If my memory is correct, the limit of the image number should be 5 (in a
> single mms message). This patch only make sure that user could attach at
> least one image in message. How about setting the image size limitation to
> (max message size / max image number)? Need UX/product input for this.

From a UX PoV I have no particular opinion about the maximum number of images that can be attached to a single message. 'What is the maximum number of images that can be attached and should we even have a maximum number of images that can be attached' are one of the outstanding questions I have that has never been definitively answered and is why the wireframes contain no reference to this. I will defer to product on this requirement


> > * The wireframes mention a "too large to attach" screen, but I don't find a
> > "would you like to resize the attachments" screen.

The last i heard about resizing of images which was during the Madrid work week, and this was that we were not resizing images. This is why there is no reference to it in the wireframes. However seen as we are now resizing images we should certainly have a message informing the end user that the image is being resized. 


> Both Android and iOS doesn't popup the warning message before resizing, and
> it seems annoying because image size limitation might be small if we could
> allow user to append 5 or more images.

Thats not quite so Steve. On one of my android reference devices that is running Andriod version 4.0.4. (Icecream sandwich) a temporary messages appears on the screen informing the end user that the attached image is being resized: "image too large. compressing…". End users are paying for the MMS service therefore if we are altering the file that is being attached in any way shape for form we should absolutely include a message that informs the end user that what the file their contact will receive is not exactly the same as the original file.

Comment 7

[Corey Frang [:gnarf]]

Comment on attachment 756484 [details]
Patch for image attachment resizing

comments on pull request - r- for now, but this is really close to done.

Comment 8

[Corey Frang [:gnarf]]

I still wonder what we need to do to get 5 images attached, do we need to resize all the currently attached images?

Comment 9

[ayman maat :maat]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #8)
> I still wonder what we need to do to get 5 images attached, do we need to
> resize all the currently attached images?

Well let me pose a question: in your opinion in the environment we are operating in what is the risk that four iterations of resizing an images would perceivably effect its quality?

If our requirement is indeed a maximum of 5 images attached to a single message, I am of the opinion that 5 should be the limit and not the traget so to speak. so if four iterations of resizing an image risks perceivably effecting its quality we should limit each image to just one or two iterations of resizing. if in that scenario the user can get 5 images into one message - great. if they cannot always, well thats just life. I say this with reference to the observation that when other file types are supported such as video and audio the user is unlikely to be able to send a video and 5 images..  so 5 images should be a limit, not a target. (hope that makes sence)

Comment 10

[Steve Chung [:steveck]]

(In reply to ayman maat :maat from comment #6)
> (In reply to Steve Chung from comment #5)

> > > * The wireframes mention a "too large to attach" screen, but I don't find a
> > > "would you like to resize the attachments" screen.
> 
> The last i heard about resizing of images which was during the Madrid work
> week, and this was that we were not resizing images. This is why there is no
> reference to it in the wireframes. However seen as we are now resizing
> images we should certainly have a message informing the end user that the
> image is being resized. 
> 
> 
> > Both Android and iOS doesn't popup the warning message before resizing, and
> > it seems annoying because image size limitation might be small if we could
> > allow user to append 5 or more images.
> 
> Thats not quite so Steve. On one of my android reference devices that is
> running Andriod version 4.0.4. (Icecream sandwich) a temporary messages
> appears on the screen informing the end user that the attached image is
> being resized: "image too large. compressing…". End users are paying for the
> MMS service therefore if we are altering the file that is being attached in
> any way shape for form we should absolutely include a message that informs
> the end user that what the file their contact will receive is not exactly
> the same as the original file.

Hi Ayman, I totally agree that we should notify user about the image rescaling. It's would be great if we can reuse the widget like toaster for "switch to mms/sms...", I will do that in this patch if you agree this UX. The thing I concerned is whether we should have a blocking warning dialog to notify user that image will be resized.

Comment 11

[ayman maat :maat]

(In reply to Steve Chung from comment #10)
> (In reply to ayman maat :maat from comment #6)
> > (In reply to Steve Chung from comment #5)
> 
> > > > * The wireframes mention a "too large to attach" screen, but I don't find a
> > > > "would you like to resize the attachments" screen.
> > 
> > The last i heard about resizing of images which was during the Madrid work
> > week, and this was that we were not resizing images. This is why there is no
> > reference to it in the wireframes. However seen as we are now resizing
> > images we should certainly have a message informing the end user that the
> > image is being resized. 
> > 
> > 
> > > Both Android and iOS doesn't popup the warning message before resizing, and
> > > it seems annoying because image size limitation might be small if we could
> > > allow user to append 5 or more images.
> > 
> > Thats not quite so Steve. On one of my android reference devices that is
> > running Andriod version 4.0.4. (Icecream sandwich) a temporary messages
> > appears on the screen informing the end user that the attached image is
> > being resized: "image too large. compressing…". End users are paying for the
> > MMS service therefore if we are altering the file that is being attached in
> > any way shape for form we should absolutely include a message that informs
> > the end user that what the file their contact will receive is not exactly
> > the same as the original file.
> 
> Hi Ayman, I totally agree that we should notify user about the image
> rescaling. It's would be great if we can reuse the widget like toaster for
> "switch to mms/sms...", I will do that in this patch if you agree this UX.

I totally agree with using the temporary banner to inform the user of image resizing. Its absolutely what i would prescribe.

> The thing I concerned is whether we should have a blocking warning dialog to
> notify user that image will be resized.

No. if you mean should we have a full screen dialogue that informs the user that an image is being resized that the user actively has to cancel in order to proceed. no, certainly not. Any dialogue should be informative and not block the users flow/progress... so use the temporary banner.

let me know if you need to discuss further. We can RFI tyler for copy.

Comment 12

[Kev Needham [:kev]]

The magic number of 300k isn't intended as a system limit, it's a follow-on to the specific requirements from a partner. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=840038#c5 for additional information.

Comment 13

[Corey Frang [:gnarf]]

So we have this concept of a "message too large" screen, we need to know the "limit" - This needs to be defined somewhere.  We are currently using the conveniently named "dom.mms.operatorSizeLimitation" as the size limitation of a MMS we should even try to send.

I propose we set this "setting" to 600k - the LIMIT, and we resize the first two images attached to 2/5th of the "limit" setting, and then again once we reach three images, we resize all of them to 1/5th the "limit".

Sound like it would work?  This way an image could only ever be resized twice during composition, I doubt much quality will be lost because of the extra iteration, we are downsizing after all.

Comment 14

[Steve Chung [:steveck]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #13)
> So we have this concept of a "message too large" screen, we need to know the
> "limit" - This needs to be defined somewhere.  We are currently using the
> conveniently named "dom.mms.operatorSizeLimitation" as the size limitation
> of a MMS we should even try to send.
> 
> I propose we set this "setting" to 600k - the LIMIT, and we resize the first
> two images attached to 2/5th of the "limit" setting, and then again once we
> reach three images, we resize all of them to 1/5th the "limit".
> 
> Sound like it would work?  This way an image could only ever be resized
> twice during composition, I doubt much quality will be lost because of the
> extra iteration, we are downsizing after all.

This idea sounds work, only some question. Why should we need to 600k limitation at first? If that default number is only when the operatorSizeLimitation is not provided by operator, I could agree with that.
About the image resizing mechanism, what if user attached 3 image(resizing to 1/5 message size) and remove 1 or 2  images? Should we resume the size to 2/5? That means we always need to cache the bigger size blob while composing and resize multiple images again in certain scenario. That's the reason why I suggest that we should avoid any extra resizing action to the other appended images.

Comment 15

[Corey Frang [:gnarf]]

I'm saying we set the operatorSizeLimitation(which is currently set to 300k yeah?) as well as the default to 600k, since this is the limit that makes more sense.  300k was never a limit for the operator size, it was actually 600k, but they wanted to fit two images + some text.

As far as resizing back up to 2/5's after going down to 1/5'th, I'm not sure that it will be necessary to do that, though if it doesn't seem too hard to implement, it might be worth it.

Comment 16

[Steve Chung [:steveck]]

Ok. For this implementation, I will handle image resizing while input event triggered. Image will be appended first with original blob(so we don't need to force append to async call) and we will get image attachment content in composeCheck, resize the each image based on current number of images and replace the attachment with resized one. Composer might need to have a new state 'complete' for checking the composer is compressing the image or not. We might also need to disable the send button while compressing. Hi Corey, it's my brief idea about the image resizing, please correct me if I miss something important or it's just way different from your thought, thanks.

Comment 17

[Corey Frang [:gnarf]]

This doesn't sound like a terrible approach, though you'll probably have to use julien for the review as I'll be out this week.

Comment 19

[Alex Keybl [:akeybl]]

Where are we on this blocker schung? Do you think this is possible in the 6/24 timeframe as Leo requested?

Comment 20

[Steve Chung [:steveck]]

(In reply to Alex Keybl [:akeybl] from comment #19)
> Where are we on this blocker schung? Do you think this is possible in the
> 6/24 timeframe as Leo requested?

Sorry for the pending... I already update the patch on github: https://github.com/mozilla-b2g/gaia/pull/9854 , but this behavior change will break many existing test cases, I'm updating the test case and trying to minimize the changes. It should be ok for 6/24 deadline.

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 756484 [details]
Patch for image attachment resizing

Hi Corey, I update the patch and add some test cases for new image attach mechanism. Could you please take a look again? Thanks.

Comment 22

[Leo]

the current patch will work only when the user choose an image which is less than 600K. If its >600K the file too large alert is shown. This is because just after a pick operation we are checking the blob size to show the alert. This will void the whole purpose of having this feature, as user cannot attach any image file which is larger than 600K.

We feel that after pick activity we should resize the image.

Comment 23

[Corey Frang [:gnarf]]

Comment on attachment 756484 [details]
Patch for image attachment resizing

Steve: Please take into account Leo's comment #22 also - The over limit size check should only be done for things that aren't images.

I also added a comment about a stray Compose.lock = true that I don't think is nessecary.

Please r? me again when you update

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 756484 [details]
Patch for image attachment resizing

Hi Corey, I update the patch and set default mms size limit to 600K, this default is for testing and it would be override by carrier customization build settings(If they provide the value).

Comment 25

[Steve Chung [:steveck]]

Hi Corey, the patch has been updated based on your suggestion in https://github.com/mozilla-b2g/gaia/pull/9854#issuecomment-19960951. Could you please review it again? Thanks!

Comment 26

[Corey Frang [:gnarf]]

Comment on attachment 756484 [details]
Patch for image attachment resizing

r=me - Thanks for all the work here Steve, this looks great!

Comment 27

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/dea31788f6da0abcd9e6d83f7dbf26b89c6f8063
v1-train: https://github.com/mozilla-b2g/gaia/commit/08f5ca19fdda8f96c2acb82de13176a6d590047f

Comment 28

[Leo]

With the new patch its not showing the attached image size. It used to show this before.

Is this expected behavior?
Shouldnt we show the file size also?

Comment 29

[Corey Frang [:gnarf]]

I think the "draft" flag might be missing?

We should probably open this (comment #28) up as a new bug.  I'm also not entirely sure it was this patch that caused it.

Comment 30

[Steve Chung [:steveck]]

(In reply to Leo from comment #28)
> With the new patch its not showing the attached image size. It used to show
> this before.
> 
> Is this expected behavior?
> Shouldnt we show the file size also?

We should, and it actually works in my local branch.
I will try to find the root cause, please fire another bug and assign it to me, thanks.

Comment 31

[Corey Frang [:gnarf]]

Created bug 887614 for size issue

Comment 32

[Cody Roesch [:croesch]]

Verified that the message for image compression is present and translated into the correct languages listed below and that the compression process actually occurs.

Verified on Leo 1.1 build with the following variables:

Environmental Variables
Build ID: 20130814041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/15813d776a69
Gaia: dd3959fa74e356a528daa76ffee14c2c728a4b56
Platform Version: 18.1
RIL Version: 01.01.00.019.190

Following languages verified:
Polish, Czech,Dutch, Russian, Greek, Slovak, Romanian,Brazilian Portuguese, Hungarian, German, Spanish, Turkish, Croatian, Serbian, Catalan, Serbian-Latin