850587 - Autoplay not working with getUserMedia in video element in Nightly

Status: VERIFIED FIXED

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

Description

[Vidhuran Harichandra Babu]

Attachment: test.html - The HTML source file of the page.

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:22.0) Gecko/20130312 Firefox/22.0
Build ID: 20130312031046

Steps to reproduce:

I have a simple test webpage (to demonstrate this bug) with mozGetUserMedia and the stream of it is assigned to a video element. 

http://rig5.kyla.fi/test.html 

I run the Nightly build of Firefox 22.0a1 (2013-03-12)


Actual results:

Despite the autoplay value showing up as "true" in the console.log , the video starts playing only after calling "play" function from console.log. 


Expected results:

The video should have started to play as soon as the "mediastream" is assigned to it.

Comment 1

[Jason Smith [:jsmith]]

Confirmed. On a reload of the test case, I end up seeing the following in the error console:

Timestamp: 3/13/2013 7:32:58 AM
Error: showBrowserSpecificIndicator: got neither video nor audio access
Source File: resource://app/modules/webrtcUI.jsm
Line: 231

Comment 2

[Jason Smith [:jsmith]]

Roc - Any ideas why autoplay is failing here?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nope, it should work.

Comment 4

[Jason Smith [:jsmith]]

This doesn't sound good on paper. My gut feeling is still ship with this bug, relnote it, and take care of this in FF 21.

Release mgmt, Randell, and Roc - What do you think?

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Jason Smith [:jsmith] from comment #4)
> This doesn't sound good on paper. My gut feeling is still ship with this
> bug, relnote it, and take care of this in FF 21.
> 
> Release mgmt, Randell, and Roc - What do you think?

I think it all depends on what our initial expected use/demo cases are, and whether this bug impacts them.

Would be good to determine whether this is a WebRTC issue or layout/video issue. If it's a WebRTC issue, I could see taking a bit more risk than normal to resolve in our last two betas (since it's a new feature).

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

You can always call video.play() when a stream gets attached. Even with autoplay not working it shouldn't stop any of the demos.

Comment 7

[Vidhuran Harichandra Babu]

Ya , that is what i did to my app to overcome this bug.

Comment 8

[Jason Smith [:jsmith]]

Right, there's a workaround to demos. I think my original concern for alarm was that this could be a potential compat issue down the line with Chrome's use of getUserMedia (especially since we are aiming to unprefix this API eventually).

Which probably means it's not end of the world if this doesn't make it into FF 20, but we should push hard to get this done sooner rather than later.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Refactor NotifyAutoplayDataReady to CheckAutoplayDataRallow autoplay to start on HAVE_CURRENT_DATA if the source is a stream

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Make NotifyHasCurrentData fire when a stream would be able to produce data if it was not blocked

Comment 11

[Jason Smith [:jsmith]]

Probably worthwhile to get a test here.

Comment 12

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

Comment on attachment 725349 [details] [diff] [review]
Part 2: Make NotifyHasCurrentData fire when a stream would be able to produce data if it was not blocked

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

::: content/media/MediaStreamGraph.cpp
@@ +165,5 @@
>      aStream->mBuffer.AdvanceKnownTracksTime(aStream->mUpdateKnownTracksTime);
>    }
> +  if (aStream->mBuffer.GetEnd() > 0) {
> +    aStream->mHasCurrentData = true;
> +  }

Minor efficiency question: does it make sense to test mHasCurrentData before checking whether to set it?  It seems to only transition from false to true, never back the other way.

