834773 - Using the pick activity and confirming an image for the gallery - app gets hung with no perceived perf for 5 - 10 seconds

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Jason Smith [:jsmith]]

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]]

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]]

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

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) (email my personal email if necessary)]

CreateObjectURL doesn't really do much. Can you try using the Gecko Profiler?

Comment 5

[Robert Nyman]

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 7

[Jason Smith [:jsmith]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=839630#c5 for a rationale for nom.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

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é]

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) No longer reading bugmail consistently]

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]]

Jonas,

This is the bug we discussed by email.  Would you mark it leo+, please?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is needed in order to properly support picking images either through <input type=file> or through webactivities.

Comment 13

[David Flanagan [:djf]]

Attachment: 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]]

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]]

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]]

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]]

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]]

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]]

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]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e64428cae922d32fc043cca390c32473963caec6

Comment 21

[David Flanagan [:djf]]

Landed on v1-train

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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]]

It looks like this did not actually get uplifted to v1-train despite what I wrote in comment 21....

Comment 25

[David Flanagan [:djf]]

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]

Added Gallery Suite Test Case #8909 - [Gallery] Using the pick activity through a website does not cause the gallery to hang