1514158 - Implement MediaRecorder's audioBitsPerSecond and videoBitsPerSecond attributes

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

We already implement these attributes in the MediaRecorderOptions object, so they are passed in and exist in c++. We just need to hook them up and make sure they're spec compliant.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Remove onwarning from MediaRecorder. r?smaug

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Add audioBitsPerSecond and videoBitsPerSecond to MediaRecorder. r?jib!,smaug!

Depends on D33760

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Update WPT expectations and make tests spec compliant. r?jib,bryce!

Depends on D33761

Comment 4

[Andreas Pehrson [:pehrsons]]

For any onlookers, I'm holding this until the spec issue at [1] is resolved.

[1] https://github.com/w3c/mediacapture-record/issues/169

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:pehrsons, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 6

[Andreas Pehrson [:pehrsons]]

Note that this will also bring MediaRecorder up to spec in general for the Constructor and the start method. Mime type handling is not up to spec yet; that's bug 1512175; but the rest should be.

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Add bitsPerSecond WPT. r?bryce

Depends on D33762

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Bring MediaRecorder to spec on new bitrate handling. r?bryce

This brings the MediaRecorder constructor and start() to spec on most parts
except mime type handling (bug 1512175). The flow is also improved to better
follow along in the algorithms of the spec.

Depends on D41584

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Make VideoTrackEncoder's key frame interval uint32_t. r?bryce

This aligns it better with MediaRecorder's timeslice which was changed from
int32 to uint32 earlier.

Depends on D41585

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Test that MediaRecorder.start() throws if MediaRecorder.stream is inactive. r?bryce

Depends on D41586

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Fix MediaRecorder static-analysis warnings. r?bryce

Depends on D41587

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Bring MediaRecorder closer to spec prose for stop, pause, resume, requestData methods. r?bryce

Depends on D41588

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1514158 - Simplify exception asserts in test_mr_state_transition.html. r?bryce

Depends on D41589

Comment 14

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Looking for clarification given the spec changes. It looks like the UA has a lot of liberty to decide on the appropriate bits per second depending on the scenario.

• The UA could conceptually pick 0 bits/s for audio|video even if they are present in a stream and no options for rates were passed? This seems silly, so I figure we can ignore this, but I want to clarify that my reading is correct.
• If a stream with only audio or video is passed and only bitsPerSecond are passed as an option, it looks like the UA has quite a lot of wiggle room. For example, UA gets a stream with only 1 audio track, bitsPerSecond = 1000 and can then decide to set audioBitsPerSecond to ~1000 and videoBitsPerSecond to 0, or divide them in some other way. Is this correct?

Comment 15

[Andreas Pehrson [:pehrsons]]

