Brogan Zumwalt [Inactive] Reporter
	Description • 9 years ago

Description:
When user tries to attach a cropped image to an MMS, the attached image does not appear to be cropped. The whole, uncropped image is sent.

Repro Steps:
1) Update a Flame to 20150529010201
2) Launch Messages app
3) Compose new message and tap paperclip to attach image from gallery
4) Crop image on image crop screen and press done
5) Tap attached image and select view

Actual:
Attached image is not cropped.

Expected:
Attached image is cropped as per user action.


Environmental Variables:
Device: Flame 3.0
Build ID: 20150529010201
Gaia: e7d268074ee3c9eeb191c2205c0e35992fb3915d
Gecko: f986e55c4e0b
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Repro frequency: 3/3, 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/10742/
See attached: Youtube video clip & logcat
Youtube video link: http://youtu.be/q2WXXDJGnyA

	Peter Bylenga [:PBylenga]
	Comment 1 • 9 years ago

[Blocking Requested - why for this release]:
Functional regression failing a smoke test.

Requesting a window.

	Brogan Zumwalt [Inactive] Reporter
	Comment 2 • 9 years ago

Issue DOES reproduce on Flame 2.2

Attached image is not cropped.

Device: Flame 2.2
Build ID: 20150529002502
Gaia: a57ec5786c9f941c690266bbb26049dbb8482b05
Gecko: 6e9875ef5297
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Issue does NOT reproduce on Flame 2.1

Attached image is cropped as per user action.

Device: Flame 2.1
Build ID: 20150529001200
Gaia: 2304a1f6327c2ccf35d6995ee16f2231ed1f22a3
Gecko: 894528758073
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

	Peter Bylenga [:PBylenga]
	Comment 3 • 9 years ago

Moving nomination to 2.2

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 4 • 9 years ago

I haven't investigated why it occurs.  FYI for Julien and David.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 5 • 9 years ago

Note: This doesn't happen on images taken by Flame's camera. We suspect it could be an image resolution/phone memory based issue.

	Julien Wajsberg [:julienw]
	Comment 6 • 9 years ago

I see this error in the log:
05-29 08:43:50.795  6718  6718 E Gallery : [JavaScript Error: "Image corrupt or truncated." {file: "blob:app://gallery.gaiamobile.org/f0b73b1e-40e5-4630-a983-30b7649a0296#xywh=2838,1987,2410,1949" line: 0}]

I think this is likely in the Gallery side.

Can you please share an image that has the issue?

	David Flanagan [:djf]
	Comment 8 • 9 years ago

Julien is right that this is not an SMS bug. I'm changing the component to Gallery for now. But based on the logcat, it might actually be Core::ImageLib

The photo is a 20mp image from Aries, not a photo from Flame.

The logcat error message is revealing: 

[JavaScript Error: "Image corrupt or truncated." {file: "blob:app://gallery.gaiamobile.org/f0b73b1e-40e5-4630-a983-30b7649a0296#xywh=2838,1987,2410,1949" line: 0}]

Seth: has the #xywh= media fragment been removed from 2.2? If so, it has caused this pretty major regression in the gallery app.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 9 • 9 years ago

Last Working
Device: Flame
BuildID: 20150520183146
Gaia: 5a7f87b1505ba89b586372cbbbe9507d1016c40c
Gecko: 1f6a0d225927
Version: 41.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

First Broken
Device: Flame
BuildID: 20150520185539
Gaia: 5a7f87b1505ba89b586372cbbbe9507d1016c40c
Gecko: edddd285c20c
Version: 41.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1f6a0d225927&tochange=edddd285c20c

Possibly caused by Bug 1154974.

Notes:
1) None of the bugs in the pushlog have been uplifted on v2.2. I re-checked v2.2 and discovered that this issue is reproducible with 319MB memory only. On v3.0, this issue will occur regardless of memory settings. If it's desired I can also find the window on v2.2.

2) I did this window with unrestricted memory because on my last working build I'd run into an issue very similar to bug 1169407 where the preview is all black. It should affect the accuracy of this window on Central.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 10 • 9 years ago

Typo: it should *NOT* affect the accuracy of this window on Central.

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 11 • 9 years ago

Let's find the 2.2 regression window here.

	David Flanagan [:djf]
	Comment 12 • 9 years ago

I can't figure out what is going on with the #xywh media fragment. I think this used to do something useful. But going back as far as 2.1 I don't see it doing anything.

