1512175 - MediaRecorder.mimeType returns empty string

Status: RESOLVED FIXED

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

Description

[zarema.khalilova]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.110 Safari/537.36

Steps to reproduce:

1. Start the stream from a webcam,
2. create an instance of MediaRecorder with that stream and without options,
3. start record,
4. stop record,
5. read mimeType of MediaRecorder

Demo: https://codepen.io/zmoki/pen/WYBWJd?editors=0011


Actual results:

mimeType by MediaRecorder returns an empty string instead of the real MIME type of recorded video.


Expected results:

Now, by default Firefox recorded video with WebM container and VP8 codec, so, MediaRecorder.mimeType in my case should return `video/webm` or `video/webm; codecs="vp8, opus`.

Comment 1

[Hani Yacoub, Desktop QA]

Reproducible on Firefox Nightly 65.0a1 (2018-12-09), Firefox 64.0 and Firefox 63.0.3 on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.13.

Comment 2

[Andreas Pehrson [:pehrsons]]

This happens because we reset the mimetype when stopping the recorder, [1].

The spec isn't exactly clear on when the mimetype gets set. I can totally see that the natural behavior is that we set the mime type in the recorder's ctor and all sessions started after that inherits that mimetype. Our current mimetype handling is a bit backwards.

[1] https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/dom/media/MediaRecorder.cpp#395

Comment 3

[homm86]

