Closed Bug 1161276 Opened 9 years ago Closed 9 years ago

Bitrate Property In MediaRecorder API

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jamie, Assigned: mreavy, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36

Steps to reproduce:

In the Firefox browser, MediaRecorder uses VP8 as the video codec. The default Constant Bit Rate is 2500kbps. This causes video quality problems when capturing from HD resolution devices. It can not currently be altered.


Actual results:

n/a


Expected results:

The VP8 encoder encodes video by a constant bit rate 2500kbps and vorbis audio at 128kbps. The  "BitRate" property described in http://www.w3.org/TR/mediastream-recording MediaRecorder API should allow for adjusting these values, but it is not currently implemented.
Flags: needinfo?(mreavy)
Product: Firefox → Core
Component: Untriaged → Video/Audio: Recording
Hi Steven - I was talking with the reporter for this bug (Jamie), and this bug is blocking this project. He can't make his tool to record HQ video interviews available until this bug to add the bitrate property to MediaRecorder is fixed. Do you think someone from your team would have time to start this in the next two to three weeks?   If not, I can probably assign it someone on my team.  What do you think?   Thanks!
Flags: needinfo?(mreavy) → needinfo?(slee)
(In reply to Jamie King from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML,
> like Gecko) Chrome/42.0.2311.135 Safari/537.36
> 
> Steps to reproduce:
> 
> In the Firefox browser, MediaRecorder uses VP8 as the video codec. The
> default Constant Bit Rate is 2500kbps. This causes video quality problems
> when capturing from HD resolution devices. It can not currently be altered.
> 
> 
> Actual results:
> 
> n/a
> 
> 
> Expected results:
> 
> The VP8 encoder encodes video by a constant bit rate 2500kbps and vorbis
> audio at 128kbps. The  "BitRate" property described in
> http://www.w3.org/TR/mediastream-recording MediaRecorder API should allow
> for adjusting these values, but it is not currently implemented.

From the w3c draft, The NEW BitRate property seems wired because we have video/audio bitrate but It just says "bitrate type". Also BitRate property havn't defined in webidl section. 
We can create two new perferences to let user to adjust the video/audio encoding bit-rate for their requirement.

Hi Roc, how do you think?
Flags: needinfo?(roc)
I think applications should have control over this.

I think it would make sense to have a bytesPerSecond property on MediaRecorder (dynamically settable) which specifies a soft target bitrate. The browser could be responsible for deciding how to assign bits for audio vs video; I don't know if there are use-cases where applications need independent bitrate control of audio vs video, and I fear that applications might make bitrate assignments that don't make sense for particular content or as codecs change over time.

I wonder what Tim thinks.
Flags: needinfo?(roc) → needinfo?(tterribe)
Well, we could certainly come up with a sane default split between video and audio, but there are definite use-cases where the distinction matters, and the application might want control (e.g., where you know one but not the other is some particularly simple content, like slides or spoken voice only).

But no one thinks about media bitrates in bytes per second. It is always and everywhere bits per second with 1 kilobit == 1000 bits.
Flags: needinfo?(tterribe)
None of the rest of the platform measures anything in bits, but I guess it's OK to go with bits per second.

Maybe we should have all three of
-- audioBitsPerSecond
-- videoBitsPerSecond
-- bitsPerSecond
and if both audioBitsPerSecond and videoBitsPerSecond are specified (e.g. nonzero), bitsPerSecond is ignored.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hi Randy -- Is this a bug that you can officially take ?  If so, may I assign it to you?  If you don't have time now/soon to work on this, then I'll need to find someone on my team to take it because I'd like to get it landed (if we can) in the Fx41 cycle.  I am also happy to help talk with the spec folks to make sure that the spec matches what we implement.  Thanks!
Flags: needinfo?(globelinmoz)
Hi Maire,
Sorry I'm not Mozilla employee right now so that I could not have the full time to implement this feature. 
You could ask others to help this. 
Randy
Flags: needinfo?(globelinmoz)
Hi Randy, I didn't realize you were no longer full-time. I am looking for someone to take this bug this cycle (for Fx41).  If there is someone in particular who would be a good fit to own this bug, please let me know.   Steven -- If there's anyone on your team who could take this bug (with a target of landing it this cycle -- before end of June), please let me know; that would be ideal. Otherwise, I assign this to someone on my team next week.  I don't think it will take very long (probably a few days or less).  Thanks!
Flags: needinfo?(globelinmoz)
Attached patch part1-interface.patch (obsolete) — Splinter Review
Flags: needinfo?(slee)
Attached patch part2-pass-parameters.patch (obsolete) — Splinter Review
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #8)
> Hi Randy, I didn't realize you were no longer full-time. I am looking for
> someone to take this bug this cycle (for Fx41).  If there is someone in
> particular who would be a good fit to own this bug, please let me know.  
> Steven -- If there's anyone on your team who could take this bug (with a
> target of landing it this cycle -- before end of June), please let me know;
> that would be ideal. Otherwise, I assign this to someone on my team next
> week.  I don't think it will take very long (probably a few days or less). 
> Thanks!
Hi Maire,
Here is my WIP patches. For some reasons, I cannot work on this bug. Hope it could help.
Hi Maire, 
ah...I left Mozilla at the begin of this year... :)
Flags: needinfo?(globelinmoz)
Steven's patches look pretty good for handling our initial use case.  I'll clean them up, add test cases as needed and put them up for review.  (Thanks, Steven!!)

