Add audioConstantBitRate flag to MediaRecorderOptions.

[ghost]

This pull request is to add the ability to specify that a constant bitrate audio file should be encoded, as detailed in #183

[ghost]

WRT IPR checks, this PR is a follow on from https://chromium-review.googlesource.com/c/chromium/src/+/1731907

[alvestrand]

Unfortunately the IPR check is the W3C IPR check, not the Google IPR check.
Is your company a W3C member?

[ghost]

No it isn't, sorry.

[ghost]

So what do I need to do WRT IPR?

[dontcallmedom]

hi @sizeak, can you get in touch with me at dom@w3.org? This will help explore the possibilities.

[dontcallmedom]

Thank you @sizeak for providing separately the proper IPR commitment - the IPR error can be safely ignored for this pull request at this time (I'll work in actually clearing it, but don't want to block this discussion on this)

[ghost]

Thanks for helping with the IPR stuff.

Does anyone have any thoughts on adding the flag?

[ghost]

Hi,

Sorry to keep pestering, but this is something causing us pain in live. How do we move this forward?

Thanks

[dontcallmedom]

@henbos @Pehrsons @jan-ivar any input on the substance of the proposed change?

[henbos]

It seems innocent to me, but I don't know about implementation details. An audio MediaStreamTrack is something playing live, do we always have enough samples and can we always transform it to a constant bitrate. @ivocreusen Can you comment on the idea of recording (through the MediaRecorder API) i.e. encoding at a constant bitrate? Context: general API about encoding a track, not specific to WebRTC.

[jan-ivar]

Can't this be set with mimeType already? E.g. "audio/webm; codecs=opus.cbr"

[ghost]

That was my suggestion to the Chrome team in the PR linked at the top of this thread, but the file owner said it was a hack.

Here is the relevant quote from the PR:

Hey, apologies, I'm gonna be a major wet blanket here. Simon, this isn't directed at you - you followed the advice of the Chrome folks.

Using mime type to encode CBR is hack. Codecs strings are formally specified and following the specification is the best way to keep UAs interoperable and reduces pain for the whole ecosystem. The MediaRecorder spec is already looser than I'd like on mime-types (e.g. codecs="h264") and its bad to open up more exceptions.

The best path forward is to to change the spec (strawman: add 'bool cbr;' to MediaRecorderOptions). This is also much more discoverable.

[jan-ivar]

My inclination is to say this is a supported feature, not a hack. Open to hear arguments to the contrary. @Pehrsons thoughts?

[jan-ivar]

To clarify: adding API surface to cover everything that can already be set through mimeType seems redundant.

[ghost]

I'll feed back the reaction here to the original Chrome PR and see if the developer that objected would like to weigh in, unless you would be willing to make the case for the mimeType suggestion on that PR yourself?

[jan-ivar]

@sizeak Feel free to point them here.

[chcunningham]

Hi group, the comment was from me. I maintain the media mime parsing code in Chrome and I work on the MediaCapabilities spec where mime/codec strings are a key input.

Both recording and playback operate on codecs in containers. Right now, when it comes to opus at least, we are unified in the the way its described.

To date, "opus.cbr" is not a valid codec string. All UAs that support opus playback implicitly support both cbr and vbr, but querying any playback API's (canPlayType(), MediaCapabilities, isTypeSupported()) with "opus.cbr" will get a return value that indicates non-support (because they don't recognize it).

The need to signal CBR support is specific to the domain of MediaRecorder, and not necessarily specific to opus.

Adding an attribute that is external to the codec string solves the issue and avoids the pitfalls above.

[chcunningham]

nit: maybe call it BitsPerSecond to match the other members

[ghost]

BitsPerSecond is a bit verbose, especially in the case of audioConstantBitsPerSecond, but you are right that it would be better to be consistent. Will update.

[jan-ivar]

To date, "opus.cbr" is not a valid codec string.

@chcunningham Thanks for clarifying. I was missing this important detail. In that case I agree mimeType is not sufficient as an API here.

[ghost]

Thanks for commenting @chcunningham, I think I'm in agreement too now. I hadn't considered this:

The need to signal CBR support is specific to the domain of MediaRecorder, and not necessarily specific to opus.

While I don't think there is an official spec for the Opus mimetype (although there may be something browser specific), I interpreted https://tools.ietf.org/html/rfc6381#page-8 as allowing "opus.cbr", but I hadn't stopped to consider that it would be meaningless specifying it in regards to anything other than an encoder, which does make it feel a bit hacky.

[Pehrsons]

I'm a bit torn about adding attributes this way because if one were to add support for all encoder capabilities we'd get a very large struct, without any apparent structure.

Unfortunately there's precedence through the BitsPerSecond attributes.

To be pragmatic and to set a precedence that hopefully avoids painting us too far into potential future corners, I suggest we change it to audioBitrateControl (or some such, bikeshedding on the name welcome), which can take the values cbr and vbr, at least. Do we need more?

Likewise for video since we're adding the spec language for this anyway.

I think this should use slots in the ctor algorithm like is done for the BitsPerSecond init dict attributes, because the start algorithm (where these attributes should be used to constrain the configuration of the recorder) doesn't have access to the init dict.

We should probably say something on what is allowed if the UA for some reason does not support a given mode.

[sizeak]

So would everybody be happy with this:

enum AudioBitrateMode {
  "cbr",
  "vbr"
};

dictionary MediaRecorderOptions {
  DOMString mimeType;
  unsigned long audioBitsPerSecond;
  unsigned long videoBitsPerSecond;
  unsigned long bitsPerSecond;
  AudioBitrateMode audioBitrateMode;
};

[Pehrsons]

I want to see

enum BitrateMode, and

BitrateMode audioBitrateMode;
BitrateMode videoBitrateMode;

Otherwise LGTM at a high level.

[chcunningham]

BitrateMode sounds good to me.

[jan-ivar]

Concept lgtm. Just need to flesh out some details.

[sizeak]

Are you happy with the changes @Pehrsons ?

[jan-ivar]

@simoncent he's going to be out for a while, but I don't think he would object. Will try to get this merged Thursday.

[sizeak]

Ah sorry, I didn't realise. That would be great, thanks!

[jan-ivar]

Ran out of time during the editor's meeting unfortunately. I'm ok with merging this. @henbos @alvestrand do you see any reasons we shouldn't?

[youennf]

I am fine with CBR for audio, I am less sure about video.
Can we split the PR for now?

[henbos]

Does this make sense for video?

[alvestrand]

I think CBR is fairly widespread for audio, but more rare wrt video (most encoders require heavy dynamic tuning to produce CBR). I'm happy with merging the audio CBR value now, and revisitng the question of whether we want video CBR later.

[jan-ivar]

will split out video for now, fix order bug, and merge.

[sizeak]

Fixes #183