Media Recording - setting the recorder state to inActive on the stop method

VERIFIED FIXED in mozilla26

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: rlin, Assigned: rlin)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FT: Media Recording, Sprint])

Attachments

(2 attachments)

The UA may call other function while recorder is stopping, that may cause unexpect problem.
Also follow by spec suggestion.
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: nobody → rlin
Posted patch patch v1Splinter Review
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+
Sure, right away~
Posted patch test caseSplinter Review
test case for this one.
Attachment #788523 - Flags: review?
Blocks: 899878
Blocks: 903781
(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?
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?
https://hg.mozilla.org/mozilla-central/rev/d54396ae70b8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
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)
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)
ya, we will remove mState check on encodedThread asap.
Flags: needinfo?(rlin)
(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?
Yes, bug 899935 covers the problem here.
Whiteboard: [FT: Media Recording, Sprint]
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording
You need to log in before you can comment on or make changes to this bug.