949941 - [Camera] Replace all memory-backed Blob instances with disk-backed File instances after saving to disk so "pick" activity consumers don't break

Status: RESOLVED FIXED

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

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

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

When handling the resize case of pick activity, we need to use new File([blob], filename) to return a blob with its original file name.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

nominate it as 1.3? since its original bug is 1.3?.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: use new file to create blob with correct filename

The patch is finished but there may be a bug in FilePick.js. I can't test it correctly.

Comment 3

[Marcia Knous [:marcia]]

Lets make sure we fix all the apps affected and get this in the next train (1.4). Not blocking it for 1.3 (if folks think otherwise, we can revisit 1.3 blocker status)

blocking-b2g: 1.3? → 1.4?

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

The gecko patch is in bug 949944.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8347933 [details] [review]
use new file to create blob with correct filename

Since the refactored version of camera landed, this patch is not valid.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Diego,

I may need your help on this bug. I found the structure had been changed at the refactoring. The original thought is to save the filename while _addPictureToStorage and set them to the activity result.

The current design is that confirm.js listens "newimage" event from camera.js and shows the dialog. While user taps select, it uses the blob from newimage to post result. In this flow, we cannot have the "saved" filename for activity result.

Is it possible to add another event, maybe newimagesaved, with filename to camera.js and use it to show the confirm.js or to enable the select button?

I can help to add that if it is ok, or you can steal this bug.

Thanks in advance and nice job of this refactoring.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

I found there is an alternative which makes confirm.js not to listen newimage event but called inside of onNewImage of controllers/camera.js.

Comment 9

[Sri Kasetti]

Diego, Can you take a look at this?
Does refactoring work we did take care of this

Comment 10

[Hema Koka [:hema]]

 Diego, please provide your input here

Comment 11

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

Comment 12

[Diego Marcos [:dmarcos]]

John. Can you confirm if the attached patch solves the issue?

Comment 13

[David Flanagan [:djf]]

Crap. We shouldn't be resizing images in the camera in the first place.

This resizing code was added in Bug 836300 because when setting wallpaper with a pick activity from the camera, we weren't smart enough to resize on our own.  And it looks like the wallpaper setting code in homescreen and settings is still sending us the screen size.

The 1.3 camera appears to just distort the image and force it to the screen aspect ratio.   I hope we're not still doing that in 1.4.

But I guess this is all out of scope and we just need to call the File constructor as this patch does.

Diego: please consider modifying the getContext() call in resize-image.js so it looks like this:

  var context = canvas.getContext('2d', { willReadFrequently: true });

The willReadFrequently thing turns out to improve memory use and speed when the canvas is just being used to resize an image.  Gallery has started doing this just about everywhere and we should do it here, too.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

I cannot test because today's PVT build cannot pick a file from <input type="file">. I think the patch is workable.

Comment 15

[Diego Marcos [:dmarcos]]

Pushed new version of the patch. Do I land it?

Comment 16

[David Flanagan [:djf]]

Diego,

When I first looked at the patch, I thought that this was only an issue for the image resizing case.  And in triage today, I recommended that this be 1.4- because I thought it wouldn't actually affect anything in practice.

Now that I see John's comment about testing with <input type="file">, I realize that the other part of this is that you should return a File instead of a memory-backed Blob in the non-resize case as well. I'm not confident that just setting a name property on a memory-backed Blob will be good enough.  I think that before you return the photo for the pick activity, you need to look up the File you've saved and return the file instead of the in-memory copy of the blob.

So actually, I don't think that this patch is good enough. I'm going to set r- on the patch and restore the 1.4?.

While thinking about this, I've realized that since you hold on to the photo blobs for the filmstrip (or for the previews even after the filmstrip is gone) it will save memory if you replace the in-memory blob with a file-backed blob as soon as you can.  So maybe as soon as you write the photo to device storage, you should read it back in and use that version of the blob.  Otherwise, I worry that camera's memory usage will grow by 200kb (or whatever) for each photo that is taken.