::: content/media/MediaStreamGraph.h
@@ +507,5 @@
>     */
>    bool mNotifiedBlocked;
> +  /**
> +   * True if some data can be produced by this stream if/when it's unblocked.
> +   * Set by the stream itself on the MediaStreamGraph thread.

Is there any way (in the future) this could ever transition from true to false?  If not (and it appears so), we may want to state so more explicitly here.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] from comment #12)
> Minor efficiency question: does it make sense to test mHasCurrentData before
> checking whether to set it?  It seems to only transition from false to true,
> never back the other way.

I don't think so.

> Is there any way (in the future) this could ever transition from true to
> false?  If not (and it appears so), we may want to state so more explicitly
> here.

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/927ff547b360
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc48d16f075a

Comment 14

[Alex Keybl [:akeybl]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Right, there's a workaround to demos. I think my original concern for alarm
> was that this could be a potential compat issue down the line with Chrome's
> use of getUserMedia (especially since we are aiming to unprefix this API
> eventually).

Given the concern is down the line, let's not relnote and instead fix in FF21.

Comment 15

[Maire Reavy [:mreavy]]

(In reply to Alex Keybl [:akeybl] from comment #14)
> (In reply to Jason Smith [:jsmith] from comment #8)
> > Right, there's a workaround to demos. I think my original concern for alarm
> > was that this could be a potential compat issue down the line with Chrome's
> > use of getUserMedia (especially since we are aiming to unprefix this API
> > eventually).
> 
> Given the concern is down the line, let's not relnote and instead fix in
> FF21.

I definitely want to uplift the fix for this to FF21. I haven't talked to Roc yet about how risky uplifting this would be.

However, I think the bigger question is whether we need to uplift the fix for this to FF20 since we are in late Beta for FF20, and gUM is enabled in FF20.

IMO I think we are ok with a relnote for FF20 saying the bug is fixed in FF21 (especially since we have a work around in the meantime), assuming Roc as the patch author thinks uplift is a good idea.

Comment 16

[Jason Smith [:jsmith]]

Note - What comment 14 implies btw is that we would *not* relnote this for 20.

Comment 17

[Maire Reavy [:mreavy]]

(In reply to Jason Smith [:jsmith] from comment #16)
> Note - What comment 14 implies btw is that we would *not* relnote this for
> 20.

Why not?

Comment 18

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #16)
> > Note - What comment 14 implies btw is that we would *not* relnote this for
> > 20.
> 
> Why not?

That's what the flag relnote-firefox = - means to my understanding. Unless akeybl corrects me otherwise.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 725349 [details] [diff] [review]
Part 2: Make NotifyHasCurrentData fire when a stream would be able to produce data if it was not blocked

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: some getUserMedia demos might not work
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): low risk. Only affects MediaStream code.
String or UUID changes made by this patch: none

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/927ff547b360
https://hg.mozilla.org/mozilla-central/rev/fc48d16f075a

Comment 21

[Jason Smith [:jsmith]]

Verified on trunk on 3/23 using existing test case - looks good.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3302b3b38631
https://hg.mozilla.org/releases/mozilla-aurora/rev/28a6a1823441

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase designed to stress text-overflow:ellipsis on small blocks

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 728925 [details] [diff] [review]
testcase designed to stress text-overflow:ellipsis on small blocks

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

I actually don't know this area of the codebase that much, so it might be better to find an alternative reviewer for the final review. I'll add comments based on what I'm seeing in the patch and in the code.

review- though for the missing document.getElementById.

::: content/media/test/test_streams_autoplay.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

The original bug was filed using a LocalMediaStream derived from the getUserMedia flow. Are we testing this flow in this test?

@@ +18,5 @@
> +if (media == null) {
> +  todo(false, "No media supported.");
> +  SimpleTest.finish();
> +} else {
> +  v1.src = media.name;

Don't you need to define what v1 is with document.getElementById?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jason Smith [:jsmith] from comment #24)
> The original bug was filed using a LocalMediaStream derived from the
> getUserMedia flow. Are we testing this flow in this test?

No, we're testing a different kind of stream because that's an easier test to write. The test still fails before this patch and passes afterward, though.

> @@ +18,5 @@
> > +if (media == null) {
> > +  todo(false, "No media supported.");
> > +  SimpleTest.finish();
> > +} else {
> > +  v1.src = media.name;
> 
> Don't you need to define what v1 is with document.getElementById?

No. "Global scope pollution" is now in the spec and we support it in both quirks and standards mode.

Comment 26

[Jason Smith [:jsmith]]

Comment on attachment 728925 [details] [diff] [review]
testcase designed to stress text-overflow:ellipsis on small blocks

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

Looks fine then after input. I might suggest though still asking for a secondary reviewer who has typically added tests in this directory.

Didn't realize you could do the v1 reference you did, but confirmed what you stated myself through a test page.

Comment 27

[Jason Smith [:jsmith]]

Comment on attachment 728925 [details] [diff] [review]
testcase designed to stress text-overflow:ellipsis on small blocks

Chris Pearce did a review in IRC, looks good.

Comment 28

[Jason Smith [:jsmith]]

Roc - We're good to land the mochitest patch, right?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I just haven't gotten to it yet

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://bugzilla.mozilla.org/show_bug.cgi?id=850587

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Errr https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed978214fdb

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> Errr https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed978214fdb

Backed out for pretty frequent failures on debug Windows XP & 7.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c50426438d90

https://tbpl.mozilla.org/php/getParsedLog.php?id=21341447&tree=Mozilla-Inbound

02:45:50     INFO -  205503 INFO TEST-START | /tests/content/media/test/test_streams_autoplay.html
02:45:50     INFO -  ++DOMWINDOW == 33 (21E6EDD8) [serial = 4263] [outer = 0D0CA438]
02:45:50     INFO -  205504 INFO TEST-PASS | /tests/content/media/test/test_streams_autoplay.html | playback started
02:45:51     INFO -  205505 INFO TEST-PASS | /tests/content/media/test/test_streams_autoplay.html | playback started
02:45:51     INFO -  205506 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_streams_autoplay.html | [SimpleTest.finish()] this test already called finish!
02:45:51     INFO -  205507 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_streams_autoplay.html | called finish() multiple times
02:45:51     INFO -  205508 INFO TEST-END | /tests/content/media/test/test_streams_autoplay.html | finished in 435ms

Comment 33

[Jason Smith [:jsmith]]

Hmm...that probably implies that the onplaying event fired multiple times within the same test run, which results in SimpleTest.finish being executed multiple times within one context of the test.

Comment 34

[Jason Smith [:jsmith]]

Attachment: Autoplay MediaStream Test

Comment 35

[Jason Smith [:jsmith]]

Attachment: Autoplay MediaStream Test

Comment 36

[Jason Smith [:jsmith]]

Comment on attachment 732694 [details] [diff] [review]
Autoplay MediaStream Test

Patched up the test to ensure that we only call SimpleTest.finish once.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 732694 [details] [diff] [review]
Autoplay MediaStream Test

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

Thanks for patching that up!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/498538b6a1cd

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/498538b6a1cd