We've got code assuming that it can use xywh to crop a large image without decoding the whole thing. If xywh fails, and the image is 20 megapixels (as it is in this case) then gecko will need 80 megabytes. So it is no surprise to me that it fails to do the crop when memory is low. That failure (with very large image, and very low memory) might to back to 2.1 or before.

I don't get what's going on with the failure on master, but it sounds like #xywh is no longer just being ignored but is now actively causing an error.

I think we might need a gecko patch to fix 3.0 and a gaia patch to fix 2.2.  But before I can do the gaia work, I need to know from Seth whether I should ever expect #xywh to work in 2.2 or whether I should just remove it completely.  I guess I'll go ahead and take this bug for myself, though. Seth can open a new one for fixing the regression caused by 1154974.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 13 • 9 years ago

This issue occurs on the earliest b2g37 build ID 20150112081448 with 319MB memory. There is no window here to find. Removing keyword.

	No-Jun Park [:njpark]
	Comment 14 • 9 years ago

Since it's a smoketest blocker that's been open for past few days, if it can't be fixed soon, could we ask for a backout of bug 1154974? Usually the rule is that smoketest failures should be fixed/backed out within a day (unless the fix is significant that warrants a few day's work, which this bug might be the case)

David, do we need Seth's input to create a fix for this bug?  When can we expect a fix to be available?

Having said that, and looking at the bug, I am less certain now that this should be a ST blocker though.  Naoki & Kevin, what do you think?  Would it make sense to downgrade this bug?

	David Flanagan [:djf]
	Comment 15 • 9 years ago

If I understand correctly, this bug only occurs if you've got a really large jpeg image to test with. I don't think that "put a 20 megapixel image on your device with a 5 megapixel camera" is part of the smoketest, so I don't think that this should be considered a smoketest blocker.

I'm still waiting for more information from Seth, and he is busy with 2.2+ bugs.  I may have time to look at this later today, but can't promise.

	David Flanagan [:djf]
	Comment 16 • 9 years ago

Seth: here's another needinfo for you, but please see comment #8 for the original needinfo.

Is backing out bug 1154974 even an option?  And in https://bugzilla.mozilla.org/show_bug.cgi?id=1154974#c1 you talk about creating a serial number for FileImpl objects. In the gallery, most of the cases you are trying to optimize will be for blobs created with a slice() call on an underlying file. (We slice to extract the EXIF preview from a jpeg file.)

	Brogan Zumwalt [Inactive] Reporter
	Comment 17 • 9 years ago

I suppose this image is a better representative photo. Issue still occurs using this 1920x1080 (2.1MP) image. Image is still full photo and not cropped after sending.

Device: Flame 3.0
Build ID: 20150602055237
Gaia: 6d477a7884273886605049b20f60af5c1583a150
Gecko: 9eae3880b132
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

	David Flanagan [:djf]
	Comment 18 • 9 years ago

I think we may have two completely different bug here.

On master, we may have a regression caused by bug 1154974.

But on 2.2, we've got a low-memory related bug, since 1154974 has not been uplifted to 2.2. It would be interesting to know if this occurs on 2.1 with the small image, or only with the giant image.

I didn't realize that 1154974 was on master only. Given that this is affecting smaller images, and given that bug 1167399 is also blamed on the same bug, we should probably just back out 1154974. I'm not able to do that myself, though.

	David Flanagan [:djf]
	Comment 19 • 9 years ago

Brogan,

Thanks for the new test image. The sample images that this happens for have EXIF previews that are too small to use as previews.  So we are probably using #-moz-samplesize to scale them down. I suspect a bad interaction between #-moz-samplesize and bug 1154974.  If I'm right, then this bug will not occur for PNG images.  You could test this by converting the image from comment #17 to PNG and testing with that.

	Brogan Zumwalt [Inactive] Reporter
	Comment 20 • 9 years ago

After converting ExampleImage to png format the issue no longer occurs. Image in messages displays and is sent as user cropped image, full image is no longer shown as the full image.

Device: Flame 3.0
Build ID: 20150602055237
Gaia: 6d477a7884273886605049b20f60af5c1583a150
Gecko: 9eae3880b132
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

	No-Jun Park [:njpark]
	Comment 21 • 9 years ago

So it is most likely caused by Bug 1154974 based on Comment 20.  Seth, should we back out this patch or can we have a quick fix for this issue?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 22 • 9 years ago

