968109 - [MediaRecorder] Support recording media stream from OfflineAudioContext

Status: RESOLVED FIXED

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

Description

[u459114]

Ringtone editing app need none real-time recording to trim a audio clip as ring tone.

One possible solution is to enable recording a media stream from offline audio context. Since we didn't handle offline mode in media recorder, buffer overflow may happen in this scenario.

Comment 1

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

It look like OfflineAudioContext can't create an offline media stream for media recorder via CreateMediaStreamDestination.. With this API, it can output pcm raw data to renderedBuffer through OfflineAudioCompletionEvent.

Comment 2

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

How about have a new Web Audio node called OggCompressor to do the transcode task and produce the opus in Ogg audio blob data?

Comment 3

[Paul Adenot (:padenot)]

Well, it would be rather nice to be able to use _some_ MediaStream related Web Platform features, with an OfflineAudioContext.

There does not seem to be something like an "isRealTime" flag on MediaStreams we could use to indicate that a particular MediaStream can be consumed as fast as possible, and therefore can be used with an OfflineAudioContext.

Adding nodes to the Web Audio spec does not seem to me the best way to solve this.

Comment 4

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

The media stream can be the source for media element, recording source, webaudio.
If the media stream can be non-realtime, would it break the world in API level?

Comment 5

[Paul Adenot (:padenot)]

Not really. We would only allow createMediaStreamSource with non-realtime stream (such as an offline audio context), and the resulting MediaStreamDestinationNode would provide a MediaStream that is also consumable in a non-realtime fashion.

Then again, we would have to change a bunch of spec.

Comment 6

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

Hi roc,
Is it possible to change the media stream behavior? ＭediaStream interface lookｓ like we prefer it to be real-time.

Comment 7

[u459114]

By using OfflineAudioContext, we can generate an AudioBuffer with PCM raw samples.
There is no way, on API level, to hand over that buffer to MediaRecorder for encoding.

*If* we have this MediaEncoder API
interface MediaEncoder {
    void encode(AudioBuffer);
    attribute EventHandler oncomplete;
};

client can get a encode Blob from oncomplete.

Comment 8

[u459114]

Here is my suggestion
1. Provide encoding function in navigator.mozMediaEncoder for B2G privilege app.
2. Convert AudioSegment into RawTrackData to feed TrackEncoder.
3. generate opus(in OGG) encode data by MediaEncoder pipeline and return encoded Blob via oncomplete callback

Comment 9

[u459114]

