Closed Bug 850587 Opened 8 years ago Closed 8 years ago

Autoplay not working with getUserMedia in video element in Nightly

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox20 --- wontfix
firefox21 + fixed
firefox22 + fixed
relnote-firefox --- -

People

(Reporter: vidhuran2012, Assigned: roc)

Details

(Whiteboard: [getUserMedia][blocking-gum-])

Attachments

(4 files, 2 obsolete files)

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.
Attachment #724336 - Attachment mime type: text/plain → text/html
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
Whiteboard: [getUserMedia]
Roc - Any ideas why autoplay is failing here?
Flags: needinfo?(roc)
Nope, it should work.
Flags: needinfo?(roc)
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?
Status: UNCONFIRMED → NEW
relnote-firefox: --- → ?
Ever confirmed: true
OS: Windows Vista → All
Hardware: x86 → All
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum-]
Version: 22 Branch → Trunk
(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).
You can always call video.play() when a stream gets attached. Even with autoplay not working it shouldn't stop any of the demos.
Ya , that is what i did to my app to overcome this bug.
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.
Probably worthwhile to get a test here.
Flags: in-testsuite?
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.
Attachment #725349 - Flags: review?(rjesup) → review+
Attachment #725348 - Flags: review?(cpearce) → review+
(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
(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.
(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.
Note - What comment 14 implies btw is that we would *not* relnote this for 20.
(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?
(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 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
Attachment #725349 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/927ff547b360
https://hg.mozilla.org/mozilla-central/rev/fc48d16f075a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Keywords: verifyme
Attachment #725349 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified on trunk on 3/23 using existing test case - looks good.
Status: RESOLVED → VERIFIED
Keywords: verifyme
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?
Attachment #728925 - Flags: review?(jsmith) → review-
(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 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.
Attachment #728925 - Flags: review- → feedback+
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.
Attachment #728925 - Flags: feedback+ → review+
Roc - We're good to land the mochitest patch, right?
Flags: needinfo?(roc)
Yes, I just haven't gotten to it yet
Flags: needinfo?(roc)
(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
Flags: in-testsuite+ → in-testsuite?
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.
Attachment #728925 - Attachment is obsolete: true
Attached patch Autoplay MediaStream Test (obsolete) — Splinter Review
Attachment #732693 - Attachment is obsolete: true
Comment on attachment 732694 [details] [diff] [review]
Autoplay MediaStream Test

Patched up the test to ensure that we only call SimpleTest.finish once.
Attachment #732694 - Flags: review?(roc)
Comment on attachment 732694 [details] [diff] [review]
Autoplay MediaStream Test

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

Thanks for patching that up!
Attachment #732694 - Flags: review?(roc) → review+
You need to log in before you can comment on or make changes to this bug.