Closed Bug 903758 Opened 10 years ago Closed 10 years ago

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


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

Not set





(Reporter: rlin, Assigned: rlin)



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


(2 files)

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
Attached 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~
Attached 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
Attachment #788523 - Flags: review?
Closed: 10 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.
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:

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.