(In reply to Bryce Seager van Dyk (:bryce) from comment #14)

Looking for clarification given the spec changes. It looks like the UA has a lot of liberty to decide on the appropriate bits per second depending on the scenario.

• The UA could conceptually pick 0 bits/s for audio|video even if they are present in a stream and no options for rates were passed? This seems silly, so I figure we can ignore this, but I want to clarify that my reading is correct.
• If a stream with only audio or video is passed and only bitsPerSecond are passed as an option, it looks like the UA has quite a lot of wiggle room. For example, UA gets a stream with only 1 audio track, bitsPerSecond = 1000 and can then decide to set audioBitsPerSecond to ~1000 and videoBitsPerSecond to 0, or divide them in some other way. Is this correct?

It picks at two different occasions with slightly different instructions:

• At construction and when inactivated: pick a "reasonable" bitrate for audio and video.
• At start(): pick a "reasonable" bitrate for audio and video given the current track set.

Yes, "reasonable" gives room to assign anything the UA likes. But IMO it should at least be possible to argue that the assigned value is indeed reasonable. Bitrate 0 for a certain type is not, if you're actually going to record that type, as that wouldn't result in an actual recording.

Also note that both cases contain a "such that the sum of videoBitsPerSecond and audioBitsPerSecond is close to the value of recorder’s [[ConstrainedBitsPerSecond]] slot" ([[ConstrainedBitsPerSecond]] is the bitsPerSecond that was passed in to the ctor). This makes it hard to make a case for choosing 0 for both audio and video for instance.

More detailed comments inlined below:

• The UA could conceptually pick 0 bits/s for audio|video even if they are present in a stream and no options for rates were passed? This seems silly, so I figure we can ignore this, but I want to clarify that my reading is correct.

Yes, silly, but nothing explicitly forbidding it.

• If a stream with only audio or video is passed and only bitsPerSecond are passed as an option, it looks like the UA has quite a lot of wiggle room. For example, UA gets a stream with only 1 audio track, bitsPerSecond = 1000 and can then decide to set audioBitsPerSecond to ~1000 and videoBitsPerSecond to 0, or divide them in some other way. Is this correct?

I assume you're talking about start() (as the only place the algorithm cares about tracks). The intention with the spec's "for recording all tracks in tracks" is that since tracks are now known, we can distribute the bits over all the tracks. In cases such as 1: (0xa, 1xv), 2: (3xa, 3xv), or 3: (19xa, 1xv) the results would be wildly different. In (1) we intended to allocate 0 bits for audio since there won't be any audio recorded, so reducing the video bits would be against the intention of application. This is again not called out explicitly to keep the spec from getting too complex. Also in (3) you'd want to allocate a larger share of bits for audio than in (2), since you'll be recording such a larger share of audio tracks.

I'll also comment on the new bitrate WPT -- since there are no strict requirements outlined by the algorithm I tried to write something with as reasonable numbers as possible, so that 1) old implementations fail it, and 2) gecko under the new spec passes it. When vendors implement the spec it's easy enough to relax the test should they need to to satisfy the way they've implemented the algorithm under the new spec.

Let me know if some parts are still unclear.

Comment 16

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Thanks for the clarifications.

(In reply to Andreas Pehrson [:pehrsons] from comment #15)

It picks at two different occasions with slightly different instructions:

• At construction and when inactivated: pick a "reasonable" bitrate for audio and video.
• At start(): pick a "reasonable" bitrate for audio and video given the current track set.

I think this is where part of my confusion stems from. The distinction here was throwing me -- it wasn't clear that the recorder shouldn't inspect the stream's tracks prior to starting. I'd read into the "reasonable" part as that that the recorder could perform inspection of the stream to make a judgement, and then that it should explicitly do so again when starting in case the tracks had changed. Rather than that it shouldn't do so before starting (because the tracks might change), and then only do so when starting.

I assume you're talking about start() (as the only place the algorithm cares about tracks). The intention with the spec's "for recording all tracks in tracks" is that since tracks are now known, we can distribute the bits over all the tracks. In cases such as 1: (0xa, 1xv), 2: (3xa, 3xv), or 3: (19xa, 1xv) the results would be wildly different. In (1) we intended to allocate 0 bits for audio since there won't be any audio recorded, so reducing the video bits would be against the intention of application. This is again not called out explicitly to keep the spec from getting too complex. Also in (3) you'd want to allocate a larger share of bits for audio than in (2), since you'll be recording such a larger share of audio tracks.

I think I've misunderstood some of the tests for case 1 above, as I read them as allocating > 0 bits for audio when there is none. I'll take another look shortly now that I have a clearer understanding, as I suspect I was dealing with cases where the recorder had not yet started.

Comment 17

[Andreas Pehrson [:pehrsons]]

This is ready to land, but I'm planning to hold it until 71 so that bug 1512175 has a chance to land in the same version.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d75b63d3a00
Remove onwarning from MediaRecorder. r=smaug
https://hg.mozilla.org/integration/autoland/rev/09d7666761b5
Add audioBitsPerSecond and videoBitsPerSecond to MediaRecorder. r=jib,smaug
https://hg.mozilla.org/integration/autoland/rev/c12cc9f6c937
Update WPT expectations and make tests spec compliant. r=jib,bryce
https://hg.mozilla.org/integration/autoland/rev/f4219cbc29e7
Add bitsPerSecond WPT. r=bryce,jib
https://hg.mozilla.org/integration/autoland/rev/67e59d2a26dc
Bring MediaRecorder to spec on new bitrate handling. r=bryce
https://hg.mozilla.org/integration/autoland/rev/6005f7761315
Make VideoTrackEncoder's key frame interval uint32_t. r=bryce
https://hg.mozilla.org/integration/autoland/rev/c60d1d4fe9f4
Test that MediaRecorder.start() throws if MediaRecorder.stream is inactive. r=bryce
https://hg.mozilla.org/integration/autoland/rev/afa7200292eb
Fix MediaRecorder static-analysis warnings. r=bryce
https://hg.mozilla.org/integration/autoland/rev/6ce6f0e0471b
Bring MediaRecorder closer to spec prose for stop, pause, resume, requestData methods. r=bryce
https://hg.mozilla.org/integration/autoland/rev/044cfbab987a
Simplify exception asserts in test_mr_state_transition.html. r=bryce

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19503 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1d75b63d3a00
https://hg.mozilla.org/mozilla-central/rev/09d7666761b5
https://hg.mozilla.org/mozilla-central/rev/c12cc9f6c937
https://hg.mozilla.org/mozilla-central/rev/f4219cbc29e7
https://hg.mozilla.org/mozilla-central/rev/67e59d2a26dc
https://hg.mozilla.org/mozilla-central/rev/6005f7761315
https://hg.mozilla.org/mozilla-central/rev/c60d1d4fe9f4
https://hg.mozilla.org/mozilla-central/rev/afa7200292eb
https://hg.mozilla.org/mozilla-central/rev/6ce6f0e0471b
https://hg.mozilla.org/mozilla-central/rev/044cfbab987a

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 24

[Eric Shepherd [:sheppy]]

Documentation updates:

• Minor touching up to the onwarning page.
• Updated https://wiki.developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/start to include some missing errors and other information.
• Submitted BCD PR 5133 which includes the needed changes.

That should do the job. LMK if more is needed or if you spot any errors!

Comment 25

[Andreas Pehrson [:pehrsons]]

FWIW we've unshipped onwarning. Not sure if the compat table supports removals or how this should be handled.

Comment 26

[Eric Shepherd [:sheppy]]

:pehrsons - do you have any idea when we stopped sending warning to MediaRecorder? That needs to be updated as well, and I've been going through code and so far haven't found where this happened. That should be the version number set as the "version_removed" for the warning event on MediaRecorder.

For now, I've set it to "71" but it should be updated to the real number when possible.

Comment 27

[Andreas Pehrson [:pehrsons]]

I did some archaeology and I can't find that we ever fired it. It looks like we just implemented the webidl for the handler and that was it.