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.

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.

The webidl for Media Recording API https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/RecordingProposal.html

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.

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.

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.

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.

1.Remove the constructor() 2.use EventHandler to replace the older attribute "function?"

correct wrong file upload...

Hi roc, We have made a simple proposal for this function, Can you take a look? https://wiki.mozilla.org/Gecko:Mediarecorder

You meant https://wiki.mozilla.org/Gecko:MediaRecorder

That looks reasonable to me!

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.

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

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.

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.

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

file crash, re-upload again.

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.

Comment on attachment 723757 [details] MediaRecorder dom api Hi Justin, Can you help to review this dom api?

Comment on attachment 723758 [details] RecordingError idl Hi Justin, Can you help to review this dom api?

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.

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.

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.

change license header.

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.

update and mark it as non-patch, sorry.

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

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

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.

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.

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.

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

Fix some core-dump issues.

add pause/resume check

Hi Curtis, Could you tell me who can help review my code?

: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.

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.

Remove RecordingError.webidl Fix some bugs.

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?

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

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

use nsTArray to replace ringBuffer.

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.

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.

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.

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.

Sync to mediaEncoder framework change. Todo: error handle, spec seems want to use the mediaError DOM object to pass the error information to UA.

Support query encoder;s MIMEtype

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".

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.

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.

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.

Remove timer for push blob to UA.

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

The first version of EncodedBufferContainer

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.

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.

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

Fix [patch] media recorder error event v1 problem

Media Recorder Event

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.)

For pause/resume operation, just throw the NS_ERROR_DOM_NOT_SUPPORTED_ERR, needs localmediastream object.

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.

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

Fix commit message

Change naming from EncodedBufferContainer to EncodedBufferCache

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.

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.

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.

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

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.

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.

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.

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.

Hi roc, I have uploaded new patch again. Thank for your review if you found anything wrong in this.

Update for using nsDOMTemporaryFileBlob

Add RecordErrorEvent.webidl, need by Bug 847611 - Paris bindings for autogenerated events

1. remove pause/resume function 2. renaming some task class 3. add more error handle

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.

Fix some nit, change naming.

Change ProcessBuffers to AppendBuffer.

avoid build warning for memory copy

Fix some nits.

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.

Comment on attachment 760767 [details] [diff] [review] RecordErrorEvent v2 Obsoletes this for new one provided.

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.

1. Move nsTemporaryFileInputStream to single file, 2. fix nits.

Fix nits, clean some unnecessary code.

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!

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.

Thanks roc. Fix nits. Also create Bug 888742 for implementing custom-blob.

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.

Fix nits, thanks for reviewing. :)

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.

Fix nits.

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?

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.

Ah yes OK.

Remove ~ExtractEncodedDataTask. Fix comment nits. :)