853349 - [camera] open in record mode when trigger camera with record activity by video type

Status: RESOLVED FIXED

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

Description

[[e-mail]]

Camera should support 'record' with 'video' type 
to fulfill the need of new video.app spec https://www.dropbox.com/s/h89kj0xrwygfj1n/Sharing-videos-via-bluetooth.pdf

from Bug 805333

video.app will triger 'take a shot' action to camera.app.

Though there's no way to turn back to video.app from camera.app.
It might be need some extra UX work to clarify the transition between video.app and camera.app

Comment 1

[[e-mail]]

Sorry spec is from bug 838028. We could check video related integration issue here

Comment 2

[[e-mail]]

Another criteria might be 

'when trigger camera with video type, open record mode instead of default shot mode'

Comment 3

[Arun Balachandran Ganesan [:abc]]

re: comment 2

Yes, it's a good idea, but we just need to make sure that if we take this route then it would still work for other scenarios that are similar — it's a tricky UX problem to solve. Is there a meta bug that tracks all places where such an interaction between apps that exists?

Cheers,
Arun

Comment 4

[Alex Keybl [:akeybl]]

This is a nice to have. Please only work on this after other blockers have been resolved, and nominate for uplift when resolved.

Comment 5

[[e-mail]]

make title more specific

Comment 6

[Dale Harvey (:daleharvey)]

Its confusing that https://bugzilla.mozilla.org/show_bug.cgi?id=879881 is leo+ and this isnt

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Pointer to GH

Comment 8

[Dale Harvey (:daleharvey)]

There is a few notes for this commit, 1 apologies for the amss change from this.captureMode to this._captureMode, it was a bug that has always existed however it never previously caused problems as we always manually set this.captureMode.

1. When we record a video, we get a black screen with the confirmation to retake or select the image, I though this would be less confusing than a live preview screen (and pausing currently doesnt work), Ideally this should be a preview of the video that can be played

2. The duration of the current duration isnt shown as it is in the same place as the cancel button, we can either hide the cancel button during recording or move the timer to the right

Both things should be fixed, but I though given the time contraints and size of change they should be done in follow ups

Comment 9

[David Flanagan [:djf]]

Comment on attachment 759576 [details]
Pointer to GH

The code looks good, but I can't figure out how to test it.

When I add an attachment in the Messaging app, it seems to be doing a pick activity with a wildcard for the type, so all apps that can handle picks are listed in the menu.  When I select the camera app, the type is not "video/*" so the camera app lets me pick a still photo, but doesn't let me pick a video.

I fear that in order to support MMS, the camera app will have to support three kinds of picks:

1) still photos only (we already had that)
2) videos only (what this patch gives)
3) either one, displaying the switch button so the user can choose.

If I'm misunderstanding, just explain and set r? again.

Comment 10

[[e-mail]]

nominate since this issue block bug 868225 and bug 879881 (both leo+)

Comment 11

[Dietrich Ayala (:dietrich)]

Blocking because MMS leo+ dependency.

Comment 12

[Dale Harvey (:daleharvey)]

