Closed
Bug 1014955
Opened 10 years ago
Closed 10 years ago
[Camera] Migrate v1.3T downsampling patches to v1.4/master
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect, P1)
Tracking
(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)
People
(Reporter: justindarc, Assigned: justindarc)
References
Details
(Keywords: perf, verifyme, Whiteboard: [c=regression p= s= u=1.4] [caf priority: p1])
Attachments
(2 files, 2 obsolete files)
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).
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!
Flags: needinfo?(rnicoletti)
Flags: needinfo?(dflanagan)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jdarcangelo
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?
Flags: needinfo?(dflanagan)
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)
Flags: needinfo?(rnicoletti)
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!
Attachment #8433666 -
Flags: review?(dmarcos)
Attachment #8433666 -
Flags: review?(dflanagan)
Updated•10 years ago
|
Target Milestone: --- → 2.0 S3 (6june)
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.
Attachment #8433666 -
Flags: review?(dflanagan) → review-
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.
Attachment #8433666 -
Flags: review- → review?(dflanagan)
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
Attachment #8433666 -
Flags: review?(dflanagan) → review+
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.
Updated•10 years ago
|
Attachment #8433666 -
Flags: review?(dmarcos) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Landed in master: https://github.com/mozilla-b2g/gaia/commit/dbd4e40dd86d0af6bf69a6bb8babee8a5e226be4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•10 years ago
|
||
Updated PR to fix tests failing on TBPL -- carrying R+ forward
Attachment #8433666 -
Attachment is obsolete: true
Attachment #8436024 -
Flags: review+
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.
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?
Flags: needinfo?(dflanagan)
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(dflanagan)
Resolution: --- → FIXED
Comment 15•10 years ago
|
||
Justin -- are you also taking care of uplifting it to 1.4?
Flags: needinfo?(jdarcangelo)
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.
Flags: needinfo?(jdarcangelo) → needinfo?(dflanagan)
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.
blocking-b2g: --- → 1.4?
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?
Flags: needinfo?(jdarcangelo)
Keywords: perf,
regression
Priority: -- → P1
Whiteboard: [c=regression p= s= u=]
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Flags: needinfo?(jdarcangelo)
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.
Flags: needinfo?(dflanagan)
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?
Flags: needinfo?(jdarcangelo)
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
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(jdarcangelo)
Resolution: --- → FIXED
Updated•10 years ago
|
Attachment #8436024 -
Flags: approval-gaia-v1.4+
Assignee | ||
Comment 23•10 years ago
|
||
Uplift of patch to v1.4 -- Carrying R+ forward
Attachment #8437189 -
Flags: review+
Whiteboard: [c=regression p= s= u=] → [c=regression p= s= u=] [CR 673864]
Updated•10 years ago
|
Whiteboard: [c=regression p= s= u=] [CR 673864] → [caf priority: p1][c=regression p= s= u=] [CR 673864]
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
Comment 25•10 years ago
|
||
Can we also please land bug 1022809 on 1.4 which should help with start up time?
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).
No longer blocks: 1022809
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.
Attachment #8436024 -
Flags: approval-gaia-v1.4+
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)
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.
Flags: needinfo?(mchang)
Keywords: perf,
regression
Whiteboard: [caf priority: p1][c=regression p= s= u=] [CR 673864] → [caf priority: p1][CR 673864]
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.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(tkundu)
Flags: needinfo?(itsay)
Whiteboard: [caf priority: p1][CR 673864] → [caf priority: p1][c=regression p= s= u=] [CR 673864]
Comment 31•10 years ago
|
||
From Justin, he already supplied a fix in bug 1022809.
Flags: needinfo?(mchang)
Updated•10 years ago
|
Keywords: perf,
regression
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.
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.
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.
Flags: needinfo?(itsay) → needinfo?(brhuang)
Comment 35•10 years ago
|
||
Hi Al, Please verify this patch. Thank you.
Flags: needinfo?(brhuang) → needinfo?(atsai)
Comment 36•10 years ago
|
||
Uplifted to v1.4: https://github.com/mozilla-b2g/gaia/commit/43da85450074e655b0ee83d3d8d5f4fb6b3c473a
Updated•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
Flags: needinfo?(dflanagan)
Reverted on 1.4 in https://github.com/mozilla-b2g/gaia/commit/7fc73d4cb1bece31f50e8ccf6fb98af3984a9ebf to fix the test failure.
Assignee | ||
Comment 39•10 years ago
|
||
Updated PR to fix `Downsample` global leak in tests -- Carrying R+ forward
Attachment #8437189 -
Attachment is obsolete: true
Attachment #8439241 -
Flags: review+
Updated•10 years ago
|
Flags: needinfo?(tkundu)
Whiteboard: [caf priority: p1][c=regression p= s= u=] [CR 673864] → [caf priority: p1][c=regression p= s= u=]
Assignee | ||
Comment 40•10 years ago
|
||
Landed on v1.4: https://github.com/mozilla-b2g/gaia/commit/0539be80035bbf318962b2b2f69d45d9d8da5f7e
Assignee | ||
Comment 41•10 years ago
|
||
Clearing NI? for David since I updated the patch to address the test failure.
Flags: needinfo?(dflanagan)
Updated•10 years ago
|
Updated•10 years ago
|
Keywords: perf
Whiteboard: [caf priority: p1][c=regression p= s= u=] → [c=regression p= s= u=1.4] [caf priority: p1]
Updated•10 years ago
|
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
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?
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
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•