1594466 - MediaRecorder crashes with a space in the codecs parameter, e.g., 'video/webm;codecs="vp8, opus"'

Status: RESOLVED FIXED

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

Description

[Cam]

Attachment: index.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

1. load attached file in firefox
2. allow webcam access
3. click the start recording button
4. wait a little bit
5. click the stop recording button
6. decide if you want to download the recording for further investigation
7. click close button to hide playback dialog
8. repeat steps 3-7

Actual results:

the first recording is a valid webm video, the second one is not

I can make the second recording work by copying the first blob from the first recording over the first blob from the second recording.

I also tried reusing the same mediarecorder object, and just starting/stopping it, but that had the same result.

Also, if I choose "video/webm;codecs=vp8" as the mime type, firefox says " NotSupportedError: An audio track cannot be recorded: video/webm;codecs=vp8 indicates an unsupported codec" and if I use 'video/webm;codecs="vp8, opus"' as the mime type firefox crashes the tab.

Expected results:

both recordings should be valid videos

Comment 1

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

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Andreas Pehrson [:pehrsons]]

[Tracking Requested - why for this release]:
71 regression.

(In reply to Cam from comment #0)

Created attachment 9106926 [details]
index.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

1. load attached file in firefox
2. allow webcam access
3. click the start recording button
4. wait a little bit
5. click the stop recording button
6. decide if you want to download the recording for further investigation
7. click close button to hide playback dialog
8. repeat steps 3-7

Actual results:

the first recording is a valid webm video, the second one is not

I can make the second recording work by copying the first blob from the first recording over the first blob from the second recording.

It sounds like the second recording is skipping over the webm header. That's a bug.

I also tried reusing the same mediarecorder object, and just starting/stopping it, but that had the same result.

Also, if I choose "video/webm;codecs=vp8" as the mime type, firefox says " NotSupportedError: An audio track cannot be recorded: video/webm;codecs=vp8 indicates an unsupported codec"

That's per spec. VP8 cannot record an audio track.

and if I use 'video/webm;codecs="vp8, opus"' as the mime type firefox crashes the tab.

Ouch. Does 'video/webm;codecs="vp8,opus"' work?

Expected results:

both recordings should be valid videos

Bug 1512175 is the regressor for the mimeType crash. The regressor of the second recording being corrupt is unclear, but likely landed around the same time.

Comment 3

[Cam]

Re the vp8 codec and audio error message, I totally believe I'm making a mistake, but just to be sure I understand you, recording video as video/webm;codecs=vp8 is invalid, and you must specify a specific audio codec too?

In a test just now, video/webm;codecs="vp8,opus" does work (at least once anyway), and doesn't crash.

It sounds like the second recording is skipping over the webm header.

In case it matters, I did some investigating of the contents of the blobs, and the first blob in both recordings is much smaller than the others (313 bytes vs ~330895 bytes), I expect it's the webm header but am not an expert in the format so I'm not sure what to check.

After writing the content of the first blobs from each recording to the console using event.data.arrayBuffer().then(e=> new Uint8Array(e)).then(e=>e.join(", ")).then(console.log) the contents of the two first blobs only differ by 8 bytes (4 consecutive bytes are different starting at offset 180 bytes, and again starting at offset 233 bytes).

recording 1:

26, 69, 223, 163, 1, 0, 0, 0, 0, 0, 0, 31, 66, 134, 129, 1, 66, 247, 129, 1, 66, 242, 129, 4, 66, 243, 129, 8, 66, 130, 132, 119, 101, 98, 109, 66, 135, 129, 2, 66, 133, 129, 2, 24, 83, 128, 103, 1, 255, 255, 255, 255, 255, 255, 255, 17, 77, 155, 116, 1, 0, 0, 0, 0, 0, 0, 0, 21, 73, 169, 102, 1, 0, 0, 0, 0, 0, 0, 73, 42, 215, 177, 131, 15, 66, 64, 68, 137, 136, 0, 0, 0, 0, 0, 0, 0, 0, 77, 128, 152, 81, 84, 109, 117, 120, 105, 110, 103, 65, 112, 112, 76, 105, 98, 87, 101, 98, 77, 45, 48, 46, 48, 46, 49, 87, 65, 153, 81, 84, 119, 114, 105, 116, 105, 110, 103, 65, 112, 112, 76, 105, 98, 87, 101, 98, 77, 45, 48, 46, 48, 46, 49, 22, 84, 174, 107, 1, 0, 0, 0, 0, 0, 0, 149, 174, 1, 0, 0, 0, 0, 0, 0, 44, 215, 129, 1, 115, 197, 132, 55, 46, 140, 231, 37, 134, 136, 131, 86, 80, 56, 131, 129, 1, 134, 133, 86, 95, 86, 80, 56, 224, 1, 0, 0, 0, 0, 0, 0, 8, 176, 130, 2, 128, 186, 130, 1, 224, 174, 1, 0, 0, 0, 0, 0, 0, 87, 215, 129, 2, 115, 197, 132, 161, 122, 216, 176, 131, 129, 2, 86, 170, 132, 0, 99, 46, 160, 86, 187, 132, 4, 196, 180, 0, 134, 134, 65, 95, 79, 80, 85, 83, 99, 162, 147, 79, 112, 117, 115, 72, 101, 97, 100, 1, 1, 56, 1, 128, 187, 0, 0, 0, 0, 0, 37, 134, 136, 132, 79, 80, 85, 83, 225, 1, 0, 0, 0, 0, 0, 0, 13, 181, 136, 64, 231, 112, 0, 0, 0, 0, 0, 159, 129, 1

recording 2:

26, 69, 223, 163, 1, 0, 0, 0, 0, 0, 0, 31, 66, 134, 129, 1, 66, 247, 129, 1, 66, 242, 129, 4, 66, 243, 129, 8, 66, 130, 132, 119, 101, 98, 109, 66, 135, 129, 2, 66, 133, 129, 2, 24, 83, 128, 103, 1, 255, 255, 255, 255, 255, 255, 255, 17, 77, 155, 116, 1, 0, 0, 0, 0, 0, 0, 0, 21, 73, 169, 102, 1, 0, 0, 0, 0, 0, 0, 73, 42, 215, 177, 131, 15, 66, 64, 68, 137, 136, 0, 0, 0, 0, 0, 0, 0, 0, 77, 128, 152, 81, 84, 109, 117, 120, 105, 110, 103, 65, 112, 112, 76, 105, 98, 87, 101, 98, 77, 45, 48, 46, 48, 46, 49, 87, 65, 153, 81, 84, 119, 114, 105, 116, 105, 110, 103, 65, 112, 112, 76, 105, 98, 87, 101, 98, 77, 45, 48, 46, 48, 46, 49, 22, 84, 174, 107, 1, 0, 0, 0, 0, 0, 0, 149, 174, 1, 0, 0, 0, 0, 0, 0, 44, 215, 129, 1, 115, 197, 132, 118, 230, 186, 114, 37, 134, 136, 131, 86, 80, 56, 131, 129, 1, 134, 133, 86, 95, 86, 80, 56, 224, 1, 0, 0, 0, 0, 0, 0, 8, 176, 130, 2, 128, 186, 130, 1, 224, 174, 1, 0, 0, 0, 0, 0, 0, 87, 215, 129, 2, 115, 197, 132, 224, 151, 55, 207, 131, 129, 2, 86, 170, 132, 0, 99, 46, 160, 86, 187, 132, 4, 196, 180, 0, 134, 134, 65, 95, 79, 80, 85, 83, 99, 162, 147, 79, 112, 117, 115, 72, 101, 97, 100, 1, 1, 56, 1, 128, 187, 0, 0, 0, 0, 0, 37, 134, 136, 132, 79, 80, 85, 83, 225, 1, 0, 0, 0, 0, 0, 0, 13, 181, 136, 64, 231, 112, 0, 0, 0, 0, 0, 159, 129, 1

Comment 4

[Andreas Pehrson [:pehrsons]]

(In reply to Cam from comment #3)

Re the vp8 codec and audio error message, I totally believe I'm making a mistake, but just to be sure I understand you, recording video as video/webm;codecs=vp8 is invalid, and you must specify a specific audio codec too?

So the spec says:

8 If the [[ConstrainedMimeType]] slot specifies a media type, container, or codec, then run the following sub steps:

1. Constrain the configuration of recorder to the media type, container, and codec specified in the [[ConstrainedMimeType]] slot.
2. For each track in tracks, if the User Agent cannot record the track using the current configuration, then throw a NotSupportedError DOMException and abort all steps.

Breaking it down:
8: Yes, it specifies media type (video), container (webm) and codec (vp8)
8.1: Ok, the media recorder is constrained to record video/webm with vp8 only
8.2: One of the track in tracks is an audio track, which cannot be recorded by the vp8 codec. Throw NotSupportedError.

If you leave out the codecs parameter (i.e., only passing in video/webm) we'll pick the codecs for you based on the tracks that were passed in.

Comment 5

[Cam]

Thanks!

Comment 6

[Andreas Pehrson [:pehrsons]]

I wrote a fiddle based on your code: https://jsfiddle.net/pehrsons/3sL10an4/1/
I added some log statements to it though. The log is for performing steps 1-5 in your STR, once. Can you spot what's wrong?

Started
Got blob of size 313
Got blob of size 181006
Got blob of size 297063
Got blob of size 296402
Grabbing recording. Size 774784
Got blob of size 88139
Stopped

If you at this point click Start recording, the first blob of the second recording ends up being the second blob in the recordedBlobs array. Obviously that cannot be decoded.

I modified the fiddle so it works as intended here: https://jsfiddle.net/pehrsons/3sL10an4/2/.

For further info about how to build your app, please refer to the spec: https://w3c.github.io/mediacapture-record/MediaRecorder.html

---

I also modified the fiddle to crash on video/webm;codecs="vp8, opus": https://jsfiddle.net/pehrsons/3sL10an4/3/

Changing this bug to be about that instead.

Comment 7

[Andreas Pehrson [:pehrsons]]

I have a pernosco recording of the crash at https://pernos.co/debug/KRyubPt26t_CUunOVDMY_A/index.html.

Well, in debug it's an assertion failure. On non-debug it seemed to be some sort of inf-loop/inf-recursion that later crashed (stack-exhaustion?), at some point after the assertion I bet. I should perhaps make these asserts diagnostic.

Comment 8

[Cam]

It seems that the docs surrounding the media recorder API are somewhat misleading. For example vp8 on its own is used here: https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/isTypeSupported#Example which is technically fine since it's just checking for support of that codec, but you then can't use that mimetype to actually record if you happen to record audio (a pretty common case I expect).

It's not technically wrong, but it is misleading IMO.

Comment 9

[Andreas Pehrson [:pehrsons]]

Well, that's not wrong. Recent spec changes are in part the reason for this too, and mdn hasn't been updated with them in mind yet.

However the way I see it is: as a developer I either

• don't care much about what codecs to use, so I don't specify them and the user agent makes an informed decision about them for me, which universally works; or
• do care about what codecs to use, so I read up on the spec to figure out how the API works, and check implementations for what's supported, so there are no surprises.

If I'm just grabbing mime type strings off (somewhat) unrelated examples on mdn I probably belong in the former group, so why specify a mime type at all?

Or if I wanted to grab the mime type off a more sensible mdn example I'd do it from https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/MediaRecorder, which is the API in use here.

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1594466 - Add crashtest. r?bryce

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1594466 - Don't rewrite mimeType if fully defined. r?bryce

Prior to this patch the mimetype was rewritten per the printf format
"%s; codecs=%s" also when codecs were defined in the constrained mime type.

The latter '%s' would be the codecs string from the mime type parser, which
would have dropped any quotation marks surrounding the string.
Hence 'codecs="vp8, opus"' would be considered supported (quotation marks
included), but when selecting mime type in start(), it would be rewritten
with quotation marks dropped. Thus looking like 'codecs=vp8,' which is not
supported.

This patch removes the rewrite step when the mime type is fully defined with
codecs, so that the quotation marks are left in place as given to the
constructor.

Depends on D52519

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a1b309ca7a1
Add crashtest. r=bryce
https://hg.mozilla.org/integration/autoland/rev/9e3f44e87a1a
Don't rewrite mimeType if fully defined. r=bryce

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/1a1b309ca7a1
https://hg.mozilla.org/mozilla-central/rev/9e3f44e87a1a

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9107800 [details]
Bug 1594466 - Don't rewrite mimeType if fully defined. r?bryce

Beta/Release Uplift Approval Request

• User impact if declined: Crashes inadvertently or at the will of an adversary.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Code now identifies the potentially crashing case and fully avoids doing a rewrite (which previously could cause a crash), to mitigate risk. No threading, ownership or lifetime changes.
• String changes made/needed:

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9107800 [details]
Bug 1594466 - Don't rewrite mimeType if fully defined. r?bryce

Crash fix, covered by tests, uplift approved for 71 beta 10, thanks.

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4428ee2cbd46
https://hg.mozilla.org/releases/mozilla-beta/rev/02d59bed4770