Last Comment Bug 834773 - Using the pick activity and confirming an image for the gallery - app gets hung with no perceived perf for 5 - 10 seconds
: Using the pick activity and confirming an image for the gallery - app gets hu...
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::Gallery (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: ---
Assigned To: David Flanagan [:djf]
: John Hammink
Mentors:
: 839630 890477 (view as bug list)
Depends on: 856908
Blocks: 832923
  Show dependency treegraph
 
Reported: 2013-01-25 10:28 PST by Jason Smith [:jsmith]
Modified: 2013-07-27 06:50 PDT (History)
12 users (show)
cschmoeckel: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
leo+
+
fixed
wontfix
wontfix


Attachments
link to patch on github (108 bytes, text/html)
2013-03-26 15:41 PDT, David Flanagan [:djf]
dominickuo: review+
Details

Description Jason Smith [:jsmith] 2013-01-25 10:28:36 PST
Build: B2G 18 1/25/2013
Device: Unagi

Steps:

1. Go to http://robnyman.github.com/Firefox-OS-Boilerplate-App/
2. Select Pick Image
3. Select Gallery
4. Select a picture
5. Confirm it in the crop UI

Expected:

If we're loading something, we should give the user clear indication that something is being loaded.

Actual:

We get hung for a few seconds (5 - 10). This is poor perceived performance - the user will think the app is hung, when in reality, there's a lot of loading happening behind the scenes.
Comment 1 David Flanagan [:djf] 2013-01-25 10:40:17 PST
I don't know what that website is doing.... Do you see a similar hang when picking an image from the Contacts app?  If not, this could be a bug in the website that launches the activity.

There was a crash in the pick image activity. A gecko issue, I think. I wonder if this could be related?
Comment 2 Jason Smith [:jsmith] 2013-01-25 10:48:11 PST
(In reply to David Flanagan [:djf] from comment #1)
> I don't know what that website is doing.... Do you see a similar hang when
> picking an image from the Contacts app?  If not, this could be a bug in the
> website that launches the activity.

Nope, I don't get this hang when I pick an image from the contacts app. It happens almost instantaneously. It could be a different gecko issue potentially. Here's the code I'm seeing:

    var pickImage = document.querySelector("#pick-image");
    if (pickImage) { 
        pickImage.onclick = function () {
             var pick = new MozActivity({
                 name: "pick",
                 data: {
                     type: ["image/png", "image/jpg", "image/jpeg"]
                 
}
             });

            pick.onsuccess = function () {

                var img = document.createElement("img");
                img.src = window.URL.createObjectURL(this.result.blob);
                var imagePresenter = document.querySelector("#image-presenter");
                imagePresenter.appendChild(img);
                imagePresenter.style.display = "block";
            };

            pick.onerror = function () {

                alert("Can't view the image!");
            };
        }
    }

Doesn't look like a lot is going on here. Maybe a perf bug with createObjectURL? What do you think?
Comment 3 Robert Nyman 2013-01-25 12:38:45 PST
I've experienced the same issue on both Unagi and in the Simulator.
For instance, I don't experience the hang if I choose Camera and take a picture - then I'm sent back right away.

It rather looks that this is an issue with the Gallery app.

And even if createObjectURL could be the culprit here (which I don't think), you should be sent back to requesting app and then fire the onsuccess event, I assume?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-29 02:36:10 PST
CreateObjectURL doesn't really do much. Can you try using the Gecko Profiler?
Comment 5 Robert Nyman 2013-01-29 02:38:23 PST
I'm sorry, but I don't really have a good dev setup, and am running this on various test phones. Feel more than free to test the code at http://robnyman.github.com/Firefox-OS-Boilerplate-App/ and see if you see anything specific.
Comment 6 Jason Smith [:jsmith] 2013-02-28 07:44:14 PST
*** Bug 839630 has been marked as a duplicate of this bug. ***
Comment 7 Jason Smith [:jsmith] 2013-02-28 07:45:05 PST
See https://bugzilla.mozilla.org/show_bug.cgi?id=839630#c5 for a rationale for nom.
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-28 08:09:31 PST
Moving to leo? since it's too late for v1.0.1 but we'll want to keep this on our radar and try to improve the perceived perf here.
Comment 9 [:fabrice] Fabrice Desré 2013-02-28 09:43:52 PST
David, do you mind verifying the issue there? I'll help you if we need to dig into the platform side.
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2013-02-28 11:22:00 PST
According to bug 839630 the problem is that the gallery app gets confused and converts the image to png. The current way that the gallery app handles the "pick" intent is actually quite broken and there is no way to simply ask for "give me a picture in whatever format".

I think we should add "image/*" to the list of types in the filter for the gallery, and then always encode to whatever format the picked image currently has if it's in the array or if "image/*" was requested.
Comment 11 David Flanagan [:djf] 2013-03-25 23:54:00 PDT
Jonas,

This is the bug we discussed by email.  Would you mark it leo+, please?
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2013-03-26 02:05:21 PDT
This is needed in order to properly support picking images either through <input type=file> or through webactivities.
Comment 13 David Flanagan [:djf] 2013-03-26 15:41:58 PDT
Created attachment 729837 [details]
link to patch on github

Dominic,

Sorry to keep asking you for reviews, but since you were the last one to work on the gallery's pick activity, I suspect you're the best qualified to check this.
Comment 14 David Flanagan [:djf] 2013-03-26 15:44:27 PDT
I'm removing the perf keyword for this, since we're now just treating this as a correctness bug, not a performance issue.  If an app asks us to convert a jpeg photo to png, it is going to take a long time, and we can't do much about that.  We could improve the UX, but we can't improve the basic conversion speed.

By fixing the correctness issue, apps won't accidentally get png images that they don't want though, so this should address the main complaint.
Comment 15 David Flanagan [:djf] 2013-03-26 16:45:37 PDT
I've updated the pull request so that when the email app sets nocrop, it still let's the user preview the selected image, but does not display crop controls for it.  This is per page 6 of https://www.dropbox.com/sh/8u8nmhvgzkltc7l/daG9W_eUCu/Apps/Email/email-attachments.pdf
Comment 16 Dominic Kuo [:dkuo] 2013-03-27 09:45:48 PDT
David,

Overall the patch looks good, the only concern is that after we change the pick type of email to "image/*", email can only pick image attachment from gallery, but according to the newest ux spec, we should also support picking from camera which is already enabled, maybe we also need to modify camera to fit this patch?
Comment 17 David Flanagan [:djf] 2013-03-29 09:38:02 PDT
Dominic,

Thanks. That's a very good point. 

I think it might be better to actually modify the email app to use [image/*, image/jpeg, image/png, image/gif] so it would match any app that supports pick.

The activity pattern matching code changed a few months ago, and I'm not sure what the best way to do this is...
Comment 18 David Flanagan [:djf] 2013-03-29 23:24:04 PDT
Dominic,

I've updated the pull request to just modify email to request image/* or image/jpeg, which should have both gallery and camera listed in the activity menu.

Would you take another look please? I still need a r+ to land.  Setting needinfo to be sure to get your attention.
Comment 19 Dominic Kuo [:dkuo] 2013-03-31 20:22:31 PDT
Comment on attachment 729837 [details]
link to patch on github

David,

The updated patch looks good, now picking images from camera works again.
Comment 20 David Flanagan [:djf] 2013-04-01 17:17:06 PDT
Landed on master: https://github.com/mozilla-b2g/gaia/commit/e64428cae922d32fc043cca390c32473963caec6
Comment 21 David Flanagan [:djf] 2013-04-01 17:27:35 PDT
Landed on v1-train
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2013-04-01 17:53:21 PDT
(In reply to Dominic Kuo [:dkuo] from comment #16)
> David,
> 
> Overall the patch looks good, the only concern is that after we change the
> pick type of email to "image/*", email can only pick image attachment from
> gallery, but according to the newest ux spec, we should also support picking
> from camera which is already enabled, maybe we also need to modify camera to
> fit this patch?

All Gaia apps which implement the "pick" activity and can return images should include "image/*" in their list of supported types. If that's not the case that needs to be fixed. It should be perfectly valid for a 3rd party app to only list "image/*" in the list of requested types.

Please fix this as a followup if it's not already fixed.
Comment 23 David Flanagan [:djf] 2013-04-01 19:27:44 PDT
It feels like a hack to use image/* in this way, but I've filed 856908 to do it as asked.  Jonas: please mark that bug leo+ if you want it uplifted.

I don't like this extra burden on activity implementors to handle the * case.  Ideally, I suppose, we'd allow regexp matching in activity requests.  Then we'd have full symmetry between the pick activity and the share activity.
Comment 24 David Flanagan [:djf] 2013-04-03 12:14:15 PDT
It looks like this did not actually get uplifted to v1-train despite what I wrote in comment 21....
Comment 25 David Flanagan [:djf] 2013-04-03 12:35:59 PDT
Actually, I did uplift this patch, but part of it was overwritten in https://github.com/mozilla-b2g/gaia/commit/433db9c80066986e6597346f3c20f3b249f86e29

I've now re-uplifted it to v1-train in: https://github.com/mozilla-b2g/gaia/commit/41393d1055b73b9da43366462d3c48f4103a5ca2
Comment 26 :CSchmoeckel 2013-07-02 16:09:09 PDT
Added Gallery Suite Test Case #8909 - [Gallery] Using the pick activity through a website does not cause the gallery to hang
Comment 27 Jason Smith [:jsmith] 2013-07-27 06:50:45 PDT
*** Bug 890477 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.