Regression: Selecting "No video" option from the permission prompt still shows the content of the device camera.

VERIFIED FIXED in Firefox 32

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: CristinaM, Assigned: gcp)

Tracking

({regression, reproducible})

Trunk
Firefox 33
ARM
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 unaffected, firefox30 unaffected, firefox31 unaffected, firefox32+ verified, firefox33 verified, fennec32+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Environment:
Build: 32.0a1 (2014-06-02)
Device: Motorola Razr (Android 4.0.4)

Steps to reproduce:
1. Go to  http://mozilla.github.com/webrtc-landing/gum_test.html;
2. Tap on "Audio & Video";
3. After the permission prompt appears, at video source select "No video" and tap on "Share".

Expected result:
No video should be displayed.

Actual result:
A video appears showing the content from the device camera.
(Reporter)

Comment 1

4 years ago
Regression window:

Last good revision: cbe4f69c2e9c (2014-05-27)
First bad revision: e017c15325ae (2014-05-28)

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae
status-firefox29: --- → unaffected
status-firefox30: --- → unaffected
status-firefox31: --- → unaffected
status-firefox32: --- → affected
(Reporter)

Comment 2

4 years ago
Possible a regression of Bug 995777?

Updated

4 years ago
tracking-fennec: --- → ?

Updated

4 years ago
tracking-firefox32: --- → ?

Updated

4 years ago
Keywords: regression, reproducible
Summary: Selecting "No video" option from the permission prompt, still shows the content of the device camera. → Regression: Selecting "No video" option from the permission prompt still shows the content of the device camera.
Backing out Bug 995777 prevents the webrtc dialog from showing when visiting webrtc sites. So this is caused by some other bug. Though 995777 may have prevented this bug from being discovered. For right now I am going to mark this blocking the webrtc upgrade bug.
Blocks: 987979
If someone on Android could look at this that'd be super-helpful; I doubt it's the 3.50 upgrade, though certainly it's possible.  Perhaps some of the other tabvideo source changesets?
(Assignee)

Comment 5

4 years ago
The 3.50 upgrade has all-new code to detect the number and name of the cameras. It's possible that a subtle or not-so-subtle change in the names it generates trips up the JS code that deals with displaying the doorhanger.
tracking-firefox32: ? → +
Assignee: nobody → gpascutto
tracking-fennec: ? → 32+
(Reporter)

Updated

4 years ago
status-firefox33: --- → affected

Updated

4 years ago
Status: NEW → ASSIGNED
Any update here?
(Assignee)

Comment 7

4 years ago
I'll tackle it as soon as I finish my review queue.

Comment 8

4 years ago
Looks like whatever option I choose to share form the contextmenu ('back facing camera', 'front facing camera', 'choose a tab to stream' or 'no video') the 'Back facing camera' will be shared.
(Assignee)

Comment 9

4 years ago
Looks like the enumeration of the cameras is fine, and their naming doesn't cause any problems or hasn't changed in a relevant way.

We seem to be arriving in this callback:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebrtcUI.js#100
but with a mismatch between videoSource and videoDevice:
D/GeckoBrowser(16648): inputs: {"videoSource":0}
(Assignee)

Comment 10

4 years ago
Created attachment 8442868 [details] [diff] [review]
Patch 1. Fix inconsistent naming

So, this was indeed regressed by bug 995777.

That bug, for some reason, decided to change the naming of things from audioDevice and videoDevice to audioDevice and videoSource...but only in some places, and not in others.

I think I'd prefer to undo that entirely, because the source right now has it inconsistent all over, but those strings are localized, so maybe that generates additional l10n churn.

This patch is the simplest fix but leaves the inconsistence.

Oh, and let me check if I can add some more tests for this.
Attachment #8442868 - Flags: review?(blassey.bugs)
(Assignee)

Updated

4 years ago
Blocks: 995777
Comment on attachment 8442868 [details] [diff] [review]
Patch 1. Fix inconsistent naming

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

The reason for the entity change is that the string changed, and that's how our l10n system works
Attachment #8442868 - Flags: review?(blassey.bugs) → review+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1025668
(Assignee)

Comment 13

4 years ago
I'm trying to expand the test coverage of gUM in Robocop, but alas:

