1014955 - [Camera] Migrate v1.3T downsampling patches to v1.4/master

Status: VERIFIED FIXED

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

Description

[Justin D'Arcangelo [:justindarc]]

Need to carry over changes to Camera in v1.3T related to using the new image downsampling API into v1.4 and master (2.0).

Comment 1

[Justin D'Arcangelo [:justindarc]]

David/Russ: Please add a comment in this bug listing the relevant 1.3T patches that need ported over. Thanks!

Comment 2

[David Flanagan [:djf]]

At this point, I think Russ has a better idea of which patches need to be picked up than I do. Russ, will you help Justin here, please?

Comment 3

[Russ Nicoletti [:russn]]

Hi Justin,

These are the 1.3t patches with camera changes:

* https://github.com/mozilla-b2g/gaia/commit/708cfea86d5db38e8adfc9fcc7593f7ef55d1a61 (bug 989290)
* bug 998228 (not sure if this requires a change on our side -- also, it's related to the build so I'm assuming the bug I created for that, bug  1003958, will take care of this change)
* bug 1000448 (again, this is "build" related, probably should be addressed as part of bug  1003958)
* https://github.com/mozilla-b2g/gaia/commit/630d955e8471afb7410cd7cfb2fbe40b213ab175 (bug 1001724)
* https://github.com/mozilla-b2g/gaia/commit/d00f5eb018d0df3495b96ab83f760c5a1f96fff6 (bug 1006882)

Comment 4

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Diego: Please review this patch which is a port of the v1.3t patches for the new resampling capabilities. I'm specifically looking for your input as to whether or not this patch fits in well with the rest of the refactored Camera app on master.

David: I'm also adding you for review since you are more familiar with the resampling API and the work that went into the v1.3t Camera app. I'm looking for your input as to whether or not it looks like I am using the new APIs correctly and to make sure I didn't forget to include anything from the v1.3t patches.

Thanks!

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8433666 [details] [review]
pull-request (master)

Thanks for tackling this, Justin. There's some pretty complicated code to port in those Tarako patches.  I've left a bunch of comments on github.  The main issues that caused the r- are:

1) there needs to be something in the build scripts to deal with GAIA_MEMORY_PROFILE=low

2) I think the patch breaks rotation of video thumbnails

Perhaps you and Diego and I (and maybe Hema) should discuss the priority of this bug and which, if any of the 1.3T features need to be in 2.0.  There are minor optimizations to be made with cropResizeRotate() for thumbnail generation, but for the most part this stuff only matters if we are ever going to run this version of the camera on hardware like Tarako. If we know that is coming, then it makes sense to prepare for it now. But if it is unlikely that Tarakos will every run 2.0, then maybe we don't have to make those changes. Also we need to think separately about the EXIF rotation changes and the low-memory changes.

Comment 6

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8433666 [details] [review]
pull-request (master)

David: I've updated my PR to address some of your concerns. I've made some changes to config/config.js to establish better defaults so we aren't doing unnecessary resizing on non-Tarako devices. In the case of Tarako, if we wanted to run v2.0 on it, we can simply tweak config/config.js and the application will automatically apply resizing where necessary. I also renamed lib/resize-image.js to lib/resize-image-and-save.js as you suggested since we *do* need to persist the image every time we call it. Also, this updated PR should fix the issue with video thumbnail rotation and actually improves the thumbnail generation code to be a bit less messy (our `media` object already contained the correct `metadata` that crop_resize_rotate.js expects).

As far as priority of this bug, I think it should land sooner rather than later. It is much better that we are no longer having to resize images in JS using canvas, so there are definite CPU as well as memory improvements in just that alone. We were having to manually resize images in JS for both activities as well as thumbnail generation. Additionally, if we would end up having to support Tarako for v2.0, this will definitely put us in a much better position.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8433666 [details] [review]
pull-request (master)

I think you probably have a failing test, but once you fix that, I'm okay with landing this.

I've left a number of comments on github about how I think things should be done.  (For example, I don't think you need to worry about maxPreviewSize, but I do think you should have a config specifying whether EXIF rotation needs to be considered.)  But those architectural decisions are probably better made by you and Diego.

We should either land this right away, or back out Russ's gallery patch that broke the camera, or land just enough to fix the broken dependency on downsample.js

Comment 8

