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

VERIFIED FIXED in Firefox 32

Status

()

defect
P1
normal
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: CristinaM, Assigned: gcp)

Tracking

({regression, reproducible})

Trunk
Firefox 33
ARM
Android
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)

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.
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
Possible a regression of Bug 995777?
tracking-fennec: --- → ?
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?
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.
Assignee: nobody → gpascutto
tracking-fennec: ? → 32+
Status: NEW → ASSIGNED
Any update here?
I'll tackle it as soon as I finish my review queue.
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.
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}
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)
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+
Duplicate of this bug: 1025668
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)
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+
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
Depends on: 1029401
Priority: -- → P1
Target Milestone: --- → Firefox 33
https://hg.mozilla.org/mozilla-central/rev/0a03d24b11e8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Relanding test part with tests disabled on 2.3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e490053263
Blocks: 1032991
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
Status: REOPENED → ASSIGNED
The bug seems to be fixed on latest Nightly (2014-07-14);
Can you please uplift to Aurora before beta?
Flags: needinfo?(gpascutto)
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)
Changing flags to reflect only the tests were backed out.
Verified as fixed in build 33.0a1 (2014-07-14);
Device: Google Nexus 7 (Android 4.4.4).
Comment on attachment 8442868 [details] [diff] [review]
Patch 1. Fix inconsistent naming

Aurora+
Attachment #8442868 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Let's close this and deal with the tests later in a split bug.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Blocks: 1042795
Target Milestone: --- → Firefox 33
Comment on attachment 8506977 [details] [diff] [review]
Expand Android gUM + gUM Doorhanger tests. r=

Wrong bug.
Attachment #8506977 - Attachment is obsolete: true
I will mark this as Verified fixed based on comment 29 and comment 32.
Status: RESOLVED → VERIFIED
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.