1036814 - [KK Only][QRD]Front camcorder preview and captured video is stretched in portrait mode and shrinked in landscape mode.

Status: RESOLVED WONTFIX

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

Description

[Pooja]

This issue is observed only on 8x10 target with 256Mb memory limit and not with higher memory targets

Also we are trying with below two settings enabled in apps/camera/js/config/config.js
key: 'pictureSizes
key: 'recorderProfiles'

STR:
1. Open camera app 
2. Switch to camcorder mode 
3. Switch to front camera 
4. Observe that the preview will be proper before you start recording.
4. Now in portrait mode start recording and observe that as soon as we start recording preview stretches.
6. Rotate to landscape mode and observe that the preview becomes very lean.

Actual behavior:-
Front camcorder preview and captured video is stretched in portrait mode and looks very lean in landscape mode.

Expected behavior:-
Preview should not stretch.

Comment 1

[bhavana bajaj [:bajaj]]

Adding qawanted to see if we can reproduce this on flame 273MB ?

Comment 2

[Hema Koka [:hema]]

Pooja: Please add a video if possible. 

QAwanted to check on 273 MB flame. 

David or Andrew, can you also please try this out. 

Thanks
Hema

Comment 3

[bhargavg1]

(In reply to Hema Koka [:hema] from comment #2)
> Pooja: Please add a video if possible. 
> 
> QAwanted to check on 273 MB flame. 
> 
> David or Andrew, can you also please try this out. 
> 
> Thanks
> Hema

That would be tricky for the front camera I believe

Comment 4

[Cody Roesch [:croesch]]

This bug does NOT repro on: Flame 2.1, Flame 2.0, Flame 1.4

Actual Result: With Flame set to 273mb, recording and recording preview through the front camera on a flame device does not cause any odd stretching/shrinking in Portrait or Landscape modes.

Environmental Variables:
Device: Flame Master
Build ID: 20140711062012
Gaia: c47094a26c87ba71a3da4bae54febd0da21f3393
Gecko: 75f66f8cb99f
Version: 33.0a1 (Master)
Firmware Version: v122
-----------------------------------------------
Environmental Variables:
Device: Flame 2.0
BuildID: 20140710213213
Gaia: 18c44a1bc31b374ba00a069904465a8d07971a60
Gecko: f880dae4fdbe
Version: 32.0a2 (2.0) 
Firmware Version: v122
-----------------------------------------------
Environmental Variables:
Device: Flame 1.4
Build ID: 20140711075012
Gaia: c78d1ab13586da4db62690fab915cf8256d09c35
Gecko: 2b648cedfc83
Version: 30.0 (1.4)
Firmware Version: v122

Comment 5

[Inder]

Adding ni on Pooja to provide a video of the issue.

Comment 6

[Pooja]

This issue is not reproducible on latest builds by some mystery fix.
Just want to keep it open for few more days to be sure about its behavior.

Comment 7

[Inder]

Closing it for now, will revisit if we reproduce it again.

Comment 8

[Pooja]

Reopening this issue again. Its not reproducible with 512MB target
Device
QRD 8x10 283Mb
Gaia:  18c44a1bc31b374ba00a069904465a8d07971a60
Gcko : 22368827423484e09c3840ebc785de09b821a039
Version 32.0a2

Simpler STR
1. Launch Camcorder and switch to front camera
2. In options, set the Camera resolution to '800*600 4:3' and Video resolution to 480p and observe that the preview will be stretched (Focus on a face, so that issue can be easily identified)
3. Now Change the Camera resolution to lower values like '320*240 4:3' and observe that the preview will be normal again.

Attaching video of an issue too.

Comment 9

[Pooja]

Attachment: preview stretch from lower to higher camera resolution

Comment 10

[Justin D'Arcangelo [:justindarc]]

(In reply to Cody Roesch [:croesch] from comment #4)
> This bug does NOT repro on: Flame 2.1, Flame 2.0, Flame 1.4
> 

Confirming that this does not reproduce on Flame 2.1, 2.0 or 1.4 with latest nightly builds. The only time I've seen this particular issue in the past has been with the Nexus 4. If I remember correctly, we debugged the Camera app from the Gaia side on Nexus 4 to see if we were somehow incorrectly stretching the viewfinder and that was *NOT* the case. So, I'm guessing if this is the same issue on the QRD, then the issue is likely at a lower level in the system (driver?). Flagging Mike for NI? to get his input.

Comment 11

[Jason Smith [:jsmith]]

QA Wanted to try testing this again with comment 8's STR.

Comment 12

[Preeti Raghunath(:Preeti)]

Hema

Please review and reassign if reproducible.

Comment 13

[Inder]

> So, I'm guessing if
> this is the same issue on the QRD, then the issue is likely at a lower level
> in the system (driver?). Flagging Mike for NI? to get his input.

This is a UI artifact due to memory restrictions and don't think it's driver related.

Comment 14

[Hema Koka [:hema]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #10)
> (In reply to Cody Roesch [:croesch] from comment #4)
> > This bug does NOT repro on: Flame 2.1, Flame 2.0, Flame 1.4
> > 
> 
> Confirming that this does not reproduce on Flame 2.1, 2.0 or 1.4 with latest
> nightly builds. The only time I've seen this particular issue in the past
> has been with the Nexus 4. If I remember correctly, we debugged the Camera
> app from the Gaia side on Nexus 4 to see if we were somehow incorrectly
> stretching the viewfinder and that was *NOT* the case. So, I'm guessing if
> this is the same issue on the QRD, then the issue is likely at a lower level
> in the system (driver?). Flagging Mike for NI? to get his input.

Justin, Did you follow the STR in comment 8 on 273MB flame and not able to reproduce it?

Comment 15

[Justin D'Arcangelo [:justindarc]]

(In reply to Hema Koka [:hema] from comment #14)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #10)
> > (In reply to Cody Roesch [:croesch] from comment #4)
> > > This bug does NOT repro on: Flame 2.1, Flame 2.0, Flame 1.4
> > > 
> > 
> > Confirming that this does not reproduce on Flame 2.1, 2.0 or 1.4 with latest
> > nightly builds. The only time I've seen this particular issue in the past
> > has been with the Nexus 4. If I remember correctly, we debugged the Camera
> > app from the Gaia side on Nexus 4 to see if we were somehow incorrectly
> > stretching the viewfinder and that was *NOT* the case. So, I'm guessing if
> > this is the same issue on the QRD, then the issue is likely at a lower level
> > in the system (driver?). Flagging Mike for NI? to get his input.
> 
> Justin, Did you follow the STR in comment 8 on 273MB flame and not able to
> reproduce it?

Yes, I edited the Camera config.js to match the configuration specified in Comment 0 (our default config doesn't let you choose picture/video sizes), then I followed the steps in Comment 8 on a 280MB Flame and was still unable to reproduce.

Comment 16

[Mike Habicher [:mikeh] (high bugzilla latency)]

Sotaro, could this be due to difference between JB and KK native windows?

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #16)
> Sotaro, could this be due to difference between JB and KK native windows?

I do not think GonkNativeWindow(ANativeWindow) could cause this problem.

Comment 18

[Sotaro Ikeda [:sotaro]]

It seems better to get layer dump. But on b2g v2.0, layer dump becomes to be truncated. To fix this problem need to apply Bug 1030245.

Comment 19

[Joshua Mitchell (Inactive)]

(In reply to Jason Smith [:jsmith] from comment #11)
> QA Wanted to try testing this again with comment 8's STR.

Comment 10 and 15 seems to satisfy this request 
"Yes, I edited the Camera config.js to match the configuration specified in Comment 0 (our default config doesn't let you choose picture/video sizes), then I followed the steps in Comment 8 on a 280MB Flame and was still unable to reproduce."

and better than QA-Wanted could have as it required edit config files (Flame device does not have resolution setting user-side).

Comment 20

[Pooja]

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #17)
> (In reply to Mike Habicher [:mikeh] from comment #16)
> > Sotaro, could this be due to difference between JB and KK native windows?
> 
> I do not think GonkNativeWindow(ANativeWindow) could cause this problem.

On JB based 2.0 builds i am not seeing this issue . I guess trying it with KK based 2.0 builds on flame will give us clear idea.

Comment 21

[Sotaro Ikeda [:sotaro]]

Pooja, what is the needinfo?

Comment 22

[Pooja]

Hi Sotaro,

This is not reproducible on JB , So was looking for your last confirmation that it could not be  due to GonkNativeWindow(ANativeWindow)

Comment 23

[Sotaro Ikeda [:sotaro]]

If the problem could be re-generated only on KK, there are the following gonk specific part that could be related.
- camera hal, CameraService
- GonkNativeWindow
- hw composer

Comment 24

[Sotaro Ikeda [:sotaro]]

Mozilla side seems not have a way to run KK flame.

Comment 25

[Justin D'Arcangelo [:justindarc]]

In Comment 10, I mentioned that I remembered seeing this on Nexus4. It is very possible that I was running KK when I saw this issue as well (I'm not certain though, I'd have to charge up my Nexus4 to check what version it has). The fact that this seems to be only reproducible on KK is interesting.

Comment 26

[Kai-Chih Hu [:khu]]

Viral, can you help to take a look on this bug? Thanks.

Comment 27

[viral [:viralwang]]

Camera is still not working(black screen) in QRD8926 with following tag.
caf_AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.149.xml

not sure if it comes from hw difference but two QRD8926 in my hands are both failed.
looking for QRD8226 device and try to put 2.0 for testing. (this tag is 1.4 gecko)

Comment 28

[Hema Koka [:hema]]

(In reply to viral [:viralwang] from comment #27)
> Camera is still not working(black screen) in QRD8926 with following tag.
> caf_AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.149.xml
> 
> not sure if it comes from hw difference but two QRD8926 in my hands are both
> failed.
> looking for QRD8226 device and try to put 2.0 for testing. (this tag is 1.4
> gecko)

Inder, Looks like the QRD that we have in hand in Taipei is on 8926, but the issue reported is on 8x10. Need your advice to see if we should try getting a 2.0 build on the QRD that we have and reproduce it there. Thanks!

Comment 29

[Inder]

Hema/Viral -- Please share the details of the failure on 8926 with the AU tag you mention over email. Please also share in the email any adb and kernel logs. I also don't have the original complete build that was there on 8926 so if you can share that details then will help identifying the root cause.
Ideally building with the AU tag mentioned and loading 2.0 gecko/gaia should work.

Please also let us know the status of the KK build on flame.
Thanks.

Comment 30

[Hema Koka [:hema]]

Viralwang: Thanks for helping reproduce this on the QRD that we have. Please provide the information requested by Inder in comment 29

Comment 31

[viral [:viralwang]]

Hi Inder, 

Already send the mail to descript the failed symptom.

Comment 32

[viral [:viralwang]]

update some info for this bug.

1) Just flash Moz 2.0 in my flame and we can't choose camera resolution in our side.
So it looks like I can not use QRD8226 by AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.149 and move gecko/gaia to Moz 2.0 to reproduce this bug

2) There's another tag for KK+2.0 as default for QRD8226/8926, but it looks like we didn't have permission for prebuild of b2g_kk_3.6. Can anyone help on this?

Comment 33

[David Flanagan [:djf]]

Justin: would you help viral with the config file edits that will allow him to set the resolution and follow the STR?

Comment 34

[David Flanagan [:djf]]

Justin: also, if you do still have a nexus 4 running kit-kat, could you try reproducing it on that device, please? Of course that won't be a low-memory device, but might still be an interesting test.

Justin: and here's another idea. Instaed of just sending a config file to viral, perhaps you could create a PR for testing this bug. Have it modify the config file so that the resolution can be set, but also add a bunch of console.log statements in the code that picks the resolution and the code that computes the CSS transform for the viewfinder, so we can see what is going on in Gaia when the bug occurs.  If you can do that, perhaps we can also get CAF to test with your PR and report what they see.

Comment 35

[David Flanagan [:djf]]

(In reply to Inder from comment #13)
> > So, I'm guessing if
> > this is the same issue on the QRD, then the issue is likely at a lower level
> > in the system (driver?). Flagging Mike for NI? to get his input.
> 
> This is a UI artifact due to memory restrictions and don't think it's driver
> related.

I don't think there is anything in the Gaia code that would allow memory level to affect the display. So if this is a memory related issue, I think it must mean that either the camera driver is sending us a distorted preview stream in low-memory conditions or that something in the video or graphics stack is distorting that preview stream while displaying it.

Comment 36

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #34)
> Justin: also, if you do still have a nexus 4 running kit-kat, could you try
> reproducing it on that device, please? Of course that won't be a low-memory
> device, but might still be an interesting test.
> 
> Justin: and here's another idea. Instaed of just sending a config file to
> viral, perhaps you could create a PR for testing this bug. Have it modify
> the config file so that the resolution can be set, but also add a bunch of
> console.log statements in the code that picks the resolution and the code
> that computes the CSS transform for the viewfinder, so we can see what is
> going on in Gaia when the bug occurs.  If you can do that, perhaps we can
> also get CAF to test with your PR and report what they see.

I remember seeing this issue on Nexus4 (which I believe was running KK). Like you said, Nexus4 is not a low-mem device so I don't think that was the cause. We spent days debugging it thinking it was a CSS transform gone wrong or something but it definitely wasn't the issue -- the actual video stream coming from mozCamera was stretched.

As far as testing out the various resolutions, you'll have to re-install the Camera app from the Gaia repo.

1.) Clone the Gaia repo
    `git clone https://github.com/mozilla-b2g/gaia.git`
2.) Open `apps/camera/js/config/settings.js`
3.) Uncomment the following lines near the bottom:
      // {
      //   key: 'pictureSizes'
      // },
      // {
      //   key: 'recorderProfiles'
      // },
4.) `make install-gaia APP=camera` to re-install the Camera app with these settings enabled

Comment 37

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v2.0)

Pull request for branch with resolution settings enabled (based on v2.0).

Comment 38

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Pull request for branch with resolution settings enabled (based on master).

Comment 39

[David Flanagan [:djf]]

Thanks, Justin.

Viral: if you are able to reproduce the bug with your QRD device and the simple patches that Justin has provided, Justin can tell you which file in the camera app sets the CSS transform on the camera preview stream.  (I would guess it is apps/camera/js/views/viewfinder.js).  If you insert console.log() statements into that file we can see how the video stream is being transformed to make it fit the screen, and see whether there is a problem in Gaia or whether the distorted video is coming from the camera driver.

Comment 40

[David Flanagan [:djf]]

Justin: have you watched the attached video? Do you agree that this is a bug?  What are we supposed to do when the video resolution does not match the screen aspect ratio? Are we supposed to crop the edges to make it fit?

Comment 41

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #40)
> Justin: have you watched the attached video? Do you agree that this is a
> bug?  What are we supposed to do when the video resolution does not match
> the screen aspect ratio? Are we supposed to crop the edges to make it fit?

Yes. We use an "aspect-fill" algorithm so that when the aspect-ratio does not match the screen, the video will be scaled so that the entire screen is filled (so either the top/bottom or left/right sides of the video will be cut off so we never have "black bars").

As for checking the CSS transform, in my experience in debugging this, the best way was to use either "App Manager" or "WebIDE" in Firefox Nightly to gain access to the Web Inspector window to examine the styles applied to the viewfinder elements. By default, debugging "certified" apps is not allowed. So you need to follow the instructions here to enable that:

https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager#Debugging_Certified_Apps

Or, if all else fails, you can just stick console.log() statements in the code as you mentioned. The code in question is in a method called `updatePreviewMetrics` in apps/camera/js/views/viewfinder.js:

https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/camera/js/views/viewfinder.js#L235

This viewfinder positioning/sizing code is pretty thoroughly tested and like I mentioned earlier, I remember seeing this on the Nexus4 with KitKat and IIRC, the issue was with a distortion in the actual preview stream itself. If you'd like to look at the unit tests, they're located here:

https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/camera/test/unit/views/viewfinder_test.js

Comment 42

[Diego Marcos [:dmarcos]]

One question here. Should we be blocking on KK specific bugs? Is there any device going to be released with KK in 2.0?

Comment 43

[viral [:viralwang]]

(In reply to David Flanagan [:djf] from comment #39)
> Thanks, Justin.
> 
> Viral: if you are able to reproduce the bug with your QRD device and the
> simple patches that Justin has provided, Justin can tell you which file in
> the camera app sets the CSS transform on the camera preview stream.  (I
> would guess it is apps/camera/js/views/viewfinder.js).  If you insert
> console.log() statements into that file we can see how the video stream is
> being transformed to make it fit the screen, and see whether there is a
> problem in Gaia or whether the distorted video is coming from the camera
> driver.

Now I can have KK+2.0 in QRD8226.
I can select different resolution with Justin's patch, but I can't reproduce it my side.

Comment 44

[Hema Koka [:hema]]

Thanks so much Viral for getting the QRD ready with KK build to test. 

Pooja,

Could you please re-test this with latest build and let us know if you are able to reproduce it?

Thanks
Hema

Comment 45

[bhavana bajaj [:bajaj]]

Pooja, the patch in comment #38 should be the one you need to include while testing.

Comment 46

[Justin D'Arcangelo [:justindarc]]

(In reply to bhavana bajaj [:bajaj] from comment #45)
> Pooja, the patch in comment #38 should be the one you need to include while
> testing.

To clarify, the one in Comment #38 is for master (v2.1) and the one in Comment #37 is for v2.1

Comment 47

[David Flanagan [:djf]]

(In reply to Diego Marcos [:dmarcos] from comment #42)
> One question here. Should we be blocking on KK specific bugs? Is there any
> device going to be released with KK in 2.0?

Given that CAF is doing their testing with KK, I assume that means that commercial versions of 2.0 are going to ship on a KK. As I understand it, we are hoping to have a KK image for Flame soon.

Comment 48

[Pooja]

(In reply to bhavana bajaj [:bajaj] from comment #45)
> Pooja, the patch in comment #38 should be the one you need to include while
> testing.

Hi As mentioned in #comment1 ,This testing includes patch suggested by you

Comment 49

[Hema Koka [:hema]]

(In reply to Pooja from comment #48)
> (In reply to bhavana bajaj [:bajaj] from comment #45)
> > Pooja, the patch in comment #38 should be the one you need to include while
> > testing.
> 
> Hi As mentioned in #comment1 ,This testing includes patch suggested by you

The patches included by Justin are for Viral to test with recording profiles/picture sizes enabled so we can follow the exact steps as in the original STR. Pooja has been testing on her QRD with this enabled already. From comment 43, we are *not* able to reproduce in QRD running KK and latest 2.0 build. Neither can we on a Flame running JB. 

Pooja, can you please confirm if this is still happening with *most recent* 2.0 build on your QRD. If it is, we need to wait for KK Flame build or a QRD that has exactly the same setup as your test to reproduce. But please confirm first if this is still an issue, before we figure out next steps. 

Thanks
Hema

Comment 50

[Pooja]

(In reply to Hema Koka [:hema] from comment #49)

> Pooja, can you please confirm if this is still happening with *most recent*
> 2.0 build on your QRD. t
> 


Yes still happening on latest  2.0 build.
Also this issue is happening because 

all the streams (snapshot,video and preview) should be of same aspect ratio. If not the one which is lower will won't work properly. This is a limitation in 8x10 because its VFE has only one FOV crop module.

EX: 1. Live Snapshot stream (LS) Ex: Camera resolution 800x600
    2. Video/Preview stream (V/P) Ex: Camcoder Resolution   720x480

Here Aspect Ratio mismatches so issue observed.

Issue is observed with 512MB as well (if we try with incorrect combo) I was trying correct combo before so didn't see this issue before.

Suggested Fix:
Either
1.       give only one option to user – video resolution like in Android OS. (or)
2.       Have both, but once video resolution is selected, filter out snapshot resolutions which are of different (than video) aspect ratio. Show only matching aspect ratio snapshot resolutions in the UI.

Comment 51

[viral [:viralwang]]

Here's the video I test in my QRD8226
https://copy.com/EfxAY3qgnWoz

Gecko:
commit af8f99bc797b6fd8c3e88a211dcc7f3d015fdc20
Author: B2G Bumper Bot <[e-mail]>
Date:   Tue Jul 29 14:30:22 2014 -0700

    Bumping manifests a=b2g-bump

Gaia: (+ comment 37)
commit abdb110c6421c53221ab2d59ddb536bfc062e7a8
Author: Cristian Rodriguez <[e-mail]>
Date:   Thu Jul 24 09:51:44 2014 +0200

    Merge pull request #22064 from crdlc/bug-1041291-v2
    
    Bug 1041291 v2(cherry picked from commit c72257b2d27135bfcd68e89dd584182797784016)

Comment 52

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #50)
> Suggested Fix:
> Either
> 1.       give only one option to user – video resolution like in Android OS.

We do this already. The ability to the user to change resolutions is disabled by default. The patches I attached to this bug were to pre-configure the app (build time) so that the resolution setting would be available to the user.

Also, it sounds like the real reason for the aspect-ratio mismatch you're talking about is because we attempt to select a preview stream for the viewfinder that is as close as possible to matching the aspect ratio/resolution of the device's screen. The reason we do this is to help reduce the amount of pixels that would end up getting cut-off by the viewfinder and we don't want to use a preview stream that is of higher resolution than the screen for efficiency reasons (all the additional detail in the preview would go to waste and memory usage would be increased).

However, if this is, in fact, the cause of this issue, we may have to change the way we select our preview size so that we match against the target video/picture aspect ratio instead of the screen aspect ratio. I can put together a patch that does this so we can check if this fixes the issue. Thanks again for all the info!

Comment 53

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #50)
> Also this issue is happening because 
> 
> all the streams (snapshot,video and preview) should be of same aspect ratio.
> If not the one which is lower will won't work properly. This is a limitation
> in 8x10 because its VFE has only one FOV crop module.
> 

Can you clarify what "VFE" is? I'm assuming "FOV" means "field of view". I'm also assuming that this is some sort of hardware component, correct? If so, is there any way to detect this? We do not see this issue on the Flame device, so if we do have to implement some sort of workaround to filter out certain resolutions, it would be nice to know if filtering is necessary with the hardware we're running on.

Comment 54

[Diego Marcos [:dmarcos]]

What does the following sentence from comment #50 mean? What's VFE? What's FOV?

"This is a limitation in 8x10 because its VFE has only one FOV crop module"

Comment 55

[Justin D'Arcangelo [:justindarc]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #52)
> Also, it sounds like the real reason for the aspect-ratio mismatch you're
> talking about is because we attempt to select a preview stream for the
> viewfinder that is as close as possible to matching the aspect
> ratio/resolution of the device's screen. The reason we do this is to help
> reduce the amount of pixels that would end up getting cut-off by the
> viewfinder and we don't want to use a preview stream that is of higher
> resolution than the screen for efficiency reasons (all the additional detail
> in the preview would go to waste and memory usage would be increased).
> 
> However, if this is, in fact, the cause of this issue, we may have to change
> the way we select our preview size so that we match against the target
> video/picture aspect ratio instead of the screen aspect ratio. I can put
> together a patch that does this so we can check if this fixes the issue.
> Thanks again for all the info!

Sorry, I was mistaken. We already do select the preview size that is *as close as possible* to the selected video/picture resolution (*NOT* the screen size). The only time screen size comes into play is to make sure that we don't select a preview size that has twice as many pixels or anything like that. 

However, we are not guaranteed to find a perfect match as far as the aspect ratio goes. I think this is probably where the mismatch is occurring. The camera has picture/video resolutions that don't have any corresponding "preview size" (matching aspect ratio).

Comment 56

[Diego Marcos [:dmarcos]]

I feel that gaia is not the appropriate place to work around those hardware quirks. When gecko queries the available preview sizes the HW/driver should filter out the bogus preview sizes that cannot be used.

Also, Is this a bug we want to block on? Is there any production device that exhibits the same limitation?

Comment 57

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Patch for master (v2.1) to filter out any picture/video resolutions with aspect ratios that don't have a matching preview size (e.g.: removes 800x600 resolution from the settings menu if there are no preview sizes with a 4:3 aspect ratio).

Comment 58

[Hema Koka [:hema]]

(In reply to Diego Marcos [:dmarcos] from comment #54)
> What does the following sentence from comment #50 mean? What's VFE? What's
> FOV?

It looks like VFE is Video Frontend End (component in their camera hardware) and FOV is Field of View. Pooja please clarify.

Comment 59

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v2.0)

Pooja: Please take a look at this patch on your QRD and see if this fixes the issue. This will remove all resolutions from the settings menu that don't have a suitable preview size (matching aspect ratio).

Comment 60

[Diego Marcos [:dmarcos]]

Could we end with an empty list? Is there always a preview size with the same aspect ratio as each of the resolutions that the user might choose? I insist that this HW limitation should not leak to gaia and we should not block on it if there won't be users affected by it.

Comment 61

[Justin D'Arcangelo [:justindarc]]

If there are no "perfect matches", the setting will not appear and the app will operate the same as it would if we didn't enable the resolution selection. I agree that this should not be fixed in Gaia, I'm just trying to get a better understanding as to the cause of the issue. If this patch successfully hides the resolutions that are problematic, then we know for sure that the root cause is mismatched aspect ratios with preview sizes.

Comment 62

[Pooja]

Attachment: Recording of issue (including patch)

Comment 63

[Pooja]

Yes Hema you are correct 
VFE: Video front end. Camera hardware component to process frame data
FOV: Field of view


Justin,
I tried out your patch .But still facing same issue also , AS you can see in attached video  the non matching camera resolution w.r.t preview aspect ratio, are not still not wiped off.

Ex Video resolution: 480p 720x480 but still i can set to camera resolution 800x600.
Please let me know if my understanding is not correct.

Comment 64

[bhavana bajaj [:bajaj]]

Setting the assignee to Justin as her is helping out here, please feel free to reassign if needed.

Comment 65

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #63)
> Yes Hema you are correct 
> VFE: Video front end. Camera hardware component to process frame data
> FOV: Field of view
> 
> 
> Justin,
> I tried out your patch .But still facing same issue also , AS you can see in
> attached video  the non matching camera resolution w.r.t preview aspect
> ratio, are not still not wiped off.
> 
> Ex Video resolution: 480p 720x480 but still i can set to camera resolution
> 800x600.
> Please let me know if my understanding is not correct.

Ah, ok. So, the issue is, you want to remove all picture resolutions that don't correspond to a video recording resolution?

This can be solved with a simple configuration change. In `apps/camera/js/config/config.js`:

Find the `pictureSizesBack` item and list the picture resolutions you want to exclude under the `exclude.keys` array:

https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/camera/js/config/config.js#L196

For example, the default config looks like:

  pictureSizesBack: {
    title: 'camera-resolution',
    header: 'camera-resolution-header',
    icon: 'icon-picture-size',
    options: [
      // {
      //   key: '2048x1536'
      // }
    ],
    exclude: {
      keys: ['1920x1088'],
      aspects: ['5:3', '11:9', '16:9'],
    },
    persistent: true,
    optionsLocalizable: false,
  },

To remove the 2592x1944, 2048x1536, 1600x1200, 1024x768 and 800x600 resolutions, it would look like:

  pictureSizesBack: {
    title: 'camera-resolution',
    header: 'camera-resolution-header',
    icon: 'icon-picture-size',
    options: [
      // {
      //   key: '2048x1536'
      // }
    ],
    exclude: {
      keys: ['1920x1088', '2592x1944', '2048x1536', '1600x1200', '1024x768', '800x600'],
      aspects: ['5:3', '11:9', '16:9'],
    },
    persistent: true,
    optionsLocalizable: false,
  },

Any picture resolutions that are seen to be problematic on your hardware can be easily disabled like this.

Comment 66

[Inder]

Bhargav -- can you please look into to see if we can bring in this change? Also, can you please provide the config.js changes we have made locally.

Comment 67

[bhargavg1]

(In reply to Inder from comment #66)
> Bhargav -- can you please look into to see if we can bring in this change?

Will try to validate the change first and see if it fixes the issue. 

> Also, can you please provide the config.js changes we have made locally.

Our change to config.js is available on CAF @https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/patch/all/gaia/0001-Enable-recorderProfiles-menu-and-1080p-video-encode.patch?h=v2.0

Comment 68

[Justin D'Arcangelo [:justindarc]]

From the changes you've made to the default config.js:

 apps/camera/js/config/config.js | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/apps/camera/js/config/config.js b/apps/camera/js/config/config.js
index b14da89..41a2559 100644
--- a/apps/camera/js/config/config.js
+++ b/apps/camera/js/config/config.js
@@ -209,7 +209,7 @@ module.exports = {
     header: 'video-resolution-header',
     icon: 'icon-video-size',
     options: [],
-    exclude: ['high', '1080p'],
+    // exclude: ['high', '1080p'],
     persistent: true,
     optionsLocalizable: false,
   },
@@ -360,12 +360,12 @@ module.exports = {
       {
         key: 'timer'
       },
-      // {
-      //   key: 'pictureSizes'
-      // },
-      // {
-      //   key: 'recorderProfiles'
-      // },
+      {
+        key: 'pictureSizes'
+      },
+      {
+        key: 'recorderProfiles'
+      },
       {
         key: 'grid'
       }

Two questions:
1.) Why are you preventing the exclusion of 'high' and '1080p' video recording sizes?
2.) Why are you enabling the 'pictureSizes' and 'recorderProfiles' settings menus?

Comment 69

[bhargavg1]

(In reply to Justin D'Arcangelo [:justindarc] from comment #68)
> 
> Two questions:
> 1.) Why are you preventing the exclusion of 'high' and '1080p' video
> recording sizes?
> 2.) Why are you enabling the 'pictureSizes' and 'recorderProfiles' settings
> menus?

commit text from the change, 

Per Mozilla UX design on v1.4, the recorderProfiles menu is disabled by default
Also 1080p video resolution is explicitly disabled due to bug 987068.

This patch reverses enables these two settings in our internal builds because:
1) 1080p works fine on 8926, and enabling doesn't cause issues on 8x10 or 7x27a
2) The recorderProfiles menu is very useful when debugging why a video
recording failed.  For example currently on 7x27a, our camera HAL
reports that we can support up to 720p video so the camera application by
default selects the maximum resolution.  However in reality we can only
support up to CIF.  Without the recorderProfiles menu enabled it would
have taken much longer to root cause such a problem.

Comment 70

[Justin D'Arcangelo [:justindarc]]

(In reply to bhargavg1 from comment #69)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #68)
> > 
> > Two questions:
> > 1.) Why are you preventing the exclusion of 'high' and '1080p' video
> > recording sizes?
> > 2.) Why are you enabling the 'pictureSizes' and 'recorderProfiles' settings
> > menus?
> 
> commit text from the change, 
> 
> Per Mozilla UX design on v1.4, the recorderProfiles menu is disabled by
> default
> Also 1080p video resolution is explicitly disabled due to bug 987068.
> 
> This patch reverses enables these two settings in our internal builds
> because:
> 1) 1080p works fine on 8926, and enabling doesn't cause issues on 8x10 or
> 7x27a
> 2) The recorderProfiles menu is very useful when debugging why a video
> recording failed.  For example currently on 7x27a, our camera HAL
> reports that we can support up to 720p video so the camera application by
> default selects the maximum resolution.  However in reality we can only
> support up to CIF.  Without the recorderProfiles menu enabled it would
> have taken much longer to root cause such a problem.

Ok, I guess I'm just confused as to why we're blocking on an issue that only arises when a feature that is disabled by default at build time is enabled. This problem with the stretched viewfinder is caused by configuring the camera to resolutions that are problematic on this specific hardware. From a user's standpoint, this menu wouldn't be available and the config.js should have been set up correctly by the vendor for the hardware they're running on.

Even still, I understand that technically we *should* be able to enable this feature at build time. However, the camera hardware is returning resolutions that are problematic and that is the root cause of this issue. I don't think we should be trying to work around special cases where the hardware is reporting incorrect capabilities.

Comment 71

[Justin D'Arcangelo [:justindarc]]

Does this issue happen on a final production configuration? If not, I don't think we should be blocking on this considering that it seems that the viewfinder is behaving as expected when the resolutions are mismatched.

Comment 72

[Hema Koka [:hema]]

Based on call with Inder on 8/7, awaiting input from Bhargav to see if this is a blocker for 2.0

Comment 73

[Pooja]

(In reply to Justin D'Arcangelo [:justindarc] from comment #65)
> (In reply to Pooja from comment #63)
> > Yes Hema you are correct 
> > VFE: Video front end. Camera hardware component to process frame data
> > FOV: Field of view
> > 
> > 
> > Justin,
> > I tried out your patch .But still facing same issue also , AS you can see in
> > attached video  the non matching camera resolution w.r.t preview aspect
> > ratio, are not still not wiped off.
> > 
> > Ex Video resolution: 480p 720x480 but still i can set to camera resolution
> > 800x600.
> > Please let me know if my understanding is not correct.
> 
> Ah, ok. So, the issue is, you want to remove all picture resolutions that
> don't correspond to a video recording resolution?
> 
> This can be solved with a simple configuration change. In
> `apps/camera/js/config/config.js`:
> 
> Find the `pictureSizesBack` item and list the picture resolutions you want
> to exclude under the `exclude.keys` array:
> 
> https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/camera/js/config/config.
> js#L196
> 
> For example, the default config looks like:
> 
>   pictureSizesBack: {
>     title: 'camera-resolution',
>     header: 'camera-resolution-header',
>     icon: 'icon-picture-size',
>     options: [
>       // {
>       //   key: '2048x1536'
>       // }
>     ],
>     exclude: {
>       keys: ['1920x1088'],
>       aspects: ['5:3', '11:9', '16:9'],
>     },
>     persistent: true,
>     optionsLocalizable: false,
>   },
> 
> To remove the 2592x1944, 2048x1536, 1600x1200, 1024x768 and 800x600
> resolutions, it would look like:
> 
>   pictureSizesBack: {
>     title: 'camera-resolution',
>     header: 'camera-resolution-header',
>     icon: 'icon-picture-size',
>     options: [
>       // {
>       //   key: '2048x1536'
>       // }
>     ],
>     exclude: {
>       keys: ['1920x1088', '2592x1944', '2048x1536', '1600x1200', '1024x768',
> '800x600'],
>       aspects: ['5:3', '11:9', '16:9'],
>     },
>     persistent: true,
>     optionsLocalizable: false,
>   },
> 
> Any picture resolutions that are seen to be problematic on your hardware can
> be easily disabled like this.

That works. But in this way we could not use excluded camera resolutions in camera case. Because they were excluded to resolve camcorder issue so by default they wont appear for camera

Our concern is providing different options to changes camera and video resolutions separately.
When user switches from camera to camcoder, different resolution will get chooses and if the aspect ratio of these resolutions mismatches, then we see this issue.
 

It won't be possible to keep all camera and video resolutions to same aspect ratio.

In case of Android there is only one option to change camera/video resolution so there won't be aspect ratio mismatch and this issue won't happen.
Here camera folks mentioned this problem is not specific to only our platform but a generic one.

So why there are 2 settings? How do I configure a single setting to change both camera/video resolutions

Comment 74

[Justin D'Arcangelo [:justindarc]]

Mike, can you answer the question in Comment 73 about why we have different resolution settings for video vs. pictures? I'm not sure where the data in the mozCamera.capabilities comes from.

Comment 75

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #73)
> In case of Android there is only one option to change camera/video
> resolution so there won't be aspect ratio mismatch and this issue won't
> happen.
> Here camera folks mentioned this problem is not specific to only our
> platform but a generic one.
> 
> So why there are 2 settings? How do I configure a single setting to change
> both camera/video resolutions

Actually, I'm not sure how it would be possible to have only one resolution setting. For example, the picture resolution could be set to something extremely high (5MP) which wouldn't be a suitable video recording resolution. Likewise, if we only limited the resolutions to one that both picture and video could use, then our maximum picture size would be equivalent to a 1080p video (which is very low for a photo).

Or are you saying that when we do video recording, we need to also set the camera's photo resolution to be the same as the video resolution?

Comment 76

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to bhargavg1 from comment #69)

> 2) The recorderProfiles menu is very useful when debugging why a video
> recording failed.  For example currently on 7x27a, our camera HAL
> reports that we can support up to 720p video so the camera application by
> default selects the maximum resolution.  However in reality we can only
> support up to CIF.  Without the recorderProfiles menu enabled it would
> have taken much longer to root cause such a problem.

Bhargav,

If your platform doesn't support recording at 720p, then you need to remove it from your media_profiles.xml. The CameraControl API merges the resolutions listed as _supported_ in that file with the _supported_ resolutions from KEY_SUPPORTED_PREVIEW_SIZES/_VIDEO_SIZES to determine the list of recorderProfiles presented to Gaia.

media_profiles.xml is the proper place to express this limitation. For Flame, I see this file in $B2G/device/qcom/common/media/media_profiles.xml.

Comment 77

[bhargavg1]

(In reply to Mike Habicher [:mikeh] from comment #76)
> (In reply to bhargavg1 from comment #69)
> media_profiles.xml is the proper place to express this limitation. For
> Flame, I see this file in $B2G/device/qcom/common/media/media_profiles.xml.

Agreed this can be the way. Also Inder has a patch to remove the unsupported profiles as well from h/w @ https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/camera/commit/?h=LNX.LF.3.5.2.1_1&id=adf0250cf64575a4d064096ee1f89b162221d2ba

But Mike the problem here is we are trying to solve comment #65

> So, the issue is, you want to remove all picture resolutions that don't correspond to a video recording resolution?
> This can be solved with a simple configuration change. In `apps/camera/js/config/config.js`

I am wondering apart from config.js can we calculate dynamically based on the screen resolution for the right set of supported aspect ratios?

Comment 78

[Mike Habicher [:mikeh] (high bugzilla latency)]

Bhargav, thanks for the information.

Is there some way of determining this limitation of the 8x10? For example, does getSupportedVideoSizes() (or some other CameraParameter) return a zero-length vector[1] (or some other value) we can use to enforce this requirement?

This seems like something that should be set in Gecko, but I need to know how to determine when to enforce it. (We can handle a CAF-specific CameraParameter key, if needed.)

1. http://androidxref.com/4.0.4/xref/frameworks/base/include/camera/CameraParameters.h#75

Comment 79

[bhargavg1]

(In reply to Mike Habicher [:mikeh] from comment #78)
> Bhargav, thanks for the information.
> 
> Is there some way of determining this limitation of the 8x10? For example,
> does getSupportedVideoSizes() (or some other CameraParameter) return a
> zero-length vector[1] (or some other value) we can use to enforce this
> requirement?
> 
> This seems like something that should be set in Gecko, but I need to know
> how to determine when to enforce it. (We can handle a CAF-specific
> CameraParameter key, if needed.)
> 
> 1.
> http://androidxref.com/4.0.4/xref/frameworks/base/include/camera/
> CameraParameters.h#75

Mike, will have to check and getback to you on this.

Given it would take time removing this from the metabug.

Comment 80

[Pooja]

Comment on attachment 8466327 [details] [review]
pull-request (v2.0)

Issue was  not fixed with it.

Comment 81

[Pooja]

(In reply to Justin D'Arcangelo [:justindarc] from comment #75)

> Or are you saying that when we do video recording, we need to also set the
> camera's photo resolution to be the same as the video resolution?

Hi Justin,

What i meant from having only one resolution is "at a time there is only one master setting available"

As in LA, in camera mode i can only set picture size(camera resolution) there is no option to set video resolution .Similarly UI has no option to set camera resolution in camcoder mode

So in whatever mode whichever setting is chosen the other one gets overwritten with appropriate values

Example:

Here in camera mode i set picture size to 8M Pixel
So stream3(snapshot) is set to 3200*2400 and stream 1 i.e preview becomes 640*480

E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10010 stream type 1 w*h 640*480
E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10011 stream type 3 w*h 3200*2400

Similarly when in camcorder mode i modified video resolution values to WVGA
then stream 3  changed to "1280*768" internally and stream1 and stream4 becomes 800*480 (as selected by user)

E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10029 stream type 3 w*h 1280*768
E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 1002a stream type 4 w*h 800*480
E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 1002c stream type 1 w*h 800*480

Comment 82

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #81)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #75)
> 
> > Or are you saying that when we do video recording, we need to also set the
> > camera's photo resolution to be the same as the video resolution?
> 
> Hi Justin,
> 
> What i meant from having only one resolution is "at a time there is only one
> master setting available"
> 
> As in LA, in camera mode i can only set picture size(camera resolution)
> there is no option to set video resolution .Similarly UI has no option to
> set camera resolution in camcoder mode
> 
> So in whatever mode whichever setting is chosen the other one gets
> overwritten with appropriate values
> 
> Example:
> 
> Here in camera mode i set picture size to 8M Pixel
> So stream3(snapshot) is set to 3200*2400 and stream 1 i.e preview becomes
> 640*480
> 
> E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10010 stream
> type 1 w*h 640*480
> E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10011 stream
> type 3 w*h 3200*2400
> 
> Similarly when in camcorder mode i modified video resolution values to WVGA
> then stream 3  changed to "1280*768" internally and stream1 and stream4
> becomes 800*480 (as selected by user)
> 
> E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10029 stream
> type 3 w*h 1280*768
> E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 1002a stream
> type 4 w*h 800*480
> E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 1002c stream
> type 1 w*h 800*480

I understand what you mean now. I do not think we should be blocking on this for 2.0 though. The resolution settings menu is not enabled by default (was only really for madai project). Therefore, this wouldn't be a user-facing feature by default. Under normal conditions, the camera/video resolutions are selected by the vendor at build time.

Inder/Hema: Does that make sense to you to take away blocking status on this?

Comment 83

[Hema Koka [:hema]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #82)
> (In reply to Pooja from comment #81)
> > (In reply to Justin D'Arcangelo [:justindarc] from comment #75)
> > 
> > > Or are you saying that when we do video recording, we need to also set the
> > > camera's photo resolution to be the same as the video resolution?
> > 
> > Hi Justin,
> > 
> > What i meant from having only one resolution is "at a time there is only one
> > master setting available"
> > 
> > As in LA, in camera mode i can only set picture size(camera resolution)
> > there is no option to set video resolution .Similarly UI has no option to
> > set camera resolution in camcoder mode
> > 
> > So in whatever mode whichever setting is chosen the other one gets
> > overwritten with appropriate values
> > 
> > Example:
> > 
> > Here in camera mode i set picture size to 8M Pixel
> > So stream3(snapshot) is set to 3200*2400 and stream 1 i.e preview becomes
> > 640*480
> > 
> > E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10010 stream
> > type 1 w*h 640*480
> > E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10011 stream
> > type 3 w*h 3200*2400
> > 
> > Similarly when in camcorder mode i modified video resolution values to WVGA
> > then stream 3  changed to "1280*768" internally and stream1 and stream4
> > becomes 800*480 (as selected by user)
> > 
> > E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 10029 stream
> > type 3 w*h 1280*768
> > E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 1002a stream
> > type 4 w*h 800*480
> > E/mm-camera-sensor(  255): port_sensor_caps_reserve:139 ide 1002c stream
> > type 1 w*h 800*480
> 
> I understand what you mean now. I do not think we should be blocking on this
> for 2.0 though. The resolution settings menu is not enabled by default (was
> only really for madai project). Therefore, this wouldn't be a user-facing
> feature by default. Under normal conditions, the camera/video resolutions
> are selected by the vendor at build time.
> 
> Inder/Hema: Does that make sense to you to take away blocking status on this?

Yes, based on a call with Inder today, this is not blocking 2.0 release. Changing tracking flag to indicate it and also clearing Inder's NI based on his request in the meeting today.

Comment 84

[Yeojin Chung [:YeojinC]]

New test case needs to be added. There is no existing test case.

Comment 85

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

Test case added in moztrap:

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

Comment 88

[Mike Habicher [:mikeh] (high bugzilla latency)]

Pooja, this bug is getting a bit old. Are you still seeing this issue with recent builds?

Comment 89

[Pooja]

Yes Mike we are still seeing this issue on 8x10 targets.

Comment 90

[Justin D'Arcangelo [:justindarc]]

Not working on this. Unassigning myself.

Comment 91

[BMO Automation]

Firefox OS is not being worked on