1065877 - [sprd][dophin][gallery]gallery may be killed when start to edit image

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect, P2)

Description

[jinchao.wang]

Attachment: Jim Corbett (10).JPG

B2G/gaia/apps/gallery/js/ImageEditor.js : 
    // The image editor does not handle EXIF rotation, so if the image has
    // EXIF orientation flags, we alter the image in place before starting
    // to edit it.
    //
    // For low-memory devices like Tarako, CONFIG_MAX_EDIT_PIXEL_SIZE
    // will be set to a non-zero value, and this may cause us to downsample
    // the image before allowing the user to edit it.
    if ((metadata.rotation !== undefined && metadata.rotation) ||
        (metadata.mirrored !== undefined && metadata.mirrored) ||
        CONFIG_MAX_EDIT_PIXEL_SIZE) {
      showSpinner();
      cropResizeRotate(file, null, CONFIG_MAX_EDIT_PIXEL_SIZE || null,
                       null, metadata,
                       function(error, rotatedBlob) {
                         hideSpinner();
                         if (error) {
                           console.error('Error while rotating image:', error);
                           rotatedBlob = file;
                         }
                         startEdit(rotatedBlob);
                       });
    }
    else {
      startEdit(file);
    }

if it is low-memory devices and setted CONFIG_MAX_EDIT_PIXEL_SIZE,it would invoke cropResizeRotate，bug cropResizeRotate function would cost more ram and app will be killed.

Dophin has 256M RAM ,Flame has 512M RAM.
I has a image about 5.9M（5456 x 3632）,it will cost 268M on flame when the highest，and gallery will be killed on dophin.

Comment 1

[jinchao.wang]

[Blocking Requested - why for this release]:

Comment 2

[Hema Koka [:hema]]

Wayne, Assuming you are triaging incoming 1.4 noms for dolphin

Comment 3

[Wayne Chang [:wchang]]

Hi David,

If I understood it correctly, CONFIG_MAX_EDIT_PIXEL_SIZE is used to restrict the size of image that we allow the editor to open. 
In which case we should use it to prevent something as big as 5456 x 3632 to be opened, right?

removing nomination here, if you believe we still have a problem here please renom it.

Comment 4

[David Flanagan [:djf]]

Jingchao,

The sample image you have attached uses EXIF orientation, so in order for the image editor to work correctly we must rotate the image before editing.  The only way to do this is to call cropResizeRotate(). If CONFIG_MAX_EDIT_PIXEL_SIZE is not zero, then cropResizeRotate() will decode the image using #-moz-samplesize so that it does not require so much ram. This 15mp image will probably be decoded at half width and half height, and will result in an image that is less than 4mp when rotated.

If CONFIG_MAX_EDIT_PIXEL_SIZE is set and you are still crashing on Dolphin, please try a lower value to avoid a crash. If it is not set at all, please set it to 3mp or 5mp, or whatever your max_image_pixel_size is.  To set this config variable for your build, edit the gallery.json file in the build directory and set the maxEditPixelSize property.

Comment 5

[David Flanagan [:djf]]

Wayne: I don't think we need to block on this for 1.4 because this can be fixed by setting config variables in the Dolphin build.

