Closed
Bug 903758
Opened 10 years ago
Closed 10 years ago
Media Recording - setting the recorder state to inActive on the stop method
Categories
(Core :: Audio/Video: Recording, defect)
Core
Audio/Video: Recording
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: rlin, Assigned: rlin)
References
Details
(Whiteboard: [FT: Media Recording, Sprint])
Attachments
(2 files)
941 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
2.82 KB,
patch
|
Details | Diff | Splinter Review |
The UA may call other function while recorder is stopping, that may cause unexpect problem. Also follow by spec suggestion.
Assignee | ||
Updated•10 years ago
|
Summary: Media Recording - setting the recorder state to invalid on the stop method → Media Recording - setting the recorder state to inActive on the stop method
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rlin
Assignee | ||
Comment 1•10 years ago
|
||
My bad, I think we may need to set to inactive first because other methods would check this state first and block the function call, for ex UA call the stop then call reqeustdata on other worker or something else.
Attachment #788513 -
Flags: review?(roc)
Comment on attachment 788513 [details] [diff] [review] patch v1 Review of attachment 788513 [details] [diff] [review]: ----------------------------------------------------------------- Needs a test!
Attachment #788513 -
Flags: review?(roc) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Sure, right away~
Updated•10 years ago
|
Blocks: MediaRecording
Assignee | ||
Comment 5•10 years ago
|
||
try record https://tbpl.mozilla.org/?tree=Try&rev=7f1491c1e960
Comment 6•10 years ago
|
||
(In reply to Randy Lin [:rlin] from comment #4) > Created attachment 788523 [details] [diff] [review] > test case > > test case for this one. I've actually already got a patch coming in bug 903781 that will cover this workflow (I confirmed it triggers this bug). I forgot to put the patch on the bug until now though (sorry about that). If you've confirmed the patch here fixes the issue manually, can we land it and use the patch from bug 903781?
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 788523 [details] [diff] [review] test case Sure, the bug 903781 test case can cover this one. try server result https://tbpl.mozilla.org/?tree=Try&rev=61e014bb9904
Attachment #788523 -
Flags: review?
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/d54396ae70b8
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d54396ae70b8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 10•10 years ago
|
||
The other try run on a separate bug shows that the mochitests do not trigger this bug with this patch, so marking as verified.
Status: RESOLVED → VERIFIED
Comment 11•10 years ago
|
||
Is this the correct fix? Setting the recording state to inactive at the call of Stop() immediately stops the while loop in ExtractEncodedData(), while the encoder might haven't finished encoding yet. Please see: http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#l157 or maybe we should just do: } while (!mEncoder->IsShutdown()); Regardless of this fix, I think the approach brought out by Rob in bug 899953 comment 5 might be a better solution for the long term.
Flags: needinfo?(roc)
Flags: needinfo?(rlin)
Comment 12•10 years ago
|
||
Sorry, should be bug 899935 comment 5.
MediaRecorder::ExtractEncodedData should not be reading mState off the main thread. That's the real problem here!
Flags: needinfo?(roc)
Assignee | ||
Comment 14•10 years ago
|
||
ya, we will remove mState check on encodedThread asap.
Flags: needinfo?(rlin)
Comment 15•10 years ago
|
||
(In reply to Randy Lin [:rlin] from comment #14) > ya, we will remove mState check on encodedThread asap. Can we get a bug on file for this?
Comment 16•10 years ago
|
||
Yes, bug 899935 covers the problem here.
Updated•10 years ago
|
Whiteboard: [FT: Media Recording, Sprint]
Updated•9 years ago
|
Component: Video/Audio → Video/Audio: Recording
Updated•9 years ago
|
No longer blocks: MediaRecording
You need to log in
before you can comment on or make changes to this bug.
Description
•