909670 - MediaRecorder - Implement MediaRecorderSession

Status: VERIFIED FIXED

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

Description

[Shelly Lin [:shelly]]

When we call stop() of MediaRecorder, the encoder might still be running. If someone calls stop() and then immediately start(), we would then encounter a problem of having two overlapping recordings.

Rob has suggested an idea of MediaRecorderSession, which is an object representing a single recording, containing information of mReadThread, mEncoder, mStream, mTrackUnionStream, mStreamPort, mEncodedBufferCache, mMimeType, mTimeSlice. MediaRecorder would keep a list of active MediaRecorderSession.

Please find more details in bug 899935 comment 5.

Comment 1

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

Take it. :)

Comment 2

[u459114]

Attachment: WIP_Record_Session

WIP.

1. Use single mReadThread for all sessions.
2. Move union stream/ port/ encoder into RecordSession

Comment 3

[Jason Smith [:jsmith]]

We probably want to get a mochitest here btw that exercises the API workflow that this bug was filed about. I think we would need a mochitest that exercises a recording session via doing start, stop, start, stop. That means we should exercise the media recorder through two recording sessions in the test.

Comment 4

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

Comment on attachment 798198 [details] [diff] [review]
WIP_Record_Session

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

Found some extra spaces in this patch.
Also need to avoid use the raw pointer to hold the recording session.

::: content/media/MediaRecorder.cpp
@@ +71,4 @@
>    nsresult mError;
>  };
>  
> +// Three main tasks 

space at tail.

@@ +83,3 @@
>    {
>    public:
> +    StartRunnable(RecordSession *session)

could we use nsRefPtr?

@@ +91,4 @@
>  
> +      mSession->PrepareInternal();
> +      mSession->StartInternal();
> +    

space

@@ +124,5 @@
> +  {
> +  public:
> +    DestroyRunnable(RecordSession *session)
> +      : mSession(session) {}
> + 

space

@@ +137,5 @@
> +  private:
> +    RecordSession *mSession;
> +  };
> +
> +  friend class StartRunnable; 

space @tail

@@ +280,1 @@
>  

shelly have removed the mState != RecordingState::Inactive.

@@ +342,5 @@
> +  // Extract task done.
> +  mSession = nullptr;
> +
> +  // By setting mState as Inactive, do-loop in 
> +  //  MediaRecorder::ExtractEncodedData end automactically.

// Media..

@@ +360,1 @@
>  

{}

@@ +369,5 @@
>      return;
>    }
> +  if (mSession)
> +    mSession->Resume();
> + 

{}

::: content/media/MediaRecorder.h
@@ +108,4 @@
>    // Runnable thread for read data from mediaEncoder. It starts at MediaRecorder::Start() and stops at MediaRecorder::Stop().
>    nsCOMPtr<nsIThread> mReadThread;
>    // The MediaEncoder object initializes on start() and destroys in ~MediaRecorder.
> +  //nsRefPtr<MediaEncoder> mEncoder;

remove it..

Comment 5

[u459114]

