Closed Bug 1514158 Opened Last year Closed 2 months ago

Implement MediaRecorder's audioBitsPerSecond and videoBitsPerSecond attributes

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Regressed 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(10 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
Rank: 25
Priority: -- → P3
Blocks: 1556710
Assignee: nobody → apehrson
Status: NEW → ASSIGNED

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

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

No longer blocks: 1556710

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.

Flags: needinfo?(apehrson)

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.

Flags: needinfo?(apehrson)
Depends on: 1411152
Attachment #9069892 - Attachment description: Bug 1514158 - Update WPT expectations. r?jib → Bug 1514158 - Update WPT expectations and make tests spec compliant. r?jib, r?bryce!

Depends on D33762

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

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

Depends on D41585

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?
Flags: needinfo?(apehrson)

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

Flags: needinfo?(apehrson)
Attachment #9084750 - Attachment description: Bug 1514158 - Simplify exception asserts in test_mr_state_transition.html.r ?bryce → Bug 1514158 - Simplify exception asserts in test_mr_state_transition.html. r?bryce

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.

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

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.

Duplicate of this bug: 1518105
Pushed by pehrsons@gmail.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19503 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot
Regressions: 1522739
Regressions: 1590319

Documentation updates:

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

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

Flags: needinfo?(eshepherd)

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

Flags: needinfo?(eshepherd) → needinfo?(apehrson)

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.

Flags: needinfo?(apehrson)
You need to log in before you can comment on or make changes to this bug.