Closed
Bug 1002593
Opened 11 years ago
Closed 10 years ago
Get Gallery 1.3t image down-sampling changes into master
Categories
(Firefox OS Graveyard :: Gaia::Gallery, defect)
Tracking
(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: rnicoletti, Assigned: djf)
References
Details
Attachments
(2 files, 3 obsolete files)
This bug is for taking the Gallery, and shared/js/media, changes from bug 989290 and getting them into master (and possibly into 1.4).
Updated•11 years ago
|
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Comment 1•11 years ago
|
||
Russ, But 989290 caused a regression in bug 1003036. So please pick up the changes in that latter bug as part of this one as well so we don't end up with the same regression on master.
Assignee | ||
Comment 2•11 years ago
|
||
Russ, The build test failures you're seeing on Travis must somehow be related to the build system changes in bug 897352. That landed a few weeks ago, though, so I don't know why the test failure is only showing up now. Maybe newer changes have landed just recently? The test that is failing is incredibly brittle. It just checks that the generated config.js file is byte-for-byte as expected. It would be fine with me to disable the test if there is not a good way to replace it with something better. But I don't want to disable it without understanding why it is failing. I don't know anything at all about the new build system, so I'm going to leave this to you to sort out, since I've got other 1.3t stuff to work on.
Assignee | ||
Updated•11 years ago
|
Summary: Get Gallery changes for bug 989290 into master → Get Gallery changes for bug 989290 and bug 1003036 into master
Reporter | ||
Comment 3•11 years ago
|
||
The build test is failing because the gallery config.js is not being generated when building the integration tests. I also see this on my box when building integration tests in my branch -- build_stage/gallery/js/config.js is not created. It seems reasonable that the lack of the gallery config.js is causing the gallery integration tests to fail. Interestingly, in my master branch the gallery config.js file is created -- and the gallery integration tests pass.
Reporter | ||
Comment 4•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/pull/18800
Attachment #8415535 -
Flags: review?(dflanagan)
Reporter | ||
Comment 5•11 years ago
|
||
Correction to comment #3. The problem is config.js is missing from profile-test/webapps/gallery.gaiamobile.org/application.zip. In addition, imagesize.js and metadata_scripts.js are also absent from the gallery application.zip in my Bug-1002593 branch whereas they exist in the application.zip of my master branch.
Reporter | ||
Comment 6•11 years ago
|
||
The reason for the missing config.sys (and other gallery js files) from application.zip was that the bug 989290 changes involved moving apps/gallery/js/imagesize.js to shared/js/media/image_size.js. The gallery's build.js was not updated as part of the bug and therefore the gallery's build was aborted when trying to copy imagesize.js from apps/gallery/js. I fixed the build.js and updated the PR. The travis build is going on, has passed the build test where it had failed earlier.
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 8•11 years ago
|
||
Attachment #8415535 -
Attachment is obsolete: true
Attachment #8415535 -
Flags: review?(dflanagan)
Attachment #8420393 -
Flags: review?(dflanagan)
Reporter | ||
Comment 9•11 years ago
|
||
I'm looking into the travis unit test and integration test failures.
Reporter | ||
Comment 10•11 years ago
|
||
Unit test failure was in camera, media_frame_test.js. The problem was that I didn't update any camera files as part of this bug as the camera changes will be handled by camera peers as they are complex (camera code in master has been re-designed). However, media_frame.js was changed in v1.3t which requires media_frame_test.js changes as well. So, I have made the media_frame_test.js changes per bug 1001724. PR has been updated.
Updated•11 years ago
|
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Reporter | ||
Comment 12•11 years ago
|
||
Attachment #8420393 -
Attachment is obsolete: true
Attachment #8420393 -
Flags: review?(dflanagan)
Attachment #8423317 -
Flags: review?(dflanagan)
Assignee | ||
Comment 13•10 years ago
|
||
Russ, This patch is at the top of my review queue, finally. What sort of testing have you done on it? A couple of things I'm nervous about: - I added spinners in a bunch of places for tarako. We probably don't want those showing up on other faster devices. I think they probably only appear when we need to downsample or rotate an image, and we shouldn't have to do that. Could you check on that? - The 1.4/2.0 camera is completely different than 1.3, and uses MediaFrame (for photo previews) in a different way that the 1.3 camera does. So that's another area I want to test carefully to make sure we didn't break. Do you have a nexus 4? If so, you could also verify that we're no longer decoding the full image on that device (it has embedded EXIF previews that are too small) and that previews appear more quickly with this patch. - It would also be very interesting to know how this patch affects image scanning time for one of the reference workloads. I think the images used there don't have EXIF previews, so the downsampling changes here ought to have a noticeable effect on speed. Also: could you make a list of the camera app changes we made on Tarako so that we can ask Justin or Diego about porting those changes to master? (Is there already a bug filed for that?) Thanks!
Flags: needinfo?(rnicoletti)
Reporter | ||
Comment 14•10 years ago
|
||
Regarding testing spinners, I'd like to confirm the Tarako vs. non-Tarako behavior: * Tarako doesn't generate EXIF preview whereas most other devices do (buri included) and therefore only on Tarako do we have the issue of needing to downsample a large file to generate a preview -- so in the case of displaying a preview only on Tarako would we need a spinner. * The assumption is CONFIG_MAX_IMAGE_PIXEL_SIZE, etc., are only set for Tarako and therefore we don't expect the code that uses these configuration values to downsample images except on Tarako. * ImageEditor will show spinner if we need to rotate image, regardless of the device. Regarding testing camera on nexus 4: I have a buri, dolphin, and flame. I don't have a nexus 4. Perhaps Justin or Diego can test camera behavior when they port the camera-specific changes. I will run a reference workload test with and without the patch. I have not created a bug for porting the camera app changes to master. I don't believe there is one. If not, I will create one and provide the list of Tarako changes to camera app.
Flags: needinfo?(rnicoletti)
Updated•10 years ago
|
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8423317 [details] Pull request: https://github.com/mozilla-b2g/gaia/pull/19295 r- because the build system changes that handle GAIA_MEMORY_PROFILE need to be ported. Also, I think there was one minor merge conflict resolution error, and a few other minor things noted on github. Overall, though, this looks good to me. I haven't tried testing it all out yet, however.
Attachment #8423317 -
Flags: review?(dflanagan) → review-
Reporter | ||
Comment 16•10 years ago
|
||
Regarding the scanning time of the reference workloads, the reference workload images *do* have EXIF previews but they are not big enough to be used as previews, which amounts to the same as not having EXIF previews in terms of having to create one from the fullsize image. In any event, the scanning time with and without the patch using reference workloads is basically the same. I used the 'medium' reference workload (50 images). With the patch the average scan time of three runs was 56 seconds. Without the patch the average scan time was 58 seconds.
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8423317 [details] Pull request: https://github.com/mozilla-b2g/gaia/pull/19295 David, I've addressed your comments in the PR and have updated the PR. Btw, it is the top-level Makefile that exports command-line variables to the 'config' object. You had made that change in 1.3t, I didn't notice it when porting the changes to master. Anyway, the PR reflects that change now.
Attachment #8423317 -
Flags: review- → review?(dflanagan)
Reporter | ||
Comment 19•10 years ago
|
||
Lastly, I fixed some problems with selecting images during pick activities and ending crop functionality. The changes are reflected in the PR. By creating a gallery.json in the 'distribution' directory, I've tested the image and filesize limits wrt CONFIG_MAX_IMAGE_PIXEL_SIZE/CONFIG_MAX_PICK_PIXEL_SIZE. I've tested pick activity flow, cropping, and 'open' flow (view image from other apps). As far as I am concerned, the patch has been fully tested.
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8423317 [details] Pull request: https://github.com/mozilla-b2g/gaia/pull/19295 r+ if you fix the failing build test. I think you just need to change the expectedScript variable at build/tests/integration/build.test.js:340 to match the new form of the config.js file. I really don't like that test because it is so brittle!
Attachment #8423317 -
Flags: review?(dflanagan) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Hema, I think we need to get this into 1.4 because otherwise the Tarako will have features (the ability to display large images) that the Dolphin does not. It is not exactly a blocker, so I don't know if I should request 1.4? on it, or approval-gaia-1.4? Can you do the appropriate thing here?
Flags: needinfo?(hkoka)
Comment 22•10 years ago
|
||
(In reply to David Flanagan [:djf] from comment #21) > Hema, > > I think we need to get this into 1.4 because otherwise the Tarako will have > features (the ability to display large images) that the Dolphin does not. It > is not exactly a blocker, so I don't know if I should request 1.4? on it, or > approval-gaia-1.4? Can you do the appropriate thing here? I will approve it for 1.4 once failing build script is fixed and landed on master.
Reporter | ||
Comment 23•10 years ago
|
||
Unit test fixed (and conflict with downsample.js resolved). Master: https://github.com/mozilla-b2g/gaia/commit/d2cfef555dabab415085e548ed44c48a99be5c32
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
status-b2g-v1.4:
--- → affected
Flags: needinfo?(hkoka)
Reporter | ||
Comment 24•10 years ago
|
||
Sheriffs: Please don't uplift this patch to 1.4 until bug 1014955 is landed in master. This patch introduces a shared resource that is required by gallery and camera. The patch takes care of the changes to gallery but not to camera. But 1014955 will address the camera changes.
Depends on: 1014955
Comment 25•10 years ago
|
||
Approving for 1.4 (depends on bug 1014955 to land as well). Got okay from Chris Lee from product point of view.
Updated•10 years ago
|
Attachment #8423317 -
Flags: approval-gaia-v1.4+
Comment 26•10 years ago
|
||
Comment on attachment 8423317 [details] Pull request: https://github.com/mozilla-b2g/gaia/pull/19295 This is not something that is a product call. We just don't add random features into a release that's trying to stabilize that has minimal QA support active right now. We know there's regression risk here as well given that there was a smoketest regression fallout. There really needs to be a really strong compelling reason why this is needed in 1.4 specifically to be considered.
Attachment #8423317 -
Flags: approval-gaia-v1.4+
Comment 27•10 years ago
|
||
(In reply to David Flanagan [:djf] from comment #21) > Hema, > > I think we need to get this into 1.4 because otherwise the Tarako will have > features (the ability to display large images) that the Dolphin does not. It > is not exactly a blocker, so I don't know if I should request 1.4? on it, or > approval-gaia-1.4? Can you do the appropriate thing here? Just because something is supported on Tarako does not necessarily make it a compelling case to take the patch into Dolphin. Dolphin has far more memory in comparison to Tarako, so there really needs a blocking case here made why Dolphin needs this.
Comment 28•10 years ago
|
||
On Tarako, we have the platform-supported downsampling capability and we are still seeing issues with OOM crashes when using the "picker" to attach photos to 3rd-party apps (e.g.: Facebook). So, not having this patch would only make the situation with that use case even worse.
Comment 29•10 years ago
|
||
Also, the regression that occurred on master a few days ago was a result of a change made to one of the "shared/js" libs that is only used by Gallery and Camera. The patch landed that changed the shared lib *before* the patch was landed in Camera to fix the dependency issue. So, it was really just a regression due to an additional required dependency for Camera and not new broken code.
Comment 30•10 years ago
|
||
(In reply to Justin D'Arcangelo [:justindarc] from comment #28) > On Tarako, we have the platform-supported downsampling capability and we are > still seeing issues with OOM crashes when using the "picker" to attach > photos to 3rd-party apps (e.g.: Facebook). So, not having this patch would > only make the situation with that use case even worse. But why is this specifically needed for Dolphin? Dolphin is a 256 MB device. We've shipped without this implemented previously in a 256 MB device setup (e.g. Buri in 1.3).
Comment 31•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #30) > But why is this specifically needed for Dolphin? Dolphin is a 256 MB device. > We've shipped without this implemented previously in a 256 MB device setup > (e.g. Buri in 1.3). True, but I am unsure of the camera hardware resolution on Dolphin. If it is higher than Buri/Hamachi, then it would be likely to also have OOM issues with the pick activity. Without this patch, resizing of images for pick activities has to be done in JS which requires decompressing the full-size image from the camera into memory (via canvas). If the camera is a higher resolution, it will require a significant amount of memory to store the raw bitmap data for the resize operation. It can also be painfully slow to perform this operation in JS, whereas the platform is capable of resizing the JPEG as it is read from disk.
Comment 32•10 years ago
|
||
(In reply to Justin D'Arcangelo [:justindarc] from comment #31) > (In reply to Jason Smith [:jsmith] from comment #30) > > But why is this specifically needed for Dolphin? Dolphin is a 256 MB device. > > We've shipped without this implemented previously in a 256 MB device setup > > (e.g. Buri in 1.3). > > True, but I am unsure of the camera hardware resolution on Dolphin. If it is > higher than Buri/Hamachi, then it would be likely to also have OOM issues > with the pick activity. Without this patch, resizing of images for pick > activities has to be done in JS which requires decompressing the full-size > image from the camera into memory (via canvas). If the camera is a higher > resolution, it will require a significant amount of memory to store the raw > bitmap data for the resize operation. It can also be painfully slow to > perform this operation in JS, whereas the platform is capable of resizing > the JPEG as it is read from disk. Sounds like there's some open questions here then. Ivan - How does the camera resolution compare on Buri vs. Dolphin?
Flags: needinfo?(itsay)
Assignee | ||
Comment 33•10 years ago
|
||
Jason: there is one very specific reason that we need this in 1.4. Along with the low-memory improvements I made for Tarako I was also able to use the downsampling ability to add a new feature in 1.3T. In that release the gallery app can display large (> 10megapixel) JPEG files. This is a feature that we've wanted for a long time, but have never been able to lift the image size limit. With this patch, we can now. Tarako devices are going to have the ability to display large images (like photos from high-end smartphones transferred via bluetooth.) If we don't get this patch into 1.4, then Tarako's big brother, Dolphin, will not have that feature, and users will be confused and angry. It is not possible to separate the feature from the rest of the patch: far less risky to take the full patch, which is now being tested on 1.3T and on master, than it would be to try to take just some of it. Gallery has changed very little over recent releases, so there is not much risk of regressions when uplifting this from master to 1.4. Also CAF really wants the corresponding set of Camera patches in 1.4, and they can't have those unless this patch also lands. So I think those are two very specific reasons to land this patch.
Flags: needinfo?(jsmith)
Comment 34•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #32) > .... > > Sounds like there's some open questions here then. > > Ivan - How does the camera resolution compare on Buri vs. Dolphin? Buri has 3.2 MPixel resolution on Camera. I ni? Steven for confirming the camera spec. on Dolphin. Steven, please help on Jason's question.
Flags: needinfo?(itsay) → needinfo?(styang)
Assignee | ||
Comment 35•10 years ago
|
||
Taking this bug in order to help get it uplifted to 1.4.
Assignee: rnicoletti → dflanagan
Reporter | ||
Comment 36•10 years ago
|
||
The patch I created for this bug is for master and does not apply cleanly to 1.4. I am creating a 1.4 patch for this bug. I should have a PR for the 1.4 patch in a couple of hours.
Comment 38•10 years ago
|
||
In addition to comment 33 from djf that basically summarizes the reasons/risk/impact: CAF is seeing OOMs in their testing for 8610 that has RAM restrictions of 256 MB. And want this landed in 1.4 In a related note, it will help if QA can also provide more info on what related tests have been run on 1.4 gallery/camera and whether they are seeing OOM issues on dolphin/buri or not (simiar to what CAF is seeing on their test devices). Also regarding previous question on camera resolution for dolphin: Per specs online, it Supports Camera sensor 3MPixel of YUB, 5Mpixel of JPEG format Thanks Hema
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(jsmith)
Assignee | ||
Comment 39•10 years ago
|
||
Russ is working on creating a PR for 1.4. There are some tricky merge conflicts though, so I've created my own version of that PR, so that Russ and I can check each other's work and be sure we've both resolved the conflicts the same way.
Comment 40•10 years ago
|
||
Talked w/djf, hema, and preeti about this - upon further discussion, I'm okay with the uplift here so long as we do the verification on Dolphin on 1.4 & QRD on 1.4.
blocking-b2g: --- → 1.4+
Flags: needinfo?(jsmith)
Comment 41•10 years ago
|
||
Ivan Please have someone from China QA to verify the patch. Tapas Please verify on QRD for the same.
blocking-b2g: 1.4+ → ---
Flags: needinfo?(tkundu)
Flags: needinfo?(itsay)
Updated•10 years ago
|
blocking-b2g: --- → 1.4+
Assignee | ||
Comment 42•10 years ago
|
||
Comment on attachment 8437882 [details] pul request for v1.4 Russ has verified his version of the pull request against mine, and has found errors in mine, so I'm marking it as obsolete. Russ will attach his PR here, and once Travis is green we will uplift this (and also bug 1014955) to v1.4.
Attachment #8437882 -
Attachment is obsolete: true
Reporter | ||
Comment 43•10 years ago
|
||
Comment 44•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 45•10 years ago
|
||
Hi Al, Please verify this patch. Thank you.
Flags: needinfo?(brhuang) → needinfo?(atsai)
Reporter | ||
Comment 46•10 years ago
|
||
1.4: https://github.com/mozilla-b2g/gaia/commit/4dd26c45f70e504c8bd83ee345069c5f56563a4f
Updated•10 years ago
|
Updated•10 years ago
|
![]() |
||
Comment 47•10 years ago
|
||
Verified fixed. Gaia 765438e65bc23bbda6c82b4093edcd70eb58e8ad Gecko https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d17ad4e49b34 BuildID 20140611160200 Version 30.0 ro.build.version.incremental=130 ro.build.date=Fri May 30 14:52:38 CST 2014
Updated•10 years ago
|
status-b2g-v2.0:
--- → fixed
(In reply to Al Tsai [:atsai] from comment #47) > Verified fixed. > > Gaia 765438e65bc23bbda6c82b4093edcd70eb58e8ad > Gecko https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d17ad4e49b34 > BuildID 20140611160200 > Version 30.0 > ro.build.version.incremental=130 > ro.build.date=Fri May 30 14:52:38 CST 2014 I tested both [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1002593#c46 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1014955#c39 This does not resolve bug 1024692 but it introduces little delay (~300ms) when we tap any image in gallery to display it . There may be some memory optimization but it is still not enough for bug 1024692
Flags: needinfo?(tkundu)
Updated•10 years ago
|
Flags: needinfo?(styang)
You need to log in
before you can comment on or make changes to this bug.
Description
•