(In reply to Randy Lin [:rlin] from comment #4)
> Comment on attachment 798198 [details] [diff] [review]
> WIP_Record_Session
> 
> Review of attachment 798198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Found some extra spaces in this patch.
> Also need to avoid use the raw pointer to hold the recording session.
I intend not to use reference count here.
MediaRecorder create RecordSession but does not control the life cycle of RecordSession.
RecordSession kill itself immidiately after session goto end state.

Comment 6

[u459114]

(In reply to Jason Smith [:jsmith] from comment #3)
> We probably want to get a mochitest here btw that exercises the API workflow
> that this bug was filed about. I think we would need a mochitest that
> exercises a recording session via doing start, stop, start, stop. That means
> we should exercise the media recorder through two recording sessions in the
> test.
Agree. I did some test on call start-stop continously, with this patch, media recorder keep stay in correct status with no abort
I will create mochitest for this bug as well.

Comment 7

[u459114]

A potentail bug in this patch is "MediaRecorder be destroy before while RecordSession is still extract encoded data from encoder". We need to make sure all RecordSession object be destory _before_ MediaRecorder destroy. (In ~MediaRecorder wait mReadThread be terminated.)

Comment 8

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

Let Cj to take this bug. :)

Comment 9

[u459114]

Attachment: WIP_RecordSession_2

1. Thread safe in RecordSession and MediaRecorder
2. Make sure app survive in the following scenarios
   a. Call MediaRecorder::start/stop/start/step 
   b. Call start/start/stop/stop
   c. Call start without stop and close the browser tab.
   d. manually call start/stop and double check stop callback at app side.

Comment 10

[u459114]

Attachment: WIP_RecordSession_3

Make sure Start/Stop/Pause/Resume in Gecko be executed in the same order with JS side.

Comment 11

[u459114]

Attachment: Record Session implementation

Comment 12

[u459114]

Attachment: Record Session Test case

Comment 13

[u459114]

Attachment: Record Session implementation

Comment 14

[u459114]

Attachment: Record Session Test case

Comment 15

[Jason Smith [:jsmith]]

Comment on attachment 798741 [details] [diff] [review]
Record Session Test case

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

review- for some cleanup needed.

Note - In your commit message, you'll want to add the r= to the end of your commit message. So modify it to:

Bug 909670 Test case - MediaRecorder - Implement MediaRecorderSession. r=jsmith

::: content/media/test/Makefile.in
@@ +71,5 @@
>  		test_bug495145.html \
>  		test_bug495300.html \
>  		test_bug654550.html \
>  		test_bug686942.html \
> +		test_bug909670.html \

Nit - Can you use a descriptive title instead of the bug #? I've found that it's easier to find the test later on if you use descriptive names rather than the bug #.

::: content/media/test/test_bug909670.html
@@ +12,5 @@
> +
> +  /** Test for Bug 909670 **/
> +  SimpleTest.waitForExplicitFinish();
> +
> +  setTimeout(completeTest, 5000);

Mochitest automatically has a built-in timeout mechanism, so you don't need this.

@@ +22,5 @@
> +  function startTest() {
> +    var mStopCallbackCount = 0;
> +    var mExpectCallbackCount = 2;
> +
> +    navigator.mozGetUserMedia({audio:true, fake:true},

Any particular reason why gUM is used here over using mozCaptureStream? I'd suggest leaning on the mozCaptureStream approach typically unless gUM is needed mainly because when new media encoders get implemented, we'll easily be able to port existing tests that use mozCaptureStream by providing a new mime type to gMediaRecorderTests.

@@ +23,5 @@
> +    var mStopCallbackCount = 0;
> +    var mExpectCallbackCount = 2;
> +
> +    navigator.mozGetUserMedia({audio:true, fake:true},
> +      function(s) {

Nit - use stream instead of s for descriptive naming.

@@ +27,5 @@
> +      function(s) {
> +        var mediaRecorder = new MediaRecorder(s);
> +
> +        // Setup event handlers.
> +        mediaRecorder.onstop = function stopCallback() {

Nit - I'd add an info statement here to indicate onstop has fired to help with debugging in case this test intermittently fails in the future.

@@ +30,5 @@
> +        // Setup event handlers.
> +        mediaRecorder.onstop = function stopCallback() {
> +          mStopCallbackCount++;
> +
> +          if (mExpectCallbackCount == mStopCallbackCount)

Nit - use triple equals.

@@ +32,5 @@
> +          mStopCallbackCount++;
> +
> +          if (mExpectCallbackCount == mStopCallbackCount)
> +          {
> +            ok(true, 'The number of onstop is correct.');

What happens if you run this test without the bug fix included? Does this test fail?

@@ +37,5 @@
> +            SimpleTest.finish();
> +          }
> +        }
> +        mediaRecorder.ondataavailable = function() {}
> +        mediaRecorder.onerror = function() {

We also need a similar onwarning event handler to this as well.

@@ +49,5 @@
> +          mediaRecorder.stop();
> +        }
> +      },
> +      function(e) {
> +        ok(false, 'Unexpected error fired with: ' + err);

This references an undefined variable - err doesn't exist. Change function(e) ==> function(err).

Comment 16

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

Comment on attachment 798740 [details] [diff] [review]
Record Session implementation

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

Got crash 
###!!! ABORT: file /media/randy-lin/aa890c59-befb-4bc6-9138-def8ebeea61c/gecko_c2/content/media/MediaRecorder.cpp, line 111
Hit MOZ_CRASH() at /media/randy-lin/aa890c59-befb-4bc6-9138-def8ebeea61c/gecko_c2/memory/mozalloc/mozalloc_abort.cpp:30

STR:
1. gUM
2. start recording
3. pause
4. resume ->firefox crashed.

::: content/media/MediaRecorder.cpp
@@ +225,5 @@
> +  void StopInternal()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +	// Shutdown mEncoder to stop RecordSession::Extract

tab + space

@@ +226,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +	// Shutdown mEncoder to stop RecordSession::Extract
> +    if (mInputPort.get())

indent, align to {}

@@ +297,5 @@
> +  nsRefPtr<ProcessedMediaStream> mTrackUnionStream;
> +  nsRefPtr<MediaInputPort> mInputPort;
> +  nsRefPtr<MediaEncoder> mEncoder;
> +  // The interval of timer passed from Start(). On every mTimeSlice milliseconds, if there are buffers store in the EncodedBufferCache,
> +  // a dataavailable event will be fired.

upper case

@@ +344,2 @@
>    } else {
> +	  timeSlice = 0;

nits: space

@@ +400,1 @@
>  

assert if mSession == null

@@ +409,5 @@
>      return;
>    }
> +
> +  if (mSession)
> +    mSession->Resume();

assert if mSession == null

Comment 17

[u459114]

(In reply to Jason Smith [:jsmith] from comment #15)
> Comment on attachment 798741 [details] [diff] [review]
> 
> Note - In your commit message, you'll want to add the r= to the end of your
> commit message. So modify it to:
> 
> Bug 909670 Test case - MediaRecorder - Implement MediaRecorderSession.
> r=jsmith
Thanks, understand.
> Any particular reason why gUM is used here over using mozCaptureStream? I'd
> suggest leaning on the mozCaptureStream approach typically unless gUM is
> needed mainly because when new media encoders get implemented, we'll easily
> be able to port existing tests that use mozCaptureStream by providing a new
> mime type to gMediaRecorderTests.
OK
> @@ +32,5 @@
> > +          mStopCallbackCount++;
> > +
> > +          if (mExpectCallbackCount == mStopCallbackCount)
> > +          {
> > +            ok(true, 'The number of onstop is correct.');
> 
> What happens if you run this test without the bug fix included? Does this
> test fail?
yes, this test fail while call mediaRecorder.stop()
failed | uncaught exception - InvalidStateError: An attempt was made to use an object that is not, or is no longer
> @@ +37,5 @@
> > +            SimpleTest.finish();
> > +          }
> > +        }
> > +        mediaRecorder.ondataavailable = function() {}
> > +        mediaRecorder.onerror = function() {
> 
> We also need a similar onwarning event handler to this as well.
OK
> @@ +49,5 @@
> > +          mediaRecorder.stop();
> > +        }
> > +      },
> > +      function(e) {
> > +        ok(false, 'Unexpected error fired with: ' + err);
> 
> This references an undefined variable - err doesn't exist. Change
> function(e) ==> function(err).
OK

Comment 18

[u459114]

(In reply to Randy Lin [:rlin] from comment #16)
> Comment on attachment 798740 [details] [diff] [review]
> Record Session implementation
> 
> Review of attachment 798740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Got crash 
> ###!!! ABORT: file
> /media/randy-lin/aa890c59-befb-4bc6-9138-def8ebeea61c/gecko_c2/content/media/
> MediaRecorder.cpp, line 111
> Hit MOZ_CRASH() at
> /media/randy-lin/aa890c59-befb-4bc6-9138-def8ebeea61c/gecko_c2/memory/
> mozalloc/mozalloc_abort.cpp:30
miss break in case RSResume. Thanks for this catch.
> STR:
> 1. gUM
> 2. start recording
> 3. pause
> 4. resume ->firefox crashed.
> 
> ::: content/media/MediaRecorder.cpp
> @@ +225,5 @@
> > +  void StopInternal()
> > +  {
> > +    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +	// Shutdown mEncoder to stop RecordSession::Extract
> 
> tab + space
> 
> @@ +226,5 @@
> > +  {
> > +    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +	// Shutdown mEncoder to stop RecordSession::Extract
> > +    if (mInputPort.get())
> 
> indent, align to {}
ok
> 
> @@ +344,2 @@
> >    } else {
> > +	  timeSlice = 0;
> 
> nits: space
> 
> @@ +400,1 @@
> >  
> 
> assert if mSession == null
OK
> @@ +409,5 @@
> >      return;
> >    }
> > +
> > +  if (mSession)
> > +    mSession->Resume();
> 
> assert if mSession == null
Thanks

Comment 19

[u459114]

Attachment: Record Session implementation

Comment 20

[u459114]

Attachment: Record Session Test case

Comment 21

[u459114]

Attachment: Record Session Test case

Comment 22

[u459114]

Attachment: Record Session implementation

Comment 23

[u459114]

Attachment: Record Session Test case

Comment 24

[u459114]

Attachment: Record Session Test case

Comment 25

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

Comment on attachment 806083 [details] [diff] [review]
Record Session implementation

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

::: content/media/MediaRecorder.h
@@ -106,2 @@
>    // Runnable thread for read data from mediaEncoder. It starts at MediaRecorder::Start() and stops at MediaRecorder::Stop().
>    nsCOMPtr<nsIThread> mReadThread;

Why isn't mReadThread part of the session?

@@ -116,2 @@
>    // This object creates on start() and destroys in ~MediaRecorder.
>    nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;

And why isn't this part of the session?

@@ -116,4 @@
>    // This object creates on start() and destroys in ~MediaRecorder.
>    nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;
>    // It specifies the container format as well as the audio and video capture formats.
>    nsString mMimeType;

And this?

Comment 26

[u459114]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> Comment on attachment 806083 [details] [diff] [review]
> Record Session implementation
> 
> Review of attachment 806083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaRecorder.h
> @@ -106,2 @@
> >    // Runnable thread for read data from mediaEncoder. It starts at MediaRecorder::Start() and stops at MediaRecorder::Stop().
> >    nsCOMPtr<nsIThread> mReadThread;
> 
> Why isn't mReadThread part of the session?
By using the a single mReadTread in MediaRecorder object, RecordSession::Extract function of different RecordSession(Object A and B) will be executed in MediaRecorder::mReadTread sequentially. Unless A::Extract is complete, B::Extract won't be execute, even if client side sent Stop and Start command quickly.

> @@ -116,2 @@
> >    // This object creates on start() and destroys in ~MediaRecorder.
> >    nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;
> 
> And why isn't this part of the session?
This one should be part of RecordSession. I will do it.
> @@ -116,4 @@
> >    // This object creates on start() and destroys in ~MediaRecorder.
> >    nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;
> >    // It specifies the container format as well as the audio and video capture formats.
> >    nsString mMimeType;
> 
> And this?
This one doesn't even need to be a member data. I will change it.

Comment 27

[u459114]

Attachment: Record Session implementation

Comment 28

[u459114]

Attachment: Record Session implementation

Comment 29

[u459114]

Comment on attachment 806526 [details] [diff] [review]
Record Session implementation

Hi ROC,
Please help to review this patch, thx

Comment 30

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

Comment on attachment 806526 [details] [diff] [review]
Record Session implementation

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

::: content/media/MediaRecorder.cpp
@@ +34,5 @@
> +
> +// Life cycle of a Session.
> +// 1. Allocation Stage(Must in main thread) - MSG layout and resource allocation.
> +// 2. Extract stage(Must in read thread) - Encode and fetch encoded AV frames
> +// 3. Destroy stage(Mist main thread) - Release resoruce and remove stream from MSG.

"Must be in read thread", "Must be in main thread". Also, "Release resource".

Here you should document what this class does and why we need it.

You should also explain in one place, maybe here, how Session lifetimes are managed.

@@ +35,5 @@
> +// Life cycle of a Session.
> +// 1. Allocation Stage(Must in main thread) - MSG layout and resource allocation.
> +// 2. Extract stage(Must in read thread) - Encode and fetch encoded AV frames
> +// 3. Destroy stage(Mist main thread) - Release resoruce and remove stream from MSG.
> +//    Self destruction.

I'm not sure what this line means.

@@ +36,5 @@
> +// 1. Allocation Stage(Must in main thread) - MSG layout and resource allocation.
> +// 2. Extract stage(Must in read thread) - Encode and fetch encoded AV frames
> +// 3. Destroy stage(Mist main thread) - Release resoruce and remove stream from MSG.
> +//    Self destruction.
> +class MediaRecorder::RecordSession

I think we can just call this class "Session".

@@ +42,5 @@
> +  enum {
> +    RSStart,
> +    RSStop,
> +    RSPause,
> +    RSResume

Name this enum so that mActionType can be declared with the enum type. Just call it "Action"?

We don't need an RS prefix like this. I would just call these START, STOP, PAUSE and RESUME.

@@ +51,5 @@
> +  class PushBlobRunnable : public nsRunnable
> +  {
> +  public:
> +    PushBlobRunnable(RecordSession* session)
> +      : mSession(session), mRecorder(session->mRecorder)

The parameter should be called aSession.

@@ +68,5 @@
> +    }
> +
> +  private:
> +    RecordSession *mSession;
> +    MediaRecorder *mRecorder;

How do we ensure that mSession and mRecorder stay alive until this event is dispatched?

@@ +93,5 @@
> +          mSession->StopInternal();
> +          break;
> +        case RSPause:
> +          if (mSession->mTrackUnionStream)
> +            mSession->mTrackUnionStream->ChangeExplicitBlockerCount(-1);

{} around if bodies. more below.

@@ +143,5 @@
> +    {
> +      MOZ_ASSERT(NS_IsMainThread() && mSession);
> +
> +      if (mSession)
> +      {

No need to null check mSession.

@@ +152,5 @@
> +      return NS_OK;
> +    }
> +
> +  private:
> +    RecordSession *mSession;

Make this an nsAutoPtr to make it clear that this event owns the object and must delete it.

@@ +173,4 @@
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +
> +    NS_DispatchToMainThread(new RecordingControlRunnable(this, RSStart));

Why do we need to dispatch events from the main thread to the main thread? Why can't we just run the start action synchronously here?

Same question for Stop()/Pause()/Resume() below.

@@ +207,5 @@
> +    aMimeType = mMimeType;
> +  }
> +
> +private:
> +  // Make Dtor be private, in case of miss kill by client.

I'm not sure what this comment means.

@@ +216,5 @@
> +    // Dispatch stop event.
> +    mRecorder->DispatchSimpleEvent(NS_LITERAL_STRING("stop"));
> +
> +    if (mInputPort.get())
> +    {

{ goes on same line as "if". More below.

@@ +218,5 @@
> +
> +    if (mInputPort.get())
> +    {
> +      mInputPort->Destroy();
> +      mInputPort = nullptr;

No need to set member variables to nullptr in the destructor.

@@ +254,5 @@
> +      mTrackUnionStream = nullptr;
> +    }
> +  }
> +
> +  // This funciton must be executed in MediaRecorder::mReadThread to

"function"

@@ +255,5 @@
> +    }
> +  }
> +
> +  // This funciton must be executed in MediaRecorder::mReadThread to
> +  // guruantee different RecordSession::Extract be execute _sequentially_.

"guarantee"

@@ +256,5 @@
> +  }
> +
> +  // This funciton must be executed in MediaRecorder::mReadThread to
> +  // guruantee different RecordSession::Extract be execute _sequentially_.
> +  // Otherwise, MediaRecorder::mEncodedBufferCache might be access

"accessed"

@@ +257,5 @@
> +
> +  // This funciton must be executed in MediaRecorder::mReadThread to
> +  // guruantee different RecordSession::Extract be execute _sequentially_.
> +  // Otherwise, MediaRecorder::mEncodedBufferCache might be access
> +  // by two different RecordSession.

"Sessions"

@@ +288,5 @@
> +    NS_DispatchToMainThread(new PushBlobRunnable(this));
> +    NS_DispatchToMainThread(new DestroyRunnable(this));
> +  }
> +
> +  void MSGLayout()

Let's call this SetupStreams().

@@ +352,1 @@
>  }

I think you should assert that mSession is null.

Comment 31

[u459114]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> @@ +68,5 @@
> > +    }
> > +
> > +  private:
> > +    RecordSession *mSession;
> > +    MediaRecorder *mRecorder;
> 
> How do we ensure that mSession and mRecorder stay alive until this event is
> dispatched?
Session has a data member 
  nsRefPtr<MediaRecorder> mRecorder;
Unless Session be destroyed, the associated recorder keep survive. So, we may need to take care about the life cycle of Session for now.

The way to destroy Session object is to call DestroyRunnable::Run. Both PushBlobRunnable and DestroyRunnable are executed in main thread by the following order:
void Session::Extract() {
  // ...
  NS_DispatchToMainThread(new PushBlobRunnable(this)); << PushBlobRunnable first.
  NS_DispatchToMainThread(new DestroyRunnable(this));  << Then, destroy Session.
}

As a result, session and recorder object reference in PushBlobRunnable must be valid.

For all the other suggestion and correctness, I will update another patch to fix all those, thank for your review.

Comment 32

[u459114]

Attachment: Record Session implementation

1. Move Read thread from MediaRecorder to Session
2. Fix typo and add comment

Comment 33

[u459114]

Attachment: Part 02 - Record Session Test case

Hi Jason, please help to review this test case patch, thanks

Comment 34

[Jason Smith [:jsmith]]

Comment on attachment 807249 [details] [diff] [review]
Part 02 - Record Session Test case

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

Looks good to me. We probably should do a try run to confirm this passes consistently.

Small nit - you probably want to update the commit message to indicate that this is a test for the MediaRecorderSession.

Comment 35

[Kai-Chih Hu [:khu]]

Sorry to jump in. I know I am not a reviewer, but just curious about the codes and would like to provide some feedback. I checked /content/media/MediaRecorder.cpp, and found one minor part: 

Line 347-349: 

else{
    timeSlice = 0;
}

Looks like it's not required, because timeSlice was already initialized as 0. Please correct me if I am wrong.

Comment 36

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

(In reply to khu from comment #35)
> Sorry to jump in. I know I am not a reviewer, but just curious about the
> codes and would like to provide some feedback.

More feedback is always good, keep it up! :-)

Comment 37

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

Comment on attachment 807207 [details] [diff] [review]
Record Session implementation

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

Looks nice. nearly done!

::: content/media/MediaRecorder.cpp
@@ +32,5 @@
>  
> +/**
> + * Session is an object to represent a single recording event.
> + * In original design, all recording context is stored in MediaRecorder, which cause
> + * a problem  if someone calls MediaRecoder::Stop and MedaiRecorder::Start quickly.

only one space before "if"

MediaRecorder::Start

@@ +43,5 @@
> + * Thread.  Each Session has it's own recording context and Read Thread, problem been
> + * resolved.
> + *
> + * Life cycle of a Session object.
> + * 1) Initialization Stage(in main thread)

Space before (
(and below)

@@ +57,5 @@
> + *
> + * Lifetime of a Session object.
> + * 1) MediaRecorder create a Session in MediaRecorder::Start function.
> + * 2) A Session be destroyed after MediaRecorder::Stop being called _and_ all encoded
> + *    media data been passed to OnDataAvailable handler.

Mention that the Session is destroyed by DestroyRunnable.

@@ +64,2 @@
>  {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaRecorder::Session)

Why are we refcounting this? Seems to me we don't need to.

@@ +91,5 @@
>    };
>  
> +  // Record thread task.
> +  // Fetch encoded Audio/Video data from MediaEncoder.
> +  class ExtractRunnable: public nsRunnable

space before ":"

@@ +131,5 @@
> +      return NS_OK;
> +    }
> +
> +  private:
> +    nsRefPtr<Session> mSession;

This should be nsAutoPtr.

@@ +142,5 @@
> +public:
> +  Session(MediaRecorder* aRecorder, int32_t aTimeSlice)
> +    : mRecorder(aRecorder), mTimeSlice(aTimeSlice)
> +  {
> +    mEncodedBufferCache = new EncodedBufferCache(MAX_ALLOW_MEMORY_BUFFER);

Assert we're on the main thread.

@@ +186,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread() && mTrackUnionStream);
> +
> +    if (mTrackUnionStream)
> +      mTrackUnionStream->ChangeExplicitBlockerCount(-1);

Remove null check of mTrackUnionStream, since you already asserted it.

@@ +322,3 @@
>  {
> +  if (mSession)
> +    mSession->GetMimeType(aMimeType);

{} around if body

Comment 38

[u459114]

(In reply to khu from comment #35)
> Sorry to jump in. I know I am not a reviewer, but just curious about the
> codes and would like to provide some feedback. I checked
> /content/media/MediaRecorder.cpp, and found one minor part: 
> 
> Line 347-349: 
> 
> else{
>     timeSlice = 0;
> }
> 
> Looks like it's not required, because timeSlice was already initialized as
> 0. Please correct me if I am wrong.
Yes, you are right. I should remove else statement here, thanks.

Comment 39

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

Just test and haven't found problems at this patch.

Comment 40

[u459114]

Attachment: Record Session implementation

1. Refined the patch base on roc and kho's suggestion
2. Thread safe protect for Session::mMimeType
3. Don't push data back to onDataAvailable if timeSlice <= 0

Comment 41

[Kai-Chih Hu [:khu]]

(In reply to C.J. Ku[:CJKu] from comment #40)
> Created attachment 808455 [details] [diff] [review]
> Record Session implementation
> 
> 1. Refined the patch base on roc and kho's suggestion
It's khu...not kho....:P

Comment 42

[u459114]

Attachment: Part 02 - Record Session Test case

Apply JSmith's suggestion- make clearer commit message.

Comment 43

[u459114]

Attachment: Part 01 - Record Session implementation

Fix some test case failed and compile-time warning on specific platform(WinXP)

Comment 44

[u459114]

Try server result
https://tbpl.mozilla.org/?tree=Try&rev=98b8d9d3e649

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/31d469022372
https://hg.mozilla.org/integration/b2g-inbound/rev/704346e888a4

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31d469022372
https://hg.mozilla.org/mozilla-central/rev/704346e888a4

Comment 47

[Jason Smith [:jsmith]]

Verified through clean landing of mochitest.

Comment 48

[u459114]

bug 924724, which is koi+, depends on the change here. Need to uplift this patch

Comment 49

[Jason Smith [:jsmith]]

(In reply to C.J. Ku[:CJKu] from comment #48)
> bug 924724, which is koi+, depends on the change here. Need to uplift this
> patch

That shouldn't influence if this bug is a blocker or not. What you need to do on bug 924724 is build a branch-specific patch to fix the issue independent of this bug.

I don't think this bug is a blocker either, as the issue filed here is easy to work around from a developer's perspective.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

See my reply in bug 919051.