1060708 - Front and back cameras on Android not recognized by facingMode gUM constraint

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: detect user and environment cameras on android

A small piece was missing for this to work on android (i.e. not a regression).

B2G has tests for this but android unfortunately does not. We should add some if possible.

Safe fix. Just string-matching. Observed to work with URL on an S4.

Try build - https://tbpl.mozilla.org/?tree=Try&rev=96d9fc2baac4

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

I was hoping to land this over the weekend so I'll take the first r+ I can get.

This bug messes up the http://mozilla.github.io/webrtc-landing/pc_test_swap.html test-page which I wanted to reference in a post to the webrtc list.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

Review of attachment 8481735 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this will work but it's quite the kludge because the facingMode is a property of the Camera at some point in the abstraction. Inferring it from the name rather than querying it directly is pretty meh.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

jib, would it possible to deal with this in:
http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#213

That is, if mFacingMode is part of the aConstraints there, then we can extend 
CaptureCapability from http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/include/vie_capture.h#31 to include the facingMode and start filling this in in the Camera drivers.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

Review of attachment 8481735 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. Surely we can do better than this? On Android we can call this to figure out front/back facing, etc: http://developer.android.com/reference/android/hardware/Camera.CameraInfo.html

Glancing at MediaEngine.h it looks like we have no way to really use that stuff right now. Someone should fix that.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Oh I didn't see gcp's comments. I agree with those, obviously.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: WIP on exposing facingMode from cameras

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> It looks like this will work but it's quite the kludge because the
> facingMode is a property of the Camera at some point in the abstraction.
> Inferring it from the name rather than querying it directly is pretty meh.

I understand, but this is on par with what the other platforms are doing for this, given that this property is not exposed in any other way. We just discovered that android wasn't working on Friday, so I'd like to land this safe and small quick-fix and try to get it uplifted asap.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Glancing at MediaEngine.h it looks like we have no way to really use that
> stuff right now. Someone should fix that.

Here's a work in progress patch, but as you can tell it's a lot larger.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> jib, would it possible to deal with this in:
> http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/
> MediaEngineWebRTCVideo.cpp#213
> 
> That is, if mFacingMode is part of the aConstraints there, then we can
> extend 
> CaptureCapability from
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> video_engine/include/vie_capture.h#31 to include the facingMode and start
> filling this in in the Camera drivers.

My patch does something similar, except it tries to pull facingMode out, much like we do for mediaSource type. Basically, there are two types of properties, global inherent ones for the physical camera (facingMode, mediaSource), and ones that change for each virtual "capability" (width/height). Unfortunately, I never fully finished plumbing capabilities into camera selection (Bug 1003274), and that's a pain here.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

just saw the r-

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

Review of attachment 8481735 [details] [diff] [review]:
-----------------------------------------------------------------

Talked to jib on irc. With the context of this being a temporary, low risk fix for now to be quickly replaced with the "right" fix, I'm fine with this landing.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

Review of attachment 8481735 [details] [diff] [review]:
-----------------------------------------------------------------

Pro forma r+ based on the previous discussion and (as said) this looking like it'll work correctly.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f73240a5521

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've opened Bug 1061704 for all follow-on work so the bug here can be closed and be used to track uplift. Thanks!

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8482739 [details] [diff] [review]
WIP on exposing facingMode from cameras

moved to Bug 1061704.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f73240a5521

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

