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

RESOLVED FIXED

Status

Firefox OS
Gaia::Gallery
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jsmith, Assigned: djf)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Keywords: perf
(Assignee)

Comment 1

4 years ago
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?
(Reporter)

Comment 2

4 years ago
(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

4 years ago
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?
CreateObjectURL doesn't really do much. Can you try using the Gecko Profiler?

Comment 5

4 years ago
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.
(Reporter)

Updated

4 years ago
Duplicate of this bug: 839630
(Reporter)

Comment 7

4 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=839630#c5 for a rationale for nom.
blocking-b2g: --- → tef?
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.
blocking-b2g: tef? → leo?
David, do you mind verifying the issue there? I'll help you if we need to dig into the platform side.
Assignee: nobody → dflanagan
blocking-b2g: leo? → -
tracking-b2g18: --- → +
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.
(Assignee)

Comment 11

4 years ago
Jonas,

This is the bug we discussed by email.  Would you mark it leo+, please?
blocking-b2g: - → leo?
Flags: needinfo?(jonas)
This is needed in order to properly support picking images either through <input type=file> or through webactivities.
blocking-b2g: leo? → leo+
Flags: needinfo?(jonas)
(Assignee)

Comment 13

4 years ago
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.
Attachment #729837 - Flags: review?(dkuo)
(Assignee)

Comment 14

4 years ago
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.
Keywords: perf
(Assignee)

Comment 15

4 years ago
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

4 years ago
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?
(Assignee)

Comment 17

4 years ago
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...
(Assignee)

Comment 18

4 years ago
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.
Blocks: 832923
Flags: needinfo?(dkuo)

Comment 19

4 years ago
Comment on attachment 729837 [details]
link to patch on github

David,

The updated patch looks good, now picking images from camera works again.
Attachment #729837 - Flags: review?(dkuo) → review+
Flags: needinfo?(dkuo)
(Assignee)

Comment 20

4 years ago
Landed on master: https://github.com/mozilla-b2g/gaia/commit/e64428cae922d32fc043cca390c32473963caec6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

4 years ago
Landed on v1-train
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
(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.
(Assignee)

Updated

4 years ago
Depends on: 856908
(Assignee)

Comment 23

4 years ago
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.
(Assignee)

Comment 24

4 years ago
It looks like this did not actually get uplifted to v1-train despite what I wrote in comment 21....
status-b2g18: fixed → affected
(Assignee)

Comment 25

4 years ago
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
status-b2g18: affected → fixed
(Reporter)

Updated

4 years ago
Flags: in-moztrap?

Updated

4 years ago
Flags: in-moztrap? → in-moztrap?(cschmoeckel)

Comment 26

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

Updated

4 years ago
Flags: in-moztrap?(cschmoeckel) → in-moztrap+
(Reporter)

Updated

4 years ago
Duplicate of this bug: 890477
You need to log in before you can comment on or make changes to this bug.