(In reply to David Flanagan [:djf] from comment #8)
> Seth: has the #xywh= media fragment been removed from 2.2? If so, it has
> caused this pretty major regression in the gallery app.

Sorry for the slow response here, but Gecko does not and never did support #xywh. I can assure you that's true, because I'm the person assigned to implement it. =)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 23 • 9 years ago

(In reply to David Flanagan [:djf] from comment #12)
> We've got code assuming that it can use xywh to crop a large image without
> decoding the whole thing.

There is no simple way of doing that. It would be possible to implement using a similar approach to downscale-during-decode, but don't hold your breath. It won't make 3.0, if we ever do implement it.

> I need to know from Seth whether I should
> ever expect #xywh to work in 2.2 or whether I should just remove it
> completely.

You should not even expect it for 3.0. My advice is to remove the code that uses it.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 24 • 9 years ago

(In reply to David Flanagan [:djf] from comment #19)
> So we are probably
> using #-moz-samplesize to scale them down. I suspect a bad interaction
> between #-moz-samplesize and bug 1154974.

Bug 1154974 could interfere with #-moz-samplesize in some cases, yes, because it could merge a #-moz-samplesize load with a non-#-moz-samplesize load.

I can fix this, and I'll write up a patch to do it. Unless this proves to be much harder than anticipated, I am strongly against backing out bug 1154974 over this issue, since it overall drastically improves performance in the Gallery app.

However, just to be clear: #-moz-samplesize is, with 100% certainty, going to be removed before 3.0 ships. It's only still in right now because I'm still fixing bugs and tweaking things related to downscale-during-decode, but at least for JPEG images I expect that work to be over pretty soon.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 25 • 9 years ago

(In reply to David Flanagan [:djf] from comment #16)
> Is backing out bug 1154974 even an option?  And in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1154974#c1 you talk about
> creating a serial number for FileImpl objects. In the gallery, most of the
> cases you are trying to optimize will be for blobs created with a slice()
> call on an underlying file. (We slice to extract the EXIF preview from a
> jpeg file.)

I think it's worth clarifying here: the slices receive a different serial number, and will not get merged with the original blob in the image cache.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 26 • 9 years ago

Here's the patch. This will prevent us from merging image cache entries for
requests whose URIs have different refs. This should eliminate problems not only
with #-moz-samplesize, but also with #-moz-resolution. Both of those require us
to create a new Image object and a new copy of the source data, so we can't
safely merge a request that uses one #-moz-samplesize value with a request that
uses a different #-moz-samplesize value, or with a request that does not use
#-moz-samplesize.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 27 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0675fe7cfdf7

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 28 • 9 years ago

(In reply to David Flanagan [:djf] from comment #12)
> I think we might need a gecko patch to fix 3.0 and a gaia patch to fix 2.2. 

Ack, maybe I should've created a new bug for the Gecko issue. Sorry about that...

	No-Jun Park [:njpark]
	Comment 29 • 9 years ago

(In reply to Seth Fowler [:seth] from comment #28)
> (In reply to David Flanagan [:djf] from comment #12)
> > I think we might need a gecko patch to fix 3.0 and a gaia patch to fix 2.2. 
> 
> Ack, maybe I should've created a new bug for the Gecko issue. Sorry about
> that...

Thanks for the fix! so I'm guessing your fix includes the gecko issue as well, so when it's merged to master, should it be uplifted to 2.2?  (if we can still uplift to 2.2)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 30 • 9 years ago

(In reply to No-Jun Park [:njpark] from comment #29)
> Thanks for the fix! so I'm guessing your fix includes the gecko issue as
> well, so when it's merged to master, should it be uplifted to 2.2?  (if we
> can still uplift to 2.2)

This fix isn't needed on 2.2. Apparently on 2.2 the problem is just a memory-related issue, so probably the bugs blocking bug 1166136 will help.

	No-Jun Park [:njpark]
	Comment 31 • 9 years ago

[Blocking Requested - why for this release]:

Thanks, I'll change the blocking flag to 3.0? based on comment 30

	David Flanagan [:djf]
	Comment 33 • 9 years ago

This is a dupe of 1169055, and 1169055 is spark+, so making this one spark+ instead.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 34 • 9 years ago

Comment on attachment 8614415 [details] [diff] [review]
Don't merge image cache entries for blob URIs with different refs

Switching the reviewer to Timothy because we need a review on this one ASAP due to spark+.

	Timothy Nikkel (:tnikkel)
	Comment 35 • 9 years ago

Comment on attachment 8614415 [details] [diff] [review]
Don't merge image cache entries for blob URIs with different refs

This should be pretty easy to write a test for, no?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 36 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #35)
> Comment on attachment 8614415 [details] [diff] [review]
> Don't merge image cache entries for blob URIs with different refs
> 
> This should be pretty easy to write a test for, no?

Yeah. It can even be a reftest. I'll add one to the final version of the patch.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 37 • 9 years ago

This updated version of the patch includes a test.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 38 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24dae619f58f

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 39 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f873b18b4bf

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 41 • 9 years ago

QAWANTED: please test fix via : https://tools.taskcluster.net/index/artifacts/#gecko.v1.mozilla-inbound.revision.linux.4f873b18b4bfa16f7678316719f82461440f8880.aries-eng/gecko.v1.mozilla-inbound.revision.linux.4f873b18b4bfa16f7678316719f82461440f8880.aries-eng.opt
on aries device.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 42 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/4f873b18b4bf

	Pi Wei Cheng [:piwei] (inactive)
	Comment 43 • 9 years ago

This issue is verified fixed on Aries but NOT on Flame 3.0. However I think it was never established that this issue occurs on Aries.

Bug 1169055 which was duped to this bug is also NOT fixed. I'm going to undupe that bug. STR on bug 1169055 is different from this bug, for this bug we use images from Gallery, and we also crop images, but on 1169055 we use the Camera app instead of Gallery, and we are not cropping images.

For this bug, if I use the image attached on comment 17, then the issue does NOT occur. But if I use an image taken by Aries, the preview shows a completely black image and will occasionally LMK the Messages app.

Verified fixed (but not established that issue occurred at first place) on:
Device: Aries 3.0 Master
BuildID: 20150605092159
Gaia: 7ff9463c63adca8e09558ae70544cb7da3bfd5e5
Gecko: 4f873b18b4bf
Gonk: 3af1ede0d0956cfbf9c549df7cd9a6807a9efdf2
Version: 41.0a1 (3.0 Master) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Verified fixed with select image (comment 17), but broken in another way with other images taken by Aries on:
Device: Flame (KK, full flashed, 319MB)
BuildID: 20150605093045
Gaia: 65369b217faac7d70c1a29100c4208c6d1db16e3
Gecko: 4f873b18b4bf
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0 Master) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

NI the assignee to get attention. I will attach another image that was used for testing (taken by Aries).

	Pi Wei Cheng [:piwei] (inactive)
	Comment 44 • 9 years ago

Please see previous comment

	David Flanagan [:djf]
	Comment 46 • 9 years ago

I think there are two bugs here. The patch that landed should have fixed the primary bug.

Similar symptoms occur if you test with a very large image (like the 20mp one attached above) on a device with low memory (like a 319mb flame).

Pi-Wei: would you check whether this reproduces for you if you have 1024mb on your Flame?

	Pi Wei Cheng [:piwei] (inactive)
	Comment 47 • 9 years ago

David, this issue, as well as the black preview issue does not occur if I use 1024MB Flame.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 48 • 9 years ago

David, what's the next steps based on PiWei's findings?  Are we to file the bug, or will you file the bug?

	Martin Shuman [:Marty]
	Comment 49 • 9 years ago

This issue is Verified Fixed on Flame 3.0.
Attached image is cropped as per user action.

Environmental Variables:
Device: Flame 3.0
Build ID: 20150608160934
Gaia: ea27c4ed5b6083c9e21d233d4804372ac4d5d353
Gecko: e10e2e8d8bf2
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

I'm upgrading bug 1169407 to a smoketest blocker, as we are still seeing the black image issue.

	David Flanagan [:djf]
	Comment 50 • 9 years ago

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #48)
> David, what's the next steps based on PiWei's findings?  Are we to file the
> bug, or will you file the bug?

Let's use bug 1169055 for the memory-related bug.  I'll update that one to make it clear that it should also cover this case.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 51 • 9 years ago

OK, it looks to me like the platform bug here is fixed, and the remaining issue is related to memory usage. Based on that, I'm going to make bug 1166136 block this bug, as that continues to be the metabug for platform-level issues related to excessive memory usage in the Gallery app. It sounds like there are also issues at the Gaia level that David is addressing in bug 1169055, so we're attacking this from multiple directions.

	Johan Lorenzo [:jlorenzo]
	Comment 52 • 9 years ago

Moving the bug to the component where the regression came from.