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

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

unspecified
Other
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18+ fixed)

Details

(Whiteboard: TaipeiWW)

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Sorry spec is from bug 838028. We could check video related integration issue here
Flags: needinfo?(abc)
(Assignee)

Comment 2

6 years ago
Another criteria might be 

'when trigger camera with video type, open record mode instead of default shot mode'
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
Flags: needinfo?(abc)
(Assignee)

Updated

6 years ago
Depends on: 838028
(Assignee)

Updated

6 years ago
blocking-b2g: --- → leo?
Summary: [camera] support 'record' activity with 'video' type → [camera] open in record mode when trigger camera with video app

Comment 4

6 years ago
This is a nice to have. Please only work on this after other blockers have been resolved, and nominate for uplift when resolved.
blocking-b2g: leo? → -
tracking-b2g18: --- → +
(Assignee)

Comment 5

6 years ago
make title more specific
Summary: [camera] open in record mode when trigger camera with video app → [camera] open in record mode when trigger camera with record activity by video type
Its confusing that https://bugzilla.mozilla.org/show_bug.cgi?id=879881 is leo+ and this isnt
Assignee: nobody → dale
Created attachment 759576 [details]
Pointer to GH
Attachment #759576 - Flags: review?(dflanagan)
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 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.
Attachment #759576 - Flags: review?(dflanagan) → review-
(Assignee)

Comment 10

6 years ago
nominate since this issue block bug 868225 and bug 879881 (both leo+)
blocking-b2g: - → leo?
blocking-b2g: leo? → leo+
Blocking because MMS leo+ dependency.
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
(Assignee)

Comment 13

6 years ago
Created attachment 760727 [details]
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
Attachment #760727 - Flags: review?(dale)
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
Attachment #760727 - Flags: review?(dale) → review-
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
Flags: needinfo?(dflanagan)
and yet I still added the flag, apologies
Flags: needinfo?(dflanagan)
(Assignee)

Comment 17

6 years ago
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.
Attachment #760727 - Flags: review?(dale)
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
Attachment #760727 - Flags: review?(dale)
Assignee: dale → gasolin
Also last comment, the branch needs rebased, it hits against a few merge conflicts, cheers
(Assignee)

Comment 20

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

Comment 21

6 years ago
Created attachment 762593 [details]
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.
Attachment #762593 - Flags: review?(dflanagan)
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.
Flags: needinfo?(gasolin)
(Assignee)

Comment 23

6 years ago
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.
Attachment #762593 - Flags: review?(dflanagan)
(Assignee)

Updated

6 years ago
Flags: needinfo?(gasolin)
(Assignee)

Comment 24

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

will add thumb to gallery, and not trigger preview while in pick mode
Attachment #762593 - Flags: review?(dflanagan)
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 (?)
Attachment #762593 - Flags: review?(dflanagan) → review-
(Assignee)

Comment 26

6 years ago
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.
Attachment #762593 - Flags: review?(dflanagan)

Updated

6 years ago
Whiteboard: TaipeiWW
(Assignee)

Comment 27

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

squashed commits and ask dale for review
Attachment #762593 - Flags: review?(dflanagan) → review?(dale)
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
Attachment #762593 - Flags: review?(dale) → review+
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-b2g18: --- → fixed
Duplicate of this bug: 873288
You need to log in before you can comment on or make changes to this bug.