I was testing by manually changing the wallpaper to ask for video/*, but you are right there is no way to capture 'video/*' 'images/*' right now, I think adding the ability for the camera to switch between modes when the activity is asking for the particular type of media shouldnt be too intrusive a patch, will come up with one now

Comment 13

[[e-mail]]

Attachment: pull request redirect to github

Dale, sorry I just saw you just start working on the following work. hope it not late...

I'm working on bug 868225 (video attachment support on MMS), so I followed your work yesterday and come out a patch.

Your previous patch did most great effort, but only support 'pick' activity, which not fulfill the origin purpose of this bug.

This patch works for

* support 'record' activity (open video app -> press record button in left bottom side)
* displaying the switch button while multiple types are declared.

I'm working based on your patch. If it works you can r? to djf. 
thanks

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 760727 [details]
pull request redirect to github

Hey Fred, no worries on taking over, so there is a few issues with this.

I will needinfo David for this, but currently we are using the 'record' activity to trigger the plain camera, and 'pick' activity to pick media from the camera to be returned

This currently breaks the normally launched camera, the capture button doesnt get enabled

The code to pick the types is confusing, there is lots of branching etc, hopefully sorting out the pick vs record will help, I also put a quick code example up there that I think makes it a little clearer.

David mentioned to me recently that the addVideo call to the filmstrip currently contains the code that generated the preview for the video that the gallery uses, so that code needs to be extracted / invoked

Comment 15

[Dale Harvey (:daleharvey)]

Actually I dont really need to needinfo david, reusing the 'record' activity name will break existing usage (launch from gallery), the SMS app has to use the 'pick' activity

Comment 16

[Dale Harvey (:daleharvey)]

and yet I still added the flag, apologies

Comment 17

[[e-mail]]

Comment on attachment 760727 [details]
pull request redirect to github

Thanks dale, I've adopt your code example, 
and the capture button doesn't get enabled issue is fixed now.

find some issues about video preview and video,
I think its not related to this issue statement, and can be done in follow ups.

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 760727 [details]
pull request redirect to github

The r? seemed to be on the wrong user so clearing that, leaving my r- for now as I would like another look at this with some ammendments before it lands, but almost there.

1. The cancel button gets disabled when switching between video / camera

2. I mentioned previously that https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/filmstrip.js#L617 is required for saved videos to be displayed in the gallery, it isnt a major issue but probably best fixed now (while we remember)

Few other comments on GH, cheers

Comment 19

[Dale Harvey (:daleharvey)]

Also last comment, the branch needs rebased, it hits against a few merge conflicts, cheers

Comment 20

[[e-mail]]

I feel that summarizing some test criteria will help goes through the following work:

record

open video app -> press record button in left bottom side -> camera opened in record mode

pick 

1) photos only

open homescreen > long click to trigger webactivity menu -> camera opened in camera mode

2) videos only

manual modify some app(ex video) to test this action.

3) either one, displaying the switch button so the user can choose.

open message app > click left bottom paper clip sign > select camera ->
open in camera mode, can switch back and forth between camera/record mode, always can see the cancel pick button.

Comment 21

[[e-mail]]

Attachment: 2nd pull request redirect to github

1. passed test criteria in comment 20

2. append blob when return pick result (thus message app will show the compose view with file size).


I tried to call Filmstrip.addVideo(videofile) in stop recording function, but doing that will start preview mode, which not the expect behavior.
So I'd leave it black in this stage.

Comment 22

[David Flanagan [:djf]]

Fred,

Please also try this test case:

- Open message app, attach a video to a message, then go to the gallery app and see if the video is displayed properly.  

Without calling Filmstrip.addVideo() for each new video, the poster image that gallery depends on will not be written. If you can't call that (and just hide the filmstrip in pick mode) then you'll need to refactor so that the poster image is generated in camera.js

I haven't had a chance to read the code enough for this to be a review but I thought I'd at least mention this part.

Comment 23

[[e-mail]]

Comment on attachment 762593 [details]
2nd pull request redirect to github

Thanks for helping tackle down the gallery-related test case, 
the poster image is not written without calling Filmstrip.addVideo().

I'd try to fix it then request review again.

Comment 24

[[e-mail]]

Comment on attachment 762593 [details]
2nd pull request redirect to github

will add thumb to gallery, and not trigger preview while in pick mode

Comment 25

[David Flanagan [:djf]]

Comment on attachment 762593 [details]
2nd pull request redirect to github

r- see github for details.

Also, you have not modified the webapp.manifest file to list video/* and video/3gpp as supported types for the activity. Without that, the activity won't work for apps that want to pick video but not still images.

You may need to create a UITest pane as part of this patch to demonstrate that the app can work in all possible cases:

- photos only
- videos only
- photos and videos, both specified
- no types specified, allow both (?)

Comment 26

[[e-mail]]

Comment on attachment 762593 [details]
2nd pull request redirect to github

Thanks for review, fixed comment in github and add UITests for pick video, pick photo/video cases.
Works fine with those uitest.

Comment 27

[[e-mail]]

Comment on attachment 762593 [details]
2nd pull request redirect to github

squashed commits and ask dale for review

Comment 28

[Dale Harvey (:daleharvey)]

Comment on attachment 762593 [details]
2nd pull request redirect to github

Thanks, tested this and is working well

Merged in: https://github.com/mozilla-b2g/gaia/commit/baea36ed4e1034af3b58e815d515b0415d80a2ce

Comment 29

[Dale Harvey (:daleharvey)]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/2dfdafa42e4e8f9b978c85100582ed628d56cb91