I'll also file a follow up bug to enable changing the bit rate on the fly.  

I'm making this a P3, but my intention is to get this up for review next week and land it as soon as we have the r+'s.
Assignee: nobody → mreavy
Rank: 31
Priority: -- → P3
Attachment #8616527 - Attachment is obsolete: true
Attachment #8661312 - Flags: review?(roc)
Attachment #8661312 - Flags: review?(bugs)
Attachment #8616528 - Attachment is obsolete: true
Attachment #8661315 - Flags: review?(roc)
Attachment #8661317 - Flags: review?(roc)
Hi Jamie, This try push will have builds that allow you to test this code and see if it works with your site:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2b96edbfa79

Let me know if it works for you or if you have any problems.  

FYI: To download a build, click on the green "B" for the build you want and then click on the "Build:" link in the lower left of the window.
Flags: needinfo?(jamie)
Why the "bitsPerSecond" property? It seems to function as basically an alias for "videoBitsPerSecond".
There's a follow up bug that I'll be filing to handle dynamic changes of bitrate and partially specified cases.  The initial use cases we want to support don't require dynamic changes.
Comment on attachment 8661312 [details] [diff] [review]
part 1 add bits-per-second interface to MediaRecorder

Has this been proposed to the spec? If not, please file a spec bug.
I don't see anything in 
https://github.com/w3c/mediacapture-record/issues
and based on that , r-. (Let's not expose APIs to the web without some spec work happening around it)
So, could you at least file a spec bug and hopefully get some positive feedback from other browser vendors, and then re-ask review.


And the current interface doesn't follow the spec, but spec will be changed, right? At least there is https://github.com/w3c/mediacapture-record/issues/19
Attachment #8661312 - Flags: review?(bugs) → review-
There's strong author feedback that bitrate control is essential.

I think we should file the spec bug and in the meantime land this behind a flag preventing it from reaching release. Olli, is that OK with you?
Flags: needinfo?(bugs)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> There's strong author feedback that bitrate control is essential.
I meant mostly that the API will look what is proposed in the patch, and not something else.
Especially given https://github.com/w3c/mediacapture-record/issues/19 which isn't in the spec yet.
Blink has different kind of ctor for MediaRecorder than we do.


> I think we should file the spec bug and in the meantime land this behind a
> flag preventing it from reaching release. Olli, is that OK with you?
Well, actually since our ctor is already against the current spec, and there are no flags for dictionary members that doesn't matter much.
Maire was about to file a spec bug.
So, I could say conditional r+, assuming there is a spec bug filed :)
Flags: needinfo?(bugs)
Attachment #8661312 - Flags: review- → review+
Comment on attachment 8661315 [details] [diff] [review]
part 2 - pass bitrate to track encoders for MediaRecorder

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

::: dom/media/MediaRecorder.cpp
@@ +1034,5 @@
> +  // Until dynamic changes are supported, I prefer to be safe and err
> +  // slightly high
> +  if (aInitDict.mBitsPerSecond.WasPassed() && !aInitDict.mVideoBitsPerSecond.WasPassed()) {
> +    mVideoBitsPerSecond = mBitsPerSecond;
> +  }

The intent of this code is not very clear.

Please explain why mBitsPerSecond is used this way, in particular, why it doesn't affect mAudioBitsPerSecond.

Actually I think it would be better to just pass all the parameters as-is to MediaEncoder and make it responsible for interpreting these values. The closer to the codec/container we make these decisions, the better, I think.

::: dom/media/encoder/VorbisTrackEncoder.cpp
@@ +61,2 @@
>  
> +  printf("quality %f \n", quality);

