MediaRecorder - Implement MediaRecorderSession

VERIFIED FIXED in mozilla27

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: shelly, Assigned: u459114)

Tracking

({dev-doc-needed})

unspecified
mozilla27
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(2 attachments, 18 obsolete attachments)

2.91 KB, patch
Details | Diff | Splinter Review
21.58 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → rlin
Posted patch WIP_Record_Session (obsolete) — Splinter Review
WIP.

1. Use single mReadThread for all sessions.
2. Move union stream/ port/ encoder into RecordSession
Attachment #798198 - Flags: review?(roc)
Attachment #798198 - Flags: feedback?(rlin)
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 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..
(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.
(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.
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.)
Let Cj to take this bug. :)
Assignee: rlin → cku
Posted patch WIP_RecordSession_2 (obsolete) — Splinter Review
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.
Attachment #798198 - Attachment is obsolete: true
Attachment #798198 - Flags: review?(roc)
Attachment #798198 - Flags: feedback?(rlin)
Attachment #798570 - Flags: review?(slin)
Posted patch WIP_RecordSession_3 (obsolete) — Splinter Review
Make sure Start/Stop/Pause/Resume in Gecko be executed in the same order with JS side.
Attachment #798570 - Attachment is obsolete: true
Attachment #798570 - Flags: review?(slin)
Attachment #798574 - Flags: review?(slin)
Posted patch Record Session implementation (obsolete) — Splinter Review
Attachment #798574 - Attachment is obsolete: true
Attachment #798574 - Flags: review?(slin)
Posted patch Record Session Test case (obsolete) — Splinter Review
Posted patch Record Session implementation (obsolete) — Splinter Review
Posted patch Record Session Test case (obsolete) — Splinter Review
Attachment #798736 - Attachment is obsolete: true
Attachment #798738 - Attachment is obsolete: true
Attachment #798740 - Flags: review?(roc)
Attachment #798740 - Flags: review?(rlin)
Attachment #798741 - Flags: review?(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).
Attachment #798741 - Flags: review?(jsmith) → review-
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
(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
(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
Posted patch Record Session implementation (obsolete) — Splinter Review
Attachment #798740 - Attachment is obsolete: true
Attachment #798741 - Attachment is obsolete: true
Attachment #798740 - Flags: review?(roc)
Attachment #798740 - Flags: review?(rlin)
Posted patch Record Session Test case (obsolete) — Splinter Review
Posted patch Record Session Test case (obsolete) — Splinter Review
Attachment #800738 - Attachment is obsolete: true
Posted patch Record Session implementation (obsolete) — Splinter Review
Attachment #800735 - Attachment is obsolete: true
Posted patch Record Session Test case (obsolete) — Splinter Review
Posted patch Record Session Test case (obsolete) — Splinter Review
Attachment #800742 - Attachment is obsolete: true
Attachment #806084 - Attachment is obsolete: true
Attachment #806083 - Flags: review?(roc)
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?
(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.
Posted patch Record Session implementation (obsolete) — Splinter Review
Attachment #806083 - Attachment is obsolete: true
Attachment #806083 - Flags: review?(roc)
Posted patch Record Session implementation (obsolete) — Splinter Review
Attachment #806519 - Attachment is obsolete: true
Comment on attachment 806526 [details] [diff] [review]
Record Session implementation

Hi ROC,
Please help to review this patch, thx
Attachment #806526 - Flags: review?(roc)
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.
(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.
Posted patch Record Session implementation (obsolete) — Splinter Review
1. Move Read thread from MediaRecorder to Session
2. Fix typo and add comment
Attachment #806526 - Attachment is obsolete: true
Attachment #806526 - Flags: review?(roc)
Attachment #807207 - Flags: review?(roc)
Hi Jason, please help to review this test case patch, thanks
Attachment #806086 - Attachment is obsolete: true
Attachment #807249 - Flags: review?(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.
Attachment #807249 - Flags: review?(jsmith) → review+
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.
(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 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
(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.
Just test and haven't found problems at this patch.
Posted patch Record Session implementation (obsolete) — Splinter Review
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
Attachment #807207 - Attachment is obsolete: true
Attachment #807207 - Flags: review?(roc)
Attachment #808455 - Flags: review?(roc)
(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
Attachment #807249 - Attachment description: Record Session Test case → Part 02 - Record Session Test case
Apply JSmith's suggestion- make clearer commit message.
Attachment #807249 - Attachment is obsolete: true
Fix some test case failed and compile-time warning on specific platform(WinXP)
Attachment #808455 - Attachment is obsolete: true
Attachment #809933 - Attachment description: Part 1: Record Session implementation → Part 01: Record Session implementation
Attachment #809933 - Attachment description: Part 01: Record Session implementation → Part 01 - Record Session implementation
https://hg.mozilla.org/mozilla-central/rev/31d469022372
https://hg.mozilla.org/mozilla-central/rev/704346e888a4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: verifyme
QA Contact: jsmith
Verified through clean landing of mochitest.
Status: RESOLVED → VERIFIED
Keywords: verifyme
bug 924724, which is koi+, depends on the change here. Need to uplift this patch
blocking-b2g: --- → koi?
(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.
blocking-b2g: koi? → -
See my reply in bug 919051.
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.