Closed Bug 803414 Opened 12 years ago Closed 11 years ago

Media Recording - Web API & Implementation

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla25

People

(Reporter: mchen, Assigned: rlin)

References

Details

(Keywords: doc-bug-filed)

Attachments

(12 files, 53 obsolete files)

23.89 KB, patch
Details | Diff | Splinter Review
23.89 KB, patch
Details | Diff | Splinter Review
4.14 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
22.57 KB, patch
roc
: review+
Details | Diff | Splinter Review
916 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
4.13 KB, patch
Details | Diff | Splinter Review
915 bytes, patch
Details | Diff | Splinter Review
9.76 KB, patch
Details | Diff | Splinter Review
5.04 KB, patch
Details | Diff | Splinter Review
22.58 KB, patch
Details | Diff | Splinter Review
Currently there is no Web API for audio recording or capturing. 
But some draft versions are on discussion including getUserMedia from WebRTC / MediaStream Recording / Web Audio.

Requirement:
  1. Can choose what audio source (mic, voice_call or FM) it want to record.
  2. Can choose what encoding format for compressing raw PCM stream or just pure PCM.
  3. Can choose where is the output of processed stream (ex: write into file or captured by application for further processing)
Is there any progress on the audio recording API? Maybe Bug 779297 "(webaudio) Implement Web Audio" is also relevant for this.
Flags: needinfo?(mchen)
Dear Mihai,

Thanks for your information here.

Actually this work is postponed now due to we are fully working on v1 milestone and this is on v2 feature set.

I had discussed web audio in media channel. The web audio seems to 
1. There is no API for web content to choose what input device should be used to record.
   Ex: Mic or voice_call or FM.
2. There is no API for web content to grab the recording data as compressed format or "encoding then writing into a file". Ex: Voice Recognize and local recording.

So currently we just aim to use the getUserMedia API from WebRTC for point 1.
And still miss the API for point 2.
Flags: needinfo?(mchen)
Assignee: nobody → rlin
Depends on: 825111
Comment on attachment 700881 [details] [diff] [review]
webIDL for mediaRecorder v1

I argued on the list that the format should just be a single string with a MIME type, and I think that's the way it's going to go. For now I think you should just take out the setOptions and pick the format automatically, until that API is better defined.

In fact, better take out all the APIs except the ones we really need and implement those first.
Attached patch webIDL for mediaRecorder v2 (obsolete) — Splinter Review
Remove the getOptions and SetOptions, takePhoto function
I will try to implement the audio recording function, first stage will output raw format first. Also need to consider to find the default encoder, invoke it and pass out the encoded data to js environment.
Attachment #700881 - Attachment is obsolete: true
Attachment #700881 - Flags: feedback?(roc)
Attachment #701675 - Flags: feedback?(roc)
Comment on attachment 701675 [details] [diff] [review]
webIDL for mediaRecorder v2

> [Constructor,

What does this constructor do? I don't think we need it.

Remove the event listeners we don't need right now. The others should be declared with "EventHandler" instead of "Function?".

The rest looks good.
Attachment #701675 - Flags: feedback?(roc) → feedback+
Hi Roc, 
1.The constructor is used for pass an mediastream into MediaRecorder object. That's required in the draft.
2.I refer the WebSocket.webidl and find they use function? instead of EventHandler in webidl.
3.I try to access the mediaStream audio track buffer but found there is no public interface for me, Is any proper way to do this? Or should I hook in the streamBuffer and pass out the track buffer to mediaRecorder?
4.From the draft, the  muteTrack pass a trackId to mute on track, How can the jscontext get this information?
(In reply to Randy Lin [:rlin] from comment #7)
> 1.The constructor is used for pass an mediastream into MediaRecorder object.
> That's required in the draft.

Yes, but you have a constructor with no parameters. What's that for?

> 2.I refer the WebSocket.webidl and find they use function? instead of
> EventHandler in webidl.

That hasn't been updated yet. Use EventHandler.

> 3.I try to access the mediaStream audio track buffer but found there is no
> public interface for me, Is any proper way to do this? Or should I hook in
> the streamBuffer and pass out the track buffer to mediaRecorder?

Add a MediaStreamListener to the MediaStream you're interested in and listen to NotifyQueuedTrackChanges.

> 4.From the draft, the  muteTrack pass a trackId to mute on track, How can
> the jscontext get this information?

I suggest leaving out the muting stuff for now. Anyway I haven't looked into track ID strings ... it may require some MediaStreamTrack APIs that we haven't implemented yet.
Attached patch webIDL for mediaRecorder v3 (obsolete) — Splinter Review
1.Remove the constructor()
2.use EventHandler to replace the older attribute "function?"
Attachment #701675 - Attachment is obsolete: true
Attached patch webIDL for mediaRecorder v3 (obsolete) — Splinter Review
correct wrong file upload...
Attachment #702150 - Attachment is obsolete: true
Depends on: 834165
Hi roc, 
We have made a simple proposal for this function, 
Can you take a look?
https://wiki.mozilla.org/Gecko:Mediarecorder
Flags: needinfo?(roc)
Hi Robert, 
As discussed with Steven Lee, Chiajung, Keven and Steven Yang, 
The recording feature is requested by the oem partner and they want us to phase in "b2g18" branch. But we found we may hit those problems.
1. WebRTC havn't tested/enabled on this branch. (mediaStream comes from getUserMedia...)
2. The m_c version of webrtc isn't stable.
3. WebIDL code generator isn't full functional on b2g-18 branch
4. Deadline is in the end of March.
Because the schedule is so tight, may we first implement the MediaRecorder as XPCOM by using the Android AudioRecord function directly encode the pcm data and pass the opus in ogg blob data to js context first?
Our implementation will try to keep the same interface as the draft of w3c.
Flags: needinfo?(roc)
(In reply to Randy Lin [:rlin] from comment #14)
> Hi Robert, 
> As discussed with Steven Lee, Chiajung, Keven and Steven Yang, 
> The recording feature is requested by the oem partner and they want us to
> phase in "b2g18" branch. But we found we may hit those problems.
> 1. WebRTC havn't tested/enabled on this branch. (mediaStream comes from
> getUserMedia...)
For FirefoxOS, the backporting effort for WebRTC is rather large. Furthermore, those patch is under reviewing. See Bug 825112 and Bug 818843.

> 2. The m_c version of webrtc isn't stable.
> 3. WebIDL code generator isn't full functional on b2g-18 branch
> 4. Deadline is in the end of March.
5. MediaEncoder API is still under construction...See Bug 842233

> Because the schedule is so tight, may we first implement the MediaRecorder
> as XPCOM by using the Android AudioRecord function directly encode the pcm
> data and pass the opus in ogg blob data to js context first?
> Our implementation will try to keep the same interface as the draft of w3c.
(In reply to Chiajung Hung [:chiajung] from comment #15)
> (In reply to Randy Lin [:rlin] from comment #14)
> > Hi Robert, 
> > As discussed with Steven Lee, Chiajung, Keven and Steven Yang, 
> > The recording feature is requested by the oem partner and they want us to
> > phase in "b2g18" branch. But we found we may hit those problems.
> > 1. WebRTC havn't tested/enabled on this branch. (mediaStream comes from
> > getUserMedia...)
> For FirefoxOS, the backporting effort for WebRTC is rather large.
> Furthermore, those patch is under reviewing. See Bug 825112 and Bug 818843.
> 
> > 2. The m_c version of webrtc isn't stable.
> > 3. WebIDL code generator isn't full functional on b2g-18 branch
> > 4. Deadline is in the end of March.
> 5. MediaEncoder API is still under construction...See Bug 842233
type: Bug 842243
> 
> > Because the schedule is so tight, may we first implement the MediaRecorder
> > as XPCOM by using the Android AudioRecord function directly encode the pcm
> > data and pass the opus in ogg blob data to js context first?
> > Our implementation will try to keep the same interface as the draft of w3c.
(In reply to Randy Lin [:rlin] from comment #14)
> As discussed with Steven Lee, Chiajung, Keven and Steven Yang, 
> The recording feature is requested by the oem partner and they want us to
> phase in "b2g18" branch.
> But we found we may hit those problems.
> 1. WebRTC havn't tested/enabled on this branch. (mediaStream comes from
> getUserMedia...)
> 2. The m_c version of webrtc isn't stable.
> 3. WebIDL code generator isn't full functional on b2g-18 branch
> 4. Deadline is in the end of March.
> Because the schedule is so tight, may we first implement the MediaRecorder
> as XPCOM by using the Android AudioRecord function directly encode the pcm
> data and pass the opus in ogg blob data to js context first?
> Our implementation will try to keep the same interface as the draft of w3c.

Really, it doesn't seem wise to me to land brand new features on the B2G18 branch at this point. At some point we have to tell partners that new features will only be in the next version of B2G. That's not my call, but I assume there is someone in charge of B2G 1.0 who makes that call.

Having said that, your plan is OK, but it'll be ugly.
Flags: needinfo?(roc)
Attached patch WIP patch v1 (obsolete) — Splinter Review
Hi roc, 
This is the first version of the audio recording.
As this early stage, I just connected to the android audio recorder to test this implement is workable or not.
Once the encoder is ready, the Recorder class should be replaced with the true encoder. Please help to feedback my implementation is ok or not for this feature. Thanks a lot.
Attachment #702151 - Attachment is obsolete: true
Attachment #720537 - Flags: feedback?
Attached file test application (obsolete) —
Comment on attachment 720537 [details] [diff] [review]
WIP patch v1

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

This will eventually need review from a DOM peer.

::: content/media/MediaRecorder.cpp
@@ +10,5 @@
> +
> +#include "nsIAuthPrompt2.h"
> +#include "nsIAuthPrompt.h"
> +#include "nsIPromptFactory.h"
> +#include "nsIWindowWatcher.h"

Hmm ... I wonder why this is needed!

@@ +69,5 @@
> +  mRingBuffer = new RingBuffer(BUF_SIZE);
> +}
> +
> +DOMMediaStream
> +MediaRecorder::GetStream(ErrorResult& rv)

Should return a DOMMediaStream* and be marked resultNotAddRefed in Bindings.conf

@@ +85,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(MediaRecorder)
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(MediaRecorder)
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_END

We don't want this skipping stuff.

@@ +161,5 @@
> +  Notify(NS_LITERAL_STRING("stop"));
> +}
> +
> +void
> +MediaRecorder::Pause(ErrorResult & aResult)

We generally don't have a space before the &

@@ +165,5 @@
> +MediaRecorder::Pause(ErrorResult & aResult)
> +{
> +  if (mState != RecordingState::Recording) {
> +      aResult.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +      return;

Fix indent.

@@ +212,5 @@
> +
> +void
> +MediaRecorder::Notify(const nsAString & aStr)
> +{
> +

Unwanted blank line

@@ +346,5 @@
> +          do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      // Get the an auth prompter for our window so that the parenting
> +      // of the dialogs works as it should when using tabs.

What dialogs? Why do we need to create dialogs?

@@ +464,5 @@
> +  Recorder* mRecorder;
> +};
> +
> +  Recorder::Recorder(DOMMediaStream& aStream, RingBuffer *aRingBuffer):
> +  mRecord(nullptr)

Fix indent. Space before :

::: content/media/MediaRecorder.h
@@ +7,5 @@
> +#define mozilla_dom_MediaRecorder_h
> +
> +#include "nsDOMEventTargetHelper.h"
> +#include "nsIInterfaceRequestor.h"
> +#include "nsWeakReference.h"

You shouldn't need these two #includes

@@ +15,5 @@
> +
> +#define NS_MediaRecorder_CID                          \
> + { /* 755e2d2d-a836-4539-83f4-16b51156341f */       \
> +  0x755e2d2d, 0xa836, 0x4539,                       \
> + {0x83, 0xf4, 0x16, 0xb5, 0x11, 0x56, 0x34, 0x2f} }

You probably shouldn't need this XPCOM stuff either

@@ +23,5 @@
> +class nsPIDOMWindow;
> +
> +namespace android {
> +class AudioRecord;
> +};

Maybe this needs to be here temporarily, but in the end --- before we check in, I mean --- there shouldn't be platform-specific code in this file. I would like to see the basic framework for cross-platform encoding (but not the implementation on all platforms, of course) before we check this in.

@@ +31,5 @@
> +namespace dom {
> +struct MediaRecorderInit;
> +class Recorder;
> +class RingBuffer
> +{

All the classes in this file need big comments explaining what they're for.

@@ +43,5 @@
> +  size_t Write(const int8_t *data, size_t bytes);
> +private:
> +  size_t mCapacity, mBegin_index, mEnd_index, mSize;
> +  int8_t* mData;
> +  Mutex mMutex;

And the fields need big comments explaining what they mean and what their invariants are.

@@ +48,5 @@
> +};
> +
> +class MediaRecorder : public nsDOMEventTargetHelper
> +                  , public nsIInterfaceRequestor
> +                  , public nsSupportsWeakReference

We shouldn't need nsIInterfaceRequestor and nsSupportsWeakReference here.

@@ +54,5 @@
> +public:
> +  MediaRecorder();
> +  virtual ~MediaRecorder();
> +  MediaRecorder(DOMMediaStream&);
> +  static bool PrefEnabled(){return true;};

Need extra spaces and no trailing semicolon:
  static bool PrefEnabled() { return true; }

@@ +57,5 @@
> +  MediaRecorder(DOMMediaStream&);
> +  static bool PrefEnabled(){return true;};
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_INHERITED(MediaRecorder,
> +                                                                   nsDOMEventTargetHelper)

You don't want SKIPPABLE.

@@ +80,5 @@
> +    return GetOwner();
> +  }
> +  
> +  static already_AddRefed<MediaRecorder>
> +  Constructor(const GlobalObject& aGlobal, JSContext* aCx, ErrorResult& aRv);

This Constructor does not seem to be necessary.

@@ +92,5 @@
> +  IMPL_EVENT_HANDLER(recording)
> +  IMPL_EVENT_HANDLER(dataavailable)
> +  IMPL_EVENT_HANDLER(warning)
> +  void Notify(const nsAString & eventTypeArg);
> +  nsresult CreateAndDispatchBlobEvent(int8_t* aData, u_int32_t aDatalen,

what type is this? I think you want uint32_t

@@ +93,5 @@
> +  IMPL_EVENT_HANDLER(dataavailable)
> +  IMPL_EVENT_HANDLER(warning)
> +  void Notify(const nsAString & eventTypeArg);
> +  nsresult CreateAndDispatchBlobEvent(int8_t* aData, u_int32_t aDatalen,
> +                                           bool isBinary);

Fix indent

@@ +99,5 @@
> +  void Init(JSContext* aCx, nsPIDOMWindow* aOwnerWindow);
> +  static void CopyData(nsITimer* aTimer, void* aClosure);
> +  nsCOMPtr<nsITimer> mDataMoveTimer;
> +private:
> +  DOMMediaStream mStream;

Seems like this should be nsRefPtr<DOMMediaStream>

@@ +118,5 @@
> + void Stop();
> + virtual ~Recorder();
> + android::AudioRecord *mRecord;
> + bool mStopped;
> + RingBuffer* mRingBuffer;

Two space-indenting please.

::: content/media/webaudio/RecordingError.h
@@ +13,5 @@
> +#ifndef mozilla_dom_RecordingError_h
> +#define mozilla_dom_RecordingError_h
> +
> +#include "nsIInterfaceRequestor.h"
> +#include "nsWeakReference.h"

Shouldn't need these two #includes

@@ +22,5 @@
> +
> +namespace dom {
> +
> +class RecordingError: public nsISupports,
> +                      public nsWrapperCache

This doesn't need to inherit from nsISupports, so it shouldn't. See for example the patch https://bug846977.bugzilla.mozilla.org/attachment.cgi?id=720207 for how to do this.

@@ +30,5 @@
> +  RecordingError();
> +  virtual ~RecordingError();
> +  NS_DECL_ISUPPORTS_INHERITED
> +  
> +  NS_DECL_NSIINTERFACEREQUESTOR

What's this for? It should go away.

@@ +35,5 @@
> +
> +  nsPIDOMWindow*
> +  GetParentObject() const
> +  {
> +    return nullptr;

This needs to return something. Could be either the StreamRecorder or the nsIDOMWindow.

@@ +40,5 @@
> +  }
> +
> +  // nsWrapperCache
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JSObject* aScope, bool* aTriedToWrap) MOZ_OVERRIDE;

This doesn't need to be virtual.

@@ +43,5 @@
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JSObject* aScope, bool* aTriedToWrap) MOZ_OVERRIDE;
> +  
> +  static already_AddRefed<RecordingError>
> +  Constructor(const GlobalObject& aGlobal, JSContext* aCx, ErrorResult& aRv);

What's this for? There's no WebIDL constructor for this type.

@@ +45,5 @@
> +  
> +  static already_AddRefed<RecordingError>
> +  Constructor(const GlobalObject& aGlobal, JSContext* aCx, ErrorResult& aRv);
> +
> +  void GetName(nsAString& aResult){aResult = mName;};

Need spaces and no trailing semicolon:
  void GetName(nsAString& aResult) { aResult = mName; }

::: dom/webidl/MediaRecorder.webidl
@@ +52,5 @@
> +  [SetterThrows]
> +  attribute EventHandler onrecording;
> +  
> +  [SetterThrows]
> +  attribute EventHandler onwarning;      

You'll want to remove trailing spaces and remove [Throws] from as many methods and attributes as you can, once you've implemented them.

::: dom/webidl/RecordingError.webidl
@@ +14,5 @@
> +     "INVALID_MEDIASTREAM_TRACK_ID", "INVALID_STATE", 
> +     "UNSUPPORTED_OPTION"};
> +
> +enum RecordingErrorName {
> +     "OUT_OF_MEMORY", "ILLEGAL_STREAM_MODIFICATION", 

remove trailing spaces

@@ +18,5 @@
> +     "OUT_OF_MEMORY", "ILLEGAL_STREAM_MODIFICATION", 
> +     "OTHER_RECORDING_ERROR"};
> +
> +interface RecordingError {
> +  readonly attribute DOMString name;

The spec is strange here --- there should just be one enum with all the possible values (say RecordingErrorName), and this attribute should be declared as RecordingErrorName not a DOMString. Please feed that back to the spec authors.
Attachment #720537 - Flags: feedback? → feedback+
Hi roc, 
Thanks for your reviewing.
I have mailed to the draft author for spec problem and wait for response. 
After fixing those error, I will include the DOM api review to help review the api part. And I will try to link the libogg/libopus to test encoding the pcm data to opus(may from media stream or audio Recorder.)
Comment on attachment 720537 [details] [diff] [review]
WIP patch v1

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

::: content/media/MediaRecorder.h
@@ +109,5 @@
> +  MediaRecorder& operator=(const MediaRecorder& x);
> +}; //end of MediaRecorder
> +
> +
> +class Recorder

Move Recorder class definition into cpp
Comment on attachment 720537 [details] [diff] [review]
WIP patch v1

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

::: content/media/MediaRecorder.cpp
@@ +127,5 @@
> +    aResult.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  }
> +
> +  mRecorder = new Recorder(mStream, mRingBuffer);

Memory leak with the following call sequence
1. Call Record
2. Call Stop
3. Call Record again
Make Recorder be reference base
Depends on: 842243
Attached file test application v2 (obsolete) —
file crash, re-upload again.
Attachment #720538 - Attachment is obsolete: true
Attached patch WIP patch v2 (obsolete) — Splinter Review
Hi Jonas, Can you help to review the DOM part?
This patch has fixed those suggestion by roc.
As the draft editor Jim reponsed via mail: The exception part, they want to sync those error handling with the gUM/webRTC and etc, so that can allow mediaRecorder to pass the same exception name to UA.
BTW, He also mentions that The takePhoto function would move to another spec.

Next step I would remove the android recorder and get data from mediaStream directly.
Attachment #722129 - Flags: review?(jonas)
Attachment #722129 - Flags: review?(jonas) → feedback?(jonas)
Attached file MediaRecorder dom api (obsolete) —
Attached file RecordingError idl (obsolete) —
Comment on attachment 723757 [details]
MediaRecorder dom api

Hi Justin, 
Can you help to review this dom api?
Attachment #723757 - Flags: review?(justin.lebar+bug)
Comment on attachment 723758 [details]
RecordingError idl

Hi Justin, 
Can you help to review this dom api?
Attachment #723758 - Flags: review?(justin.lebar+bug)
Attachment #722129 - Flags: feedback?(jonas)
Hi Peter, 
Due to Justin is PTO, Could you help to review the dom api?
For those two files. 
*MediaRecorder dom api
*RecordingError idl
Thanks a lot.
Flags: needinfo?(peterv)
Comment on attachment 723757 [details]
MediaRecorder dom api

How does one get at the recorded data? I.e. shouldn't you return a Blob from somewhere once the recording is done?

I'm not sure what the need for the events for state changes are. The state only changes in response to explicit calls to .record/stop/pause/resume, right?

The copyright doesn't seem right. Search for the standard Mozilla license boilerplates.

In general, I think this API needs Roc's review.
Attachment #723757 - Flags: review?(justin.lebar+bug) → review?(roc)
The apps can get blob data from ondataavailable event, this event can be triggered by 
1. every timeslice(ms) assigned in record function 2. call the RequestData function call.
2. Those are designed for the async function call. So those stage changed event would be raised when the encoder subsystem is paused/stopped/resumed/etc...
3. find an new license boilerplates to replace it.
Attached patch MediaRecorder.webidl (obsolete) — Splinter Review
change license header.
Attachment #723757 - Attachment is obsolete: true
Attachment #723758 - Attachment is obsolete: true
Attachment #723757 - Flags: review?(roc)
Attachment #723758 - Flags: review?(justin.lebar+bug)
Attachment #724840 - Flags: review?(roc)
Attached patch MediaRecorder.webidl (obsolete) — Splinter Review
update the license header.
Can you fix the attachments? You've attached files and marked them as patches, which they aren't. I'm also wondering why you have two different files both named MediaRecorder.webidl.
Attached file MediaRecorder.webidl (obsolete) —
update and mark it as non-patch, sorry.
Attachment #724840 - Attachment is obsolete: true
Attachment #724841 - Attachment is obsolete: true
Attachment #724840 - Flags: review?(roc)
Attachment #725248 - Flags: review?(roc)
Attached file RecordingError.webidl (obsolete) —
Attachment #725249 - Flags: review?(roc)
Roc, Randy: I still don't understand how this API is intended to be used.

Shouldn't there be some way to configure the encoder, such as deciding if you want opus vs. vorbis, and what bitrate you want (I don't know if vbr is a given these days?)

