Closed Bug 896907 Opened 7 years ago Closed 7 years ago

[Camera] No preview shown for successful video pick activity

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 verified, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE5
blocking-b2g leo+
Tracking Status
b2g18 --- verified
b2g-v1.1hd --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: djf)

References

Details

Attachments

(3 files, 2 obsolete files)

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
Flags: needinfo?(dale)
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
Flags: needinfo?(dale)
blocking-b2g: --- → leo+
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
Dale, Please provide the target date for this issue. Leo's QE5 ends by this week.
Flags: needinfo?(dale)
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.
Flags: needinfo?(dflanagan)
meant to clear my needinfo, David if you want me to work on this then just assign it
Flags: needinfo?(dale)
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
Assignee: nobody → dflanagan
Flags: needinfo?(dflanagan)
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.
Attached file Pointer to Pull request (obsolete) —
Hi David,

Please review the patch for showing video file preview using filmstrip methods.
Updated more comments in github.
Attachment #780828 - Flags: review?(dflanagan)
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.
Attachment #780828 - Flags: review?(dflanagan) → review-
Attached patch Patch for review (obsolete) — Splinter 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.
Attachment #780828 - Attachment is obsolete: true
Attachment #781497 - Flags: review?(dflanagan)
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.
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.
Attachment #781497 - Attachment is obsolete: true
Attachment #781497 - Flags: review?(dflanagan)
Attachment #782233 - Flags: review?(dale)
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.
Attachment #782233 - Flags: feedback?(leo.bugzilla.gaia)
Attachment #782233 - Flags: feedback?(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.
Attachment #782233 - Flags: feedback?(johu) → feedback+
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)
Attachment #782233 - Flags: review?(dale) → review+
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.
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
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.
Thanks for the review, John and Dale!

Landed on master: https://github.com/mozilla-b2g/gaia/commit/862de8489b648a9af7e8a5b88be031b5479404ba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
v1.1.0hd: 6e298d95674a2f546a40217eddbc4544cf82a4cc
v1.1.0hd: 1befac19a7bdcdccf0e7871760ba2628e38647f3
Attachment #782233 - Flags: feedback?(leo.bugzilla.gaia) → feedback+
You need to log in before you can comment on or make changes to this bug.