Video Encode: Flicker in the preview can be observed when we change the resolution in front camera

RESOLVED FIXED in Firefox OS v2.0

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bhargavg1, Assigned: justindarc)

Tracking

unspecified
2.1 S1 (1aug)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [caf priority: p3][CR 698566], URL)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
STR:
1. While in camcorder switch to front camera
2. Select 480p resolution and then switch to QCIF resolution and vice versa
3. Observe the flicker when we are switching between the resolutions.

Actual behavior:-
Flicker in the preview can be observed when we change the resolution in front camera

Expected behavior:-
Flicker should not be observed.

Please note build is made at after enabling 'RecorderProfiles' [1]

[1] https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/camera/js/config/config.js#L363

Comment 1

4 years ago
[Blocking Requested - why for this release]:
Blocks: 1041241
No longer blocks: 1011657
blocking-b2g: --- → 2.0?
Flagging qawanted to get a video.
Keywords: qawanted

Updated

4 years ago
Whiteboard: [CR 698566] → [caf priority: p3][CR 698566]
(In reply to Jason Smith [:jsmith] - At Work Week, Slow to Respond from comment #2)
> Flagging qawanted to get a video.

Qa-Wanted can not full-fill this request. This issue is not for Flame devices - which do not have resolution settings

Will re-direct to bug reporter - Please provide a video of this issue
Flags: needinfo?(bhargavg1)
Keywords: qawanted
NI Hema to help here, if this is a something that we are able to support in 2.0 given the config is disabled by default ? I am guessing we are not ready to support it yet hence the pref off, but can you confirm ?

(Same thought applies to 1043705)
Flags: needinfo?(hkoka)
Also adding :dflanagan for input on comment #4 as Hema is OOO.
Flags: needinfo?(dflanagan)

Comment 6

4 years ago
Noticed that this is marked p3, bhargav: could you please provide video here to see how bad it is so we can make a call on blocker. 

Wilson or Diego, could you check if you are seeing the flicker issue when we change resolutions that are configured using config.js (Perhaps it is a viewfinder fade in/fade out issue here that Wilson is fixing in another bug)
Flags: needinfo?(wilsonpage)
Flags: needinfo?(hkoka)
Flags: needinfo?(dmarcos)

Updated

4 years ago
Flags: needinfo?(dflanagan)
(Reporter)

Comment 7

4 years ago
(In reply to Joshua Mitchell [:Joshua_M] from comment #3)
> (In reply to Jason Smith [:jsmith] - At Work Week, Slow to Respond from
> comment #2)
> > Flagging qawanted to get a video.
> 
> Qa-Wanted can not full-fill this request. This issue is not for Flame
> devices - which do not have resolution settings
> 
> Will re-direct to bug reporter - Please provide a video of this issue

Uploaded image at, https://drive.google.com/file/d/0B0zTAnPOpx-xVVJOb0FUV1F0RDQ/edit?usp=sharing
Flags: needinfo?(bhargavg1)
Created attachment 8464725 [details] [review]
pull-request (master)

REASON

Removing the settings view and switching the camera stream at the same time seemed to cause this flicker.

FIX

- I have added subtle transitions to quickly fade the settings is and out.
- We now change settings only when the settings menu has been removed from the the DOM.
- Found a tricky bug in the SettingsAlias class related to event propagation. Fixed, simplified and added more tests.
Attachment #8464725 - Flags: review?(dmarcos)
Flags: needinfo?(wilsonpage)
Will require uplift to 'v2.0' after bug 1038172 has landed.
Depends on: 1038172
Flags: needinfo?(dmarcos)
Attachment #8464725 - Flags: review?(dmarcos) → review+
Blocking as per request from Inder (CAF).
blocking-b2g: 2.0? → 2.0+

Updated

4 years ago
Assignee: nobody → wilsonpage
Whiteboard: [caf priority: p3][CR 698566] → [caf priority: p3][CR 698566] [patch ready]
Wilson - Shouldn't we close this, since this landed on master?
Flags: needinfo?(wilsonpage)
(In reply to Jason Smith [:jsmith] - At Work Week, Slow to Respond from comment #12)
> Wilson - Shouldn't we close this, since this landed on master?

We still have the v2.0 uplift. Feel free to close it if you like. I'm just concerned that if closed, it may get lost :-/
Flags: needinfo?(wilsonpage)

Updated

4 years ago
Target Milestone: --- → 2.1 S1 (1aug)
Is there a branch-specific patch required for 2.0? If not, we should mark this resolved to trigger the uplift.
Nevermind, I see comment #9 now. I think all of these bugs that are waiting on bug 1038172 should be marked resolved/fixed and then whiteboarded with [NO_UPLIFT] to prevent the uplift until you're ready.

That will give us a clearer picture of what work is outstanding.

Comment 16

4 years ago
Justin is helping create a 2.0 patch (lets not wait on 1038172 any more)
Assignee: wilsonpage → jdarcangelo
(Assignee)

Comment 17

4 years ago
Created attachment 8468103 [details] [review]
pull-request (v2.0)

Patch rebased on v2.0 -- Carrying R+ forward
Attachment #8468103 - Flags: review+
(Assignee)

Comment 18

4 years ago
Ryan: As per Hema's comment 16, the v2.0-rebased version of this patch is ready to land in v2.0.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 19

4 years ago
Forgot to set NI?
Flags: needinfo?(ryanvm)
v2.0: https://github.com/mozilla-b2g/gaia/commit/5ba22d458fdb63bd72c59de53c701d0efe35c1e2

This missed the 4pm PT nightlies, so it'll be tomorrow before this is available for testing.
status-b2g-v2.0: affected → fixed
Flags: needinfo?(ryanvm)
Whiteboard: [caf priority: p3][CR 698566] [patch ready] → [caf priority: p3][CR 698566]
(Assignee)

Comment 21

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> 5ba22d458fdb63bd72c59de53c701d0efe35c1e2
> 
> This missed the 4pm PT nightlies, so it'll be tomorrow before this is
> available for testing.

No problem. Thanks Ryan!

Updated

4 years ago
No longer depends on: 1038172

Comment 22

4 years ago
This will need to be covered by a new test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(srapanan)
Test case has been added in moztrap:

https://moztrap.mozilla.org/manage/case/14384/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(srapanan)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.