896907 - [Camera] No preview shown for successful video pick activity

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect, P1)

Description

[Leo]

1. Title :  No preview shown for successful video pick activity
2. Precondition : Camera and SMS apps should be working
3. Tester's Action : 1. Open SMS app -> New message -> attachment -> camera -> video mode -> record a video
4. Detailed Symptom (ENG.) : When user completes recording a video file, a confirmation screen comes with "retake" and "select" options. But the screen is black and there is no preview of video file captured.

5. Expected : Preview of video file recorded should be shown.

6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 94bd91967d99ceb9e072486fc91687c221628e6d

Comment 1

[Dale Harvey (:daleharvey)]

Yup, during a pick the user still has the ability to retake or select the video, but we should definitely be showing a preview, it wasnt implemented on the first time round for the patch but is ready to be implemented now

Comment 2

[Leo]

Dale, Please provide the target date for this issue. Leo's QE5 ends by this week.

Comment 3

[Dale Harvey (:daleharvey)]

Forwarding the needinfo to djf, this came up the first time select was implemented so not sure if its on the media teams backlog, I should have time to work on it on friday if nobody else picks it up before.

Comment 4

[Dale Harvey (:daleharvey)]

meant to clear my needinfo, David if you want me to work on this then just assign it

Comment 5

[Leo]

It's a kind of new implentation to be added. we are trying to display thumbnail of the video with play option byusing previewItem(0) at the end of recording. we have issues with proper alignment of thumbnail image as the image is very small and those width and height calculations are done dynamically in 'shared' code (media_frame.js & video_player.js) .

we are trying to avoid changes in shared code and make fixes in app itself

Comment 6

[David Flanagan [:djf]]

This seems harder than I expected. It appears that the preview for still photos occurs as a side effect of how the camera works. The camera app doesn't have any explict code to display the still photo. This means that it isn't just a matter of obtaining the correct video preview image to display; we also need the code to display it.  I'm currently debating whether to try to reuse the filmstrip preview code for pick confirmations, or to modify the existing confirmation element to include a preview image in addition to the Select and Discard buttons.

Comment 7

[Leo]

Attachment: Pointer to Pull request

Hi David,

Please review the patch for showing video file preview using filmstrip methods.
Updated more comments in github.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 780828 [details]
Pointer to Pull request

Giving r- for this patch because of its dependence on setTimeout(). It doesn't seem safe to do it that way.

I'm working on a patch myself.

Comment 9

[Leo]

Attachment: Patch for review

Hi David, previewItem() be called without using the setTimeout() function.
In filmstrip, now we are calling previewItem function from addItem() by testing pendingPick and item.isVideo vars.

Please let me know if this patch is good enough to commit.

Comment 10

[David Flanagan [:djf]]

Leo,

This bug was unassigned, so I assigned it to myself two days ago, and have been working on it since then. Reviewing your patches only distracts me from the patch I'm working on myself.  If I had not already put many hours into this, I'd just resassign it to you.  But at this point, I plan to finish my own patch.

In the future, if you are actively working on a bug, please assign it to yourself so that someone else does not take it.

Comment 11

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch modifies the Camera apps so that during a pick activity, it uses a MediaFrame object to preview the most recently recorded photo or video so that the user can decide whether to pick it or to take a new photo or video instead.  Using MediaFrame means that:

1) The user can zoom and pan on photos, to check for bluriness red-eye, etc.

2) The user can preview videos, and can even play them before deciding. Adding video preview for picks was the point of this patch.

Making #2 work was non-trivial, but is a big improvement overall, and I think this was a bug that was worth taking the time to fix right.

The changes in this patch are these:

1) Move the code for creating and saving video poster images out of filmstrip.js and into camera.js. This is core camera functionality, so it is where it belongs.
Modify the code in filmstrip.js that creates video thumbnails to create the thumbnail from the poster image that is passed in.