Approval Request Comment
[Feature/regressing bug #]: Unfortunate omission, not regression.
[User impact if declined]:

On Firefox for Android:

- Users get the back-camera as the default choice in the camera permission
  doorhanger on webpages that state they prefer the front camera.

- With persistent or disabled permissions, users get the back-camera always
  on webpages that state they prefer the front camera, with no good way to
  switch to front.

- Users are treated as if they have no camera on webpages that state they
  require either the front or the back camera specifically.

[Describe test coverage new/current, TBPL]:

Landed on m-c. I've verified that it works on an my S4 using the URL in this bug. Unfortunately, there is no automated test on android for this. B2G has one, but Idon't know what fake devices are available in the test harness to test this on android.

[Risks and why]: 
Code is extremely safe and simple, and pretty much mirrors what we do on b2g: It basically string-compares the internally-generated capture device names which follow a known pattern, and returns the correct enum if a match signifying front or back is found.

[String/UUID change made/needed]: none

Comment 15

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

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

We'll take this temporary fix in 34 until a proper fix can be created in bug 1061704. Aurora+

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8481735 [details] [diff] [review]
detect user and environment cameras on android

Approval Request Comment
[Feature/regressing bug #]: Unfortunate omission, not regression.
[User impact if declined]:

On Firefox for Android:

- Users get the back-camera as the default choice in the camera permission
  doorhanger on webpages that state they prefer the front camera.

- With persistent or disabled permissions, users get the back-camera always
  on webpages that state they prefer the front camera, with no good way to
  switch to front.

- Users are treated as if they have no camera on webpages that state they
  require either the front or the back camera specifically.

[Describe test coverage new/current, TBPL]:

Landed on m-c and new aurora 34 cycle. I've verified that it works on my S4 using the URL in this bug. There is no automated test on android for this. B2G has one, but I don't know what fake devices are available in the test harness to test this on android.

[Risks and why]: 
Code is extremely safe and simple, and pretty much mirrors what we do on b2g: It basically string-compares the internally-generated capture device names which follow a known pattern, and returns the correct enum if a match signifying front or back is found.

Hope to get this in the wild asap.

[String/UUID change made/needed]: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6a935cc1874

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fbc322c42d06

Comment 19

[Sudhir]

If this bug has some test steps to verify the issue. If so, can someone to update the bug with test steps information on the same.

I would like to pick up this bug for testing

Regards,
Sudhir

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

STR:
1. Open URL
2. Hit [Start!] and verify that doorhanger opens with "front facing camera" as the default choice.
3. Hit Share, and verify that you see yourself in one self-view (small) and one remote-view (large).
4. hit [Swap video] and verify that:
  - both videos turn off and
  - doorhanger opens with "back facing camera" as the default choice.
5. Hit Share, and verify that you see your environment instead of yourself in the same views.

Comment 21

[Sudhir]

The above test steps followed to test the bug. Looks everything fine. 

Following hardware and software configurations:

Hardware configurations:
Model Number:        Micromax AQ4501
Andorid Version :      4.4.4
Baseband version:    WR8.W1412.MD.WG.GMP.SP.W14.26.P3, 2014/08/15 11:39
Kernal version:         3.4.67 [e-mail] #1
                               wed Aug 20 14:13:29 PDT 2014
Build number:          KPW53

Software configurations:
Software configurations:
 Application version:     Firefox 36.0a 1 2014-11-20
 Application name:       Feenec
 Architecture :             armeabi-v7a
 Application build id:    20141120030202
 

I found the following logs on the URL. However, I have not validated any signalling(SIP) for the same.

pc1 found ICE candidate: {"candidate":"candidate:0 1 UDP 2129133823 192.168.1.2
51938 typ host","sdpMid":"","sdpMLineIndex":0}pc1 found ICE candidate: {"candidate":"candidate:0 2 UDP 2129
pc1 found ICE candidate: {"candidate":"candidate:0 2 UDP 2129133822 192.168.1.2
57718 typ host","sdpMid":"","sdpMLineIndex":0}
pc1 found ICE candidate: {"candidate":"candidate:0 1 UDP 2129133823 192.168.1.2
33968 typ host","sdpMid":"","sdpMLineIndex":1}
pc1 found ICE candidate: {"candidate":"candidate:0 2 UDP 2129133822 192.168.1.2
60146 typ host","sdpMid":"","sdpMLineIndex":1}
pc1 found ICE candidate: {"candidate":"candidate:1 1 UDP 1692991487
117.206.192.77 51938 typ srflx raddr 192.168.1.2 rport
51938","sdpMid":"","sdpMLineIndex":0}
pc1 found ICE candidate: {"candidate":"candidate:1 2 UDP 1692991486
117.206.192.77 57718 typ srflx raddr 192.168.1.2 rport
57718","sdpMid":"","sdpMLineIndex":0}
pc1 found ICE candidate: {"candidate":"candidate:1 2 UDP 1692991486
117.206.192.77 60146 typ srflx raddr 192.168.1.2 rport
60146","sdpMid":"","sdpMLineIndex":1}
pc1 found ICE candidate: {"candidate":"candidate:1 1 UDP 1692991487
117.206.192.77 33968 typ srflx raddr 192.168.1.2 rport
33968","sdpMid":"","sdpMLineIndex":1}
pc1 got end-of-candidates signal
HIP HIP HOORAY
pc2 got remote stream from pc1 addstream
replace_track = true
[object VideoStreamTrack]
Replaced video with fake
stopped old_video[object VideoStreamTrack]
Disabled fake
Replaced video with new video

Please let me know if I need to update with further logs on the same

Comment 22

[Sudhir]

Jan-Ivar,
As of now I am not able to change the status of bug to verified. Can you please update the same?

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sure thing. Thanks!