983172 - Parsing jpeg header information for downsampling the image for thumbnail

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

+++ This bug was initially created as a clone of Bug #949779 +++

It's follow up for Bug #949779 that we could try to simply parse the jpeg header for image width/height and downsampling the image to thumbnail based on the dimension instead of size to avoid some edge case(jpg image with super high compression ratio, for example).

Comment 1

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, this WIP use some existing API for fetching image width and height. It would be helpful if you could provide some early feedback, but take your time because we are not hurry for this :)

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8390540 [details] [review]
Link to github

Gave a first feedback on github

Comment 3

[Julien Wajsberg [:julienw]]

http://blog.nihilogic.dk/2008/08/imageinfo-reading-image-metadata-with.html could be very useful. Since we don't need the ajax nor the exif stuff, we can probably reduce it a little more. We should also rewrite BinaryFile using DataView [1]. I think DataView can take a blob directly as parameter, but need to check.

[1] https://developer.mozilla.org/en-US/docs/Web/API/DataView

Comment 4

[Julien Wajsberg [:julienw]]

David Flanagan landed recently shared/js/image_utils.js [1] that seems to have no dependency and so we can use it directly here.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/image_utils.js

Comment 5

[Steve Chung [:steveck]]

Attachment: Using image utils for parsing size and resizing

Hi Julien, it's one of my WIP which used Image utils getSizeAndType for original dimension and resizeAndCropToCover for thumbnail. In workload BIG-THREAD-MMS, memory usage increased from ~20MB to the maximum ~28MB while generating thumbnails, and reduce to ~23 after coll down. CPU usage reaches maximum 30~40%. The original memory usage has nearly same value at max, but still keeps at 28MB after cool down, maybe the canvas resource is not released properly in original thumbnail creation. BTW the cpu usage is around 30% at max.

Comment 6

[Steve Chung [:steveck]]

Attachment: Using image utils for parsing size only

Another WIP that only utilize getSizeAndType for image dimension. Here I use the background-size property to fit the thumbnail container. Since this patch won't use canvas for additional resizing, the max memory usage will keep at ~23MB while creating thumbnail, and max CPU usage will be around 20%. The drawback is app will require more memory while handling big size image(If image is still bigger than max thumbnail container size after downsampling). Maybe we can combine 2 solution that resize again when image is too big, but handling the 'oversized' thumbnail that should be less than 1M resolution seems not really a huge effort, do you think we should handle this case for saving memory?

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8466933 [details] [diff] [review]
Using image utils for parsing size only

Review of attachment 8466933 [details] [diff] [review]:
-----------------------------------------------------------------

I like it when we remove so much code !

::: apps/sms/js/attachment_renderer.js
@@ +211,2 @@
>  
>      var sizeAdjuster = function(width, height) {

about this sizeAdjuster, I wonder if we can not ask Jenny to always have 120px for the height; it would be a lot better for rendering as we wouldn't "jump" anymore...

@@ +243,5 @@
> +        fragment = ImageUtils.Downsample.sizeNoMoreThan(scale);
> +        deferred.resolve({
> +          width: adjustedData.width,
> +          height: adjustedData.height,
> +          dataUrl: window.URL.createObjectURL(blob) + fragment

heh it's not a dataUrl anymore (and I like that !).

... but you'll have to revoke the URL at one point !

If the blob comes directly from indexedDB (I think it does) then it should be quite cheap to revoke it when we leave the panel only.

@@ +256,5 @@
> +        });
> +      }
> +    );
> +
> +    return deferred.promise;

Now we don't need the deferred anymore :)

return ImageUtils.getSizeAndType(blob).then(
  function func1() {
    ...
    return {
      width: ...,
      height: ...,
      dataUrl: ...
    };
  }
).catch(function func2() {
  ...
  return {
    width: ...,
    height: ...,
    dataUrl: ...
  };
});

I use a final "catch" instead of the second argumnent to "then" in case sizeNoMoreThan or sizeAdjuster throw something.

