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

VERIFIED FIXED in Firefox 33

Status

()

Core
WebRTC: Audio/Video
--
major
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: jib, Assigned: jib)

Tracking

Trunk
mozilla35
All
Android
Points:
---

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8481735 [details] [diff] [review]
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
Attachment #8481735 - Flags: review?(blassey.bugs)
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.
Attachment #8481735 - Flags: review?(snorp)
Attachment #8481735 - Flags: review?(gpascutto)
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.
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.
Flags: needinfo?(jib)
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.
Attachment #8481735 - Flags: review?(snorp) → review+
Oh I didn't see gcp's comments. I agree with those, obviously.
Attachment #8481735 - Flags: review?(blassey.bugs) → review-
Created attachment 8482739 [details] [diff] [review]
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) (jwillcox@mozilla.com) 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.
Attachment #8482739 - Flags: feedback?(gpascutto)
Flags: needinfo?(jib)
Keywords: checkin-needed
Keywords: checkin-needed
just saw the r-
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.
Attachment #8481735 - Flags: review- → review+
Keywords: checkin-needed
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.
Attachment #8481735 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f73240a5521
Keywords: checkin-needed
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 on attachment 8482739 [details] [diff] [review]
WIP on exposing facingMode from cameras

moved to Bug 1061704.
Attachment #8482739 - Attachment is obsolete: true
Attachment #8482739 - Flags: feedback?(gpascutto)
https://hg.mozilla.org/mozilla-central/rev/7f73240a5521
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8481735 - Flags: approval-mozilla-aurora?
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+
Attachment #8481735 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8481735 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6a935cc1874
status-firefox33: --- → affected
status-firefox34: --- → fixed
status-firefox35: --- → fixed
Attachment #8481735 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/fbc322c42d06
status-firefox33: affected → fixed

Comment 19

3 years ago
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
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

3 years ago
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 Android-build@kpdm5.cbf.corp.google.com #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

3 years ago
Jan-Ivar,
As of now I am not able to change the status of bug to verified. Can you please update the same?
Sure thing. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.