delete this printf
Attachment #8661315 - Flags: review?(roc) → review-
Blocks: 1205552
Spec bug for this has been filed: https://github.com/w3c/mediacapture-record/issues/20
Attachment #8661315 - Attachment is obsolete: true
Attachment #8662200 - Flags: review?(roc)
Updated try:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4dc188a34dd

Jamie -- Please use this try build ^^ for your testing since that's what I'm looking to land. Thanks!
Blocks: 1205865
Disables test_mediarecorder_bitrate on Android 2.3
Attachment #8661317 - Attachment is obsolete: true
Flags: needinfo?(mreavy)
Attachment #8662664 - Flags: review?(roc)
Comment on attachment 8662664 [details] [diff] [review]
part 3 - tests for bitrate property in MediaRecorder

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

::: dom/media/test/mochitest.ini
@@ +662,5 @@
>  [test_media_sniffer.html]
>  skip-if = (toolkit == 'android' && processor == 'x86') #x86 only bug 914439
>  [test_mediarecorder_avoid_recursion.html]
>  tags=msg
> +skip-if = android_version == '10' # See bug 1205865 

trailing whitespace
Attachment #8662664 - Flags: review?(roc) → review+
I did more investigation last night and found that if I replaced the source file that I'm encoding, Android 2.3 is happy.  (Android 2.3 seems to have trouble playing back "seek.webm", but likes "gizmo.mp4".) Further, I found that the B2G emulator was timing out on this test -- which doesn't surprise me.  The B2G emulator is too slow for many of our tests;  so I'm disabling the test on B2G emulator as we have done on other tests.  So I'm asking review for:
(1) replacing "seek.webm" with "gizmo.mp4"
(2) disabling this test on B2G
The rest of patch, Roc has already r+'d.  

FYI: I talked with Roc last night, and he's fine with disabling tests under these circumstances.
Attachment #8662664 - Attachment is obsolete: true
Attachment #8662944 - Flags: review?(jib)
Comment on attachment 8662944 [details] [diff] [review]
part 3 - tests for bitrate property in MediaRecorder

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

lgtm. Some nits (I looked at the whole patch before realizing this was a small interdiff request).

In the future I think we could clean up how we run these two tests in parallel to use promises, as I found the results[] business a bit hard to follow. Something like:

    Promise.all(test1(), test2())
    .then(([result1, result2]) => {
      /* compare result1 with result2 */
    }).catch(failure);

::: dom/media/test/mochitest.ini
@@ +663,5 @@
>  skip-if = (toolkit == 'android' && processor == 'x86') #x86 only bug 914439
>  [test_mediarecorder_avoid_recursion.html]
>  tags=msg
> +[test_mediarecorder_bitrate.html]
> +skip-if = toolkit == 'gonk' # B2G emulator is too slow to run this without timing out 

trailing whitespace

::: dom/media/test/test_mediarecorder_bitrate.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test MediaRecorder Record No Timeslice</title>

Update title

@@ +13,5 @@
> +var results = [];
> +
> +/**
> + * Starts a test on every media recorder file included to check that a
> + * stream derived from the file can be recorded with no time slice provided.

update comment
Attachment #8662944 - Flags: review?(jib) → review+
Carrying forward roc's r+.  After looking at the trys I've decided to disable on android 2.3 and b2g.  I believe I can fix the Android 2.3 problem, but want to go ahead and land it disabled, and fix android 2.3 on the dependent bug.
Attachment #8662944 - Attachment is obsolete: true
Attachment #8663075 - Flags: review+
Blocks: 1159171
I've documented this:

* https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder (added link to new constructor page, and also mimeType property page, which I missed the first time around)
* https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/MediaRecorder (constructor page)
* https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/mimeType (not strictly part of this work, but hey)

I've also added a note to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/43#Miscellaneous

A tech review would be great - thanks!

One thing I'd love to add to the Constructor page is information on what defaults are adopted if mimeType/bitrates are not specified in the options object. Can someone advise?
Default is 2.5Mbps for video. Audio is adaptive to the sample rate and number of channels.

Chris -- Is this the info you need?  ^^
Flags: needinfo?(cmills)
This is perfect, thanks Maire!
Flags: needinfo?(cmills)
Is there an issue/bug for supporting additional codecs? (e.g. h.264/aac, vp9, etc) I did not manage to find any relevant issues so far, but sorry if I'm just missing something.

I think currently "mimeType" entry in the options object for MediaRecorder constructor is not being used (not in effect) -- Firefox seems to select specific one depending on platforms (I saw someone in other forum stating that desktop firefox uses vp8 and android/firefoxos uses h.264).
You need to log in before you can comment on or make changes to this bug.