[B2G][Helix][Camera]video resolution is clip with CIF resolution only

VERIFIED FIXED

Status

Firefox OS
Gaia::Camera
P1
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: DingYu, Assigned: yurenju)

Tracking

unspecified

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 779154 [details]
camera.js

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; Tablet PC 2.0)

Steps to reproduce:

1.Open the camera app and toggle to the video mode
2.Start and stop a video recording
3.Analyze the video imformation


Actual results:

video resolution is clip with CIF resolution(352x288) only, I  change the code of the camera.js according to the Bug 838512. But the viewfinder is frozen.


Expected results:

The video can support VGA resolution, becase we use the 5M sensor, it can support 720p.
(Reporter)

Updated

5 years ago
Severity: normal → critical
Priority: -- → P1
Tim, can you help with suggestions here?
Flags: needinfo?(timdream)
I think we should do bug 838512 comment 3, i.e. move the resolution restriction to a build time config, maybe set-able from gonk build flags. David, do you agree?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(timdream) → needinfo?(dflanagan)
This is a valid bug for helix device.
blocking-b2g: --- → hd?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #2)
> I think we should do bug 838512 comment 3, i.e. move the resolution
> restriction to a build time config, maybe set-able from gonk build flags.
> David, do you agree?

Yes, I agree that the resolution should be configurable at build-time. I don't have any opinion on what the best way to do that is, because I don't know how any of our build-time configuration works.

Also, note that bug 892852 just changed the video configuration code. CIF is still the default but sometimes it uses QCIF instead when recording videos for MMS attachments.  In any case, it should be slightly easier to convert to a build-time configuration with the new code.

I'd like to learn how we do build-time configuration, so I'd be happy to review a patch for this bug.  Or, if someone (Tim?) explains how we do configuration, I'd be happy to take this bug myself.
Flags: needinfo?(dflanagan)
(In reply to David Flanagan [:djf] from comment #4)
> I'd like to learn how we do build-time configuration, so I'd be happy to
> review a patch for this bug.  Or, if someone (Tim?) explains how we do
> configuration, I'd be happy to take this bug myself.

There are two ways to create build config, one would be putting the config in settings db in build/settings.py, and the other would be have build/applications-data.js generate application-specific JSON. Both of them are overwrite-able with vendor config.

Settings db might be the better choice in this case. However, that just free up the config to be vendor config-able, not exactly responsive automatically to device target. Need another patch for that. Yuren, you are the best person to work on this part IMHO.
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(dflanagan)
sure.

my plan is adding a new settings key/value in GAIA_DIR/build/settings.py and use this value in camera.js to config resolution.
Assignee: nobody → yurenju.mozilla
Flags: needinfo?(yurenju.mozilla)
HD+, device specific here.
Group: mozilla-corporation-confidential
blocking-b2g: hd? → hd+
Created attachment 783727 [details] [diff] [review]
camera-customization.patch

WIP here. I use '720p' profile for testing, but device can't save this video file to sdcard after record stoped.
I filed a bug for 720p issue (see bug 900327) but this bug shouldn't block by bug 900327 because 480p works.
sorry, s/shouldn't block/shouldn't be blocked/
Created attachment 784255 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/11286

Hi David Flanagan,

this patch will create a key/value in mozSettings to customize recording profile, vendor can use a string array to config it, for example, I can put settings.json to GAIA_DIR/distribution/ with:
> { 
>    "camera.recording.profiles": ["720", "480p"]
> }

to config recording profile to 720p, if 720p profile isn't available it will fallback to 480p.

could you help to review this PR? thank you!
Attachment #783727 - Attachment is obsolete: true
Attachment #784255 - Flags: review?(dflanagan)
Comment on attachment 784255 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/11286

Yuren,

You are on the right path here, but the patch as its stands has some problems:

1) It breaks the qcif recording mode for MMS video attachments

2) It adds a settings query to the startup sequence that is usually unnecessary and will make Camera slower to show the user a preview

3) I have some naming nits

Also, at the end of comment 5, Tim suggests some kind of customization that would automate this further by setting the settings.py value at build time based on the device being built.  Are you planning to do that as well, or will vendors have to customize this setting manually?
Attachment #784255 - Flags: review?(dflanagan) → review-
David Flanagan,

thank you for your review, I'll send another pull request to you accroding the comments on github. for configruation by build time issue, I have no idea for now. in my solution venders have to customize manually by putting a settings.json in GAIA_DIR/settings.json with recording profile name.
Created attachment 785693 [details] [diff] [review]
bug896425-wip.patch

WIP here. I'll finish it tomorrow.
Comment on attachment 784255 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/11286

Hi David Flanagan, I have updated a new version of PR for this issue, could you review it again? thanks!
Attachment #784255 - Flags: review- → review?(dflanagan)
Comment on attachment 784255 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/11286

I have not tested the bug, but the code looks good.

There is a spelling error in a variable name to fix, and one or two formatting nits on github.  If you fix those, and if you have tested, then r+ to land.

But let's also ask Tim about his intention in comment 2 and 5. Tim, is this patch enough, or do we also need a followup bug to alter the default setting value based on gonk build flags?
Attachment #784255 - Flags: review?(dflanagan) → review+

Updated

5 years ago
Flags: needinfo?(dflanagan) → needinfo?(timdream)
(In reply to David Flanagan [:djf] from comment #16)
> But let's also ask Tim about his intention in comment 2 and 5. Tim, is this
> patch enough, or do we also need a followup bug to alter the default setting
> value based on gonk build flags?

Customization is good enough IMHO. I will loop PM to confirm that.
Flags: needinfo?(timdream)
hi all,

fixed issues which djf addressed on github, it's ready to land :D
merged.

master: https://github.com/mozilla-b2g/gaia/commit/c354940

hd: https://github.com/mozilla-b2g/gaia/commit/3126b65 

thank you djf and Tim!
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-b2g-v1.1hd: --- → fixed
Sorry after reviewing this bug again I think it was accidentally set to confidential by me.

Unsetting that.
Group: mozilla-corporation-confidential

Updated

4 years ago
See Also: → bug 914602
v1.1.0hd: 52d4e499261a0a27a5341a51b751a68b0ddf64ce
v1.1.0hd: 351e2488ecdede3771dd69eb8158fc472f5c396c
Attachment mime type: text/plain → text/x-github-pull-request

Comment 22

4 years ago
[2013/10/21 Helix Testing]
Gaia:     c829a2042594b6c3a4899ee27979799a0f301534
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f7c657f6d019
BuildID   20131015042201
Version   18.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.