Russ Nicoletti [:russn] Reporter
	Description • 11 years ago

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

	David Flanagan [:djf] 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.

	David Flanagan [:djf] 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.

	Russ Nicoletti [:russn] 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.

	Russ Nicoletti [:russn] Reporter
	Comment 4 • 11 years ago

https://github.com/mozilla-b2g/gaia/pull/18800

	Russ Nicoletti [:russn] 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.

	Russ Nicoletti [:russn] 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.

	Russ Nicoletti [:russn] Reporter
	Comment 9 • 11 years ago

I'm looking into the travis unit test and integration test failures.

	Russ Nicoletti [:russn] 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.

	Russ Nicoletti [:russn] Reporter
	Comment 11 • 11 years ago

Travis is green with latest PR

	David Flanagan [:djf] 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!

	Russ Nicoletti [:russn] 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.

	David Flanagan [:djf] 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.

	Russ Nicoletti [:russn] 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.

	Russ Nicoletti [:russn] 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.

	Russ Nicoletti [:russn] 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.

	David Flanagan [:djf] 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!

	David Flanagan [:djf] 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?

	Hema Koka [:hema]
	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.

	Russ Nicoletti [:russn] Reporter
	Comment 23 • 10 years ago

Unit test fixed (and conflict with downsample.js resolved).

Master: https://github.com/mozilla-b2g/gaia/commit/d2cfef555dabab415085e548ed44c48a99be5c32

	Russ Nicoletti [:russn] 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.

	Hema Koka [:hema]
	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.

	Jason Smith [:jsmith]
	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.

	Jason Smith [:jsmith]
	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.

	Justin D'Arcangelo [:justindarc]
	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.

	Justin D'Arcangelo [:justindarc]
	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.

	Jason Smith [:jsmith]
	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).

	Justin D'Arcangelo [:justindarc]
	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.

	Jason Smith [:jsmith]
	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?

	David Flanagan [:djf] 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.

	Ivan Tsay (:ITsay)
	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.

	David Flanagan [:djf] Assignee
	Comment 35 • 10 years ago

Taking this bug in order to help get it uplifted to 1.4.

	Russ Nicoletti [:russn] 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.

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

Will discuss this on the 2pm call.

	Hema Koka [:hema]
	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

	David Flanagan [:djf] 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.

	Jason Smith [:jsmith]
	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.

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

Ivan

Please have someone from China QA to verify the patch.

Tapas

Please verify on QRD for the same.

	David Flanagan [:djf] 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.

	Ivan Tsay (:ITsay)
	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.

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

Hi Al,

Please verify this patch. Thank you.

	Russ Nicoletti [:russn] Reporter
	Comment 46 • 10 years ago

1.4:

https://github.com/mozilla-b2g/gaia/commit/4dd26c45f70e504c8bd83ee345069c5f56563a4f

	Al Tsai [:atsai]
	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

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)
	Comment 48 • 10 years ago

(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