::: apps/sms/js/utils.js
@@ -646,5 @@
> -            width: img.width,
> -            height: img.height
> -          };
> -
> -          canvas = Utils.imageToCanvas(

looks like imageToCanvas is not used anymore now

::: apps/sms/style/attachment.css
@@ +137,5 @@
>  
>    background-image: none;
>    background-repeat: no-repeat;
> +  background-position: center center;
> +  background-size: cover;

Now that we use directly the cheap blobs that come from the database, I think we should then use images instead of background images, for these reasons:

* semantic and accessibility: attachments are content and so should be in the DOM
* performance: I think that images memory is being freed when it's out of the screen; this is not something we do for background images (yet?).

But not for this patch, I think.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8466927 [details] [diff] [review]
Using image utils for parsing size and resizing

Review of attachment 8466927 [details] [diff] [review]:
-----------------------------------------------------------------

I prefer the other patch because here we have in-memory blobs instead of blobs coming from a db. This is a lot more expensive in memory.

Comment 9

[Julien Wajsberg [:julienw]]

Wondering if your proposed patch does not fix bug 996516 as well.

Will have to check with Fang/Jenny about the various cases (portrait/landscape images, images with unusual ratios) too.

Comment 10

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Wondering if your proposed patch does not fix bug 996516 as well.
> 
> Will have to check with Fang/Jenny about the various cases
> (portrait/landscape images, images with unusual ratios) too.

I think this patch could fix bug 996516 in better way, so I will close bug 996516 later.
Let's ni? UX here about the thumbnail size for comment 7.

Comment 11

[Jenny Lee]

(In reply to Steve Chung [:steveck] from comment #10)
> (In reply to Julien Wajsberg [:julienw] from comment #9)
> > Wondering if your proposed patch does not fix bug 996516 as well.
> > 
> > Will have to check with Fang/Jenny about the various cases
> > (portrait/landscape images, images with unusual ratios) too.
> 
> I think this patch could fix bug 996516 in better way, so I will close bug
> 996516 later.
> Let's ni? UX here about the thumbnail size for comment 7.

Hi Steve, per discussion, I don't think it's necessary :) IMO, the bubble height change in initial loading is acceptable, whereas using a predefined thumbnail frame size can potentially create weirdly cropped images. Tks!

Comment 12

[Julien Wajsberg [:julienw]]

Jenny, the bubble height change is really painful once you have MMS in the middle of a thread. You can check this quite easily with one of the predefined workloads:

  APP=sms make reference-workload-medium

and load the thread "BIG-THREAD-MMS" in the Messaging app.

You'll see that there are some weird bugs coming because of this, especially sometimes we're not scrolled to the bottom after the full load of a thread. We _could_ fix this but it would be a lot easier to simply not change the height.

As a matter of fact, I don't quite understand why we limit the width to be 120px anyway. The max width for a bubble is about 280/300px (I haven't checked exactly) so we have plenty of space. Most images are 3/2 or 2/3, and the max weirdest ratio would be 16/9, which would mean a max width of 214px for height 120px. It's still below the max width for a bubble.

If I take the maxwidth for a bubble to be 280px (again: I haven't checked) the ratio for 120px height would be 2.33, means 21/9 or 7/3. If an image exceeds this ratio, then we could _reduce_ the height from 120px to something else. This has the following advantages:

* This wouldn't cause the scrolling issue I outlined earlier (because we reduce the height, we don't increase it) (Probably we could do this with the current approach though)
* it would happen a lot less: it would be an edge case, while with the current approach it happens about 50% of the time (each time an image is in portrait).

Comment 13

[Jenny Lee]

Hey Julien, 

Not sure if I understand you correctly, but in short, you'd prefer a fixed-max-height for both landscape and portrait photos? and when loading a thread, preload/reserve the space with max height for the photo to avoid the "jumping" we see right now? 

Also, I checked with Fang, the latest measurement is as follow:
Max bubble width = 265px
Max landscape image width = 150px
Max landscape image height = 85px

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Jenny Lee from comment #13)
> Hey Julien, 
> 
> Not sure if I understand you correctly, but in short, you'd prefer a
> fixed-max-height for both landscape and portrait photos? and when loading a
> thread, preload/reserve the space with max height for the photo to avoid the
> "jumping" we see right now? 

yep that's it ;)

and possibly reduce the space for the edge cases, but for the "normal" cases, not change it.

> 
> Also, I checked with Fang, the latest measurement is as follow:
> Max bubble width = 265px

ok, this does not change that much my arguments ;)

