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

Status: VERIFIED FIXED

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

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The UA may call other function while recorder is stopping, that may cause unexpect problem.
Also follow by spec suggestion.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 788513 [details] [diff] [review]
patch v1

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

Needs a test!

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Sure, right away~

Comment 4

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case

test case for this one.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

try record
https://tbpl.mozilla.org/?tree=Try&rev=7f1491c1e960

Comment 6

[Jason Smith [:jsmith]]

(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 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d54396ae70b8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d54396ae70b8

Comment 10

[Jason Smith [:jsmith]]

The other try run on a separate bug shows that the mochitests do not trigger this bug with this patch, so marking as verified.

Comment 11

[Shelly Lin [:shelly]]

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.

Comment 12

[Shelly Lin [:shelly]]

Sorry, should be bug 899935 comment 5.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

MediaRecorder::ExtractEncodedData should not be reading mState off the main thread. That's the real problem here!

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

ya, we will remove mState check on encodedThread asap.

Comment 15

[Jason Smith [:jsmith]]

(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

[Shelly Lin [:shelly]]

Yes, bug 899935 covers the problem here.