Justin D'Arcangelo [:justindarc] Assignee
	Description • 10 years ago

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).

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 1 • 10 years ago

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

	David Flanagan [:djf]
	Comment 2 • 10 years ago

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?

	Russ Nicoletti [:russn]
	Comment 3 • 10 years ago

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)

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 4 • 10 years ago

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!

	David Flanagan [:djf]
	Comment 5 • 10 years ago

Comment on attachment 8433666 [details]
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.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 6 • 10 years ago

Comment on attachment 8433666 [details]
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.

	David Flanagan [:djf]
	Comment 7 • 10 years ago

Comment on attachment 8433666 [details]
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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 8 • 10 years ago

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.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 9 • 10 years ago

Landed in master:

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

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 11 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 12 • 10 years ago

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.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 13 • 10 years ago

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?

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 14 • 10 years ago

(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.

	Inder
	Comment 15 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 16 • 10 years ago

(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.

	Preeti Raghunath(:Preeti)
	Comment 17 • 10 years ago

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.

	Mason Chang [Inactive] [:mchang]
	Comment 18 • 10 years ago

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?

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 19 • 10 years ago

(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.

	David Flanagan [:djf]
	Comment 20 • 10 years ago

(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.

	David Flanagan [:djf]
	Comment 21 • 10 years ago

(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?

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 22 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 23 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 24 • 10 years ago

Comment on attachment 8437189 [details]
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

	Mason Chang [Inactive] [:mchang]
	Comment 25 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 26 • 10 years ago

(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] 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).

	Jason Smith [:jsmith]
	Comment 27 • 10 years ago

Comment on attachment 8436024 [details]
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.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 28 • 10 years ago

(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)

	Jason Smith [:jsmith]
	Comment 29 • 10 years ago

(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.

	Preeti Raghunath(:Preeti)
	Comment 30 • 10 years ago

Ivan

Please have someone from China QA verify this patch.

Tapas,

Please verify with the QRD if this patch looks fine.

	Mason Chang [Inactive] [:mchang]
	Comment 31 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 32 • 10 years ago

(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.

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

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.

	Ivan Tsay (:ITsay)
	Comment 34 • 10 years ago

Hi Brian,

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

	Brian Huang [:brianhuang][:brhuang]
	Comment 35 • 10 years ago

Hi Al,

Please verify this patch. Thank you.

	David Flanagan [:djf]
	Comment 36 • 10 years ago

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

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

(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

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

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 39 • 10 years ago

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 40 • 10 years ago

Landed on v1.4:

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

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 41 • 10 years ago

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

	jinchao.wang
	Comment 43 • 10 years ago

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？

	William Hsu [:whsu]
	Comment 44 • 10 years ago

(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

	Al Tsai [:atsai]
	Comment 45 • 10 years ago

Thanks to :whsu. clear ni