(In reply to C.J. Ku[:CJKu] from comment #8)
> Here is my suggestion
> 1. Provide encoding function in navigator.mozMediaEncoder for B2G privilege
> app.
> 2. Convert AudioSegment into RawTrackData to feed TrackEncoder.
type: AudioBuffer(from OfflineAudioContext)
> 3. generate opus(in OGG) encode data by MediaEncoder pipeline and return
> encoded Blob via oncomplete callback

Comment 10

[u459114]

another suggestion is to convert AudioBuffer into wav format at js side, save it as ringtone

Comment 11

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

Generate a ringtone wave files seems to be a short solution for this, but workable.
30sec wave file size would be 2.8M in 2ch, 48khz.

Comment 12

[Marvin Khoo [:Marvin_Khoo]]

Hi Guys,
As discussed with Sri (Media) PM, that the music trimmer has to be done in 1.4.

let's discuss and break down the work.

BR,
Marvin

Comment 13

[(no longer active)]

MediaStreams and OfflineAudioContexts do not mix.  MediaStream has all sort of real time assumptions built into it, which is the reason that it's not possible to connect them to OfflineAudioContexts in any way.  That was done quite deliberately.

What is the format of the audio clip that you need to trim?  If you have all of the original data in an AudioBuffer for example, then it should be possible to write it in the wave format without needing any additional APIs.

Comment 14

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #13)
> MediaStreams and OfflineAudioContexts do not mix.  MediaStream has all sort
> of real time assumptions built into it, which is the reason that it's not
> possible to connect them to OfflineAudioContexts in any way.  That was done
> quite deliberately.

I don't think that's true actually. I think we could quite easily implement a solution that allows you to call createMediaStreamDestination in an OfflineAudioContext, get an "offline MediaStream", and create a MediaRecorder for that.

However this would require spec work which would probably be quite controversial.

Perhaps a better approach would be to add a method to AudioContext, createRecorder(), which creates a MediaRecorder object consuming the destination node of the AudioContext, and allow this to work on both online and offline AudioContexts. I think this would be easy for us to implement. I can propose it to public-audio and public-media-capture.

Script could easily wrap an audioBuffer in a WAV file --- some of our test script already does that --- but it would be nice to do this properly.

Comment 15

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Perhaps a better approach would be to add a method to AudioContext,
> createRecorder(), which creates a MediaRecorder object consuming the
> destination node of the AudioContext, and allow this to work on both online
> and offline AudioContexts. I think this would be easy for us to implement. I
> can propose it to public-audio and public-media-capture.

Actually we'd probably be better off adding a MediaRecorder(AudioContext) constructor, so that any changes to the constructors (e.g. adding new options) would be easy to make in one spec.

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #13)
> > MediaStreams and OfflineAudioContexts do not mix.  MediaStream has all sort
> > of real time assumptions built into it, which is the reason that it's not
> > possible to connect them to OfflineAudioContexts in any way.  That was done
> > quite deliberately.
> 
> I don't think that's true actually. I think we could quite easily implement a
> solution that allows you to call createMediaStreamDestination in an
> OfflineAudioContext, get an "offline MediaStream", and create a MediaRecorder
> for that.
> 
> However this would require spec work which would probably be quite
> controversial.

Right, that's what I meant.  :-)  It's not an implementation problem in Gecko.

> Perhaps a better approach would be to add a method to AudioContext,
> createRecorder(), which creates a MediaRecorder object consuming the
> destination node of the AudioContext, and allow this to work on both online and
> offline AudioContexts. I think this would be easy for us to implement. I can
> propose it to public-audio and public-media-capture.

That (the ctor syntax in comment 15) sounds good.  We should make sure that it's implementable by other engines, as I know that at least Safari has weird setups for some of their media stream types.

> Script could easily wrap an audioBuffer in a WAV file --- some of our test
> script already does that --- but it would be nice to do this properly.

Yeah, well, and we need to encode to other formats anyways.

Comment 17

[Karl Tomlinson (:karlt)]

Perhaps MediaRecorder(AudioNode, optional output), so that output need not go to hardware in an online context.  This may also allow optimization for offline contexts because the buffer for OfflineAudioContext oncomplete can be null.

For an online AudioNode.context, this would be sugar for

dest = AudioNode.context.createMediaStreamDestination();
AudioNode.connect(dest, output);
MediaRecorder(dest.stream);

Comment 18

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

Add MediaRecorder(AudioContext) constructor also sounds good to me, too.

Comment 19

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

Free it if anyone wants.

Comment 20

[John Lin [:jhlin][:jolin]]

Attachment: Add constructor to support recording audio from OfflineAudioContext.

WIP implementation to support recording audio from OfflineAudioContext.
- new MediaRecorder constructor which takes Web Audio destination node as input source
- modify recorder object to hold native media stream object rather than DOM wrapper 
- rename AfterTracksAdded() to InitEncoder() to make it more clear what the function does.

Comment 21

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

Comment on attachment 8468261 [details] [diff] [review]
Add constructor to support recording audio from OfflineAudioContext.

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

LGTM. :)

Comment 22

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

Comment on attachment 8468261 [details] [diff] [review]
Add constructor to support recording audio from OfflineAudioContext.

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

Basically looks good! Glad it's so simple. Does it work? :-)

Someone needs to get this added to the spec. Anyone volunteering for that?

::: dom/webidl/MediaRecorder.webidl
@@ +12,5 @@
>  
>  enum RecordingState { "inactive", "recording", "paused" };
>  
> +[Constructor(MediaStream stream, optional MediaRecorderOptions options),
> + Constructor(AudioDestinationNode node, optional MediaRecorderOptions options)]

See Karl's comment. We should take an AudioNode here.

Comment 23

