Closed
Bug 1395022
Opened 7 years ago
Closed 7 years ago
MediaRecorder fires start event when erroring
Categories
(Core :: Audio/Video: Recording, defect, P2)
Core
Audio/Video: Recording
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bryce, Assigned: bryce)
References
Details
Attachments
(2 files)
There are several error paths that result in the start event being fired by the MediaRecorder. This will often lead to a second start event being fired after the recorder has already started. This is undesirable for a number of reasons such as not meeting the spec, and resulting in the onstart handler being called again. JSFiddle showing the issue: https://jsfiddle.net/SingingTree/LrpdLnr6/ See the web console for "onstart". The cause is that in many failure cases the MediaRecorder makes an internal call to Session::DoSessionEndTask[1]. This method fires a start event to cover the case where a start was not already fired, it then sets a flag indicating that DoSessionEndTask is not required (which is also set elsewhere). There is inconsistent checking around the flag that gates calls to DoSessionEndTask such that DoSessionEndTask will be called without first checking that flag in many cases. To complicate matters, DoSessionEndTask performs other functionality that is important. It's not clear to me that the firing of start and these other functions should be coupled as it is. However, I've not had time to investigate further. [1]: https://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#817
Updated•7 years ago
|
Rank: 25
Priority: -- → P2
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b8c9612d54f
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fca25ed16859
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8904347 [details] Bug 1395022 - Fix MediaRecorder firing unnecessary start event when erroring. https://reviewboard.mozilla.org/r/176134/#review181194
Attachment #8904347 -
Flags: review?(apehrson) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8904348 [details] Bug 1395022 - Add test to check only one start event is fired by MediaRecorder when erroring. https://reviewboard.mozilla.org/r/176136/#review181196 ::: dom/media/test/test_mediarecorder_fires_start_event_once_when_erroring.html:7 (Diff revision 1) > +<html> > +<head> > + <title>Bug 1395022 - MediaRecorder fires start event when erroring.</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > + <script type="text/javascript" src="manifest.js"></script> I don't think you need manifest.js here. It's for managing testing multiple media files in the same test. ::: dom/media/test/test_mediarecorder_fires_start_event_once_when_erroring.html:21 (Diff revision 1) > + let oscilator2 = audioContext.createOscillator(); > + oscilator2.connect(destination2); > + oscilator2.start(); Shouldn't be a need for `oscilator2` to trigger an error. Unsure if `oscilator1` is needed. Doubtful but maybe.
Attachment #8904348 -
Flags: review?(apehrson) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904348 [details] Bug 1395022 - Add test to check only one start event is fired by MediaRecorder when erroring. https://reviewboard.mozilla.org/r/176136/#review181196 > Shouldn't be a need for `oscilator2` to trigger an error. > > Unsure if `oscilator1` is needed. Doubtful but maybe. Removed oscialtor 2. Leaving 1 in as without some data source feeding into the MediaStreamDestination we never fire start (this may change in future, as Chrome don't bother waiting).
Comment 9•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s a4fafdfadf35 -d 08d59706aac1: rebasing 418132:a4fafdfadf35 "Bug 1395022 - Fix MediaRecorder firing unnecessary start event when erroring. r=pehrsons" merging dom/media/MediaRecorder.cpp warning: conflicts while merging dom/media/MediaRecorder.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/35a2a0327e51 Fix MediaRecorder firing unnecessary start event when erroring. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/dfc055f4d93e Add test to check only one start event is fired by MediaRecorder when erroring. r=pehrsons
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/35a2a0327e51 https://hg.mozilla.org/mozilla-central/rev/dfc055f4d93e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Assignee: nobody → bvandyk
You need to log in
before you can comment on or make changes to this bug.
Description
•