But there is a real bug here: if we have not specified a max edit size, we should use the max image size instead, and we're not doing that, so we end up trying to decode the full 15mp image. This crashes the flame with a nightly build on master, too.  So I need to fix this, and I'm going to propose that we uplift the fix all the way to 2.0 because it will affect any large image with EXIF rotation.  (This wasn't caught earlier because typically the only EXIF rotated photos we see are from Tarako and they are small.)

If we do uplift this patch to 2.0, Spreadtrum should consider cherry-picking it into their tree for Dolphin as well.

I suspect there is a similar bug with the max_pick_pixel_size config variable as well.

I'm tempted to request 2.0+ blocking on this on this because it is a stupid error that causes pretty much guaranteed OOMs for certain images and the fix is trivial. Even though this only affects large images with EXIF rotation, any user who is trying to interoperate with a camera that produces images like that will have a very bad experience.  But the threshold for blocking is so high these days that instead I'll just request approval to uplift a fix to 2.1 and 2.0

Comment 6

[David Flanagan [:djf]]

I've verified that picking or sharing the attached image can also cause OOMs for the same basic reason and will include a fix for those cases in my patch.

I've also noticed that the huge size of this image is also sometimes causing the same issue as in bug 1058853. Because the image is so big, gecko sometimes refuses to decode it and the drawImage() call in cropResizeRotate() is throwing an exception that we do not handle. This means that the callback is never invoked, and apps end up hanging with an infinitely spinning spinner.

Since progress on bug 1058853 seems stalled, I might just include Jan's fix in my patch here.

Comment 7

[David Flanagan [:djf]]

And now I've found yet another OOM issue exposed by this really big test image...

The cropResizeRotate() function makes the assumption that the #xywh media fragment crops an image while decoding it and uses less memory than a full decode would require. Apparently, however, it does not do that so if we use it on a big image we use the full amount of memory required to decode the full image.

This manifests with the test image if we select it with a pick activity and then crop in as far as possible. Cropping in that far switches us from using #-moz-samplesize to #xywh within cropResizeRotate and the memory savings that come from downsampling while decoding go away and we OOM.

I should have tested this more carefully when I introduced it. Unfortunately, I don't know of any way to write automated tests that verify memory usage assumptions.

Comment 8

[David Flanagan [:djf]]

Attachment: patch for master

Russ: I'm asking for your review because you did the original reviews for the cropResizeRotate code, I think. Let me know if you're busy with other things, though and I can find a different reviewer if you'd prefer.

This bug report and the attached sample image turned out to be quite productive, and I think I've fixed five different issues in the process of addressing the original bug report.  Here's an explanation of what is in the patch.

1) The first bug is the one originally reported: trying to edit this really large image causes an OOM. That is because we were trying to call cropResizeRotate() on it without specifying a maximum decode size, so the full 18mp were being decoded. The problem is that I should have been using the defualt image size if the max edit size was not defined. The changes in ImageEditor.js fix this bug

2) There is a very similar bug when sharing a large image that has EXIF rotation. If there is not a max pick size defined, we need to use the max image size so we don't decode the entire giant image. The changes in frames.js fix this bug.

3) And the same bug also affects pick activities. The changes in gallery.js fix that.

4) But it turns out that there was another bug with the pick activity and large images. If you crop way, way in on a big image, you can still get an OOM even with the patch for #3 above. This is because I misunderstood the #xywh media fragment when I wrote cropResizeRotate(). I apparently assumed that it would save image memory in the same way that #-moz-samplesize does. It turns out that it does not and that the full image still has to be decoded before the crop is done. So the first set of modifications to shared/js/media/crop_resize_rotate.js fix this bug by using #xywh only on images where the full image size is less than the specified maximum decode size.

5) The bugs above do not always cause OOMs. Sometimes gecko recognizes that it does not have enough memory available to decode the image and this causes an exception to be thrown in cropResizeRotate() while we're trying to copy the image to the canvas. The exception is uncaught, so the callback function never gets called, and we end up with a frozen UI and a spinner that will never stop spinning.  The uncaught exception is the same basic issue behind bug 1058853, so I've included a fix for that bug here as well. This is the try/catch/finally blocks in crop_resize_rotate.js

Comment 9

[David Flanagan [:djf]]

Attachment: patch for 1.4

Jingchao,

This version of the pull request is for the 1.4 branch. Does it fix the bug for you?

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8490622 [details] [review]
patch for master

Requesting uplift of this patch to 2.1 and 2.0

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): I caused these bugs when I wrote the orignal downsample-when-decoding patch and failed to consider the case of really large images, especially really large images that use EXIF orientation.


[User impact] if declined: Large images, especially those that use EXIF orientation like the image (from some Sony hardware) attached have problems with editing, sharing and picking. Sometimes we see OOMs. Other times we see frozen UX, with spinners that spin forever. See comment 8 for a full description.


[Testing completed]: I've tested lightly, and expect to do another round before landing. There are not automated tests for these things because I don't know of a way to write tests that check the peak memory usage of image manipulation.

[Risk to taking this patch] (and alternatives if risky): Not very risky. The changes generally just impose tighter restrictions on the maximum decode size of images. It seems completely obvious to me that we should uplift this to 2.1. 2.0 seems not as obvious, but given the number of issues fixed here, I think we probably should.

[String changes made]: none

Comment 11

[Russ Nicoletti [:russn]]

Comment on attachment 8490622 [details] [review]
patch for master

A couple of comments/questions in the PR. 

Also, given the new understanding of using the #xywh media fragment, I suggest rewording this comment: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/media/crop_resize_rotate.js#L330-L338

Comment 12