[John Lin [:jhlin][:jolin]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 8468261 [details] [diff] [review]
> Add constructor to support recording audio from OfflineAudioContext.
> 
> Review of attachment 8468261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Basically looks good! Glad it's so simple. Does it work? :-)

 Yes. I've run my test script on Firefox and FxOS. \o/

> 
> Someone needs to get this added to the spec. Anyone volunteering for that?

 Sure. Never done this before, where do I start? 

> 
> ::: dom/webidl/MediaRecorder.webidl
> @@ +12,5 @@
> >  
> >  enum RecordingState { "inactive", "recording", "paused" };
> >  
> > +[Constructor(MediaStream stream, optional MediaRecorderOptions options),
> > + Constructor(AudioDestinationNode node, optional MediaRecorderOptions options)]
> 
> See Karl's comment. We should take an AudioNode here.

 Okay.

Comment 24

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

(In reply to John Lin[:jolin][:jhlin] from comment #23)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> > Comment on attachment 8468261 [details] [diff] [review]
> > Add constructor to support recording audio from OfflineAudioContext.
> > 
> > Review of attachment 8468261 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Basically looks good! Glad it's so simple. Does it work? :-)
> 
>  Yes. I've run my test script on Firefox and FxOS. \o/
> 
> > 
> > Someone needs to get this added to the spec. Anyone volunteering for that?
> 
>  Sure. Never done this before, where do I start? 
You could write this idea in mail and post to this mailist
http://lists.w3.org/Archives/Public/public-media-capture/
> > 
> > ::: dom/webidl/MediaRecorder.webidl
> > @@ +12,5 @@
> > >  
> > >  enum RecordingState { "inactive", "recording", "paused" };
> > >  
> > > +[Constructor(MediaStream stream, optional MediaRecorderOptions options),
> > > + Constructor(AudioDestinationNode node, optional MediaRecorderOptions options)]
> > 
> > See Karl's comment. We should take an AudioNode here.
> 
>  Okay.

Comment 25

[John Lin [:jhlin][:jolin]]

(In reply to Karl Tomlinson (:karlt) from comment #17)
> Perhaps MediaRecorder(AudioNode, optional output), so that output need not
> go to hardware in an online context.  This may also allow optimization for
> offline contexts because the buffer for OfflineAudioContext oncomplete can
> be null.
> 
> For an online AudioNode.context, this would be sugar for
> 
> dest = AudioNode.context.createMediaStreamDestination();
> AudioNode.connect(dest, output);
> MediaRecorder(dest.stream);

Hi Karl,
 Just want to be sure that I understand it correctly:
 * |output| is an audio node
 * when supplied, the audio, besides being recorded by recorder, will also go to the its input
 * if undefined or null
   => the audio won't go to audio hardware (no sound) for online context
   => OfflineAudioCompletionEvent.renderedBuffer will be null for offline context
 Right?

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to John Lin[:jolin][:jhlin] from comment #25)
> (In reply to Karl Tomlinson (:karlt) from comment #17)
> > Perhaps MediaRecorder(AudioNode, optional output),

I mean MediaRecorder(AudioNode node, optional unsigned long output = 0).

Some AudioNodes have more than one output.  The |output| parameter specifies which output is connected to the MediaRecorder.  If |output| is not provided, then output number 0 from the AudioNode will be recorded.

Compare
http://webaudio.github.io/web-audio-api/#widl-AudioNode-connect-void-AudioNode-destination-unsigned-long-output-unsigned-long-input

If the AudioNode output is also connected to other AudioNodes or AudioParams then it will still go to those other objects, as when not connected to the MediaRecorder.

I'm not sure about passing undefined or null for an unsigned long parameter, but I guess the code generated from webidl will throw an exception.

>    => the audio won't go to audio hardware (no sound) for online context
>    => OfflineAudioCompletionEvent.renderedBuffer will be null for offline

Whether any sound goes to hardware (online) or renderedBuffer (offline) depends on whether the AudioNode is connected to the AudioContext::destination.  Creating a MediaRecorder does not affect this.

Comment 27

[John Lin [:jhlin][:jolin]]

(In reply to Karl Tomlinson (:karlt) from comment #26)
> (In reply to John Lin[:jolin][:jhlin] from comment #25)
> > (In reply to Karl Tomlinson (:karlt) from comment #17)
> > > Perhaps MediaRecorder(AudioNode, optional output),
> 
> I mean MediaRecorder(AudioNode node, optional unsigned long output = 0).
> 
> Some AudioNodes have more than one output.  The |output| parameter specifies
> which output is connected to the MediaRecorder.  If |output| is not
> provided, then output number 0 from the AudioNode will be recorded.
> 
> Compare
> http://webaudio.github.io/web-audio-api/#widl-AudioNode-connect-void-
> AudioNode-destination-unsigned-long-output-unsigned-long-input
> 
> If the AudioNode output is also connected to other AudioNodes or AudioParams
> then it will still go to those other objects, as when not connected to the
> MediaRecorder.
> 
> I'm not sure about passing undefined or null for an unsigned long parameter,
> but I guess the code generated from webidl will throw an exception.
> 
> >    => the audio won't go to audio hardware (no sound) for online context
> >    => OfflineAudioCompletionEvent.renderedBuffer will be null for offline
> 
> Whether any sound goes to hardware (online) or renderedBuffer (offline)
> depends on whether the AudioNode is connected to the
> AudioContext::destination.  Creating a MediaRecorder does not affect this.

Uh-oh, I COMPLETELY misunderstood it. :-$
Thanks a lot for the prompt response!

Comment 28

[John Lin [:jhlin][:jolin]]

Attachment: Add constructor to support recording audio from OfflineAudioContext.

Code change according to Karl's comment. It now can record from audio node other than destination.

Comment 29

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

Comment on attachment 8475040 [details] [diff] [review]
Add constructor to support recording audio from OfflineAudioContext.

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

Please send that email to public-media-capture!

Comment 30

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

Comment on attachment 8475040 [details] [diff] [review]
Add constructor to support recording audio from OfflineAudioContext.

LGTM, Thanks.
The MediaRecorder.webidl requires sr to review it.

Comment 31

[John Lin [:jhlin][:jolin]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 8475040 [details] [diff] [review]
> Add constructor to support recording audio from OfflineAudioContext.
> 
> Review of attachment 8475040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please send that email to public-media-capture!

Thanks!
The message was sent out yesterday. I guess it didn't show up yet because I'm a 1st-time poster. :)

Comment 32

[John Lin [:jhlin][:jolin]]

Attachment: Hide the newly added constructor behind pref.

Couldn't figure out how to hide just one constructor behind [Pref] in WebIDL. Throw TypeError in the implementation to achieve same effect.

Comment 33

[John Lin [:jhlin][:jolin]]

Attachment: Add tests for recording from audio nodes.

Some basic tests to check:
 - newly added constructor is hidden behind pref
 - recording works for various context and node types

Comment 34

[Alfredo Yang (:alfredo)]

Comment on attachment 8478884 [details] [diff] [review]
Hide the newly added constructor behind pref.

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

::: content/media/MediaRecorder.cpp
@@ +816,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aGlobal.GetAsSupports());
> +  if (!sgo) {

I can't find where it is used. Do we need to check it?

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478884 [details] [diff] [review]
Hide the newly added constructor behind pref.

>+/* static */ already_AddRefed<MediaRecorder>
>+MediaRecorder::Constructor(const GlobalObject& aGlobal,
>+                           AudioNode& aSrcAudioNode,
>+                           uint32_t aSrcOutput,
>+                           const MediaRecorderOptions& aInitDict,
>+                           ErrorResult& aRv)
>+{
>+  // Only enable recording from audio node when pref is on.
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+  bool enabled = false;
>+  if (prefs == nullptr ||
>+       NS_FAILED(prefs->GetBoolPref("media.recorder.audio_node.enabled", &enabled)) ||
>+       !enabled) {
>+    // Pretending that this constructor is not defined.
>+    NS_NAMED_LITERAL_STRING(argStr, "Argument 1 of MediaRecorder.constructor");
>+    NS_NAMED_LITERAL_STRING(typeStr, "MediaStream");
>+    aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &typeStr);
>+    return nullptr;
>+  }
Do we want a pref or permission check?
I think pref is probably fine here, but it does mean the API is exposed everywhere, in all the apps when the pref is on.

To check the pref value, please use Preferences::GetBool("media.recorder.audio_node.enabled", false)



>+
>+  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aGlobal.GetAsSupports());
>+  if (!sgo) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
You don't use sgo for anything, so drop this stuff

Comment 36

[John Lin [:jhlin][:jolin]]

Attachment: Add constructor to support recording audio from OfflineAudioContext.

- address review comment
- carry r+
- fix non-unified bustage

Comment 37

[John Lin [:jhlin][:jolin]]

Attachment: Add tests for recording from audio nodes.

- carry r+
- fix test failures caused by finishing test before all test cases are complete

Comment 38

[John Lin [:jhlin][:jolin]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=abce8856b8e7

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aba6bb5a51b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68f1bafdf74

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4aba6bb5a51b
https://hg.mozilla.org/mozilla-central/rev/a68f1bafdf74