[Justin D'Arcangelo [:justindarc]]

David: Thank you for reviewing. The only reason the maxPreviewSize is considered is for low-memory devices (which isn't a concern ATM, but may be in the future). I agree with your comment about obeying the GAIA_MEMORY_PROFILE environment variable, but this would need some additional time to modify our build process in Camera. I think we can do this in a follow-up, however, I think Diego and I are both in agreement that we need to move towards a "profile-based" configuration (e.g.: low, medium, high). So, I think we can tackle that in a follow-up bug that could also contain some refactoring to clean up all the detection code we currently have scattered. I think we have a bug filed for this already but I don't have the bug number handy. But, if Diego is ok with landing this patch with a follow-up for the profile-based configuration, we can land it. Otherwise, we can just back out the offending patch and fix the deps.

Comment 9

[Justin D'Arcangelo [:justindarc]]

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/dbd4e40dd86d0af6bf69a6bb8babee8a5e226be4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures.
Master: https://github.com/mozilla-b2g/gaia/commit/75bfdcb18899c2752d2e3c6c16f98fbc2cafbcc6

https://tbpl.mozilla.org/php/getParsedLog.php?id=41221411&tree=B2g-Inbound

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Updated PR to fix tests failing on TBPL -- carrying R+ forward

Comment 12

[Justin D'Arcangelo [:justindarc]]

I updated the patch and I no longer see TBPL test failures for Camera on Gaia-Try. However, I do see a test failure for Calendar?:

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=551af4dee2bd

This seems to be unrelated to this patch so I'm gonna go ahead and try landing again.

Comment 13

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/5475755df9d5e16a221eb628b963172fc996f95f

djf: Looks like the tree was branched before we could re-land this. Do you need to flag this for approval to uplift to 2.0?

Comment 14

[Justin D'Arcangelo [:justindarc]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #13)
> Landed on master:
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 5475755df9d5e16a221eb628b963172fc996f95f
> 
> djf: Looks like the tree was branched before we could re-land this. Do you
> need to flag this for approval to uplift to 2.0?

Disregard.. kgrandon merged everything from master into v2.0 yesterday since the branching seems to have been premature. Closing now.

Comment 15

[Inder]

Justin -- are you also taking care of uplifting it to 1.4?

Comment 16

[Justin D'Arcangelo [:justindarc]]

(In reply to Inder from comment #15)
> Justin -- are you also taking care of uplifting it to 1.4?

Inder, I can.. this bug needs to be flagged as approval to uplift to v1.4 first though. Setting NI? for djf.

Comment 17

[Preeti Raghunath(:Preeti)]

Nom'ing for 1.4:

David,

Please weigh in with technical risks. If there's a strong case from user perspective and low tech risk, I'd like this to be approved today.

Comment 18

[Mason Chang [Inactive] [:mchang]]

This causes a 30ms start up Camera regression on a flame device:

Gaia: c006aaa4eadc7ea56bc244ad63a1862a37133fbe Gecko: 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
Test: Camera  Median: 593 Mean: 590 Std Dev: 15                                                      

Gaia: 5475755df9d5e16a221eb628b963172fc996f95f Gecko: 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
Test: Camera  Median: 610 Mean: 616 Std Dev: 23                                                      

Gaia: 087409542c9b1d51ccf76f1fe54160c11f042bff Gecko: 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
Test: Camera  Median: 603 Mean: 607 Std Dev: 22                                                      

Gaia: 3553e4fed932419b957e083fdda441358e4eb834 Gecko: 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
Test: Camera  Median: 618 Mean: 616 Std Dev: 16                                                      

Any thoughts on what we can do here?

Comment 19

[Justin D'Arcangelo [:justindarc]]

(In reply to Mason Chang [:mchang] from comment #18)
> This causes a 30ms start up Camera regression on a flame device:
> 
> Gaia: c006aaa4eadc7ea56bc244ad63a1862a37133fbe Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 593 Mean: 590 Std Dev: 15                             
> 
> 
> Gaia: 5475755df9d5e16a221eb628b963172fc996f95f Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 610 Mean: 616 Std Dev: 23                             
> 
> 
> Gaia: 087409542c9b1d51ccf76f1fe54160c11f042bff Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 603 Mean: 607 Std Dev: 22                             
> 
> 
> Gaia: 3553e4fed932419b957e083fdda441358e4eb834 Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 618 Mean: 616 Std Dev: 16                             
> 
> 
> Any thoughts on what we can do here?

This patch requires 3 additional JS files from the shared/js directory which I'm assuming is accounting for the 30ms. Aside from that, no additional code was added to the startup critical path.

Comment 20

[David Flanagan [:djf]]

(In reply to Preeti Raghunath(:Preeti) from comment #17)
> Nom'ing for 1.4:
> 
> David,
> 
> Please weigh in with technical risks. If there's a strong case from user
> perspective and low tech risk, I'd like this to be approved today.

Preeti,

I'd say that the risk of this patch is medium-low. The bulk of the code is already in 1.3T and is presumably being well tested in that release. Because the camera has been completely refactored between 1.3 and 1.4, however, the parts of the patch that tie the app into the new code are different and are less well tested, having just landed in master.

The primary benefit of this patch is that it prepares our camera app code base for use on Tarako-like devices. If a vendor wants to run 1.4 on a low-memory device, or on a device that relies on EXIF orientation like the Tarako does, then having this patch in 1.4 will be critical.  (As far as I know we do not currently have plans to do that: Dolphin has more memory than Tarako, and I'm told it will do real image rotation not EXIF-based rotation.) 

A secondary reason to land this is that there is a corresponding batch of Gallery patches awaiting uplift to 1.4 (already 1.4+). But if we uplift that patch, we will break the camera app, so we need to make at least a minimal change to Camera in 1.4.

I've discussed this with Justin and Diego and the consensus seems to be that we should get this code landed so we're prepared for all possible hardware, rather than waiting and having to uplift something to a device-specific branch.

Overall, I recommend that we make make this 1.4+ and uplift.

Comment 21

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #19)
> (In reply to Mason Chang [:mchang] from comment #18)
> > This causes a 30ms start up Camera regression on a flame device:
> > Any thoughts on what we can do here?
> 
> This patch requires 3 additional JS files from the shared/js directory which
> I'm assuming is accounting for the 30ms. Aside from that, no additional code
> was added to the startup critical path.

Justin,

Is the require() call made on the startup path?  None of the code should be necessary until after a photo is taken, right?

Does r.js allow you to specify that some modules should be dynamically loaded (i.e. an actual XHR done after startup) or does everything get converted to one big string at build time and loaded at startup?

Comment 22

[Justin D'Arcangelo [:justindarc]]

I spoke with mchang about this startup time regression on IRC and it is acceptable for now (until new perf events are added). Also, we are including lazy_loader.js but it is no longer needed. This is likely contributing to some of the startup time. I've filed the following bug to track its removal (patch on its way):

Bug 1022809 - [Camera] Remove lazy_loader.js script tag

Comment 23

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v1.4)

Uplift of patch to v1.4 -- Carrying R+ forward

Comment 24

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8437189 [details] [review]
pull-request (v1.4)

This PR is ready to land on v1.4, however the following patch should land first:

Bug 1002593 - Get Gallery 1.3t image down-sampling changes into master

I will land this on v1.4 as soon as russn is ready to land Bug 1002593

Comment 25

[Mason Chang [Inactive] [:mchang]]

Can we also please land bug 1022809 on 1.4 which should help with start up time?

Comment 26

[Justin D'Arcangelo [:justindarc]]

(In reply to Mason Chang [:mchang] from comment #25)
> Can we also please land bug 1022809 on 1.4 which should help with start up
> time?

The patch for v1.4 in attachment 8437189 [details] [review] already includes the removal of `lazy_loader.js` from `index.html` (was a one-line removal that shouldn't have been in there in the first place; likely a leftover from a bad conflict resolution).

Comment 27

[Jason Smith [:jsmith]]

Comment on attachment 8436024 [details] [review]
pull-request (master)

Clearing the approval flag here - way too late to consider this for 1.4 at this point. This is a feature request from what I can tell.

Comment 28

[Justin D'Arcangelo [:justindarc]]

(In reply to Mason Chang [:mchang] from comment #25)
> Can we also please land bug 1022809 on 1.4 which should help with start up
> time?

Mason: If this happens to not get uplift to v1.4, I'm sure we can get Bug 1022809 uplifted (its one-line removal of a JS include that is no longer used by the app -- zero risk)

Comment 29

[Jason Smith [:jsmith]]

(In reply to Mason Chang [:mchang] from comment #18)
> This causes a 30ms start up Camera regression on a flame device:
> 
> Gaia: c006aaa4eadc7ea56bc244ad63a1862a37133fbe Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 593 Mean: 590 Std Dev: 15                             
> 
> 
> Gaia: 5475755df9d5e16a221eb628b963172fc996f95f Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 610 Mean: 616 Std Dev: 23                             
> 
> 
> Gaia: 087409542c9b1d51ccf76f1fe54160c11f042bff Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 603 Mean: 607 Std Dev: 22                             
> 
> 
> Gaia: 3553e4fed932419b957e083fdda441358e4eb834 Gecko:
> 187405:a2f0e0619332ba4ae1f37aab37bff6e71ffa19ab
> Test: Camera  Median: 618 Mean: 616 Std Dev: 16                             
> 
> 
> Any thoughts on what we can do here?

Mason - Can you file a separate bug for this? This is a followup to this bug, so we should not pollute the original bug with the regression information.

Comment 30

[Preeti Raghunath(:Preeti)]

Ivan

Please have someone from China QA verify this patch.

Tapas,

Please verify with the QRD if this patch looks fine.

Comment 31

[Mason Chang [Inactive] [:mchang]]

From Justin, he already supplied a fix in bug 1022809.

Comment 32

[Justin D'Arcangelo [:justindarc]]

(In reply to Preeti Raghunath(:Preeti) from comment #30)
> Ivan
> 
> Please have someone from China QA verify this patch.
> 
> Tapas,
> 
> Please verify with the QRD if this patch looks fine.

FYI, this patch for v1.4 cannot land without first landing the v1.4 patch for Bug 1002593 (which I believe djf is working on). It might be worthwhile to check what's on v2.0 on the QRD though since both patches have already landed there.

Comment 33

[David Flanagan [:djf]]

This patch can't land on v1.4 until the patch in 1002593 lands in v1.4.  At this point we just have to wait for Travis to be green on the v1.4 pull request in bug 1002593.

Comment 34

[Ivan Tsay (:ITsay)]

Hi Brian,

Since Pei-pei is in MAE, please help to find QA in Taipei office for verifying the patch. Thanks.

Comment 35

[Brian Huang [:brianhuang][:brhuang]]

Hi Al,

Please verify this patch. Thank you.

Comment 36

[David Flanagan [:djf]]

Uplifted to v1.4: https://github.com/mozilla-b2g/gaia/commit/43da85450074e655b0ee83d3d8d5f4fb6b3c473a

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to David Flanagan [:djf] from comment #36)
> Uplifted to v1.4:
> https://github.com/mozilla-b2g/gaia/commit/
> 43da85450074e655b0ee83d3d8d5f4fb6b3c473a

I think this broke a gaia-unit test: https://tbpl.mozilla.org/php/getParsedLog.php?id=41565952&tree=Mozilla-B2g30-v1.4

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reverted on 1.4 in https://github.com/mozilla-b2g/gaia/commit/7fc73d4cb1bece31f50e8ccf6fb98af3984a9ebf to fix the test failure.

Comment 39

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v1.4)

Updated PR to fix `Downsample` global leak in tests -- Carrying R+ forward

Comment 40

[Justin D'Arcangelo [:justindarc]]

Landed on v1.4:

https://github.com/mozilla-b2g/gaia/commit/0539be80035bbf318962b2b2f69d45d9d8da5f7e

Comment 41

[Justin D'Arcangelo [:justindarc]]

Clearing NI? for David since I updated the patch to address the test failure.

Comment 43

[jinchao.wang]

Dear Justin D'Arcangelo:
    The submission  caused a problem:
    Settings > Display > wllpaper pick icon > select "Camera" > take a photo > click "Select" Button ，but the button is not responding.
    Can check it？

Comment 44

[William Hsu [:whsu]]

(In reply to jingchao.wang from comment #43)
> Dear Justin D'Arcangelo:
>     The submission  caused a problem:
>     Settings > Display > wllpaper pick icon > select "Camera" > take a photo
> > click "Select" Button ，but the button is not responding.
>     Can check it？

A side effect?
I have submitted a bug. (Bug 1035082)
Suggest to trace remaining work on Bug 1035082.
Thanks!


* Build information:
 - Gaia      9ec45071c9c6bf921061580363257907e0465e16
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7280e2146451
 - BuildID   20140704000201
 - Version   30.0

Comment 45

[Al Tsai [:atsai]]

Thanks to :whsu. clear ni