1055270 - [KK Only][Flame][Camera] Face detection coordinates are inverted

Status: VERIFIED FIXED

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

Description

[Justin D'Arcangelo [:justindarc]]

With the latest KK-based build for Flame (v165), the face detection coordinates are inverted. For example, if a face is detected in the top-left corner of the viewfinder, the circle indicator will be drawn in the bottom-right corner instead.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: face_detect_jb.log

Reference logs on JB.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: face_detect_kk.log

Logs from KK, flame v165 image. Face detected is in the same physical location in the preview but the coordinates are reversed. This reversal also applies to touch-to-focus.

Inder: could you please take a look on the driver side and confirm the behaviour? thanks!

Comment 4

[Pooja]

We are observing similar issue on our QRD.
FD circles are misplaced.

The same driver works well in LA. So i guess issue is with the way we are drawing these FD circles in FFOS.

Else kindly let me know how to verify at GECKO end whether the received co-ordinates are proper or not.

Comment 5

[David Scravaglieri [:scravag]]

[Blocking Requested - why for this release]: Blocks CAF-v2.0-CC-metabug

Comment 6

[Jason Smith [:jsmith]]

QA Wanted to check if this happens on Flame KK 1.4

Comment 7

[No-Jun Park [:njpark]]

This bug is no longer reproducible in latest 2.0 and 2.1 KK build with v180.  I think we can close this issue?

Comment 8

[No-Jun Park [:njpark]]

Hi, can you still reproduce this issue with the latest base image on your side?

Comment 9

[Justin D'Arcangelo [:justindarc]]

I am still able to reproduce this with v180.

Comment 10

[No-Jun Park [:njpark]]

My apologies, this is still reproducible in 2.0, and not reproducible in 2.1 and master.

Easy test is to switch to self-facing camera, and turn your chin up and down.  the circle will move in reverse direction.

Comment 11

[Justin D'Arcangelo [:justindarc]]

(In reply to No-Jun Park [:njpark] from comment #10)
> My apologies, this is still reproducible in 2.0, and not reproducible in 2.1
> and master.
> 
> Easy test is to switch to self-facing camera, and turn your chin up and
> down.  the circle will move in reverse direction.

Hmm, I was still able to reproduce on master.

Comment 12

[No-Jun Park [:njpark]]

(In reply to No-Jun Park [:njpark] from comment #10)
> My apologies, this is still reproducible in 2.0, and not reproducible in 2.1
> and master.
> 
> Easy test is to switch to self-facing camera, and turn your chin up and
> down.  the circle will move in reverse direction.

Apologies again, it reproduces on 2.1 and master, the bug is still present.  Previously the circle was way off the mark, so I thought it was resolved.  Now it is closer, but still incorrect.

Comment 13

[Hema Koka [:hema]]

Adding Wesly for triage. I was told TAMs are triaging 2.0 noms at this point 

(Issue most likely on the driver side, Andrew will do a quick check with v180 and add his input here)

Thanks
Hema

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: JB/KK v180 logs

On flame-kk, v180 with today's gecko/gaia from b2g-inbound, holding the device in portrait with the face in the top-right hand corner of the preview, we get the following as the coordinates:

09-22 18:45:17.949  1256  1326 I Gecko   : Camera face[0] appended: id=1, score=71, bound=(546, 537)-(740, 887)

Where we assume that left=546, top=537, right=740, bottom=887. Since the coordinate system is fixed at landscape, that would put it in the bottom-right quadrant in landscape, or bottom-left in portrait (as observed in the app) despite the fact the face is in the top-right.

Using the same gecko, on flame-jb, same face in the same position, we get the following as the coordinates:

09-22 19:13:22.099  1236  1303 I Gecko   : Camera face[0] appended: id=1, score=63, bound=(-656, -792)-(-467, -451)

The face is in the same physical location but the coordinates are swapped and negated and fit with my expectations.

We should be interpreting the rect[0-4] structure correctly (0=left, 1=top, 2=right, 3=bottom) as per the header and our implementation:
https://www.codeaurora.org/cgit/quic/la/platform/system/core/tree/include/system/camera.h?h=aosp-new/master#n213
http://hg.mozilla.org/integration/b2g-inbound/file/5f06a3b5fd42/dom/camera/GonkCameraControl.cpp#l1163

The only difference in my build from the stock images is that I pushed all of the Gecko camera logs to logcat. Will attach a patch so you can reproduce if desired.

Comment 15

[Joshua Mitchell (Inactive)]

removing qa-wanted tag - 1.4 kk base is no longer being tested, and comment 14 and comment 12 satisfy current branch checking

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Turn on all Gecko camera logging, v1

pooja: Could you take a look at the logs/info I provided? If you apply this patch to a gecko tree, it will enable the prints I quoted in order to do your own testing/verification.

Comment 17

[Wayne Chang [:wchang]]

blocking for broken feature.

note - this seems fine on partner's kk/2.0 device.

Comment 18

[Hema Koka [:hema]]

(In reply to Wayne Chang [:wchang] from comment #17)
> blocking for broken feature.
> 
> note - this seems fine on partner's kk/2.0 device.

Wayne: Which partner KK based device in particular is working fine? Per Andrew's testing with same gaia/gecko version the behavior is different between JB and KK Flame. Its an issue on QRD also per Pooja's earlier comment 4. 

The logs from comment 14 shows that the coordinates coming out of the driver are opposite of what we should be getting. Please bring this up with the Flame partner. 

In the meantime, if Pooja can test with the patch on QRD, it will confirm if the issue is with driver on QRD or not. 

Thanks
Hema

Comment 19

[Wayne Chang [:wchang]]

It's on the unannounced device running 8x26 - Faramarz took one back last week.

Comment 20

[Pooja]

Attachment: screen shot of detected face

Hi I got below co-ordinates. Also have shared screen shot of FD circle.

I am not sure how can we conclude here that passed co-ordinates are not correct.

01-01 00:03:43.609  1358  1380 I Gecko   : Camera detected 1 face(s)
01-01 00:03:43.609  1358  1380 I Gecko   : Camera face[0] appended: id=1, score=52, bound=(10, -625)-(310, -92)
01-01 00:03:43.609  1358  1380 I Gecko   :     Left eye detected at (204, -437)
01-01 00:03:43.609  1358  1380 I Gecko   :     Right eye detected at (200, -242)
01-01 00:03:43.609  1358  1380 I Gecko   :     Mouth detected at (79, -353)

Comment 21

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #20)
> Created attachment 8494498 [details]
> screen shot of detected face
> 
> Hi I got below co-ordinates. Also have shared screen shot of FD circle.
> 
> I am not sure how can we conclude here that passed co-ordinates are not
> correct.
> 
> 01-01 00:03:43.609  1358  1380 I Gecko   : Camera detected 1 face(s)
> 01-01 00:03:43.609  1358  1380 I Gecko   : Camera face[0] appended: id=1,
> score=52, bound=(10, -625)-(310, -92)
> 01-01 00:03:43.609  1358  1380 I Gecko   :     Left eye detected at (204,
> -437)
> 01-01 00:03:43.609  1358  1380 I Gecko   :     Right eye detected at (200,
> -242)
> 01-01 00:03:43.609  1358  1380 I Gecko   :     Mouth detected at (79, -353)

In your screenshot, the circle is not aligned with the face in the viewfinder. If you point the viewfinder so that the face is in one of the corners of the frame, you'll see a much more exaggerated example of the problem.

Comment 22

[Justin D'Arcangelo [:justindarc]]

Pooja: Can you please verify using the info I provided in Comment 21?

Comment 23

[Pooja]

Attachment: screenshot with hardcoded FD co-ordinates in HAL

Hi Justin,
I verified this.
And so to debug this further I hardcoded  the left,top,right,bottom in HAL to below value.

left= rect[0]= -1000 , top = rect[1] = -1000, right=rect[2]= 0; bottom=rect[3] = 0;

As (-1000, -1000) represents the top-left of the camera field of view, and (1000, 1000) represents the bottom-right of the field of view

So with above hardcoded HAL values i.e (-1000,-1000) (0,0) the FD circle should always come at 1st quarter of FOV. But if you see the attached screen shot it was at 2nd quarter of FOV.

This proves that drivers are not sending inverted co-ordinates.


E/QCamera2HWI(  192): Pooja: faces[0].score: 53
E/QCamera2HWI(  192): Pooja: faces[0].rect[0]: -1000
E/QCamera2HWI(  192): Pooja: faces[0].rect[1]: -1000
E/QCamera2HWI(  192): Pooja: faces[0].rect[2]: 0
E/QCamera2HWI(  192): Pooja: faces[0].rect[3]: 0
I/Gecko   ( 3856): Camera face[0] appended: id=1, score=53, bound=(-1000, -1000)-(0, 0)

Justin your thoughts on this?

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Pooja from comment #23)
> Created attachment 8495908 [details]
> screenshot with hardcoded FD co-ordinates in HAL
> 
> Hi Justin,
> I verified this.
> And so to debug this further I hardcoded  the left,top,right,bottom in HAL
> to below value.
> 
> left= rect[0]= -1000 , top = rect[1] = -1000, right=rect[2]= 0;
> bottom=rect[3] = 0;
> 
> As (-1000, -1000) represents the top-left of the camera field of view, and
> (1000, 1000) represents the bottom-right of the field of view
> 
> So with above hardcoded HAL values i.e (-1000,-1000) (0,0) the FD circle
> should always come at 1st quarter of FOV. But if you see the attached screen
> shot it was at 2nd quarter of FOV.
> 
> This proves that drivers are not sending inverted co-ordinates.
> 
> 
> E/QCamera2HWI(  192): Pooja: faces[0].score: 53
> E/QCamera2HWI(  192): Pooja: faces[0].rect[0]: -1000
> E/QCamera2HWI(  192): Pooja: faces[0].rect[1]: -1000
> E/QCamera2HWI(  192): Pooja: faces[0].rect[2]: 0
> E/QCamera2HWI(  192): Pooja: faces[0].rect[3]: 0
> I/Gecko   ( 3856): Camera face[0] appended: id=1, score=53, bound=(-1000,
> -1000)-(0, 0)
> 
> Justin your thoughts on this?

Okay, I see now. In JB, landscape mode was used as the camera field of view. Now in KK, you are stating portrait mode is the camera field of view. The coordinate system seems to be fixed in both cases, independent of rotation (as you would expect).

Is it your opinion that the coordinates presented by the JB driver are *wrong* then? One of the drivers must be wrong since they present completely different values with the same gecko/gaia/image. We can work with that, but either way, the driver behaviour has changed between releases and we need to know *why* to ensure we understand what the right thing to do is.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

> Okay, I see now. In JB, landscape mode was used as the camera field of view.
> Now in KK, you are stating portrait mode is the camera field of view. The
> coordinate system seems to be fixed in both cases, independent of rotation
> (as you would expect).
> 
> Is it your opinion that the coordinates presented by the JB driver are
> *wrong* then? One of the drivers must be wrong since they present completely
> different values with the same gecko/gaia/image. We can work with that, but
> either way, the driver behaviour has changed between releases and we need to
> know *why* to ensure we understand what the right thing to do is.

Also: Is there any driver parameter we should be looking at, that alters how we interpret the coordinates given? If the difference between JB and KK is merely a configuration setting that has changed (default or otherwise) which we ignore, that could explain the difference.

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

Justin: Mike was kind enough to point out there is a CameraControl.sensorOrientation property that may be applicable. It is possible this changed between JB and KK from the driver perspective, and it needs to be taken into consideration for the conversion algorithm. I don't see it used though:

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/convert-face-to-pixel-coordinates.js
https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/viewfinder.js#L219

This might be the problem...

Comment 27

[Justin D'Arcangelo [:justindarc]]

(In reply to Andrew Osmond [:aosmond] from comment #26)
> Justin: Mike was kind enough to point out there is a
> CameraControl.sensorOrientation property that may be applicable. It is
> possible this changed between JB and KK from the driver perspective, and it
> needs to be taken into consideration for the conversion algorithm. I don't
> see it used though:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/convert-
> face-to-pixel-coordinates.js
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/
> viewfinder.js#L219
> 
> This might be the problem...

But, the sensor's orientation hasn't changed, right? The way I understood sensorOrientation was that it indicates the angle (0, 90, 180, 270) that the camera hardware is placed in respect to the device itself. I would not expect sensorOrientation to be different when the hardware itself hasn't changed.

However, I suppose that its possible. We need to look at what the value is for sensorOrientation between JB and KK to know if this has any impact.

Comment 28

[Justin D'Arcangelo [:justindarc]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #27)
> (In reply to Andrew Osmond [:aosmond] from comment #26)
> > Justin: Mike was kind enough to point out there is a
> > CameraControl.sensorOrientation property that may be applicable. It is
> > possible this changed between JB and KK from the driver perspective, and it
> > needs to be taken into consideration for the conversion algorithm. I don't
> > see it used though:
> > 
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/convert-
> > face-to-pixel-coordinates.js
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/
> > viewfinder.js#L219
> > 
> > This might be the problem...
> 
> But, the sensor's orientation hasn't changed, right? The way I understood
> sensorOrientation was that it indicates the angle (0, 90, 180, 270) that the
> camera hardware is placed in respect to the device itself. I would not
> expect sensorOrientation to be different when the hardware itself hasn't
> changed.
> 
> However, I suppose that its possible. We need to look at what the value is
> for sensorOrientation between JB and KK to know if this has any impact.

Confirming that the reason for the inverted coordinates is that the `sensorAngle` is different between JB and KK.

JB = 90
KK = 270

Comment 29

[Hema Koka [:hema]]

Thanks to Pooja, Mike, Andrew and Justin to narrow down the cause of this issue!

Comment 30

[Pooja]

Thanks Justing, Andrew and Mike for taking it further.
Removing NI as nothing pending from my side.

Comment 31

[Justin D'Arcangelo [:justindarc]]

Re-assigning to Diego.

Comment 32

[Diego Marcos [:dmarcos]]

I have a patch that fixes the issue but I don't feel comfortable with the solution. I came up with it after some trial and error. I haven't been able to find any docs that state how the coordinates system of the sensor are supposed to be oriented based on the sensor angle / orientation. Andrew pointed me to these android docs:

http://developer.android.com/reference/android/hardware/Camera.CameraInfo.html#orientation

It explains how the screen is oriented with respect to the sensor but not how the android coordinate system is affected with the sensor angle / orientation. 

After experimentation it looks like the sensor coordinates increase from the top-left corner of the sensor on landscape orientation (when sensorAngle API reports 90 on flame). I just want to find an official spec so I can solve this bug properly and it doesn't bite us again on other devices / sensors / orientations / firmware.

Comment 33

[Diego Marcos [:dmarcos]]

Needinfo to Inder to see if he can provide the specs requested on comment #32

Comment 34

[Inder]

Bhargav is checking if we have such a document.

Comment 35

[Diego Marcos [:dmarcos]]

More questions. Why does the sensor orientation reported from the driver has changed from 90 to 270? Was it incorrect before or is it incorrect now?

Comment 36

[Pooja]

(In reply to Diego Marcos [:dmarcos] from comment #35)
> More questions. Why does the sensor orientation reported from the driver has
> changed from 90 to 270? Was it incorrect before or is it incorrect now?

Its not related to JB or KK
sensor orientation depends on the sensor/board. it depends on how the sensor is mounted on a device

Also Unfortunately , we didn't found any document on "how the android coordinate system is affected with the sensor angle / orientation".

[1] is the link on how LA takes care of various sensor angle.
Please check once it can help.

[1]https://www.codeaurora.org/cgit/quic/la/platform/packages/apps/Camera2/tree/src/com/android/camera/ui/FaceView.java?h=LNX.LA.3.7_master#n195

Comment 37

[Justin D'Arcangelo [:justindarc]]

(In reply to Pooja from comment #36)
> (In reply to Diego Marcos [:dmarcos] from comment #35)
> > More questions. Why does the sensor orientation reported from the driver has
> > changed from 90 to 270? Was it incorrect before or is it incorrect now?
> 
> Its not related to JB or KK
> sensor orientation depends on the sensor/board. it depends on how the sensor
> is mounted on a device
> 
If this is true, then either the JB driver or the KK driver is broken. On the same Flame device, a different sensorAngle is reported with the KK driver versus the JB driver.

Comment 38

[Diego Marcos [:dmarcos]]

This is marked as a blocker but this bug only affects Flame + KK.

Comment 39

[David Flanagan [:djf]]

Mike: do you agree that this is an issue with the KK driver on the Flame?

Hema: if this is a driver bug, do you know how to escalate it to T2M so it gets fixed in the next base image?

Comment 40

[Hema Koka [:hema]]

David, 

Once we can confirm that it is a driver issue (this is happening on KK based Flame and KK based QRD), we can ping Wesly Huang to follow up with T2M. 

Thanks
Hema

Comment 41

[bhargavg1]

(In reply to Diego Marcos [:dmarcos] from comment #38)
> This is marked as a blocker but this bug only affects Flame + KK.

This is seen on MTP devices as well in CAF testing, right from v2.0 

Basically camera app should be able to handle differences in hardware something similar to,comment 36

Comment 42

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #28)
> 
> Confirming that the reason for the inverted coordinates is that the
> `sensorAngle` is different between JB and KK.
> 
> JB = 90
> KK = 270

If the sensor angle changed between the releases, why are our previews still coming out the right way? We use the sensorAngle to figure out how to rotate the preview stream, right?  But the preview stream still looks right. So in addition to changing the sensorAngle, the raw preview stream returned by the driver must have changed as well so that when rotated by the new sensorAngle it looks right.

My point is this: sensorAngle is conceptually the angle between the orientation of the camera sensor and the normal operating orientation of the phone.  But it does not actually have to match the physical hardware in practice. The driver has to provide an angle that is consistent with the drivers other outputs.  So even though the sensor chip in our Flame devices has not moved between JB and KK, if the firmware has changed so that the driver now reads pixels from the bottom to the top of the sensor instead of the top to the bottom, then the driver could legitimately be returning a new sensor angle.

The easiest explanation is that this is a driver issue. But if we want to claim that it is the driver's fault, we have to verify that:

1) There is no where in gaia that we are assuming a 90 degree sensor angle without actually using the sensorAngle property. So we should double-check all of the focus and face detection coordinate system code in gaia to make sure it is all properly parameterized by sensorAngle

2) Gecko is not manipulating any of the focus or face detection coordinates in any way

3) Android has not introduced any new API for KK that would have some kind of "rotate by 180" flag. If something like this exists and we have not picked it up in Gecko that would also explain the bug.

If we're certain about 1, 2, and 3, then we should ask CAF to look at their driver code.

Comment 43

[Diego Marcos [:dmarcos]]

Pooja said:

"Its not related to JB or KK sensor orientation depends on the sensor/board. it depends on how the sensor is mounted on a device"

Why then the driver reports 90 degrees in JB and now on KK it reports 270? It's clear that either there is or there was a bug in the driver. Pooja, can you please explain why the driver behavior has changed?

Comment 44

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #39)
> Mike: do you agree that this is an issue with the KK driver on the Flame?
> 
> Hema: if this is a driver bug, do you know how to escalate it to T2M so it
> gets fixed in the next base image?

The `sensorAngle` is off by 180deg between JB and KK. However, the video stream coming from the camera driver between JB and KK is also rotated 180deg. In the Camera app, we correctly apply the proper rotation to the viewfinder based on the `sensorAngle` value which is why this difference wasn't noticed until this week. Therefore, AFAICT, the `sensorAngle` is actually correct. What is not understood is why there is this discrepancy between JB and KK. Even still though, I'm leaning towards this *NOT* being a driver issue because our app is correctly rendering the viewfinder in the proper orientation between both JB and KK. IMO, if this was a driver issue, our viewfinder would be upside-down in either JB or KK.

With that being said, we do not rotate the face detection coordinates in the Camera app. It seems like the proper fix would be to apply a rotation transformation to the face detection coordinates in the application. According to the AOSP docs, there is a little Java code snippet provided that shows how to normalize the face detection coordinates into viewport coordinates:

http://developer.android.com/reference/android/hardware/Camera.Face.html#rect

To port this to JavaScript, we would just need to provide a JavaScript alternative to the Java `Matrix` class being used to apply the mathematical transformation.

Comment 45

[David Flanagan [:djf]]

Here's another thought...  If we are using sensorAngle in the computation of face tracking or focus coordinates, perhaps that is the wrong thing to do. In the code that Bhargav links to in comment 36, I don't see the sensorAngle used at all. So maybe we were not supposed to use it at all. In that case, our calculations probably have an implicit assumption of -90 degrees that is cancelled out by the 90 degree sensorAngle. And now that we're getting 270 they don't cancel out anymore.

Comment 46

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

Looking at the code linked-to in comment 36, it's more than coordinate rotation going on. Before doing the rotation, coordinates are swapped depending on the orientation, and what's larger: height or width.

            int rw, rh;
            rw = mUncroppedWidth;
            rh = mUncroppedHeight;
            // Prepare the matrix.
            if (((rh > rw) && ((mDisplayOrientation == 0) || (mDisplayOrientation == 180)))
                    || ((rw > rh) && ((mDisplayOrientation == 90) || (mDisplayOrientation == 270)))) {
                int temp = rw;
                rw = rh;
                rh = temp;
            }

It _looks_ like the driver is expected to return bogus values, and the app is compensating for them.

Comment 47

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

Ignore comment 46. After much discussion, here's what's going on:

https://etherpad.mozilla.org/sensor-orientation

Comment 48

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

Comment 49

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8499100 [details] [review]
Pull Request

I think this might be easier to read/follow if all the camera <-> screen conversion functions were in a single module (perhaps the CameraUtils module). Also, I see the fix for drawing the face circles to the viewfinder, but I don't see anything done to the touch-to-focus code. Have we confirmed if this issue also affects touch-to-focus? (I had a difficult time confirming this)

Comment 50

[Diego Marcos [:dmarcos]]

PR will be submitted at the end of the day

Comment 51

[Diego Marcos [:dmarcos]]

Comment on attachment 8499100 [details] [review]
Pull Request

The attached PR solves the issue. Pooja, can you please verify that it works for you?

The patch is ready for review. I will be adding tests over the weekend to cover the new added functions.

Comment 52

[David Flanagan [:djf]]

Comment on attachment 8499100 [details] [review]
Pull Request

See my comments on github. Overall, this looks like a really good piece of work, especially given the complexity of working through coordinate system transformations.

However, I'm giving r- because there are a number of places in the code where you swap the X and Y coordinates of a point (passing top for the X coordinate and left for the Y coordinate).  These seem like errors.  Either fix them, or add comments explaining why the swapping is being done.

I have not tried the code, but assume that it actually works. Presumably you swap the coordinates enough that everything comes out right in the end. But if we leave it like this it will make the code unmaintanable because fixing the mistakes in one place will break things.

Comment 53

[David Flanagan [:djf]]

A couple more things I noticed about this patch. It rotates in the same direction for converting sensor to screen coordinates as it does for the reverse conversion. I would expect it to have to rotate in the opposite directly.

Also, I've added another comment on github with a proposed simplification of the rotateArea() function that avoids the issues of swapping X and Y twice. I have not tested that it works, however.

Comment 54

[Diego Marcos [:dmarcos]]

Comment on attachment 8499100 [details] [review]
Pull Request

- Fixed all the coordinates inversions.
- Addressed review comments.
- The coordinates are now mirrored when detecting faces on the front camera

Pending:

- Improve documentation
- Add unit tests

Comment 55

[Diego Marcos [:dmarcos]]

Update PR with unit tests. I still want to do a 2nd pass and add more.

Comment 56

[Pooja]

(In reply to Diego Marcos [:dmarcos] from comment #51)
> Comment on attachment 8499100 [details] [review]
> Pull Request
> 
> The attached PR solves the issue. Pooja, can you please verify that it works
> for you?

FD circles appears properly with the your patch Diego.

Comment 57

[Diego Marcos [:dmarcos]]

Comment on attachment 8499100 [details] [review]
Pull Request

- I added more tests
- I updated docs based on Mike's suggestions and flagged him for review:

Mike, you don't have to review the code just the docs/comments. They are all contained in this file:

https://github.com/mozilla-b2g/gaia/pull/24720/files#diff-2

This patch is 95% complete. I just want to do one more pass to the tests. 

As soon I get the r+ I can land.

Comment 58

[David Flanagan [:djf]]

Comment on attachment 8499100 [details] [review]
Pull Request

r+ once the mirroring for TTF on the front camera is addressed properly.  I think the faceToCamera() function does the mirroring and rotation in the wrong order. (Of course it is hard without a device to test on!)  I think you need to mirror the pixels and then do the rotation. Note that since the pixel coordinate system is not centered at zero, mirroring is slightly more difficult and you can't reuse your mirrorArea() function.

I tested the patch (with all but the most recent commit) carefully on KK and on JB an am happy with the way it works.  All the code looks good except for the one item noted above.

Also, please file a non-blocking followup bug for further investigation and fine-tuning of the conversions:

 - I'm pretty sure that we're doing the conversions based on the viewfinder/screen size right now but that we should be doing them based on the scaled size of the preview stream.  We've got a 4:3 preview stream but are converting sensor to screen coordinages based on a 3:2 screen. Face detection and focus will be slightly more precise if we fix this. (Though I don't actually see any obvious bug when I test it.)

 - I'd like to test face detection and TTF at high zoom levels because there may be errors there. Or we may cause problems with those if we switch to using the scaled preview size for the conversions.

Comment 59

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

Comment on attachment 8499100 [details] [review]
Pull Request

LGTM with the one cosmetic issue addressed.

Comment 60

[Diego Marcos [:dmarcos]]

Attachment: Pull Request for 2.0

Carrying over r+

Comment 61

[Diego Marcos [:dmarcos]]

Waiting for green on travis before landing on master.

The patch for master cleanly uplifts to 2.1

This is the PR for 2.0:

https://github.com/mozilla-b2g/gaia/pull/24850

Comment 62

[bhargavg1]

(In reply to Inder from comment #34)
> Bhargav is checking if we have such a document.

-ni myself as Pooja answered in comment 36

Comment 63

[Diego Marcos [:dmarcos]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/71238815f3e632d26d78253e041a4428c4946a76

Comment 64

[Diego Marcos [:dmarcos]]

Comment on attachment 8499100 [details] [review]
Pull Request

[Approval Request Comment]
 CAF Blocker
 The PR on master uplifts cleanly to 2.1
 There's a separate PR for 2.0
 https://github.com/mozilla-b2g/gaia/pull/24850
[User impact] Face tracking and touch to focus don't work on KK
[Testing completed]: Verified by CAF

Comment 65

[Diego Marcos [:dmarcos]]

Comment on attachment 8500691 [details] [review]
Pull Request for 2.0

[Approval Request Comment]
 CAF Blocker
[User impact] Face tracking and touch to focus don't work on KK
[Testing completed]: Verified by CAF

Comment 66

[bhavana bajaj [:bajaj]]

Comment on attachment 8499100 [details] [review]
Pull Request

Approving for uplift and requesting qa verification on both 2.0 and 2.1 once this lands...

Comment 67

[Pooja]

Comment on attachment 8499100 [details] [review]
Pull Request

Issue get resolve with fix.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/da328c6cbabf2cffc2d362e282cacc93325d1f43

Comment 69

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8500691 [details] [review]
Pull Request for 2.0

Looks like this has real Gaia unit test orange.
https://tbpl.mozilla.org/php/getParsedLog.php?id=49675555&tree=Gaia-Try

Comment 70

[Diego Marcos [:dmarcos]]

That module doesn't exist anymore but the test suite remained after rebasing. I deleted the file and pushed again. It should go green now.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/c1f60895e5bcc6a951f3667c2a1e1bd39d393420

Comment 72

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/14dcfae1e42012ad5639dc9bd8d736b932da5afd

Comment 73

[Shine(Leave from Mozilla)]

Attachment: screenshoot

This issue can't repro on Flame 2.0/2.1/2.2, woodduck 2.0
See attachment: Verify_screenshoot.png
Reproducing rate: 0/5

Woodduck 2.0 biuld:
Gaia-Rev        cc690f8016b672475dc186bc7fd58aef45e684b7
Gecko-Rev       03d3ab62d5b07b915434f2d1d68495ad5915ecd2
Build-ID        20141118184148
Version         32.0
Flame 2.0 build:
Gaia-Rev        1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2bea026d4f86
Build-ID        20141118000207
Version         32.0
Flame 2.1 build:
Gaia-Rev        1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152
Build-ID        20141118001204
Version         34.0
Flame 2.2 build:
Gaia-Rev        4aee256937afe9db2520752650685ba61ce6097d
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/084441e904d1
Build-ID        20141118144012
Version         36.0a1