If you just turn the memory-backed blob into a file-backed blob right away, then the activity code that uses that blob should just work as it is now.  (With the File constructor for the resize case).

Alternatively, you can look up the file in device storage right before returning from the activity, if you can somehow communicate the filename to the activity code that displays the confirm dialog.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8381996 [details] [review]
Pull Request

Overriding wilson's review since I now understand the bug more fully.  See my comment above and the comment on github.

Comment 18

[Preeti Raghunath(:Preeti)]

Is this inline with the camera features in 1.4? Is LG going expecting this or going to help fix this?

Comment 19

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #18)
> Is this inline with the camera features in 1.4? Is LG going expecting this
> or going to help fix this?

Nope. This was a followup for naming blobs returned from a pick activity. Probably doesn't need to block.

Comment 20

[David Flanagan [:djf]]

Nothing to do with LG's work. We should fix this for 1.4, but we don't need to block on it.  If I understand correctly, the user-visible symptom is that images picked from the camera end up with a generic name like "blob.jpg" instead of their real name like "IMG_0017.jpg"

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

I am re-noming for v1.4 since e-mail is apparently no longer able to attach images from the camera in bug 981010 because the memory-backed Blob is ending up dead before we can finish processing it.  Platform may have recently-ish stopped keeping the Camera process alive for the e-mail app's benefit in bug 968542 which is on v1.3 and v1.4 now.  I am investigating the platform issues and how they relate to the Blob spec guarantees now so if there's a platform correctness issue, we can also address it.  But in terms of correct fix for gaia and in terms of resource utilization, it seems like the Camera fix is the right fix.  I'm somewhat expecting to hear about a similar problem to that e-mail bug on Tarako, for example.

Diego, are you actively working this still?  You're not assigned right now, but you were never assigned while working on it.

Comment 22

[Jason Smith [:jsmith]]

IRC context:

10:41 AM <jsmith_> asuth: So I'm trying to understand why bug 949941 moved from being known as an enhancement request to becoming required to fix a blocker. Why couldn't we just backout the patch that is forcing bug 949941 to be required?
10:42 AM <asuth> jsmith_: I'm still investigating the underlying platform issues; it seems like the underlying platform change landed over 1 year ago, though, so that will be hard to back out
10:43 AM <asuth> the manifestation of the regression seems like it's either GC improvements or changes to the Gaia system app
10:43 AM <asuth> it's really a question of when the camera app is getting killed
10:43 AM <asuth> I think it's getting killed earlier than it used to
10:43 AM <asuth> and memory-blobs have been messed up since a year ago
10:43 AM <asuth> (again) 
10:44 AM <asuth> jsmith_: in all cases it seems like the easiest means of addressing the problem is to have camera return a File since it's dumb to return a memory-backed blob
10:45 AM <asuth> I have a v1.3 build in process right now to better understand the change in behaviour though, and will chime in on the root e-mail bug and others when I have some more info
10:45 AM <jsmith_> asuth: Ok - I'm going to wait for more info on the investigation then
10:46 AM <asuth> jsmith_: also worth noting, when I reproduced the e-mail bug on v1.4 with a custom build earlier today, the e-mail app actually crashed
10:46 AM <asuth> there was an IPC read problem and that killed the e-mail app for some reason
10:46 AM <asuth> well corrupt IPC read

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Okay, per the logs from my b2g v1.3 foray at https://bugzilla.mozilla.org/show_bug.cgi?id=981010#c4 the only reason e-mail doesn't crash is because of some type of race about the shutdown of the camera app's process.  It may be a deterministic race because of some quirk of what's on the stack, I don't know.

Because a backout of a platform patch that's been landed for >1 year is not likely, and a complete overhaul of the platform Blob machinery for v1.4 is not a great idea at this time either, I strongly suggest we treat this as a v1.4 blocker and land the likely quite small fix.