> Max landscape image width = 150px
> Max landscape image height = 85px

I think that's not what we do do in the code right now...

Comment 15

[Jenny Lee]

So after discussed with Mike just now, we propose using fixed size square. We want the overall view to look more clean and organized. Hope this works for you too!
Fang will provide a new visual spec on this. Thanks Fang :)

Comment 16

[Fang Shih [:fang]]

Attachment: 2.1_MessagingThread_VisualSpec_attachment_0819.zip

Attached the new attachment size spec for messaging thread view! Thanks!

Comment 17

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Oleg,
In this patch I utilize the ImageUtils to generate thumbnail and remove the unused utils and test. It definitely  need your help for reviewing the changes since you've done a lot of efforts in this part, and you must know which adjustment is the best for attachment rendering :)

Comment 18

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8485415 [details]
Link to github

(In reply to Steve Chung [:steveck] from comment #17)
> Created attachment 8485415 [details]
> Link to github
> 
> Hi Oleg,
> In this patch I utilize the ImageUtils to generate thumbnail and remove the
> unused utils and test. It definitely  need your help for reviewing the
> changes since you've done a lot of efforts in this part, and you must know
> which adjustment is the best for attachment rendering :)

Thanks for this clean-up! I've left some minor comments on Github, but overall 
it looks and works great!  Removing r? flag only because some changes are still needed to make TBPL happy :)

Thanks!

Comment 19

[Steve Chung [:steveck]]

Hi Fang, could you please confirm the provided image bit depth again? We use 8 bit image originally and it's much smaller than the new attachment sprites which is using 32 bit, and I think it's reasonable that keep the image size if the quality is acceptable. Thanks!

Comment 20

[Fang Shih [:fang]]

Attachment: Assets.zip

Hi Steve, 
I tried to bring down the image to 8 bit, but it affects the quality of transparent icon. So I compressed the images instead. The image size should be smaller now, and looks fine. Let me know if the size works for you guys! Thanks!

Comment 21

[Fang Shih [:fang]]

Attachment: Attachment_Sprite_8bit.zip

Attached is the attachment sprite assets in 8 bit. Thanks! : )

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8485415 [details]
Link to github

Hi Oleg, issue addressed in another commit, could you please revisit again? Thanks.

Comment 23

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8485415 [details]
Link to github

Looks great, r=me! I've left just a few nits at Github + can you please check if patch is related to one strange issue I'm observing currently:

1. Go to the composer and tap on "Add attachment" button;
2. Gallery->Select image->Crop image as much as possible (I've selected "firefox logo" image from reference workload and cropped its width to the right as much as possible keeping original height);
3. Press send;

Actual result: sometime app crashes on the first such mms, sometime on the second..

I wasn't able to reproduce this issue without patch. I'm using the latest PVT build on Flame.

Thanks!

Comment 24

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #23)
> Comment on attachment 8485415 [details]
> Link to github
> 
> Looks great, r=me! I've left just a few nits at Github + can you please
> check if patch is related to one strange issue I'm observing currently:
> 
> 1. Go to the composer and tap on "Add attachment" button;
> 2. Gallery->Select image->Crop image as much as possible (I've selected
> "firefox logo" image from reference workload and cropped its width to the
> right as much as possible keeping original height);
> 3. Press send;
> 
> Actual result: sometime app crashes on the first such mms, sometime on the
> second..
> 
> I wasn't able to reproduce this issue without patch. I'm using the latest
> PVT build on Flame.
> 
> Thanks!

This issue is similar to bug 1059233, and it's only reproducible while image cropped as well.

Actually I just try out a patch that make a local copy by store blob in asyncStorage and get the blob again. Both issues are fixed after this patch applied. Do you prefer solving this issue right in this bug, or address this issue in bug 1059233?

Comment 25

[Oleg Zasypkin [:azasypkin]]

Thanks for investigation! I think that bug 1059233 is more appropriate place for this fix.

Comment 26

[Steve Chung [:steveck]]

In master: 855be6ade407c26e0596e7306a44deebc3f60933

Comment 27

[Francis Lee [:frlee]]

hi Bhavana, 
can you evaluate if this fix can be landed to 2.1? 1076729 needs it

Comment 28

[bhavana bajaj [:bajaj]]

Clearing the NI as the comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1076729#c36 applies here.

Comment 29

[Steve Chung [:steveck]]

Comment on attachment 8485415 [details]
Link to github

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): The original behavior from the begining
[User impact] if declined: User might see blur thread when APZ opened because of high cpu usage(see bug 1123598).
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #29)

> [Risk to taking this patch] (and alternatives if risky): Low

That's not really true, as this patch saw several regressions.
The alternative is fixing the root cause in bug 1123598, especially because the same gecko bug could happen in other situations.

I'd like to to wait how bug 1123598 moves and then we can decide depending on both patches' risk profiles.

Comment 31

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> (In reply to Steve Chung [:steveck] from comment #29)
> 
> > [Risk to taking this patch] (and alternatives if risky): Low
> 
> That's not really true, as this patch saw several regressions.

Actually we saw one regression only.

> The alternative is fixing the root cause in bug 1123598, especially because
> the same gecko bug could happen in other situations.
> 
> I'd like to to wait how bug 1123598 moves and then we can decide depending
> on both patches' risk profiles.

This patch has the advantage of being in the code for some time now so it should still be reasonably safe. Still I think we need to find the root cause in Gecko and decide only then.

Comment 32

[bhavana bajaj [:bajaj]]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> (In reply to Julien Wajsberg [:julienw] from comment #30)
> > (In reply to Steve Chung [:steveck] from comment #29)
> > 
> > > [Risk to taking this patch] (and alternatives if risky): Low
> > 
> > That's not really true, as this patch saw several regressions.
> 
> Actually we saw one regression only.
> 
> > The alternative is fixing the root cause in bug 1123598, especially because
> > the same gecko bug could happen in other situations.
> > 
> > I'd like to to wait how bug 1123598 moves and then we can decide depending
> > on both patches' risk profiles.
> 
> This patch has the advantage of being in the code for some time now so it
> should still be reasonably safe. Still I think we need to find the root
> cause in Gecko and decide only then.

The gecko bug you referred to may not that helpfil un the shortterm. OK to land the gaia patch here?

Comment 33

[Julien Wajsberg [:julienw]]

OK, but please don't forget bug 1081207 otherwise we'll leak blobs.

Comment 34

[Julien Wajsberg [:julienw]]

Bhavana, 

The gecko bug 1123598 just landed and is a one-line patch.

I have mixed feelings between uplifting a one-line gecko bug that just landed and uplifting a bigger gaia patch that landed months ago. Also, it's not sure the gecko bug can be uplited easily, I just asked kats on that bug.

In the end, the decision is yours :D

Comment 35

[bhavana bajaj [:bajaj]]

(In reply to Julien Wajsberg [:julienw] from comment #34)
> Bhavana, 
> 
> The gecko bug 1123598 just landed and is a one-line patch.

ah, clearing the approval here given https://bugzilla.mozilla.org/show_bug.cgi?id=1123598 landed.
> 
> I have mixed feelings between uplifting a one-line gecko bug that just
> landed and uplifting a bigger gaia patch that landed months ago. Also, it's
> not sure the gecko bug can be uplited easily, I just asked kats on that bug.
> 
> In the end, the decision is yours :D