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

Status: RESOLVED FIXED

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

Description

[bhargavg1]

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

[Inder]

[Blocking Requested - why for this release]:

Comment 2

[Jason Smith [:jsmith]]

Flagging qawanted to get a video.

Comment 3

[Joshua Mitchell (Inactive)]

(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

Comment 4

[bhavana bajaj [:bajaj]]

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)

Comment 5

[bhavana bajaj [:bajaj]]

Also adding :dflanagan for input on comment #4 as Hema is OOO.

Comment 6

[Hema Koka [:hema]]

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)

Comment 7

[bhargavg1]

(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

Comment 8

[Wilson Page [:wilsonpage]]

Attachment: 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.

Comment 9

[Wilson Page [:wilsonpage]]

Will require uplift to 'v2.0' after bug 1038172 has landed.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Blocking as per request from Inder (CAF).

Comment 11

[Wilson Page [:wilsonpage]]

Landed on 'master'

https://github.com/mozilla-b2g/gaia/commit/296c4022541dd964048e489ccaab01e3968d1307

Comment 12

[Jason Smith [:jsmith]]

Wilson - Shouldn't we close this, since this landed on master?

Comment 13

[Wilson Page [: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 :-/

Comment 14

[Dylan Oliver [:doliver]]

Is there a branch-specific patch required for 2.0? If not, we should mark this resolved to trigger the uplift.

Comment 15

[Dylan Oliver [:doliver]]

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

[Hema Koka [:hema]]

Justin is helping create a 2.0 patch (lets not wait on 1038172 any more)

Comment 17

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v2.0)

Patch rebased on v2.0 -- Carrying R+ forward

Comment 18

[Justin D'Arcangelo [:justindarc]]

Ryan: As per Hema's comment 16, the v2.0-rebased version of this patch is ready to land in v2.0.

Comment 19

[Justin D'Arcangelo [:justindarc]]

Forgot to set NI?

Comment 20

[Ryan VanderMeulen [: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.

Comment 21

[Justin D'Arcangelo [:justindarc]]

(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!

Comment 22

[[:SalvadorR]]

This will need to be covered by a new test case.

Comment 23

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case has been added in moztrap:

https://moztrap.mozilla.org/manage/case/14384/