I am taking this bug and will provide a fix because I heard a rumour that we get free ice cream for fixing blockers.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

Attachment: In both storage.addImage cases, swap out our memory blob with the newly saved File blob

:djf, bugzilla says your review queue is nuts (22), so please feel free to defer this to someone else.  But since you explicitly weighed in on this bug already and I think I implemented what you requested, I'm directing this at you for now.

I've tested the "take a picture from the camera app and stay in the camera app" and the "take a video from the camera app and stay in the camera app" cases in addition to the "take a picture from a pick activity and return to e-mail" case.  They all seem happy.

The major control-flow change here is that the 'newimage' and 'newvideo' emits don't happen until the saves complete.  This is absolutely required for correctness in the image pick case, but in both cases does add a new failure path since a DeviceStorage save failure or load failure will end up emitting an 'error' but not emitting newimage/newvideo.  It seems like the state machine of the camera is simple enough that this is unlikely to cause any serious breakage, but I'm just inferring from using the UI.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 8390036 [details] [review]
In both storage.addImage cases, swap out our memory blob with the newly saved File blob

Andrew,

Thank you! Your code looks good, though I've asked for a comment clarifying one bit of it.

This looks like it fixes the bug you're experiencing with a pick activity to the email app.

Note that this does not fix the case where we resize an image before returning it. The bug was originally filed because there was not a filename associated with the blob in that case. I'm not sure that is relevant anymore. But possibly adding new File() in lib/resize-image.js would be a good idea.  Maybe you can take a look at the obsolete patches and salvage something from there?

Or just convince me that the new File() fix isn't needed.

Setting needinfo for Diego so he is aware that this camera patch may land on master, and that camera-new-feature will have to be rebased.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

I've updated the patch and addressed the review requests: comment removed, comment added, resize-image.js now wraps the resized Blob in a File that propagates the original filename if present.  Also, resize-image.js was failing to revokeObjectURL its URL which may have led to leaking the memory-backed Blob for the extremely limited lifetime of the document.  The resizing code-paths that matter (thumbnailing, video posters) properly revoke the URL so in general this is just a hygiene correctness fix.

Tested MMS attach from camera, wallpaper select from camera, e-mail attach from camera.

I'll be landing once Travis shows green.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

landed in gaia/master:
https://github.com/mozilla-b2g/gaia/pull/17136
https://github.com/mozilla-b2g/gaia/commit/99890f3d9309ed5c696e110533aea5b6281f65d6

needinfo-ing myself for 1.3T patch variant/uplift etc.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: camera_file_blob_v1_3.patch

Here is a partial backport to v1.3.  It turned out not to be significant for what I was working so I did not complete it.  Just putting it here in case it helps for the ultimate backport.

I think the main thing its missing is making resized blobs Files.

Comment 29

[Andrew Sutherland [:asuth] (he/him)]

On the authority of blocking-b2g=1.3T+ bug 988749 that existed only to track the e-mail fall-out from this and get this uplifted:

landed in gaia/v1.3t:
https://github.com/mozilla-b2g/gaia/pull/17136
https://github.com/mozilla-b2g/gaia/commit/99890f3d9309ed5c696e110533aea5b6281f65d6


Thanks to :bkelly for performing most of the backport work.  I verified the reasonably straightforward changes and duplicated my original testing on a Tarako device.  (Which was a major pain.  Is it expected that the permissions and privileges of the pvtbuilds will be messed up and require manual remounting and chmod 777-ing everything?)

The one notable different that I think may exist on v1.3T versus what landed on v1.4 is that in the pre-refactor v1.3T it looks like generated video posters will remain Blobs where they might have been refreshed from the file-system on v1.4.  The patch for v1.4 here was not addressing this problem and I considered it riskier/out-of-scope given the motivating concern was for e-mail.  Also, I'm not sure what the desired poster life-cycle was.

Comment 30

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530