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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Cristina Madaras, QA [:CristinaM]]

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.

Comment 1

[Cristina Madaras, QA [:CristinaM]]

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

Comment 2

[Cristina Madaras, QA [:CristinaM]]

Possible a regression of Bug 995777?

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[Aaron Train [:aaronmt]]

Any update here?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

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

Comment 8

[Flaviu Cos]

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.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

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}

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: 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.

Comment 11

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

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

Comment 13

[Gian-Carlo Pascutto [:gcp]]

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)

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Add some more tests for gUM setup

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Attachment: 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.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

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

Comment 17

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c18081e9d032

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c18081e9d032

Comment 19

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0d227e054d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a03d24b11e8

Comment 20

[Carsten Book [:Tomcat]]

backed out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42620970&tree=Mozilla-Inbound , sorry

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a03d24b11e8

Comment 22

[Gian-Carlo Pascutto [:gcp]]

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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57e490053263

Comment 24

[Ed Morley [:emorley]]

Backed out for causing bug 1032991:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff73cfaac13

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/2ff73cfaac13

Comment 26

[Flaviu Cos]

The bug seems to be fixed on latest Nightly (2014-07-14);
Can you please uplift to Aurora before beta?

Comment 27

[Gian-Carlo Pascutto [:gcp]]

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

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Changing flags to reflect only the tests were backed out.

Comment 29

[Flaviu Cos]

Verified as fixed in build 33.0a1 (2014-07-14);
Device: Google Nexus 7 (Android 4.4.4).

Comment 30

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

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

Aurora+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f59e93abd36

Comment 32

[Teodora Vermesan (:TeoVermesan)]

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

Comment 33

[Gian-Carlo Pascutto [:gcp]]

Let's close this and deal with the tests later in a split bug.

Comment 34

[Gian-Carlo Pascutto [:gcp]]

Attachment: Expand Android gUM + gUM Doorhanger tests. r=

Comment 35

[Gian-Carlo Pascutto [:gcp]]

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

Wrong bug.

Comment 36

[Cristina Madaras, QA [:CristinaM]]

I will mark this as Verified fixed based on comment 29 and comment 32.

Comment 37

[Gian-Carlo Pascutto [:gcp]]

We also have in-tree tests for this now after bug 1042795 landed.