(In reply to Andreas Pehrson [:pehrsons] from comment #2)
> This happens because we reset the mimetype when stopping the recorder, [1].
> 
> The spec isn't exactly clear on when the mimetype gets set. I can totally
> see that the natural behavior is that we set the mime type in the recorder's
> ctor and all sessions started after that inherits that mimetype. Our current
> mimetype handling is a bit backwards.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> fd62b95c187a40b328d9e7fd9d848833a6942b57/dom/media/MediaRecorder.cpp#395

I've added mimetype logging on every step, it's still empty:
https://codepen.io/anon/pen/KbzdNN?editors=0011

Comment 4

[Andreas Pehrson [:pehrsons]]

You're right. It looks like our setting of the mime type is broken when no mimetype is passed in through MediaRecorderOptions.

More than one issue to fix here.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Add MediaRecorder MIMEType WPT. r?jib

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Set MediaRecorder MIMEType on construction. r?bryce

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Refactor MediaEncoder::GetEncodedMetadata. r?bryce

This makes GetEncodedMetadata set metadata on the writer in a single call.
It also removes the ctor argument from the WebMWriter since it can now rely on
the single SetMetadata() instead.
To comply with the ContainerWriter::SetMetadata() docs,
WebMWriter::SetMetadata() will now also sanity check metadata.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Extend the constrained MediaRecorder mime type when needed. r?jib

We used to set the mime type based on what tracks were fed to the recorder,
which was spec incompliant. Now we set the mime type immediately in the ctor if
it's defaulted or supported, or throw otherwise.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Remove test_mr_getencodeddata.html. r?jib

This test relied on us choosing to record audio/ogg because of the only audio
track, which would fail and result in an error with ogg support disabled.

We now per spec select mime type synchronously on construction instead of after
detecting the stream's tracks - and so fall back to video/webm when the mime
type is defaulted. With this the test times out because we're recording fine.

When disabling also webm encoding we follow the spec's "note that this case is
essentially equivalent to leaving up to the UA the choice of container and
codecs on constructor", and choose some unknown mime type that is not supported
(none are), thus ending up throwing NotSupportedError.

Not much remains of this test with these changes, and what actually remains is
covered by other tests, which warrants removal.

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Rename and adapt test_mr_unsupported_src.html. r?jib

With spec compliant mime type handling we no longer fire "error" on start for a
audio-only recording of a (currently) video-only MediaStream.

This patch adapts the test to more accurately name what we originally wanted to
test - that pause() after an error doesn't crash but throws. It does this by
triggering another kind of error, one that happens because we try to removed
the recorded track from the recorded stream. We still keep the behavior of
start()ing the recording before any supported tracks are available, because we
support that.

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Adapt mochitests to spec compliant mime type handling. r?jib

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1014393 - Remove MediaRecorder::Session::PushBlobRunnable. r?bryce

This moves the impl of PushBlobRunnable from a runnable to MozPromise, which
let's us more easily modularize it's parts (gather the blob, fire dataavailable)
to make individual code paths more explicit.

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Unify MediaRecorder session shutdown paths and fix event timing when stopping per spec. r?bryce

Comment 14

[Andreas Pehrson [:pehrsons]]

So this has hit a snag because while the spec is fairly clear (it's not straight forward, but you can figure out the intention of the author) Chrome is not following it. Since our old behavior is closer to Chrome's than the spec, we're now discussing updating the spec to follow closer to reality, and hopefully making it clearer in the process.

This work will block bug 1518105 as well. At least for now.

Comment 15

[Phabricator Automation]

Comment on attachment 9039495 [details]
Bug 1014393 - Remove MediaRecorder::Session::PushBlobRunnable. r?bryce

Revision D17813 was moved to bug 1014393. Setting attachment 9039495 [details] to obsolete.

Comment 16

[Phabricator Automation]

Comment on attachment 9039496 [details]
Bug 1512175 - Unify MediaRecorder session shutdown paths and fix event timing when stopping per spec. r?bryce

Revision D17814 was moved to bug 1014393. Setting attachment 9039496 [details] to obsolete.

Comment 17

[Andreas Pehrson [:pehrsons]]

This bug is currently blocked on the resolution of the spec issue at https://github.com/w3c/mediacapture-record/issues/170

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Implement MediaRecorder MIME type constraining to spec. r?jib!,smaug!,bryce!

This leaves out support for extending the mime type with the selected container
and codecs, and support in MediaEncoder for using a specific mime type.

Depends on D17806

Comment 20

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Expand on MediaRecorder DOM exception throwing with messages. r?jib

Depends on D17812

Comment 21

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Don't remove tracks from the MediaEncoder on stream-removal. r?jib

We're immediately after explicitly stopping the recording anyway.

Depends on D46464

Comment 22

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Ensure there is data available immediately after the "start" event. r?jib

This to ensure that when the "start" event is fired, there is already some data
in the blob.

Depends on D46465

Comment 23

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Expand mochitest for webm mime type support with various casing. r?jib

Comment 24

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512175 - Make test_mr_creation_fail.html spec compliant and test the error name. r?jib

Depends on D46466

Comment 25

[Andreas Pehrson [:pehrsons]]

This implements the mimeType handling of MediaRecorder per spec. Note that there were some recent spec changes to clarify the algorithms here.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b90cfb7ecee3
Expand mochitest for webm mime type support with various casing. r=jib
https://hg.mozilla.org/integration/autoland/rev/a76ec7f58d50
Add MediaRecorder MIMEType WPT. r=jib
https://hg.mozilla.org/integration/autoland/rev/c08c0b553f15
Implement MediaRecorder MIME type constraining to spec. r=jib,smaug,bryce
https://hg.mozilla.org/integration/autoland/rev/3bc62d21ad37
Extend the constrained MediaRecorder mime type when needed. r=jib
https://hg.mozilla.org/integration/autoland/rev/d3c6648507db
Remove test_mr_getencodeddata.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/dca395faee36
Rename and adapt test_mr_unsupported_src.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/85af0c8641ab
Adapt mochitests to spec compliant mime type handling. r=jib
https://hg.mozilla.org/integration/autoland/rev/b43a92f049c1
Expand on MediaRecorder DOM exception throwing with messages. r=jib
https://hg.mozilla.org/integration/autoland/rev/343e4bf9c1ed
Don't remove tracks from the MediaEncoder on stream-removal. r=jib
https://hg.mozilla.org/integration/autoland/rev/d65b19aacfbd
Ensure there is data available immediately after the "start" event. r=jib
https://hg.mozilla.org/integration/autoland/rev/8f67e6f39d7b
Make test_mr_creation_fail.html spec compliant and test the error name. r=jib

Comment 27

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

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

Comment 28

[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 29

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b90cfb7ecee3
https://hg.mozilla.org/mozilla-central/rev/a76ec7f58d50
https://hg.mozilla.org/mozilla-central/rev/c08c0b553f15
https://hg.mozilla.org/mozilla-central/rev/3bc62d21ad37
https://hg.mozilla.org/mozilla-central/rev/d3c6648507db
https://hg.mozilla.org/mozilla-central/rev/dca395faee36
https://hg.mozilla.org/mozilla-central/rev/85af0c8641ab
https://hg.mozilla.org/mozilla-central/rev/b43a92f049c1
https://hg.mozilla.org/mozilla-central/rev/343e4bf9c1ed
https://hg.mozilla.org/mozilla-central/rev/d65b19aacfbd
https://hg.mozilla.org/mozilla-central/rev/8f67e6f39d7b

Comment 30

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

Upstream PR merged by moz-wptsync-bot

Comment 31

[Eric Shepherd [:sheppy]]

Documentation updates:

• https://wiki.developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/mimeType has had some work done to clarify some points

Submitted BCD PR 5133 for this change.

Let me know if more is required!

Comment 32

[homm86]

How is it fixed if mediaRecorder.mimeType is still empty string on each stage of recording?

Demo is the same: https://codepen.io/zmoki/pen/WYBWJd?editors=0011

Comment 33

[homm86]

Quote from the documentation:

The mimeType read-only property returns the MIME media type that was specified when creating the MediaRecorder object, or, if none was specified, which was chosen by the browser.

mimeType should contain the type chosen by the browser, which isn't happening.

Comment 34

[Andreas Pehrson [:pehrsons]]

(In reply to homm86 from comment #33)

Quote from the documentation:

The mimeType read-only property returns the MIME media type that was specified when creating the MediaRecorder object, or, if none was specified, which was chosen by the browser.

mimeType should contain the type chosen by the browser, which isn't happening.

The spec says to set the mimeType attribute before firing "start" and set it to the empty string before firing "stop". You can consider it valid between those two events. Or look at the mimeType of one of the blobs that came out of the recorder.