08:16:26     INFO -  06-23 08:16:16.414 D/****CameraHAL( 1293): 148: camera_start_preview() ENTER
08:16:26     INFO -  06-23 08:16:16.414 I/CameraHardware( 1293): startPreview: in startpreview
08:16:26     INFO -  06-23 08:16:16.414 I/CameraHardware( 1293): trying the node /dev/video0 width=640 height=480
08:16:26     INFO -  06-23 08:16:16.445 E/V4L2Camera( 1293): StartStreaming: Unable to start capture: Invalid argument
08:16:26     INFO -  06-23 08:16:16.445 I/CameraHardware( 1293): startPreview: Camera.StartStreaming failed
08:16:26     INFO -  06-23 08:16:16.445 E/V4L2Camera( 1293): Uninit: VIDIOC_DQBUF Failed
08:16:26     INFO -  06-23 08:16:16.445 E/V4L2Camera( 1293): Uninit: VIDIOC_DQBUF Failed
08:16:26     INFO -  06-23 08:16:16.445 E/V4L2Camera( 1293): Uninit: VIDIOC_DQBUF Failed
08:16:26     INFO -  06-23 08:16:16.500 E/WEBRTC-JC( 3122): startCapture failed
08:16:26     INFO -  06-23 08:16:16.500 E/WEBRTC-JC( 3122): java.lang.RuntimeException: startPreview failed
08:16:26     INFO -  06-23 08:16:16.500 E/WEBRTC-JC( 3122): 	at android.hardware.Camera.startPreview(Native Method)
08:16:26     INFO -  06-23 08:16:16.500 E/WEBRTC-JC( 3122): 	at org.webrtc.videoengine.VideoCaptureAndroid.startCapture(VideoCaptureAndroid.java:207)
08:16:26     INFO -  06-23 08:16:16.500 E/WEBRTC-JC( 3122): 	at dalvik.system.NativeStart.run(Native Method)
(Assignee)

Comment 14

4 years ago
Created attachment 8444592 [details] [diff] [review]
Patch 2. Add some more tests for gUM setup
Attachment #8444592 - Flags: review?(mark.finkle)
(Assignee)

Comment 15

4 years ago
Created attachment 8444594 [details] [diff] [review]
Patch 3. Workaround for buggy drivers

That function isn't supposed to fail, but bugs in Android and bugs in the Camera driver can cause it to fail anyway. Protecting against it won't hurt.
Attachment #8444594 - Flags: review?(blassey.bugs)
Attachment #8444594 - Flags: review?(blassey.bugs) → review+
Attachment #8444592 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 16

4 years ago
Unfortunately making the tests use Tab Streaming seems to expose another bug. I'll land the first patch with the fix for this bug and will look at this one so we can enable more tests.

16:20:57     INFO -   0  libxul.so!mozilla::MediaEngineTabVideoSource::StopRunnable::Run() [MediaEngineTabVideoSource.cpp:3522953e529c : 64 + 0x2]
16:20:57     INFO -       r4 = 0x52005d90    r5 = 0x00000000    r6 = 0x4752683c    r7 = 0x4752686f
16:20:57     INFO -       r8 = 0x472022e0    r9 = 0x00000001   r10 = 0x00000000    fp = 0x0000000f
16:20:57     INFO -       sp = 0x47526808    lr = 0x4db4c593    pc = 0x4e399fe0
16:20:57     INFO -      Found by: given as instruction pointer in context
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c18081e9d032
Keywords: leave-open
(Assignee)

Updated

4 years ago
Depends on: 1029401
https://hg.mozilla.org/mozilla-central/rev/c18081e9d032

Updated

4 years ago
Priority: -- → P1
Target Milestone: --- → Firefox 33
(Assignee)

Comment 19

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0d227e054d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a03d24b11e8
Keywords: leave-open
backed out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42620970&tree=Mozilla-Inbound , sorry
https://hg.mozilla.org/mozilla-central/rev/0a03d24b11e8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

4 years ago
Relanding test part with tests disabled on 2.3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e490053263

Updated

4 years ago
Blocks: 1032991
https://hg.mozilla.org/mozilla-central/rev/57e490053263
Backed out for causing bug 1032991:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff73cfaac13
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 33 → ---
No longer blocks: 1032991
Depends on: 1032991
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/2ff73cfaac13

Updated

4 years ago
Status: REOPENED → ASSIGNED

Comment 26

4 years ago
The bug seems to be fixed on latest Nightly (2014-07-14);
Can you please uplift to Aurora before beta?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 27

4 years ago
Comment on attachment 8442868 [details] [diff] [review]
Patch 1. Fix inconsistent naming

Approval Request Comment
[Feature/regressing bug #]: Bug 995777?
[User impact if declined]: Sharing of video camera even if user denies.
[Describe test coverage new/current, TBPL]: Test coverage backed out of m-c due to yellows.
[Risks and why]: No real risk of breaking it further.
[String/UUID change made/needed]: N/A
Attachment #8442868 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 28

4 years ago
Changing flags to reflect only the tests were backed out.
status-firefox33: affected → fixed

Comment 29

4 years ago
Verified as fixed in build 33.0a1 (2014-07-14);
Device: Google Nexus 7 (Android 4.4.4).
status-firefox33: fixed → verified
Comment on attachment 8442868 [details] [diff] [review]
Patch 1. Fix inconsistent naming

Aurora+
Attachment #8442868 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Whiteboard: [checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f59e93abd36
status-firefox32: affected → fixed
Whiteboard: [checkin-needed-aurora]
Tested with:
Device: Samsung Galaxy Nexus
OS: Android 4.2.1

No video is displayed, so verified fixed on:
Build: Firefox for Android 32 Beta 1
status-firefox32: fixed → verified
(Assignee)

Comment 33

3 years ago
Let's close this and deal with the tests later in a split bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1042795
Target Milestone: --- → Firefox 33
(Assignee)

Comment 34

3 years ago
Created attachment 8506977 [details] [diff] [review]
Expand Android gUM + gUM Doorhanger tests. r=
(Assignee)

Comment 35

3 years ago
Comment on attachment 8506977 [details] [diff] [review]
Expand Android gUM + gUM Doorhanger tests. r=

Wrong bug.
Attachment #8506977 - Attachment is obsolete: true
(Reporter)

Comment 36

3 years ago
I will mark this as Verified fixed based on comment 29 and comment 32.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 37

3 years ago
We also have in-tree tests for this now after bug 1042795 landed.
You need to log in before you can comment on or make changes to this bug.