Closed Bug 1512175 Opened Last year Closed 2 months ago

MediaRecorder.mimeType returns empty string

Categories

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

63 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
geckoview64 --- wontfix
geckoview65 --- wontfix
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: zarema.khalilova, Assigned: pehrsons)

References

Details

(Keywords: dev-doc-complete)

Attachments

(11 files, 4 obsolete 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
47 bytes, text/x-phabricator-request
Details | Review
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`.
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.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Rank: 25
Priority: -- → P3
See Also: → 1501308
(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
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.
Assignee: nobody → apehrson
Blocks: 1518105
Status: NEW → ASSIGNED

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.

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.

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.

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.

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.

Depends on: 1523563

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.

Attachment #9039495 - Attachment description: Bug 1512175 - Remove MediaRecorder::Session::PushBlobRunnable. r?bryce → Bug 1014393 - Remove MediaRecorder::Session::PushBlobRunnable. r?bryce

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.

Attachment #9039495 - Attachment is obsolete: true

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.

Attachment #9039496 - Attachment is obsolete: true

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

Duplicate of this bug: 1574805

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

Attachment #9039491 - Attachment description: Bug 1512175 - Let MediaRecorder decide on the mime type for recording on constructing. r?bryce → Bug 1512175 - Extend the constrained MediaRecorder mime type when needed. r?jib

We're immediately after explicitly stopping the recording anyway.

Depends on D46464

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

Depends on D46465

Attachment #9039490 - Attachment is obsolete: true
Attachment #9039489 - Attachment is obsolete: true
Attachment #9093901 - Attachment description: Bug 1512175 - Trigger data extraction before dispatching the task to fire the "start" event. r?jib → Bug 1512175 - Ensure there is data available immediately after the "start" event. r?jib
Attachment #9093898 - Attachment description: Bug 1512175 - Implement MediaRecorder MIME type constraining to spec. r?jib!,smaug! → Bug 1512175 - Implement MediaRecorder MIME type constraining to spec. r?jib!,smaug!,bryce!

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

Keywords: dev-doc-needed
Pushed by pehrsons@gmail.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19505 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: 1586328
Regressions: 1586248
Regressions: 1590997
QA Whiteboard: [qa-71b-p2]
Regressions: 1594466

Documentation updates:

Submitted BCD PR 5133 for this change.

Let me know if more is required!

You need to log in before you can comment on or make changes to this bug.