I also don't understand how you are supposed to get the data out of the recorder. Shouldn't requestData() return something like a DOMRequest or a Future [1] so that we can asynchronously produce a Blob?

And what's the use cases for all the events?

[1] https://github.com/slightlyoff/DOMFuture
Flags: needinfo?(roc)
(In reply to Jonas Sicking (:sicking) from comment #39)
> Shouldn't there be some way to configure the encoder, such as deciding if
> you want opus vs. vorbis, and what bitrate you want (I don't know if vbr is
> a given these days?)

There will be eventually, but we can and should support autoconfiguration.

> I also don't understand how you are supposed to get the data out of the
> recorder. Shouldn't requestData() return something like a DOMRequest or a
> Future [1] so that we can asynchronously produce a Blob?

Yes, this probably should use DOMRequest or future! Good point. Can you suggest the API? Me or Randy can forward that suggestion to public-media-capture.

> And what's the use cases for all the events?

You'd have to grovel through the mailing list to get the full story, but I think most of them are obviously useful. Perhaps onrecording and onwarning aren't needed.
Flags: needinfo?(roc)
There are two ways to get encoding data. 
1. Set timeslice on record function, that means the recorder would automatic push the blob to UA every timeslice ms. 
2. Use the requestData to retrieve blob, this function would pull out those encoded data in buffer. 
The UA will receive encoded data via ondataavailable callback.
Oooh, does the current API get at the data through the ondataavailable event? I.e. the Blob is a property on the Event object somewhere?

I think we can do better than that. That feels somewhat awkward.

Is the current API documented anywhere? It looks like a lot of the events are only fired in response to direct function calls, which makes them fairly useless as far as I can tell. For example is onpause or onresume fired in response to anything other than calls to pause() and resume()? And is onrecording fired any other time than when record() is called?

Also, we should absolutely not fire any of these events synchronously. At the earliest we should fire them at the end of the microtask.
Hi Jonas,
We follow this draft to implement the MediaRecorder function:
https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/RecordingProposal.html
I think they design so many events which can reflect the mediaStream/Encoder status and can feedback to UA.
(In reply to Jonas Sicking (:sicking) from comment #42)
> Oooh, does the current API get at the data through the ondataavailable
> event? I.e. the Blob is a property on the Event object somewhere?

Yes.
Attached patch WIP patch v3 (obsolete) — Splinter Review
Hi Robert,
This is my part of mediaEncoder.
Integrate with WebRTC/MediaEncoder, It works well on DT/b2g.
There are still something TODO:
1. throw customize exception, hmm, try to find an example for this...
2. setOption/getOption,
3. release procedure.
4. audomatic testing
5. minetype, now support opus in ogg.
those onXXX events I think aren't necessary because the start/stop function are Synchronize.
Attachment #720537 - Attachment is obsolete: true
Attachment #722115 - Attachment is obsolete: true
Attachment #722129 - Attachment is obsolete: true
Attachment #732197 - Flags: feedback?(roc)
Flags: needinfo?(peterv)
Attached file test application v3 (obsolete) —
Comment on attachment 732197 [details] [diff] [review]
WIP patch v3

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

Where's the MediaEncoder code?

::: content/base/src/nsGkAtomList.h
@@ +1889,5 @@
>  GK_ATOM(ratechange, "ratechange")
>  GK_ATOM(durationchange, "durationchange")
>  GK_ATOM(volumechange, "volumechange")
> +GK_ATOM(ondataavailable, "ondataavailable")
> +GK_ATOM(onwarning, "onwarning")

Don't add onwarning/onresume/onstop here or in the IDL, since you don't support them.

::: content/events/public/nsEventNameList.h
@@ +614,5 @@
> +
> +NON_IDL_EVENT(stop,
> +              NS_STOP,
> +              EventNameType_None,
> +              NS_EVENT)

Don't add them here either.

::: content/media/MediaRecorder.cpp
@@ +18,5 @@
> +//#include "VideoUtils.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::dom;
> +#define BUF_SIZE 1 << 17

parentheses: (1 << 17)

@@ +30,5 @@
> +
> +#define SET_PROPERTY(finalObject, object, property)   \
> +val = OBJECT_TO_JSVAL(object);                        \
> +if (!JS_DefineProperty(cx, finalObject, #property,    \
> +        val, nullptr, nullptr, JSPROP_ENUMERATE))     \

Why are these macros here? They seem to be unused...

::: content/media/MediaRecorder.h
@@ +38,5 @@
> +  size_t Write(const unsigned char* data, size_t bytes);
> +private:
> +  size_t mCapacity, mBegin_index, mEnd_index, mSize;
> +  unsigned char* mData;
> +  ReentrantMonitor mReentrantMonitor;

Document these. Especially, document which members are protected by mReentrantMonitor (all of them I guess).

@@ +56,5 @@
> +{
> +public:
> +  virtual ~MediaRecorder();
> +  MediaRecorder(DOMMediaStream&);
> +  static bool PrefEnabled() { return true; };

no trailing ;

@@ +75,5 @@
> +  void Start(int32_t timeSlice, ErrorResult & aResult);
> +  void Stop(ErrorResult& aResult);
> +  void Pause(ErrorResult & aResult);
> +  void Resume(ErrorResult & aResult);
> +  void RequestData(ErrorResult & aResult);

No space before &

@@ +92,5 @@
> +protected:
> +  void Init(JSContext* aCx, nsPIDOMWindow* aOwnerWindow);
> +  static void PushEncodeData(nsITimer* aTimer, void* aClosure);
> +  void ReadEncodeData();
> +  nsCOMPtr<nsITimer> mDataMoveTimer;

Document these.

@@ +94,5 @@
> +  static void PushEncodeData(nsITimer* aTimer, void* aClosure);
> +  void ReadEncodeData();
> +  nsCOMPtr<nsITimer> mDataMoveTimer;
> +
> +private:

You don't need both protected and private. Just make everything protected I think.

@@ +99,5 @@
> +  RecordingState mState;
> +  int32_t mTimeSlice;
> +  nsRefPtr<DOMMediaStream> mStream;
> +  MediaRecorder(const MediaRecorder& x);   // prevent bad usage
> +  void DisconnectFromOwner();

Document this.

@@ +105,5 @@
> +  RingBuffer *mRingBuffer;
> +  nsRefPtr<MediaEncoder> mEncoder;
> +  nsCOMPtr<nsIThread> mReadThread;
> +  void Notify(const nsAString & eventTypeArg);
> +  nsresult CreateAndDispatchBlobEvent(unsigned char* aData, int32_t aDatalen);

Don't mix methods and data members in order. Generally we put all the methods first and put the data members all last, ordered by alignment from largest-alignment to smallest-alignment to minimize wasted space due to alignment.

::: content/media/webaudio/RecordingError.h
@@ +34,5 @@
> +  }
> +  
> +  // WebIDL
> +  MediaRecorder* GetParentObject() const;
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope);

I think WrapObject doesn't need to be virtual here.

::: dom/webidl/MediaRecorder.webidl
@@ +43,5 @@
> +  [SetterThrows]
> +  attribute EventHandler onwarning;
> +
> +  [Throws]
> +  void start(long timeSlice);

This should be optional.
Flags: sec-review?
Attached patch WIP patch v4 (obsolete) — Splinter Review
Attachment #732197 - Attachment is obsolete: true
Attachment #732197 - Flags: feedback?(roc)
Add one 48k mono channel sound for test case.
Now test for recording the playing audio clips sound. verify by checking if any data return.
sec note: needs a team reaview
Flags: sec-review? → sec-review?(curtisk)
Component: General → Video/Audio
OS: Gonk (Firefox OS) → All
Product: Boot2Gecko → Core
Hardware: ARM → All
Attached patch Patch V5 (obsolete) — Splinter Review
Fix some core-dump issues.
Attachment #735039 - Attachment is obsolete: true
Attached patch test case v4Splinter Review
add pause/resume check
Attachment #732198 - Attachment is obsolete: true
Hi Curtis, 
Could you tell me who can help review my code?
Flags: needinfo?(curtisk)
:rlin - we're not looking to do a code review on this issue. We're looking to have a more high level review of the architecture. I'll file a blocking bug for the review and the base questions to get the ball rolling on this task.
Flags: needinfo?(curtisk)
I'm currently working at a project to develop an App for "firefox os" similar to: http://www.zoobe.com/.
So it's basicly a hosted app, where you can choose a 3D-character. Then you have to make a picture for the background and record your voice on the phone. A Video will be rendered then and the 3d-character will speak the recorded message for you!

I would like to know, if the recording API is ready now for this kind of project and if its generally possible? 
Would be greatful for any evaluations, tips and help!
I think your apps wants to capture the user voice via pcm format, right?
You may use gUM(Bug 825112)  and use this webaudio ScriptProcessorNode(Bug 834513) api to get the pcm buffer. If you want to get opus/ogg format via mediarecorder, you can patch the Bug 825112 + Bug 842243 + this patch to have a try on b2g.
Attached patch patch v6 (obsolete) — Splinter Review
Remove RecordingError.webidl
Fix some bugs.
Attachment #736151 - Attachment is obsolete: true
Attachment #739526 - Flags: review?(roc)
Comment on attachment 739526 [details] [diff] [review]
patch v6

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

::: content/media/MediaRecorder.h
@@ +34,5 @@
> +  ~RingBuffer();
> +  // The buffer size share with encoder and recorder
> +  size_t Size() const { return mSize; }
> +  // The remaining buffer size can be used
> +  size_t Capacity() const { return mCapacity; }

These aren't really clear.

Normally "capacity" means an amount that doesn't change as data is added and removed. It's how much data fits into the container before there's some kind of overflow. It sounds like you're using the word differently here.

@@ +60,5 @@
> +{
> +public:
> +  virtual ~MediaRecorder();
> +  MediaRecorder(DOMMediaStream&);
> +  static bool PrefEnabled() { return true; };

If this is always true, why are we pref-controlling this? Shouldn't we check a real pref here?

@@ +63,5 @@
> +  MediaRecorder(DOMMediaStream&);
> +  static bool PrefEnabled() { return true; };
> +  // nsWrapperCache
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;

This line shouldn't need to wrap

@@ +68,5 @@
> +
> +  nsPIDOMWindow*
> +  GetParentObject() const
> +  {
> +    return GetOwner();

Nor does this one. We only wrap before the function name for functions or methods which are defined outside a class definition.

@@ +98,5 @@
> +  friend class ReadEncodedDataTask;
> +protected:
> +  void Init(JSContext* aCx, nsPIDOMWindow* aOwnerWindow);
> +  // Pooling the encoder and write encoded data to RingBuffer
> +  void ReadEncodeData();

You mean "polling"?

@@ +101,5 @@
> +  // Pooling the encoder and write encoded data to RingBuffer
> +  void ReadEncodeData();
> +  // When UA disconnect the mediaRecorder, stop the encoder thread and release object
> +  void DisconnectFromOwner();
> +  MediaRecorder& operator = (const MediaRecorder& x);

Use MOZ_DELETE?

@@ +105,5 @@
> +  MediaRecorder& operator = (const MediaRecorder& x);
> +  // Create event and sent to UA
> +  void Notify(const nsAString & eventTypeArg);
> +  // Create dataavailable event with Blob data, sent to UA
> +  static void PushEncodeData(nsITimer* aTimer, void* aClosure);

The comments for ReadEncodeData, DisconnectFromOwner, Notify and PushEncodeData should all be more detailed and more clear.

@@ +107,5 @@
> +  void Notify(const nsAString & eventTypeArg);
> +  // Create dataavailable event with Blob data, sent to UA
> +  static void PushEncodeData(nsITimer* aTimer, void* aClosure);
> +
> +  MediaRecorder(const MediaRecorder& x);   // prevent bad usage

Use MOZ_DELETE

@@ +110,5 @@
> +
> +  MediaRecorder(const MediaRecorder& x);   // prevent bad usage
> +  // create dataavailable event with Blob data.
> +  nsresult CreateAndDispatchBlobEvent(unsigned char* aData, int32_t aDatalen);
> +  // The interval of timer which generate the dataavailable event, unit is millisecond.

What does this comment refer to?

@@ +124,5 @@
> +  nsRefPtr<DOMMediaStream> mStream;
> +
> +  RecordingState mState;
> +  // The cyclic buffer which store the encoded data.
> +  RingBuffer *mRingBuffer;

Use nsAutoPtr?

::: widget/nsGUIEvent.h
@@ +410,5 @@
>  
>  #define NS_MESSAGE_EVENT_START       4700
>  #define NS_MESSAGE                   (NS_MESSAGE_EVENT_START)
> +#define NS_DATAAVAILABLE             (NS_MESSAGE_EVENT_START+1)
> +#define NS_WARNING_                  (NS_MESSAGE_EVENT_START+2)

I don't know why you've added these to this section. We probably need a new section.
Randy - In your old patches, you had a test included in your patch. This new patch however doesn't have the tests. Was that intentional?
Hi Jason, 
I put my test case on  "test case v4"
Is it you want?
Attached patch patch v7 (obsolete) — Splinter Review
1. remove PrefEnabled control (It should be a public api..)
2. add MOZ_DELETE
3. remove notify, DisconnectFromOwner function (found useless now)
4. move NS_DATAAVAILABLE/NS_ERROR/NS_WARNING into new message event id group
5. fix unclear/wrong comment problem(capacity vs size is wrong comment...)
I saw some guys had a proposal to change the error handle, so I still keep the original one.
TODO: handle mine-type query, set
Attachment #739526 - Attachment is obsolete: true
Attachment #739526 - Flags: review?(roc)
Attachment #741236 - Flags: review?(roc)
Comment on attachment 741236 [details] [diff] [review]
patch v7

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

Let's break RingBuffer into its own patch please, with its own header file and .cpp file. It can stay in content/media for now.

::: content/media/MediaRecorder.cpp
@@ +86,5 @@
> +MediaRecorder::PushEncodeData(nsITimer* aTimer, void* aClosure) {
> +  nsRefPtr<MediaRecorder> thisObject = static_cast<MediaRecorder*>(aClosure);
> +
> +  unsigned char outBuff[16384];
> +  size_t readbyte =  thisObject->mRingBuffer->Read(outBuff, BUF_SIZE);

I'm confused ... BUF_SIZE is 128K, but you're passing in a buffer of size 16K?

::: content/media/MediaRecorder.h
@@ +32,5 @@
> +public:
> +  RingBuffer(size_t capacity);
> +  ~RingBuffer();
> +  // The remaining buffer size can be used
> +  size_t Size() const { return mSize; }

I would expect Size() to mean the size of the data currently in the buffer instead of the remaining free space.

@@ +39,5 @@
> +  // Consume the buffer, return the consuming buffer size
> +  size_t Read(unsigned char* data, size_t aMaxDataLentgh);
> +  // Store the data into Buffer. If the Capacity is less than the aDataLentgh,
> +  // only the mCapacity can be written, also return the written data lenght
> +  size_t Write(const unsigned char* data, size_t aDataLentgh);

aDataLength

@@ +41,5 @@
> +  // Store the data into Buffer. If the Capacity is less than the aDataLentgh,
> +  // only the mCapacity can be written, also return the written data lenght
> +  size_t Write(const unsigned char* data, size_t aDataLentgh);
> +private:
> +  size_t mCapacity, mBegin_index, mEnd_index, mSize;

these still need to be documented.

@@ +42,5 @@
> +  // only the mCapacity can be written, also return the written data lenght
> +  size_t Write(const unsigned char* data, size_t aDataLentgh);
> +private:
> +  size_t mCapacity, mBegin_index, mEnd_index, mSize;
> +  unsigned char* mData;

How about using an nsTArray<char> so the length is part of the array and the data buffer is freed automatically.

@@ +107,5 @@
> +  int32_t mTimeSlice;
> +  // The timer which used for generate dataavailable event
> +  nsCOMPtr<nsITimer> mDataMoveTimer;
> +  // Runnable thread for encoder
> +  nsCOMPtr<nsIThread> mReadThread;

Seems to me the MediaEncoder should own this thread. If we need to dispatch something to it from MediaRecorder, provide a GetThread method on MediaEncoder so we can access it here.

::: content/media/moz.build
@@ +64,5 @@
>      'MediaDecoderReader.h',
>      'MediaDecoderStateMachine.h',
>      'MediaMetadataManager.h',
>      'MediaResource.h',
> +    'MediaRecorder.h',

This belongs before MediaResource.h
Attached patch patch v8 (obsolete) — Splinter Review
use nsTArray to replace ringBuffer.
Attachment #741236 - Attachment is obsolete: true
Attachment #741236 - Flags: review?(roc)
Attachment #742277 - Flags: review?(roc)
Comment on attachment 742277 [details] [diff] [review]
patch v8

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

Thanks, getting rid of RingBuffer simplified this code a lot.

It's a bit hard to review some of this until the MediaEncoder API is better documented.

::: content/media/MediaRecorder.cpp
@@ +18,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +// This task class is used for release encoder resource in mainthread,

"Used to release the encoder resource on the main thread"

@@ +27,5 @@
> +    : mRecorder(aRecorder)
> +    , mEncoder(aEncoder) {}
> +
> +  NS_IMETHODIMP Run() {
> +    MOZ_ASSERT(!NS_IsMainThread());

Hmm, your comment says this is supposed to run on the main thread, but this assertion requires us to not be on the main thread...

@@ +37,5 @@
> +    return NS_OK;
> +  }
> +private:
> +  MediaRecorder*  mRecorder;
> +  MediaEncoder*   mEncoder;

These should be nsRefPtrs otherwise the objects might be deleted while this Runnable is pending.

@@ +51,5 @@
> +
> +  if (mEncoder) {
> +    already_AddRefed<MediaEncoder> t= mEncoder.forget();
> +    delete t.get();
> +    mEncoder = nullptr;

Why not just mEncoder = nullptr?

In fact, why have this code at all?

@@ +54,5 @@
> +    delete t.get();
> +    mEncoder = nullptr;
> +  }
> +
> +  mReadThread = nullptr;

You shouldn't need this either.

@@ +67,5 @@
> +}
> +
> +MediaRecorder::MediaRecorder(DOMMediaStream& aStream)
> +  : mTimeSlice(0), mState(RecordingStateValues::Inactive),
> +    mReentrantMonitor("MediaRecorder::EncoedBuffer")

EncodedBuffer

@@ +87,5 @@
> +}
> +
> +void
> +MediaRecorder::ReadEncodeData() {
> +  unsigned char outBuff[8192];

uint8_t

@@ +91,5 @@
> +  unsigned char outBuff[8192];
> +  int readbyte = 0;
> +  this->mEncoder->GetEncodedData(outBuff, sizeof(outBuff), readbyte);
> +  while(mState == RecordingState::Recording &&
> +        mEncoder->GetCurrentState() != MediaEncoder::SHUTDOWN) {

space before (

@@ +97,5 @@
> +      this->mReentrantMonitor.Enter();
> +      this->mEncoedBuffer.AppendElements(outBuff, readbyte);
> +      this->mReentrantMonitor.Exit();
> +    }
> +    this->mEncoder->GetEncodedData(outBuff, sizeof(outBuff), readbyte);

Restructure this loop so there's only one call to GetEncodedData.

@@ +128,5 @@
> +      mDataMoveTimer = do_CreateInstance("@mozilla.org/timer;1");
> +      MOZ_ASSERT(mDataMoveTimer);
> +      nsresult rv = mDataMoveTimer->InitWithFuncCallback(PushEncodeData, this, mTimeSlice,
> +                                                         nsITimer::TYPE_REPEATING_SLACK);
> +      if (NS_FAILED(rv))

Share this code with Resume() somehow. Probably use a helper method StartTimer().

@@ +228,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIPrincipal> principal = scriptPrincipal->GetPrincipal();
> +  if (!principal) {

You're not using this principal!

@@ +255,5 @@
> +nsresult
> +MediaRecorder::CreateAndDispatchBlobEvent()
> +{
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> +  nsresult rv = NS_OK;

Every time we're about to deliver data in an event, you need to get the principal for this MediaRecorder --- say from the DOM Window or its document --- and check that it subsumes the principal of mStream, so this recorder is allowed to access the data from the MediaStream.

@@ +256,5 @@
> +MediaRecorder::CreateAndDispatchBlobEvent()
> +{
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> +  nsresult rv = NS_OK;
> +  mReentrantMonitor.Enter();

Use AutoMonitorEnter.

@@ +261,5 @@
> +  void* blobData = moz_malloc(mEncoedBuffer.Length() * sizeof(unsigned char));
> +  nsCOMPtr<nsIDOMBlob> blob;
> +  if (blobData) {
> +    memcpy(blobData, mEncoedBuffer.Elements(),
> +           mEncoedBuffer.Length() * sizeof(unsigned char));

It's OK to assume that sizeof(unsigned char) is 1 :-).

@@ +263,5 @@
> +  if (blobData) {
> +    memcpy(blobData, mEncoedBuffer.Elements(),
> +           mEncoedBuffer.Length() * sizeof(unsigned char));
> +    blob = new nsDOMMemoryFile(blobData, mEncoedBuffer.Length() * sizeof(unsigned char),
> +                               NS_LITERAL_STRING("audio/ogg"));

We should really get the type from mEncoder instead of hardcoding it here.

@@ +267,5 @@
> +                               NS_LITERAL_STRING("audio/ogg"));
> +    mEncoedBuffer.Clear();
> +  } else {
> +    mReentrantMonitor.Exit();
> +    free(blobData);

blobData is definitely null here so no need to free it.

::: content/media/MediaRecorder.h
@@ +24,5 @@
> + * Implementation for https://dvcs.w3.org/hg/dap/raw-file/default/media-stream-capture/MediaRecorder.html
> + * The UA must pass the mediaStream to the MediaRecorder and let Encoder to get the raw data, encoding it.
> + * This class would create a thread to read encoded data from Encoder and pop-up block data to js context on timeSlice.
> + *
> + * Todo: SetOptions: Allow UA to choose encoder via mine type

MIME type

@@ +31,5 @@
> +class MediaRecorder : public nsDOMEventTargetHelper
> +{
> +public:
> +  virtual ~MediaRecorder();
> +  MediaRecorder(DOMMediaStream&);

You need to implement cycle collection for this class. In particular your cycle collection code needs to traverse mStream.

@@ +34,5 @@
> +  virtual ~MediaRecorder();
> +  MediaRecorder(DOMMediaStream&);
> +  // nsWrapperCache
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
> +  nsPIDOMWindow* GetParentObject() { return GetOwner(); }

Put a blank line above // nsWrapperCache and below GetParentObject.

@@ +37,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope) MOZ_OVERRIDE;
> +  nsPIDOMWindow* GetParentObject() { return GetOwner(); }
> +  static already_AddRefed<MediaRecorder>
> +  Constructor(const GlobalObject& aGlobal, JSContext* aCx, DOMMediaStream& aStream,
> +              ErrorResult& aRv);

Move "Constructor" to the WebIDL section.

@@ +51,5 @@
> +  void Resume(ErrorResult& aResult);
> +  // Force to generate the dataavailable event and consume all the encoded buffer.
> +  void RequestData(ErrorResult& aResult);
> +  DOMMediaStream* GetStream(ErrorResult& rv);
> +  RecordingState GetState(ErrorResult& rv) { return mState; }

Make GetStream inline. GetStream and GetState don't need to be faillable; remove [Throws] from their WebIDL and remove ErrorResult& rv here.

@@ +62,5 @@
> +  friend class ReadEncodedDataTask;
> +protected:
> +  void Init(JSContext* aCx, nsPIDOMWindow* aOwnerWindow);
> +  // copy encoded data from encoder to mEncoedBuffer
> +  void ReadEncodeData();

Document which thread this method runs on

@@ +66,5 @@
> +  void ReadEncodeData();
> +
> +  MediaRecorder& operator = (const MediaRecorder& x) MOZ_DELETE;
> +
> +  // Read encoded data from mEncoedBuffer and create a dataavailable event with encoded blob, pass to UA

"pass to UA" doesn't make sense since we are the UA. We're passing it to the application.

@@ +73,5 @@
> +
> +  MediaRecorder(const MediaRecorder& x) MOZ_DELETE; // prevent bad usage
> +
> +  // create dataavailable event with Blob data.
> +  nsresult CreateAndDispatchBlobEvent();

Move this up next to PushEncodeData so there's a blank line between methods and data.

@@ +80,5 @@
> +  // The timer which used for generate dataavailable event
> +  nsCOMPtr<nsITimer> mDataMoveTimer;
> +  // Runnable thread for encoder
> +  nsCOMPtr<nsIThread> mReadThread;
> +  // Encoder object, initial in start(), destory in ~MediaRecorder

"initialized in start()"

"destroyed in ~MediaRecorder"

@@ +87,5 @@
> +  nsRefPtr<DOMMediaStream> mStream;
> +
> +  RecordingState mState;
> +  // The buffer which is used to store the encoded data.
> +  nsTArray<unsigned char> mEncoedBuffer;

mEncodedBuffer.

Use uint8_t instead of unsigned char.

@@ +89,5 @@
> +  RecordingState mState;
> +  // The buffer which is used to store the encoded data.
> +  nsTArray<unsigned char> mEncoedBuffer;
> +  // This Monitor used to wait for read from Encoder /write bytes to blobData
> +  ReentrantMonitor mReentrantMonitor;

Need to be more specific about exactly which fields this lock protects. Generally we declare the fields protected by the lock directly below the lock.
Attached patch patch v9 (obsolete) — Splinter Review
Thanks roc's detail review. I learn a lot.
Fix most of the problems + fix build break on WrapObject.
There are several things to be done:
1. principals, how to check if the recorder can use the mediaStream.
2. MIME type handle
3. use the nsRefPtr to hold the MediaRecorder object, but found MOZ_CRASH on 
ReadEncodedDataTask, still debuging....
4. thread control.
Attachment #742277 - Attachment is obsolete: true
Attachment #742277 - Flags: review?(roc)
Attachment #743547 - Flags: review?
Attachment #743547 - Flags: review? → review?(roc)
Is it OK if I do the next review after you've done those things? If you have any specific questions, just ask me on IRC or here.
Attached patch patch v10 (obsolete) — Splinter Review
Hi roc, 
Could you help to check the principal implement is right or not?
The major change is 
1. fix the crash of readEncodedData thread.
2. add mimetype attribute in webIDL (BTW, what's the default encoder format? :)
3. Implement the principal checking.
TODO: 1. exception feedback to UA
      2. add setOptions & getOptions.
Attachment #743547 - Attachment is obsolete: true
Attachment #743547 - Flags: review?(roc)
Attachment #745094 - Flags: feedback?(roc)
Attached patch patch v11 (obsolete) — Splinter Review
Sync to mediaEncoder framework change.
Todo: error handle, spec seems want to use the mediaError DOM object to pass the error information to UA.
Attachment #745094 - Attachment is obsolete: true
Attachment #745094 - Flags: feedback?(roc)
Attachment #745783 - Flags: feedback?(roc)
Attached patch patch v12 (obsolete) — Splinter Review
Support query encoder;s MIMEtype
Attachment #745783 - Attachment is obsolete: true
Attachment #745783 - Flags: feedback?(roc)
Attachment #746883 - Flags: feedback?(roc)
Comment on attachment 746883 [details] [diff] [review]
patch v12

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

::: content/media/MediaRecorder.cpp
@@ +54,5 @@
> +  //this task is used for avoiding the mRecorder release crash on cycle collection.
> +  class ReleaseRecorderTask : public nsRunnable {
> +  public:
> +
> +    ReleaseRecorderTask(already_AddRefed<MediaRecorder> recorder) : mRecorder(recorder) {

Remove blank line above constructor

@@ +108,5 @@
> +  thisObject->CreateAndDispatchBlobEvent();
> +}
> +
> +void
> +MediaRecorder::ReadEncodeData() {

"ExtractEncodedData" would be a better name.

@@ +110,5 @@
> +
> +void
> +MediaRecorder::ReadEncodeData() {
> +  nsAutoTArray<uint8_t, MAX_BUFFER_SIZE> outBuff;
> +  int readbyte = 0;

"bytesRead"

@@ +115,5 @@
> +  do {
> +    this->mEncoder->GetEncodedData(&outBuff, outBuff.Capacity(), readbyte);
> +    if (readbyte > 0) {
> +      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +      this->mEncodedBuffer.AppendElements(outBuff.Elements(), readbyte);

There's unnecessary data copying here. Right now GetEncodedData copies into outBuff, then we copy that into mEncodedBuffer, then we copy that into a new malloced block which is transferred to an nsDOMMemoryFile. I think all those copies are probably unnecessary. I think we should fix this now, or at least the parts that involve this code.

So I think mEncoder->GetEncodedData should return a new heap-allocated (moz_malloc) block each time it's called (or null if it has no data to return). Then we can make mEncodedBuffer a list of those blocks. Then when we want to deliver a Blob in the dataavailable event, the simple thing to do would be to concatenate the data blocks together (one copy) and pass the result to nsDOMMemoryFile. The better thing to do would be to define a new class nsDOMMultipleMemoryFile which is like nsDOMMemoryFile but takes a list of blocks instead of just a single block.

@@ +359,5 @@
> +  if (!doc || !principal)
> +    return false;
> +
> +  bool subsumes;
> +  if (NS_FAILED(principal->Subsumes(doc->NodePrincipal(), &subsumes)))

This should be the other way around. The document's principal must subsume the stream principal. More-privileged principals subsume less-privileged ones.

::: dom/webidl/MediaRecorder.webidl
@@ +46,5 @@
> +
> +  [Throws]
> +  void requestData();
> +
> +  AvailableSettings getOptions();

Can we drop support for this method or now? I think this is a really terrible interface and I've been arguing against it in media-capture.
The nsDOMMemoryFile implementation seems need to malloc a void buffer before use it and it will call moz_free to free this buffer on ~DataOwner...
(In reply to Randy Lin [:rlin] from comment #71)
> The nsDOMMemoryFile implementation seems need to malloc a void buffer before
> use it and it will call moz_free to free this buffer on ~DataOwner...

Yes. That's why I said "the simple thing to do would be to concatenate the data blocks together (one copy) and pass the result to nsDOMMemoryFile".
Attached patch media recorder error event v1 (obsolete) — Splinter Review
Hi ROC,
I found the spec want media recorder object send the DOMError object through the onerror event handler. So I create new one for this purpose. The DOMRequest can't isn't suitable because it sent the error event and want UA get error information through error attribute.
Attachment #751521 - Flags: feedback?(roc)
Attached patch patch v13 (obsolete) — Splinter Review
This version change to
1. store the encoded data pointer into one array and void one memory copy.
2. push out the encoded data when end the encoder task.

I create another one
Bug 874303 - [Media][Recorder] Implement temporary storage for encoding data.
I will create a class that has
*read (for push in the encoded data) 
*consume (for read encoded data)
This class would check the internal queue buffer size, if this length larger than 1M (magic number?), this would change to store all the encoded data into temporary folder.
Also would think how to avoid another memory copy from this buffer to blob object.
Attachment #746883 - Attachment is obsolete: true
Attachment #746883 - Flags: feedback?(roc)
Attachment #751983 - Flags: feedback?(roc)
Comment on attachment 751983 [details] [diff] [review]
patch v13

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

We talked about moving the timing code into ExtractEncodedData. Did you decide to do that in bug 874303? I think it would be better to do it in this patch.

::: content/media/MediaRecorder.cpp
@@ +6,5 @@
> +
> +#include "AudioSegment.h"
> +#include "EncoderTraits.h"
> +#include "GeneratedEvents.h"
> +#include "MediaRecorder.h"

Move MediaRecorder.h to be the first #include.
If I move the timer to the ExtractEncodedData, The push blob timing may not on schedule(The GetEncodedData is blocking). 
If this is allowed, I can implement it by setting the checking point on the return of GetEncodedData, and do the push blob data to UA if the buffer timing is exceeded the time slice passed on start function.

The other one also needs encoder to create the encoded buffer.
Attached patch patch v14 (obsolete) — Splinter Review
Remove timer for push blob to UA.
Attachment #751983 - Attachment is obsolete: true
Attachment #751983 - Flags: feedback?(roc)
(In reply to Randy Lin [:rlin] from comment #76)
> If I move the timer to the ExtractEncodedData, The push blob timing may not
> on schedule(The GetEncodedData is blocking). 
> If this is allowed, I can implement it by setting the checking point on the
> return of GetEncodedData, and do the push blob data to UA if the buffer
> timing is exceeded the time slice passed on start function.

I think that's what we should do. The timing isn't exact anyway.
ya, just remove it on patch v14, 
Also I'm implementing the temporary file solution for saving big encoded data on patch v15.
Attached patch EncodedBufferContainer patch v1 (obsolete) — Splinter Review
The first version of EncodedBufferContainer
Attachment #753554 - Flags: feedback?(roc)
Comment on attachment 754254 [details] [diff] [review]
media recorder event v1

Add stop event which is used for notify the recorder has finished recording and already push all the queue encoded data.
Attachment #754254 - Flags: review?(roc)
Comment on attachment 751521 [details] [diff] [review]
media recorder error event v1

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

I asked you why this wasn't using WebIDL and you gave me a good answer, but I forgot what it was. Can you tell me again?

Maybe Olli can tell us how to use WebIDL here.
Attachment #751521 - Flags: review?(bugs)
Comment on attachment 753554 [details] [diff] [review]
EncodedBufferContainer patch v1

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

::: content/media/EncodedBufferContainer.h
@@ +16,5 @@
> +/**
> + * Implement the buffer management between memory and file system. The client can use this class to store the buffer and read it.
> + * The critial to save into temp folder is base in the maximal size passed from the Init function.
> + * When the queue buffer size is over the maximal size, those queued data would be dump into temp file and
> + * start to write it on every write function call. The Comsume function call will retrieve all of the queue buffer and close the temp folder.

You can just say "Data is moved into a temporary file when it grows beyond the maximal size passed in the Init function."

@@ +17,5 @@
> + * Implement the buffer management between memory and file system. The client can use this class to store the buffer and read it.
> + * The critial to save into temp folder is base in the maximal size passed from the Init function.
> + * When the queue buffer size is over the maximal size, those queued data would be dump into temp file and
> + * start to write it on every write function call. The Comsume function call will retrieve all of the queue buffer and close the temp folder.
> + * The Write / Comsume can be executed on different thread.

"Write and Consume can be called by different threads."

@@ +25,5 @@
> +public:
> +  EncodedBufferContainer();
> +  ~EncodedBufferContainer();
> +  // write buffer to container, check if the queue is too large then switch to write buffer to file system
> +  nsresult Write(nsAutoTArray<uint8_t, 8092>* aArr);

Hmm. I think we want to pass an uint8_t* here, don't we? And specify that Write takes ownership of the buffer and the buffer should be freed by delete[].

@@ +27,5 @@
> +  ~EncodedBufferContainer();
> +  // write buffer to container, check if the queue is too large then switch to write buffer to file system
> +  nsresult Write(nsAutoTArray<uint8_t, 8092>* aArr);
> +  // Read all buffer from memory or filesystem, and switch back to memory, also remove the temp file
> +  nsresult Comsume(void* &aBuff, uint32_t &aLength);

If we've stored the data in a file I think we don't want to read it back into memory. That could easily kill the device since the recording could be very large.

I think EncodedBufferContainer should have a Consume method that runs on the main thread, consumes all the data currently in the container and returns a Blob containing that data. Then it can construct either a Blob of memory buffers or an nsDOMFile Blob.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> Comment on attachment 751521 [details] [diff] [review]
> media recorder error event v1
> 
> Review of attachment 751521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I asked you why this wasn't using WebIDL and you gave me a good answer, but
> I forgot what it was. Can you tell me again?
> 
> Maybe Olli can tell us how to use WebIDL here.

For this kind of simple event, the :smaug has already implement some simple way to create those kind of event. So we can just declare the idl and modify the config, then the JS layer can access this event data through event handler.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #84)
> Comment on attachment 753554 [details] [diff] [review]
> EncodedBufferContainer patch v1
> 
> Review of attachment 753554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/EncodedBufferContainer.h
> @@ +16,5 @@
> > +/**
> > + * Implement the buffer management between memory and file system. The client can use this class to store the buffer and read it.
> > + * The critial to save into temp folder is base in the maximal size passed from the Init function.
> > + * When the queue buffer size is over the maximal size, those queued data would be dump into temp file and
> > + * start to write it on every write function call. The Comsume function call will retrieve all of the queue buffer and close the temp folder.
> 
> You can just say "Data is moved into a temporary file when it grows beyond
> the maximal size passed in the Init function."
> 
> @@ +17,5 @@
> > + * Implement the buffer management between memory and file system. The client can use this class to store the buffer and read it.
> > + * The critial to save into temp folder is base in the maximal size passed from the Init function.
> > + * When the queue buffer size is over the maximal size, those queued data would be dump into temp file and
> > + * start to write it on every write function call. The Comsume function call will retrieve all of the queue buffer and close the temp folder.
> > + * The Write / Comsume can be executed on different thread.
> 
> "Write and Consume can be called by different threads."
> 
> @@ +25,5 @@
> > +public:
> > +  EncodedBufferContainer();
> > +  ~EncodedBufferContainer();
> > +  // write buffer to container, check if the queue is too large then switch to write buffer to file system
> > +  nsresult Write(nsAutoTArray<uint8_t, 8092>* aArr);
> 
> Hmm. I think we want to pass an uint8_t* here, don't we? And specify that
> Write takes ownership of the buffer and the buffer should be freed by
> delete[].
> 
> @@ +27,5 @@
> > +  ~EncodedBufferContainer();
> > +  // write buffer to container, check if the queue is too large then switch to write buffer to file system
> > +  nsresult Write(nsAutoTArray<uint8_t, 8092>* aArr);
> > +  // Read all buffer from memory or filesystem, and switch back to memory, also remove the temp file
> > +  nsresult Comsume(void* &aBuff, uint32_t &aLength);
> 
> If we've stored the data in a file I think we don't want to read it back
> into memory. That could easily kill the device since the recording could be
> very large.
> 
> I think EncodedBufferContainer should have a Consume method that runs on the
> main thread, consumes all the data currently in the container and returns a
> Blob containing that data. Then it can construct either a Blob of memory
> buffers or an nsDOMFile Blob.

Hi Robert,
I'm in another small work-week in San Diego now. I will provide another patch that contains 
1. remove minetype query, get current mimetype used form encoder 
2. change to use void*/pass buffer array to encoder
3. change the implementation of pause()/resume() by localmediastream 
4. change the threading model of recorder /encoder.
Comment on attachment 751521 [details] [diff] [review]
media recorder error event v1

The patch which makes event code generator to deal with idl & webidl events is still under review. So, for now using .idl is fine.
(The patch for codegen has a small tool to generate .webidl from simple events' .idl so adding support for .webidl will be easy and I'll do the conversion for all the events once the review for that code generator patch is ready.)

nsIDOMRecordErrorEvent has odd indentation. Parameters should be aligned.
Make initRecordErrorEvent [noscript]. New events shouldn't have scriptable
init*Event. r+ with those fixed.

But, I don't see RecordErrorEvent in the spec
Attachment #751521 - Flags: review?(bugs) → review+
Depends on: MediaEncoder
Attached patch media recorder error event v2 (obsolete) — Splinter Review
Fix [patch] media recorder error event v1 problem
Attachment #725248 - Attachment is obsolete: true
Attachment #725249 - Attachment is obsolete: true
Attachment #752050 - Attachment is obsolete: true
Attachment #753554 - Attachment is obsolete: true
Attachment #754254 - Attachment is obsolete: true
Attachment #753554 - Flags: feedback?(roc)
Attached patch media recorder event define v2 (obsolete) — Splinter Review
Media Recorder Event
Attachment #751521 - Attachment is obsolete: true
Attachment #751521 - Flags: feedback?(roc)
Attachment #760051 - Flags: review?(roc)
Attached patch EncodedBufferContainer v2 (obsolete) — Splinter Review
For the Comsume function, If we can map the temp file into DOMFile object, Do we have any interface for UA to re-save this file into filesystem? (It seems like move buffer file from temp folder to device stroage.)
Attachment #760058 - Flags: review?(roc)
Attached patch MediaRecorder v2 (obsolete) — Splinter Review
For pause/resume operation, just throw the NS_ERROR_DOM_NOT_SUPPORTED_ERR, 
needs localmediastream object.
Attachment #760061 - Flags: review?(roc)
Comment on attachment 760046 [details] [diff] [review]
media recorder error event v2

Hi Olli, 
The media recorder Error Handling section want to use DOMError to notify UA if something wrong.
interface DOMError {
  readonly attribute DOMString name;
};
So I create this one for the onerror event handler.
Attachment #760046 - Flags: review?(bugs)
(In reply to Randy Lin [:rlin] from comment #92)
> Comment on attachment 760046 [details] [diff] [review]
> media recorder error event v2
> 
> Hi Olli, 
> The media recorder Error Handling section want to use DOMError to notify UA
> if something wrong.
> interface DOMError {
>   readonly attribute DOMString name;
> };
> So I create this one for the onerror event handler.


I don't understand this comment. The event handler is for an event. The patch creates such. I don't see DOMError anywhere. The spec talks about RecordingError, which is totally useless interface, since there is
http://dom.spec.whatwg.org/#interface-domerror
The spec says also "In all other cases, an error object must be provided to the failure callback." but I don't see any failure callbacks in the spec.
Comment on attachment 760058 [details] [diff] [review]
EncodedBufferContainer v2

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

::: content/media/EncodedBufferContainer.h
@@ +16,5 @@
> +/**
> + * Data is moved into a temporary file when it grows beyond
> + * the maximal size passed in the Init function.
> + */
> +class EncodedBufferContainer

Let's not call this "Container" since it could be confused with the media container format.

The main purpose of this class is to manage a list of data buffers that might be too large to fit in memory, so we can fall back to a file when the data gets too large. We should call it something that reflects that purpose. Maybe "FileBackedBufferList"?

Also I think this class should have a method ExtractBlob that returns a blob with the existing data and empties the list.
(In reply to Olli Pettay [:smaug] from comment #93)
> (In reply to Randy Lin [:rlin] from comment #92)
> > Comment on attachment 760046 [details] [diff] [review]
> > media recorder error event v2
> > 
> > Hi Olli, 
> > The media recorder Error Handling section want to use DOMError to notify UA
> > if something wrong.
> > interface DOMError {
> >   readonly attribute DOMString name;
> > };
> > So I create this one for the onerror event handler.
> 
> 
> I don't understand this comment. The event handler is for an event. The
> patch creates such. I don't see DOMError anywhere. The spec talks about
> RecordingError, which is totally useless interface, since there is
> http://dom.spec.whatwg.org/#interface-domerror
> The spec says also "In all other cases, an error object must be provided to
> the failure callback." but I don't see any failure callbacks in the spec.
Hi Olli,
Due to MediaStream Recording spec isn't stable yet,
I think the callbacks should be this one.
===
onerror of type EventHandler,
    Called to handle the DOMError event. 
So Is it possible to pass out the DOMError object through the onerror event?
EventHandler takes an Event as a parameter, not DOMError.
But we want to pass the domerror likely object through onerror event, So I create a RecordErrorEvent for this purpose. Then the mediaRecorder can fire RecordErrorEvent and let UA to handle it. The error may like "Out of Memory, SecurityError, other recording error and etc.."
onerror isn't any event. It is event handler.
But sure, you could have readonly attribute DOMError; in RecordErrorEvent, though, such API
would looks really odd.
Why not just use RecordErrorEvent and its name attribute?
ya, onerror is and event handler and UA can register a callback and get the event (now is RecordErrorEvent) through the first parameter. I also think the DOMError isn't event, so I create the RecordErrorEvent with name attribute to let UA get the error message through the name attribute.
Attached patch RecordErrorEvent v2 (obsolete) — Splinter Review
Fix commit message
Attachment #760046 - Attachment is obsolete: true
Attachment #760051 - Attachment is obsolete: true
Attachment #760058 - Attachment is obsolete: true
Attachment #760061 - Attachment is obsolete: true
Attachment #760046 - Flags: review?(bugs)
Attachment #760051 - Flags: review?(roc)
Attachment #760058 - Flags: review?(roc)
Attachment #760061 - Flags: review?(roc)
Attachment #760767 - Flags: review?(bugs)
Attached patch EncodedBufferCache v3 (obsolete) — Splinter Review
Change naming from EncodedBufferContainer to EncodedBufferCache
Attachment #760768 - Flags: review?(roc)
Attached patch MediaRecorder v3 (obsolete) — Splinter Review
Attachment #760769 - Flags: review?(roc)
Attachment #760767 - Flags: review?(bugs) → review+
Comment on attachment 760768 [details] [diff] [review]
EncodedBufferCache v3

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

::: content/media/EncodedBufferCache.h
@@ +25,5 @@
> +  ~EncodedBufferCache();
> +  // Process buffers in cache, check if the queue is too large then switch to write buffer to file system
> +  nsresult ProcessBuffers();
> +  // Read all buffer from memory or file System, and switch back to memory, also remove the temp file
> +  nsresult Comsume(void* &aBuffs, uint32_t &aLength);

Consume.

What about comment #94? Either address the comment, or explain why you don't want to :-).
Hi Robert,
So EncodedBufferCache class should have two functions: 
Consume: return the binary raw data,
ExtractBlob: for returning the Blob data in memory or temporary file.
Do you mean this? nsDOMMemoryFile <--> temporary file.
(In reply to Randy Lin [:rlin] from comment #104)
> Hi Robert,
> So EncodedBufferCache class should have two functions: 
> Consume: return the binary raw data,

Do we need Consume? What for?

> ExtractBlob: for returning the Blob data in memory or temporary file.
> Do you mean this? nsDOMMemoryFile <--> temporary file.

I think ExtractBlob should return a new nsIDOMFile. This doesn't actually have to be a file; if you haven't already written a file, you can return a new nsDOMMemoryFile, or a nsDOMMultipartFile. If you have written a file, you can return a new nsDOMFileFile.
The Consume is for extracting the data in EncodedBufferCache. If we have ExtractBlob, I will remove it.
Flags: sec-review?(curtisk) → sec-review+
For temporary file implementation, Cervantes just told me that there is a bug would block the content process open file descriptor directly. 
https://bugzilla.mozilla.org/show_bug.cgi?id=790923
This may require to implement IPC to create/access for this purpose.

I checked the nsIDOMBlob behavior, the rawData would be read through nsIInputStream when data is on-demand. For example:play the blob data.
And release the raw pointer when those blobs have been collected by GC.
So the temporary file should also need to be seekable and keep opened until JS de-reference the blob object that implemented by nsDOMTemporaryFileBlob(?).
(In reply to Randy Lin [:rlin] from comment #107)
> For temporary file implementation, Cervantes just told me that there is a
> bug would block the content process open file descriptor directly. 
> https://bugzilla.mozilla.org/show_bug.cgi?id=790923
> This may require to implement IPC to create/access for this purpose.

We need some kind of temporary file API. Media cache also needs it.

With sandboxing, there might be a way to have the parent process open a file descriptor for a temporary file and pass it to the content process which is only allowed to read/write/close the file.

> I checked the nsIDOMBlob behavior, the rawData would be read through
> nsIInputStream when data is on-demand. For example:play the blob data.
> And release the raw pointer when those blobs have been collected by GC.
> So the temporary file should also need to be seekable and keep opened until
> JS de-reference the blob object that implemented by
> nsDOMTemporaryFileBlob(?).

Yes. Your Blob implementation will need to hold onto the file descriptor.
Attached patch WIP for nsDOMTemporaryFileBlob (obsolete) — Splinter Review
Hi Roc, 
Here is the first implementation of Temporary File Blob,
But we may have some problems:
1. How to deal with the JS playback the Blob during Recording? Now we has created a FD for writing, but if we try to read the Blob data that just put into temporary file, we may cause the seek position wrong (Use global mutex to protect?). Use another read FD may not solve this problem for those OS don't allow open the same file in the same time. 
2. Create FD for every blob, hmm, we may run out of the File Descriptor.

Todo: File Descriptor management, keep alive until the JS de-reference the nsDOMTemporaryFileBlob object.
Attachment #764046 - Flags: feedback?(roc)
(In reply to Randy Lin [:rlin] from comment #109)
> Here is the first implementation of Temporary File Blob,
> But we may have some problems:
> 1. How to deal with the JS playback the Blob during Recording? Now we has
> created a FD for writing, but if we try to read the Blob data that just put
> into temporary file, we may cause the seek position wrong (Use global mutex
> to protect?). Use another read FD may not solve this problem for those OS
> don't allow open the same file in the same time.

I think you can use a mutex in the Blob object.

> 2. Create FD for every blob, hmm, we may run out of the File Descriptor.

I think we have to assume that an application will use its Blobs quickly and release them. Otherwise, if it requests Blobs frequently (e.g. every 25ms) we'll simply run out of memory and die. So I wouldn't worry about running out of FDs for now. We may need to make sure GC runs often enough during recording that these Blobs get GCed quickly.

If we find some applications do strange things like generate a lot of Blobs and store them in an array, we can use some mitigation technique like count how many Blobs are alive for a given MediaRecorder and if that number gets too large, have the MediaRecorder stop generating new Blobs and just wait until the recording finishes and then deliver the rest of the data in a single file. But we don't need to worry about that now.
Hi Robert,
I will create new FD on every recording start function call, keep alive until all blob was released. That may avoid too many FDs usage. Does it make sense?
So you're going to share a single FD among all blobs? How is that going to work?
Hmm, that may be a bad idea and hardly to maintain. I will create a new FD for every dispatchBlobEvent to UA.
But there is a blob slice function that I need to consider for the FD sharing problem.
For slicing, I think you can just make the slice Blob hold a reference to the original Blob, and the original Blob holds the FD.
Attached patch nsDOMTemporaryFileBlob patch v2 (obsolete) — Splinter Review
Hi Roc, 
I have changed to create FD for each blobs.
And create a FileDescOwner for FileDesc maintenance.
Please kindly feedback for my implementation. Thanks a lot.
Attachment #764046 - Attachment is obsolete: true
Attachment #764046 - Flags: feedback?(roc)
Attachment #764557 - Flags: review?(roc)
Comment on attachment 764557 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v2

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

Looks reasonable except that there's no locking around the seek/read. (Too bad there's no NSPR equivalent of pread.) Probably should move FileDescOwner to nsTemporaryFileInputStream and put a mutex in it.
Attached patch nsDOMTemporaryFileBlob patch v3 (obsolete) — Splinter Review
Add mutex for avoiding read the blob share the same FD at the same time.
This implementation doesn't protect write because this blob would be Immutable on JS side.
Attachment #764557 - Attachment is obsolete: true
Attachment #764557 - Flags: review?(roc)
Attachment #764685 - Flags: review?(roc)
Comment on attachment 764685 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v3

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

::: content/base/public/nsDOMFile.h
@@ +523,5 @@
> +  // Create as blob
> +  nsDOMTemporaryFileBlob(PRFileDesc* aFD, uint64_t aStart, uint64_t aLength,
> +                         const nsAString& aContentType)
> +    : nsDOMFile(aContentType, aLength),
> +      mFileDescOwner(new FileDescOwner(aFD)),

Comment that this takes ownership of aFD and the caller must not close it.

::: content/base/src/nsDOMFile.cpp
@@ +783,5 @@
> +                                    const nsAString& aContentType)
> +{
> +  nsCOMPtr<nsIDOMBlob> t =
> +    new nsDOMTemporaryFileBlob(this, aStart, aLength, aContentType);
> +  return t.forget();

Don't you need to add mStart to aStart? And check that aStart + aLength <= mLength?

::: content/media/EncodedBufferCache.cpp
@@ +61,5 @@
>    if (mTempFileEnabled) {
> +    // regenerate new FD for next write
> +    blob = new nsDOMTemporaryFileBlob(mFD, 0, mTempFileSize, aContentType);
> +    // fallback to memory blob
> +    mTempFileEnabled = false;

I think you need to set mFD to null so it doesn't get closed later.

::: netwerk/base/src/nsFileStreams.cpp
@@ +1036,5 @@
> +
> +NS_IMETHODIMP
> +nsTemporaryFileInputStream::Read(char* buffer, uint32_t count, uint32_t* bytesRead)
> +{
> +  mozilla::MutexAutoLock lock(*mFileDescOwner->FileMutex());

Move this down to where we actually start using the FD.

@@ +1053,5 @@
> +
> +  if (count > mEnd - mPos)
> +    count = mEnd - mPos;
> +
> +  if (mFileDescOwner->mFD) {

Why this check? It can't be null.

::: netwerk/base/src/nsFileStreams.h
@@ +298,5 @@
>          Close();
>      }
>  };
>  
> +//used for release the PRFileDesc

// Used to release a PRFileDesc

@@ +302,5 @@
> +//used for release the PRFileDesc
> +class FileDescOwner
> +{
> +friend class nsDOMTemporaryFileBlob;
> +friend class nsTemporaryFileInputStream;

Indent these friend declarations. But you shouldn't friend  nsDOMTemporaryFileBlob from here, that seems wrong.

I would actually put this class inside nsTemporaryFileInputStream. Or else rename it to nsFileDescOwner, since we're not in a mozilla namespace here.

@@ +310,5 @@
> +    : mFD(aFD),
> +      mMutex("FileDescOwner::mMutex") { }
> +  ~FileDescOwner()
> +  {
> +    MOZ_ASSERT(mFD);

Move this assert to the constructor so any bug is found closer to the cause.

@@ +312,5 @@
> +  ~FileDescOwner()
> +  {
> +    MOZ_ASSERT(mFD);
> +    PR_Close(mFD);
> +    mFD = nullptr;

No point in assigning null here. The object is destroyed immediately anyway.

@@ +314,5 @@
> +    MOZ_ASSERT(mFD);
> +    PR_Close(mFD);
> +    mFD = nullptr;
> +  }
> +  mozilla::Mutex* FileMutex() { return &mMutex; }

return a reference, not a pointer

@@ +335,5 @@
> +private:
> +  uint32_t mEnd;
> +  uint32_t mPos;
> +  uint32_t mStart;
> +  nsRefPtr<FileDescOwner> mFileDescOwner;

Make this the first field. Saves space.
Attached patch nsDOMTemporaryFileBlob patch v4 (obsolete) — Splinter Review
Hi roc,
I have uploaded new patch again.
Thank for your review if you found anything wrong in this.
Attachment #764685 - Attachment is obsolete: true
Attachment #764685 - Flags: review?(roc)
Attachment #765218 - Flags: review?(roc)
Attached patch EncodedBufferCache v4 (obsolete) — Splinter Review
Update for using nsDOMTemporaryFileBlob
Attachment #760768 - Attachment is obsolete: true
Attachment #760768 - Flags: review?(roc)
Attachment #765224 - Flags: review?(roc)
Add RecordErrorEvent.webidl, need by
Bug 847611 - Paris bindings for autogenerated events
Attached patch MediaRecorder v4 (obsolete) — Splinter Review
1. remove pause/resume function
2. renaming some task class
3. add more error handle
Attachment #760769 - Attachment is obsolete: true
Attachment #760769 - Flags: review?(roc)
Attachment #765248 - Flags: review?(roc)
Comment on attachment 765218 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v4

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

::: content/base/src/nsDOMFile.cpp
@@ +786,5 @@
> +      aLength > mLength || (aStart + aLength) > (mStart + mLength))
> +  return nullptr;
> +
> +  nsCOMPtr<nsIDOMBlob> t =
> +    new nsDOMTemporaryFileBlob(this, aStart, aLength, aContentType);

This code is wrong. aStart should be relative to mStart --- i.e., aStart==0 means that the slice starts from mStart.

::: netwerk/base/src/nsFileStreams.cpp
@@ +1062,5 @@
> +    return rv;
> +  }
> +
> +  mPos += (uint32_t)result;
> +    *bytesRead = (uint32_t)result;

Fix indent.

::: netwerk/base/src/nsFileStreams.h
@@ +335,5 @@
> +private:
> +  nsRefPtr<nsFileDescOwner> mFileDescOwner;
> +  uint32_t mEnd;
> +  uint32_t mPos;
> +  uint32_t mStart;

These need to be uint64_t>. Media files can be over 4GB.
Comment on attachment 765224 [details] [diff] [review]
EncodedBufferCache v4

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

::: content/media/EncodedBufferCache.h
@@ +26,5 @@
> +  ~EncodedBufferCache();
> +  // Process buffers in cache, check if the queue is too large then switch to write buffer to file system
> +  nsresult ProcessBuffers();
> +  // Read all buffer from memory or file System, also Remove the temporary file
> +  nsresult ExtractBlob(const nsString aContentType, nsIDOMBlob** aBlob);

const nsAString& aContentType

return already_AddRefed<nsIDOMBlob>.

@@ +28,5 @@
> +  nsresult ProcessBuffers();
> +  // Read all buffer from memory or file System, also Remove the temporary file
> +  nsresult ExtractBlob(const nsString aContentType, nsIDOMBlob** aBlob);
> +  // Init class member and clean the buffer in this object
> +  void Init(uint32_t aMaxMemoryStorage);

Document what aMaxMemoryStorage means.

@@ +30,5 @@
> +  nsresult ExtractBlob(const nsString aContentType, nsIDOMBlob** aBlob);
> +  // Init class member and clean the buffer in this object
> +  void Init(uint32_t aMaxMemoryStorage);
> +
> +  nsTArray<nsTArray<uint8_t> >* GetBuffers() { return mEncodedBuffers; }

It's not clear how to use this object. It looks like you're expecting people to append their own buffers to the result of GetBuffers. It would be easier to understand if you instead offered an AppendBuffer method taking an nsTArray<uint8_t> (and using SwapElements with it).

if you do that, then I think you don't need ProcessBuffers. We can just do it after every AppendBuffer.
Attached patch nsDOMTemporaryFileBlob patch v5 (obsolete) — Splinter Review
Fix some nit, change naming.
Attachment #765218 - Attachment is obsolete: true
Attachment #765218 - Flags: review?(roc)
Attachment #768200 - Flags: review?(roc)
Attached patch EncodedBufferCache v5 (obsolete) — Splinter Review
Change ProcessBuffers to AppendBuffer.
Attachment #765224 - Attachment is obsolete: true
Attachment #765224 - Flags: review?(roc)
Attachment #768202 - Flags: review?(roc)
Attached patch EncodedBufferCache v5 (obsolete) — Splinter Review
avoid build warning for memory copy
Attachment #768202 - Attachment is obsolete: true
Attachment #768202 - Flags: review?(roc)
Attachment #768204 - Flags: review?(roc)
Attached patch MediaRecorder v5 (obsolete) — Splinter Review
Fix some nits.
Attachment #765248 - Attachment is obsolete: true
Attachment #765248 - Flags: review?(roc)
Attachment #768205 - Flags: review?(roc)
Comment on attachment 765226 [details] [diff] [review]
RecordErrorEvent v2-1

Hi Olli,
This one also need your review again because it add new webIdl for this event. Thanks a lot.
Attachment #765226 - Flags: review?(bugs)
Comment on attachment 760767 [details] [diff] [review]
RecordErrorEvent v2

Obsoletes this for new one provided.
Attachment #760767 - Attachment is obsolete: true
Can I add the RecordErrorEvent into test case?
3:41:32 INFO - 836 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface RecordErrorEvent to all webpages as a property on the window?
Bug 881402 - Intermittent test_interfaces.html | Exited with code 1 during test run | application crashed [@ nsCycleCollector::CollectWhite(nsICycleCollectorListener*)]
Comment on attachment 768200 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v5

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

::: content/base/public/nsDOMFile.h
@@ +546,5 @@
> +private:
> +  nsRefPtr<nsFileDescOwner> mFileDescOwner;
> +  uint64_t mLength;
> +  uint64_t mStartPos;
> +  nsString mContentType;

Move mFileDescOwner to just above mContentType, so the 64-bit types are all together.

::: content/base/src/nsDOMFile.cpp
@@ +782,5 @@
> +nsDOMTemporaryFileBlob::CreateSlice(uint64_t aStart, uint64_t aLength,
> +                                    const nsAString& aContentType)
> +{
> +  // aStart is the blob offset
> +  if ((aStart > mLength) || (aLength > mLength))

This should check aStart + aLength > mLength.

@@ +794,5 @@
> +NS_IMETHODIMP
> +nsDOMTemporaryFileBlob::GetInternalStream(nsIInputStream **aStream)
> +{
> +  if (mLength > INT32_MAX)
> +    return NS_ERROR_FAILURE;

you won't need this check.

::: netwerk/base/src/nsFileStreams.cpp
@@ +1013,5 @@
> +nsTemporaryFileInputStream::nsTemporaryFileInputStream(nsFileDescOwner* aFileDescOwner, uint32_t aStartPos, uint32_t aEndPos)
> +  : mFileDescOwner(aFileDescOwner),
> +    mEndPos(aEndPos),
> +    mStartPos(aStartPos),
> +    mClosed(false) { }

assert here that aStartPos <= aEndPos.

@@ +1026,5 @@
> +NS_IMETHODIMP
> +nsTemporaryFileInputStream::Available(uint64_t * bytesAvailable)
> +{
> +  if (mClosed) return NS_BASE_STREAM_CLOSED;
> +  if (mEndPos < mStartPos) return NS_ERROR_UNEXPECTED;

This should be an assertion. It should never happen.

@@ +1045,5 @@
> +  if (mStartPos == mEndPos) {
> +    return NS_OK;
> +  }
> +  if (mStartPos > mEndPos) {
> +    return NS_ERROR_UNEXPECTED;

This should be an assertion. It should never happen.

@@ +1049,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  if (count > mEndPos - mStartPos)
> +    count = mEndPos - mStartPos;

count = std::min(count, mEndPos - mStartPos);

@@ +1072,5 @@
> +                                         void *            closure,
> +                                         uint32_t          count,
> +                                         uint32_t *        bytesRead)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

Are you sure you don't need to implement this?

::: netwerk/base/src/nsFileStreams.h
@@ +324,5 @@
> +
> +class nsTemporaryFileInputStream : public nsIInputStream
> +{
> +public:
> +  nsTemporaryFileInputStream(nsFileDescOwner* aFileDescOwner, uint32_t aStartPos, uint32_t aEndPos);

These need to be uint64_t!

@@ +334,5 @@
> +
> +private:
> +  nsRefPtr<nsFileDescOwner> mFileDescOwner;
> +  uint64_t mEndPos;
> +  uint64_t mStartPos;

Put mStartPos before mEndPos.
Comment on attachment 768204 [details] [diff] [review]
EncodedBufferCache v5

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

::: content/media/EncodedBufferCache.cpp
@@ +23,5 @@
> +    delete mEncodedBuffers;
> +}
> +
> +nsresult
> +EncodedBufferCache::AppendBuffer(nsTArray<nsTArray<uint8_t> >& aOutBufs)

It's probably simpler to have AppendBuffer just take a single nsTArray<uint8_t> and append it. The caller can call AppendBuffer in a loop if necessary.

@@ +31,5 @@
> +  nsresult rv = NS_OK;
> +  int64_t memBuffsCount = 0;
> +  for (uint32_t i = 0; i < aOutBufs.Length(); i++) {
> +    mEncodedBuffers->AppendElement();
> +    mEncodedBuffers->LastElement().SwapElements(aOutBufs[i]);

You can write this as
  mEncodedBuffers.AppendElement()->SwapElements(aOutBufs[i]);

@@ +34,5 @@
> +    mEncodedBuffers->AppendElement();
> +    mEncodedBuffers->LastElement().SwapElements(aOutBufs[i]);
> +  }
> +  for (uint32_t i = 0; i < mEncodedBuffers->Length(); i++)
> +    memBuffsCount += mEncodedBuffers->ElementAt(i).Length();

{} around this statement.

Either create a helper method that adds up the sizes of everything in mEncodedBuffers, so you can call it here and in ExtractBlob below, or else keep track of it in a member of this class. In fact you could replace mTempFileSize with a field mDataSize which contains the sum of the sizes of the buffers passed to AppendBuffer since the last ExtractBlob. I'd prefer the latter.

@@ +39,5 @@
> +
> +  if (memBuffsCount > mMaxMemoryStorage && !mTempFileEnabled) {
> +    // dump queue memory buffer into temporary file.
> +    rv = InitTempFile();
> +    NS_ENSURE_SUCCESS(rv, rv);

I think AppendBuffer should be not return a result. If we fail to initialize the temporary file, just keep the data in memory.

@@ +43,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  } else if (mTempFileEnabled) {
> +    //has created temporary file, write buffer in it
> +    for (uint32_t i = 0; i < mEncodedBuffers->Length(); i++) {
> +      int64_t amount = PR_Write(mFD, mEncodedBuffers->ElementAt(i).Elements(), mEncodedBuffers->ElementAt(i).Length());

Instead of having two copies of the file-writing code, you can just have one. Make InitTempFile just create the file but not write any data. Then here, take out the "else" above so this loop writes everything in mEncodedBuffers to the file and then empties mEncodedBuffers.

@@ +48,5 @@
> +      if (amount <  mEncodedBuffers->ElementAt(i).Length()) {
> +        NS_WARNING("Failed to write media cache block!");
> +        return NS_ERROR_FAILURE;
> +      }
> +      mEncodedBuffers->ElementAt(i).Clear();

No need to clear here. The final Clear() below will take care of everything.

@@ +73,5 @@
> +  } else {
> +    //extract Encoded data in memory
> +    int memBuffsCount = 0;
> +    for (uint32_t i = 0; i < mEncodedBuffers->Length(); i++)
> +      memBuffsCount += mEncodedBuffers->ElementAt(i).Length();

{} around this statement.

::: content/media/EncodedBufferCache.h
@@ +23,5 @@
> +{
> +public:
> +  EncodedBufferCache();
> +  ~EncodedBufferCache();
> +  // Append buffers in cache, check if the queue is too large then switch to write buffer to file system

Also explain that aOutBufs is cleared.

@@ +26,5 @@
> +  ~EncodedBufferCache();
> +  // Append buffers in cache, check if the queue is too large then switch to write buffer to file system
> +  nsresult AppendBuffer(nsTArray<nsTArray<uint8_t> >& aOutBufs);
> +  // Read all buffer from memory or file System, also Remove the temporary file
> +  already_AddRefed<nsIDOMBlob> ExtractBlob(const nsAString &aContentType, nsresult aResult);

Why are we passing aResult here? That doesn't really make sense.

Also, mention that this clears the internal data buffers so we're ready for more buffers to be appended.

@@ +29,5 @@
> +  // Read all buffer from memory or file System, also Remove the temporary file
> +  already_AddRefed<nsIDOMBlob> ExtractBlob(const nsAString &aContentType, nsresult aResult);
> +  // Init class member and clean the buffer in this object, aMaxMemoryStorage is defined how much buffers can be allowed in memory.
> +  // If the mEncodedBuffers size is over the aMaxMemoryStorage, temorary file will be opened for storing the encoded data.
> +  void Init(uint32_t aMaxMemoryStorage);

Why do we need an Init method? We can just pass this parameter to the constructor.

@@ +39,5 @@
> +  bool mTempFileEnabled;
> +  // the current buffer size has been written to temporary file
> +  uint32_t mTempFileSize;
> +  //store the pointer contains the encoded data.
> +  nsTArray<nsTArray<uint8_t> >* mEncodedBuffers;

I don't see why this is a pointer to an array. It should just be nsTArray<nsTArray<uint8_t> >.

@@ +45,5 @@
> +  PRFileDesc* mFD;
> +  // The maximal buffer allowed in memory
> +  uint32_t mMaxMemoryStorage;
> +  // Used to protect the mEncodedBuffer for avoiding AppendBuffer/Consume on different thread at the same time.
> +  ReentrantMonitor mReentrantMonitor;

Reorder the fields so that pointers or structures containing pointers are first, then uint32_ts, then bools.
Attachment #765226 - Flags: review?(bugs) → review+
Attached patch nsDOMTemporaryFileBlob patch v6 (obsolete) — Splinter Review
1. Move nsTemporaryFileInputStream to single file, 
2. fix nits.
Attachment #768200 - Attachment is obsolete: true
Attachment #768200 - Flags: review?(roc)
Attachment #768847 - Flags: review?(roc)
Attached patch EncodedBufferCache v5 (obsolete) — Splinter Review
Fix nits, clean some unnecessary code.
Attachment #768204 - Attachment is obsolete: true
Attachment #768204 - Flags: review?(roc)
Attachment #768875 - Flags: review?(roc)
Comment on attachment 768847 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v6

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

::: content/base/public/nsDOMFile.h
@@ +525,5 @@
> +                         const nsAString& aContentType)
> +    : nsDOMFile(aContentType, aLength),
> +      mLength(aLength),
> +      mStartPos(aStartPos),
> +      mContentType(aContentType) { mFileDescOwner = new nsTemporaryFileInputStream::FileDescOwner(aFD); }

Put this statement on a new line

::: content/base/src/nsDOMFile.cpp
@@ +781,5 @@
> +already_AddRefed<nsIDOMBlob>
> +nsDOMTemporaryFileBlob::CreateSlice(uint64_t aStart, uint64_t aLength,
> +                                    const nsAString& aContentType)
> +{
> +  // aStart would start from 0

This comment isn't clear. Probably just remove it.

@@ +782,5 @@
> +nsDOMTemporaryFileBlob::CreateSlice(uint64_t aStart, uint64_t aLength,
> +                                    const nsAString& aContentType)
> +{
> +  // aStart would start from 0
> +  if ((aStart +aLength) > mLength)

space before aLength. No parens needed around aStart + aLength

@@ +793,5 @@
> +
> +NS_IMETHODIMP
> +nsDOMTemporaryFileBlob::GetInternalStream(nsIInputStream **aStream)
> +{
> +  nsCOMPtr<nsTemporaryFileInputStream> stream = new nsTemporaryFileInputStream(mFileDescOwner, mStartPos, mStartPos + mLength);

Put a line break after =

::: netwerk/base/src/nsTemporaryFileInputStream.cpp
@@ +11,5 @@
> +nsTemporaryFileInputStream::nsTemporaryFileInputStream(FileDescOwner* aFileDescOwner, uint64_t aStartPos, uint64_t aEndPos)
> +  : mFileDescOwner(aFileDescOwner),
> +    mStartPos(aStartPos),
> +    mEndPos(aEndPos),
> +    mClosed(false) { NS_ASSERTION(aStartPos <= aEndPos, "StartPos should less equal than EndPos!"); }

Statement on new line.

@@ +23,5 @@
> +
> +NS_IMETHODIMP
> +nsTemporaryFileInputStream::Available(uint64_t * bytesAvailable)
> +{
> +  if (mClosed) return NS_BASE_STREAM_CLOSED;

"return" goes on a new line.

@@ +48,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  if (count > mEndPos - mStartPos)
> +    count = std::min((uint64_t)count, mEndPos - mStartPos);

Shouldn't this just be count = uint32_t(mEndPos - mStartPos). No point in using min if you know mEndPos - mStartPos is smaller.

@@ +84,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (count > mEndPos - mStartPos)
> +    count = std::min((uint64_t)count, mEndPos - mStartPos);

Same comment as above

@@ +88,5 @@
> +    count = std::min((uint64_t)count, mEndPos - mStartPos);
> +
> +  void* buffer = moz_malloc(count);
> +  if (!buffer) {
> +    return NS_ERROR_OUT_OF_MEMORY;

Instead of doing a potentially huge allocation here, I suggest using a stack buffer of fixed size and writing the data out in chunks of that size, in a loop of reads and calls to 'writer'.
Comment on attachment 768875 [details] [diff] [review]
EncodedBufferCache v5

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

::: content/media/EncodedBufferCache.cpp
@@ +20,5 @@
> +
> +  if (!mTempFileEnabled && mDataSize > mMaxMemoryStorage) {
> +    nsresult rv = NS_OpenAnonymousTemporaryFile(&mFD);
> +    if (!NS_FAILED(rv))
> +      mTempFileEnabled = true;

{} around this statement.

@@ +24,5 @@
> +      mTempFileEnabled = true;
> +  }
> +
> +  if (mTempFileEnabled) {
> +    //has created temporary file, write buffer in it

space after //

@@ +33,5 @@
> +      }
> +    }
> +    mEncodedBuffers.Clear();
> +  } else {
> +    //do nothing, ExtractBlob would read the original buffer data.

Don't need this comment.

@@ +59,5 @@
> +      for (uint32_t i = 0, offset = 0; i < mEncodedBuffers.Length(); i++) {
> +        memcpy((uint8_t*)blobData + offset, mEncodedBuffers.ElementAt(i).Elements(),
> +               mEncodedBuffers.ElementAt(i).Length());
> +        offset += mEncodedBuffers.ElementAt(i).Length();
> +      }

This allocation and copy can be avoided, by creating a custom nsIDOMBlob implementation that owns an nsTArray<nsTArray<uint8_t> > and transfering mEncodedBuffers to it, but let's do that in a followup bug.

@@ +63,5 @@
> +      }
> +      blob = new nsDOMMemoryFile(blobData, mDataSize,
> +                                 aContentType);
> +      mEncodedBuffers.Clear();
> +      mDataSize = 0;

mDataSize = 0 can be moved out to just before blob.forget().

::: content/media/EncodedBufferCache.h
@@ +26,5 @@
> +  : mFD(nullptr),
> +    mReentrantMonitor("EncodedBufferCache.Data.Monitor"),
> +    mDataSize(0),
> +    mMaxMemoryStorage(aMaxMemoryStorage),
> +    mTempFileEnabled(false) { }

I think we need a destructor here. It should assert that mDataSize is 0, i.e., ExtractBlob was called before this object was destroyed.

@@ +30,5 @@
> +    mTempFileEnabled(false) { }
> +
> +  // Append buffers in cache, check if the queue is too large then switch to write buffer to file system
> +  // aBufs will append to mEncodedBuffers or temporary File
> +  void AppendBuffer(nsTArray<uint8_t> & aBufs);

Should be called aBuf now there's only one. Say in your comment that aBuf will be cleared.

@@ +42,5 @@
> +  PRFileDesc* mFD;
> +  // Used to protect the mEncodedBuffer for avoiding AppendBuffer/Consume on different thread at the same time.
> +  ReentrantMonitor mReentrantMonitor;
> +  // the current buffer size can be read
> +  uint32_t mDataSize;

This should be a uint64_t!
Attached patch nsDOMTemporaryFileBlob patch v7 (obsolete) — Splinter Review
1. Fix format problem
2. Use small buffer for copying data from temporary files to blob
3. Re-use ReadSegments for read function by NS_CopySegmentToBuffer.
Attachment #768847 - Attachment is obsolete: true
Attachment #768847 - Flags: review?(roc)
Attachment #769497 - Flags: review?(roc)
Depends on: 888742
Attached patch EncodedBufferCache v6 (obsolete) — Splinter Review
Thanks roc. 
Fix nits.
Also create Bug 888742 for implementing custom-blob.
Attachment #768875 - Attachment is obsolete: true
Attachment #768875 - Flags: review?(roc)
Attachment #769498 - Flags: review?(roc)
Comment on attachment 769497 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v7

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

::: netwerk/base/src/nsTemporaryFileInputStream.cpp
@@ +57,5 @@
> +  }
> +
> +  if (mStartPos == mEndPos) {
> +    return NS_OK;
> +  }

I think you won't need this check actually.

@@ +62,5 @@
> +
> +  mozilla::MutexAutoLock lock(mFileDescOwner->FileMutex());
> +  PR_Seek64(mFileDescOwner->mFD, mStartPos, PR_SEEK_SET);
> +
> +  count = uint32_t(mEndPos - mStartPos);

This isn't right. You're losing the passed-in value of 'count'. You need to use std::min or an "if" statement (not both)

@@ +65,5 @@
> +
> +  count = uint32_t(mEndPos - mStartPos);
> +  uint32_t remainBufCount = count;
> +
> +  char bufs[4096];

'buf' since there's only one buffer

@@ +67,5 @@
> +  uint32_t remainBufCount = count;
> +
> +  char bufs[4096];
> +  while (remainBufCount > 0) {
> +    uint32_t bufCount = remainBufCount > 4096 ? 4096 : remainBufCount;

use sizeof(buf) instead of repeating 4096

@@ +71,5 @@
> +    uint32_t bufCount = remainBufCount > 4096 ? 4096 : remainBufCount;
> +    int32_t read_result = PR_Read(mFileDescOwner->mFD, bufs, bufCount);
> +    if (read_result < 0) {
> +      rv = NS_ErrorAccordingToNSPR();
> +      return rv;

Just "return NS_ErrorAccordingToNSPR();"

@@ +75,5 @@
> +      return rv;
> +    }
> +    uint32_t write_result = 0;
> +    rv = writer(this, closure, bufs,
> +                count - remainBufCount, bufCount, &write_result);

The declaration of 'rv' can be moved here.

@@ +80,5 @@
> +    remainBufCount -= bufCount;
> +    if (NS_SUCCEEDED(rv)) {
> +      NS_ASSERTION(write_result <= bufCount,
> +                   "writer should not write more than we asked it to write");
> +      mStartPos += bufCount;

We can update mStartPos even if 'writer' failed.
Comment on attachment 769498 [details] [diff] [review]
EncodedBufferCache v6

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

Great, this is nice and simple now.
Attachment #769498 - Flags: review?(roc) → review+
Attached patch nsDOMTemporaryFileBlob patch v8 (obsolete) — Splinter Review
Fix nits, thanks for reviewing. :)
Attachment #769497 - Attachment is obsolete: true
Attachment #769497 - Flags: review?(roc)
Attachment #769716 - Flags: review?(roc)
Comment on attachment 769716 [details] [diff] [review]
nsDOMTemporaryFileBlob patch v8

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

::: netwerk/base/src/nsTemporaryFileInputStream.cpp
@@ +63,5 @@
> +  uint32_t remainBufCount = count;
> +
> +  char buf[4096];
> +  while (remainBufCount > 0) {
> +    uint32_t bufCount = remainBufCount > sizeof(buf) ? sizeof(buf) : remainBufCount;

std::min(remainBufCount, sizeof(buf))

@@ +71,5 @@
> +    }
> +    uint32_t write_result = 0;
> +    nsresult rv;
> +    rv = writer(this, closure, buf,
> +                count - remainBufCount, bufCount, &write_result);

nsresult rv = writer(...)
Comment on attachment 769498 [details] [diff] [review]
EncodedBufferCache v6

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

::: content/media/EncodedBufferCache.h
@@ +17,5 @@
> +class ReentrantMonitor;
> +/**
> + * Data is moved into a temporary file when it grows beyond
> + * the maximal size passed in the Init function.
> + */

Please add to this comment, to explain that the AppendBuffer and ExtractBlob methods are thread-safe and can be called on different threads at the same time.
Comment on attachment 768205 [details] [diff] [review]
MediaRecorder v5

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

::: content/media/MediaRecorder.cpp
@@ +32,5 @@
> +
> +NS_IMPL_ADDREF_INHERITED(MediaRecorder, nsDOMEventTargetHelper)
> +NS_IMPL_RELEASE_INHERITED(MediaRecorder, nsDOMEventTargetHelper)
> +
> +//this task is used for triggering the create blob function call. The recorder object won't release in this class life.

I'm not sure what the second sentence means.

Space after //.

@@ +43,5 @@
> +  NS_IMETHODIMP Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    nsresult rv = NS_OK;
> +    rv = mRecorder->CreateAndDispatchBlobEvent();

why set it to NS_OK and then override it? Just do "nsresult rv = mRecorder->CreateAndDispatchBlobEvent();"

@@ +45,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +    nsresult rv = NS_OK;
> +    rv = mRecorder->CreateAndDispatchBlobEvent();
> +    if NS_FAILED(rv)
> +      mRecorder->NotifyError(rv);

{} around this statement

@@ +51,5 @@
> +    return NS_OK;
> +  }
> +  MediaRecorder* mRecorder;
> +};
> +// Push out Error message task, run in main thread

Blank line after the end of the previous class

@@ +105,5 @@
> +  };
> +
> +  virtual ~ExtractEncodedDataTask()
> +  {
> +    NS_DispatchToMainThread(new ReleaseEncoderThreadTask(mRecorder.forget()));

This is a bit weird.

I don't think we need ExtractEncodedDataTask. You should be able to use NS_NewRunnableMethod to call MediaRecorder::ExtractEncodedData. Then ExtractEncodedData, when it exits, can dispatch a ReleaseEncoderThreadTask.

@@ +135,5 @@
> +  SetIsDOMBinding();
> +}
> +
> +void
> +MediaRecorder::ExtractEncodedData(MediaRecorder* aRecorder)

isn't aRecorder==this? Why is it needed?

@@ +140,5 @@
> +{
> +  TimeStamp lastBlobTimeStamp = TimeStamp::Now();
> +  do {
> +    nsTArray<nsTArray<uint8_t> > outputBufs;
> +    this->mEncoder->GetEncodedData(&outputBufs, mMimeType);

don't need explicit 'this' here.

@@ +148,5 @@
> +        break;
> +    }
> +    if ((TimeStamp::Now() - lastBlobTimeStamp).ToMilliseconds() > mTimeSlice) {
> +      NS_DispatchToMainThread(new PushBlobTask(aRecorder));
> +      lastBlobTimeStamp += TimeDuration::FromMilliseconds(mTimeSlice);

Set it to TimeStamp::Now(). For example, if for some reason there was a 100ms stall during encoding and the timeslice is 25ms, we don't want to suddenly fire 4 notifications. We should just fire 1.

@@ +183,5 @@
> +
> +  mEncodedBufferCache->Init(MAX_ALLOW_MEMORY_BUFFER);
> +
> +  if (mEncoder == nullptr)
> +    mEncoder = MediaEncoder::CreateEncoder(NS_LITERAL_STRING(""));

{}

@@ +289,5 @@
> +  return DispatchDOMEvent(nullptr, event, nullptr, nullptr);
> +}
> +
> +void
> +MediaRecorder::Notify(const nsAString & aStr)

Call this DispatchSimpleEvent.

@@ +329,5 @@
> +    return;
> +  }
> +  nsString errorMsg;
> +  switch (aRv)
> +  {

{ on same line as 'switch'

@@ +365,5 @@
> +    return false;
> +
> +  bool subsumes;
> +  if (NS_FAILED(doc->NodePrincipal()->Subsumes(principal, &subsumes)))
> +    return false;

You need to check the value of 'subsumes'!!!

::: content/media/MediaRecorder.h
@@ +23,5 @@
> +
> +/**
> + * Implementation for https://dvcs.w3.org/hg/dap/raw-file/default/media-stream-capture/MediaRecorder.html
> + * The UA must pass the mediaStream to the MediaRecorder and let Encoder to get the raw data, encoding it.
> + * This class would create a thread to read encoded data from Encoder and pop-up block data to JS context on every timeSlice.

I'm not sure what "pop-up block data" means.

You should explain the threading model in more detail. Explain that mEncoder is used only on the encoding thread.

@@ +89,5 @@
> +
> +  MediaRecorder(const MediaRecorder& x) MOZ_DELETE; // prevent bad usage
> +
> +  // The interval of timer which generate the dataavailable event, unit is millisecond.
> +  int32_t mTimeSlice;

Put this last, after the pointers and structs which contain pointers.

@@ +91,5 @@
> +
> +  // The interval of timer which generate the dataavailable event, unit is millisecond.
> +  int32_t mTimeSlice;
> +
> +  // Runnable thread for encodernsAString aErrorMsg

some kind of typo here

@@ +93,5 @@
> +  int32_t mTimeSlice;
> +
> +  // Runnable thread for encodernsAString aErrorMsg
> +  nsCOMPtr<nsIThread> mReadThread;
> +  // Encoder object, initialized in start(), destoryed in ~MediaRecorder

destroyed

@@ +98,5 @@
> +  nsRefPtr<MediaEncoder> mEncoder;
> +  // MediaStream passed from js context
> +  nsRefPtr<DOMMediaStream> mStream;
> +  // The current state of the MediaRecorder object.
> +  RecordingState mState;

Put this last, after mTimeSlice.

@@ +100,5 @@
> +  nsRefPtr<DOMMediaStream> mStream;
> +  // The current state of the MediaRecorder object.
> +  RecordingState mState;
> +  // encoder buffer cache object
> +  EncodedBufferCache* mEncodedBufferCache;

Use nsAutoPtr so you don't need any manual 'delete'.
Add comment.
Attachment #769498 - Attachment is obsolete: true
Attachment #769996 - Flags: review?(roc)
Fix nits.
Attachment #769716 - Attachment is obsolete: true
Attachment #769716 - Flags: review?(roc)
Attachment #769997 - Flags: review?(roc)
Attached patch MediaRecorder v6 (obsolete) — Splinter Review
1. Fix nits
2. For the ExtractEncodedDataTask, I try to use the NS_NewRunnableMethod to invoke the ExtractEncodedData, but found it would crash when exit this function call.
That cause by exiting thread and try to gc this object, but the nsDOMEventTargetHelper would check if this action is running at main threading or not....
3. For pause/resume, still invegest, I try to use the CreateTrackUnionStream and set FLAG_BLOCK_OUT flag, but still finding a way to stop feeding raw data to encoder. Do we have any function can do this stuff? Or use graph to build connection and add/remove output?
Attachment #768205 - Attachment is obsolete: true
Attachment #768205 - Flags: review?(roc)
Attachment #770340 - Flags: review?(roc)
Comment on attachment 770340 [details] [diff] [review]
MediaRecorder v6

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

::: content/media/MediaRecorder.cpp
@@ +109,5 @@
> +  };
> +
> +  virtual ~ExtractEncodedDataTask()
> +  {
> +    NS_DispatchToMainThread(new ReleaseEncoderThreadTask(mRecorder.forget()));

Why not dispatch this from ExtractEncodedData just before it returns? That seems simpler.

@@ +130,5 @@
> +  BindToOwner(aOwnerWindow);
> +}
> +
> +MediaRecorder::MediaRecorder(DOMMediaStream& aStream)
> +  : mEncodedBufferCache(nullptr),

nsAutoPtr automatically initializes to null

::: content/media/MediaRecorder.h
@@ +51,5 @@
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(MediaRecorder,
> +                                           nsDOMEventTargetHelper)
> +
> +  // WebIDL
> +  // Start record, if timeSlice has been provided, every timeSlice milliseconds will

// Start recording. Also, make sentences instead of using comma everywhere.

@@ +104,5 @@
> +  // MediaStream passed from js context
> +  nsRefPtr<DOMMediaStream> mStream;
> +  // encoder buffer cache object
> +  nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;
> +  //The MIME type for recording. It specifies the container format as well as the audio and video capture formats.

Space after //. Please stop making this mistake :-)
(In reply to Randy Lin [:rlin] from comment #148)
> 2. For the ExtractEncodedDataTask, I try to use the NS_NewRunnableMethod to
> invoke the ExtractEncodedData, but found it would crash when exit this
> function call.
> That cause by exiting thread and try to gc this object, but the
> nsDOMEventTargetHelper would check if this action is running at main
> threading or not....

I don't understand this. NS_NewRunnableMethod creates something that looks just like ExtractEncodedDataTask, holding a ref to the target object until the runnable is destroyed.

> 3. For pause/resume, still invegest, I try to use the CreateTrackUnionStream
> and set FLAG_BLOCK_OUT flag, but still finding a way to stop feeding raw
> data to encoder. Do we have any function can do this stuff? Or use graph to
> build connection and add/remove output?

Let's split Pause and Resume into a separate bug that can land later.

To pause/resume the internal stream, call MediaStream::ChangeExplicitBlockerCount. Pass 1 to pause the stream, -1 to resume it.
The NS_NewRunnableMethod will new nsRunnableMethodImpl object and pass the recorder pointer in it. When the recording task terminated, The nsDOMEventTargetHelper cycle collect is called and abort on nsDOMEventTargetHelper.cpp:73, _mOwningThread.GetThread() == PR_GetCurrentThread() nsDOMEventTargetHelper is not thread-safe.
Depends on: 889720
Remove ~ExtractEncodedDataTask.
Fix comment nits. :)
Attachment #770340 - Attachment is obsolete: true
Attachment #770340 - Flags: review?(roc)
Attachment #770650 - Flags: review?(roc)
We need to add this to avoid test case fail.
Attachment #770654 - Flags: review?(bugs)
Comment on attachment 770650 [details] [diff] [review]
MediaRecorder v6.1

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

OK, I'm done now :-)
Attachment #770650 - Flags: review?(roc) → review+
Of course, we're going to need some tests here :-).
Depends on: 889772
I fired new bug 889772 for test case. 
Robert, Thanks for your review detailly. I learn a lot. :)
Attachment #770654 - Flags: review?(bugs) → review+
\0/
Whiteboard: checkin-needed
It's not clear which patches should land here, and in what order.  Also, please see <https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F> on how to prepare patches so that other people can check them in for you comfortably.  Specifically, it would be great if you could edit the commit message in each patch to reflect what that part of the patch does, as opposed to just repeating the bug summary!  :-)

Last but not least, checkin-needed is a keyword; putting it in the status whiteboard field would most likely make it go unnoticed by people who look for that keyword.
Whiteboard: checkin-needed
I have Updated the check-in patch again and modified the comment message, please help check-in from part 0 to part 5, thanks a lot.
Whiteboard: checkin-needed
Hi Ryan, 
We will add test case on bug 889772, Thanks for your reminding. :)
Summary: Audio Recording - Web API & Implementation → Media Recording - Web API & Implementation
Depends on: 891706
Depends on: 891722
Depends on: 891782
Follow up for documentation on bug 891782
Is the MediaRecording API available for web page on Nightly?
Yes, but currently we only support recording audio to Opus format in an Ogg container.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #172)
> Yes, but currently we only support recording audio to Opus format in an Ogg
> container.

I encountered such an error when called |mediaRecorder.start(1000)|:
  [22:36:55.212] SecurityError: The operation is insecure.
(In reply to Pin Zhang [:pzhang] from comment #173)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #172)
> > Yes, but currently we only support recording audio to Opus format in an Ogg
> > container.
> 
> I encountered such an error when called |mediaRecorder.start(1000)|:
>   [22:36:55.212] SecurityError: The operation is insecure.

I disabled the principal checking temporarily, and it works so that I can continue writing my sample codes, however, I am confused about the parameter of |start|, i.e. timeslice, if I set
|timeslice| to 0, I can still receive |dataavailable| event periodically. 

Is the |start| method the same as the |record| method defined in [1]? As [1] described, if |timeslice| is not provided, no |ondataavailable| will be fired.

[1] https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/RecordingProposal.html
If you set the timeslice to 0 or not set, you should call the requestData() on js and handle the ondataavailable event.
(In reply to Pin Zhang [:pzhang] from comment #174)
> (In reply to Pin Zhang [:pzhang] from comment #173)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> > #172)
> > > Yes, but currently we only support recording audio to Opus format in an Ogg
> > > container.
> > 
> > I encountered such an error when called |mediaRecorder.start(1000)|:
> >   [22:36:55.212] SecurityError: The operation is insecure.
> 
> I disabled the principal checking temporarily, and it works so that I can
> continue writing my sample codes, however, I am confused about the parameter
> of |start|, i.e. timeslice, if I set
> |timeslice| to 0, I can still receive |dataavailable| event periodically. 
> 
> Is the |start| method the same as the |record| method defined in [1]? As [1]
> described, if |timeslice| is not provided, no |ondataavailable| will be
> fired.
> 
> [1]
> https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/
> RecordingProposal.html

Where is the mediaStreaam come from?
I use the gUM and it works well.
(In reply to Randy Lin [:rlin] from comment #176)
> 
> Where is the mediaStreaam come from?
> I use the gUM and it works well.

gUM too, here is my code:
  https://github.com/PinZhang/sample-codes/blob/master/html5/media_recording.html
please help to check if there is a mistake I made, thanks!
Alias: MediaRecording
(In reply to Pin Zhang [:pzhang] from comment #177)
> (In reply to Randy Lin [:rlin] from comment #176)
> > 
> > Where is the mediaStreaam come from?
> > I use the gUM and it works well.
> 
> gUM too, here is my code:
>  
> https://github.com/PinZhang/sample-codes/blob/master/html5/media_recording.
> html
> please help to check if there is a mistake I made, thanks!

Hi Randy, if I set the stream that coming from gUM to an <audio>, and then record the stream object returned by |mozCaptureStreamUntilEnded()| of the audio element (just like what did in attachment 736153 [details] [diff] [review]), security error will be reported, here is the sample codes:
  https://github.com/PinZhang/sample-codes/blob/mediarecorder-security/html5/media_recording.html
the previous example I posted here is fine now, please ignore it.
Keywords: verifyme
QA Contact: jsmith
Depends on: 894134
Depends on: 894413
Blocks: 894848
Blocks: 894861
Depends on: 894348
Depends on: 894341
Depends on: 895730
Depends on: 896303
No longer depends on: 825111
Blocks: MediaRecording
No longer blocks: 894848, 894861
No longer depends on: 855952
For the sake of avoiding duplicating tracking, I'm removing the dependencies here and just using the dependency tree built on bug 896935 for tracking.
Alias: MediaRecording
Depends on: 897776
No longer depends on: 897776
No longer depends on: 902194
Verified smoketests are passing. Mochitests were landed in relevant followup bugs under the tracker bug dependencies in bug 889772.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
Keywords: verifyme
What kind of data we will get in ondataavailable().

1)will receive only audio encoded data?
2)will receive only video encoded data?
3)will receive both audio and video encoded data?

what is the role of mimeType here?

Could you please provide the use cases with this data?
I think you are asking how to control the encoder when UA try to recording. 
In current design, we just implement the audio recording function. So the UA would got audio only data from recorder. 

mimeType attribute is used to indicate the media MIMEType of the blob. So if you got audio, currently we have audio/ogg as mimeType for audio data. If we finished the video part, the UA would get the video/webm video stream.
so it means we have to send encoded audio data and encoded video data separately according to mimeType of blob in ondataavailable(). 

Please correct me if am wrong.
Not really, for video part, we will sent out the encoded data with mineType such as video/webm. So UA don't need to mux the audio/video in JS side.
(In reply to Dhamodaran from comment #183)
> so it means we have to send encoded audio data and encoded video data
> separately according to mimeType of blob in ondataavailable(). 
> 
> Please correct me if am wrong.

Encoded AV data had been multiplexed and put into container(ogg for audio only, webm/mp4 for AV tracks) before.
Depends on: 938107
No longer depends on: 938107
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.