[David Flanagan [:djf]]

Updated the PR to address Russ's review comments. This only modifies a comment in the code, so no behavior change.

Before landing, I'm going to verify that this fixes (or partially fixes) bug 1058853.

Comment 13

[David Flanagan [:djf]]

Attachment: The corrupt image from bug 1058853 and 1051764

Comment 14

[David Flanagan [:djf]]

Attachment: a valid image with a corrupt exif preview from bug 1058853

Comment 15

[David Flanagan [:djf]]

I have confirmed that both of these recently attached images cause the gallery to hang forever while scanning when running on master. (Bug 1058853 and bug 1051764).  

With the attached patch, however, the scan completes normally. The fully-corrupt image is correctly rejected and does not appear. And the image with the broken EXIF preview displays correctly.

Comment 16

[David Flanagan [:djf]]

[Blocking Requested - why for this release]:

Wayne,

The patch for this bug fixes bug 1058853, which is marked 1.4+. The work in bug 1058853 seems stalled, so I'm re-nominating this one.  Maybe it is too late to land anything in the 1.4 tree, but if not, there is a PR here ready to go for spreadtrum.

Comment 17

[David Flanagan [:djf]]

Fabrice: another reason to approve this bug for uplift to 2.0 is that it fixes bug 1058853, which was 1.4+.

Comment 18

[David Flanagan [:djf]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/d37950eb09e28aa18d0e01df9ff90574bd4337e0

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8490622 [details] [review]
patch for master

Fabrice: I'm told you're the one who is doing uplift approvals for 2.1. See comment #10 for uplift reasons and note also that this patch fixes a bug that is 1.4+

Comment 21

[Wayne Chang [:wchang]]

Thanks a lot David, we'll take it on 1.4 and yes I think it should get approval on all subsequent branches.

Also ni?:bajaj to check approval

Comment 22

[jinchao.wang]

(In reply to David Flanagan [:djf] from comment #9)
> Created attachment 8490625 [details] [review]
> patch for 1.4
> 
> Jingchao,
> 
> This version of the pull request is for the 1.4 branch. Does it fix the bug
> for you?

Hi David:
    You patch is good,It can share and edit large picture correctly.Thanks for you job.

    But I found another problem and add comment on you github.

gaia/apps/gallery/js/frames.js:
    var button = $('fullscreen-share-button');
    button.classList.add('disabled');
Element id 'fullscreen-share-button' doesn't match any elenment in gallery on mozilla-b2g:v1.4.I suggest to use ‘fullscreenButtons.share.’.

    Thanks!

Comment 23

[jinchao.wang]

Comment on attachment 8490625 [details] [review]
patch for 1.4

Get,the patch works well.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please request v1.4 approval on the patch when you get a chance.

Comment 25

[David Flanagan [:djf]]

(In reply to jingchao.wang from comment #22)
> 
> 
> Hi David:
>     You patch is good,It can share and edit large picture correctly.Thanks
> for you job.
> 
Thanks for testing it.

>     But I found another problem and add comment on you github.
> 
> gaia/apps/gallery/js/frames.js:
>     var button = $('fullscreen-share-button');
>     button.classList.add('disabled');
> Element id 'fullscreen-share-button' doesn't match any elenment in gallery
> on mozilla-b2g:v1.4.I suggest to use ‘fullscreenButtons.share.’.
> 

This is bug 1039988. It is a separate bug, but I agree that we should fix it. I have requested uplift approval for that bug.

Comment 26

[David Flanagan [:djf]]

Comment on attachment 8490622 [details] [review]
patch for master

Bhavana,

See comment 10 for my reasons for wanting to uplift this. And see comment 21 for Wayne's "we'll take this in 1.4" decision. I think he's deferring to you to actually set the approval flag.

I've requested uplift to 2.0 and 2.1 as well, and asked Fabrice for that, but I'd be happy to have you set those flags instead.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/dec422ff5890be5b74a2a606da7a2a78529e7d9f

Comment 28

[bhavana bajaj [:bajaj]]

Comment on attachment 8490622 [details] [review]
patch for master

Approving on2.0 given the memory improvements gained here

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/8d7f2ac85f3154bdb149d67e5c2f9b035f5e4105
v1.4: https://github.com/mozilla-b2g/gaia/commit/d2d0bcbc77c7a3b696aec6d3b4353cc644736585

Comment 30

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/8d7f2ac85f3154bdb149d67e5c2f9b035f5e4105