2) Create new files confirm.js and confirm.css for displaying the select/retake confirmation dialog. Since confirm.js and filmstrip.js both use MediaFrame, the confirm.css file has CSS styles that are similar to those in filmstrip.css

3) Remove the pan-and-zoom event handlers from filmstrip.js and factor them out into a new file panzoom.js so that they can be used by both filmstrip.js and confirm.js

4) Modify camera.js and camera.css to remove the old pick confirmation code and styles since they are now replaced by confirm.js and confirm.css.

5) The camera app used to only save the photo that the user actually selected and never save photos if the user clicked 'retake'. After consulting with Rob, I've changed this so that the app saves every photo and video.

Comment 12

[David Flanagan [:djf]]

Comment on attachment 782233 [details]
link to patch on github

John,

Dale is the module owner here and this is a big enough patch that I've asked for his review. But since you're following this bug, and we need to build knowledge of the camera app within the media team, I'd also be happy to get your feedback on the changes I've made here.

Leo: since you've been working on this bug as well, your feedback is welcome here as well.

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 782233 [details]
link to patch on github

I had tested the whole code and it works fine. But I had found two risky points:

1. The overlay and confirm are using 100 in z-index. When confirm shows, the overlay can't be visible. I think overlay should be higher than confirm.
2. The name in response result of activity is removed. We can't make sure if other apps already use it or not. I think it is nice to have the name field, so that the app can show some hints to let user understand the picked image or video is also saved in storage.

Another suggestion is: the camera app uses object structure to host functions. I found the confirm.js and panzoom.js does not. IMHO, to have the same pattern may be good.

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 782233 [details]
link to patch on github

This works great

I agree with Johns comments, it seems like the name would be very useful in the activity data but will leave that up to you as you have a better idea about activity data as a whole

The confirmation should stay as it is above the overlay, the overlay would prevent you from taking an image but once it has been taken then you dont need access to storage and the confirmation can succeed (this works when plugging in after taking a photo), if you press retake then the overlay shows, I think that should be clarified by setting confirm above 100 though (and possibly grouping the z-index definitions somewhere)

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Dale, 

I have different thought with the confirmation and overlay. If we only take photo, the confirmation can be above the overlay. But if we take video, the video is not playable when it is mounted as USB mass storage. In this case, the overlay should be above the confirmation.

Comment 16

[Dale Harvey (:daleharvey)]

Good point, I tested that and in practice the video recording blocks the mass storage being mounted, however once the video has finished recording we should assume that the storage will be able to be mounted, I may have just tested too quickly

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: A video is recorded and usb mounted

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: after the play button is pressed and usb is still mounted.

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

I had put two screenshots of taking video. The first one attachment 783490 [details] is a took video with mounted as USB mass storage, see the icon on the status bar. The second one, attachment 783491 [details], is to press the play button on the video. Logcat: 

07-31 10:25:37.267: E/GeckoConsole(724): [JavaScript Warning: "Media resource blob:69ee1a45-ef6e-44b1-89c2-d2111e42e96a could not be decoded." {file: "app://camera.gaiamobile.org/index.html#pick" line: 0}]


BTW, I never know the video recording can block the mounting procedure. Thanks for this information.

Comment 20

[David Flanagan [:djf]]

Thanks for the review, John and Dale!

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

Comment 21

[David Flanagan [:djf]]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/6e298d95674a2f546a40217eddbc4544cf82a4cc

Comment 22

[Montoya Clemmons (MClemmons)]

Verified fixed, the issue no longer reproduces.

Preview of video file recorded is shown.


Environmental Variables
Build ID: 20130805071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a2a9b89ef5ee
Gaia: 45f6a739b09292e16717fb21003386c914ca29c2
Platform Version: 18.1
Firmware Version: D300f08o

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 6e298d95674a2f546a40217eddbc4544cf82a4cc
v1.1.0hd: 1befac19a7bdcdccf0e7871760ba2628e38647f3