-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mediarecorder: add dropdown for codec selection #1424
mediarecorder: add dropdown for codec selection #1424
Conversation
8bf78a6
to
bc75df4
Compare
@@ -32,7 +34,8 @@ recordButton.addEventListener('click', () => { | |||
|
|||
const playButton = document.querySelector('button#play'); | |||
playButton.addEventListener('click', () => { | |||
const superBuffer = new Blob(recordedBlobs, {type: 'video/webm'}); | |||
const mimeType = codecPreferences.options[codecPreferences.selectedIndex].value.split(';', 1)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred to use mediaRecorder.mimeType
here but it is
"video/x-matroska;codecs=avc1,opus"
when one selects H264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, and also l.75 won't work because MP4 container is not supported.
These two files should give an idea of what's exactly supported:
- https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/fast/mediarecorder/MediaRecorder-isTypeSupported-avc1.html?q=istypesupported
- https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/fast/mediarecorder/MediaRecorder-isTypeSupported.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also another note, IIRC the ondataavailable
event listener's event.data.type
should carry the actual mimeType
used in the recording (which might be slightly different from the one specified):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 75 is for Safari which presumably only supports MP4 -- see https://webkit.org/blog/11353/mediarecorder-api/
It doesn't work in Chrome but that just means it will be filtered.
e.data.type is the same as mediaRecorder.mimeType
for easier testing of issues like https://bugs.chromium.org/p/chromium/issues/detail?id=117681 and Safari support
bc75df4
to
3325e8e
Compare
@yellowdoge can you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM % comments left in the code.
(I'm not sure if I can approve PRs in this repo BTW).
@@ -32,7 +34,8 @@ recordButton.addEventListener('click', () => { | |||
|
|||
const playButton = document.querySelector('button#play'); | |||
playButton.addEventListener('click', () => { | |||
const superBuffer = new Blob(recordedBlobs, {type: 'video/webm'}); | |||
const mimeType = codecPreferences.options[codecPreferences.selectedIndex].value.split(';', 1)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, and also l.75 won't work because MP4 container is not supported.
These two files should give an idea of what's exactly supported:
- https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/fast/mediarecorder/MediaRecorder-isTypeSupported-avc1.html?q=istypesupported
- https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/fast/mediarecorder/MediaRecorder-isTypeSupported.html
@@ -32,7 +34,8 @@ recordButton.addEventListener('click', () => { | |||
|
|||
const playButton = document.querySelector('button#play'); | |||
playButton.addEventListener('click', () => { | |||
const superBuffer = new Blob(recordedBlobs, {type: 'video/webm'}); | |||
const mimeType = codecPreferences.options[codecPreferences.selectedIndex].value.split(';', 1)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also another note, IIRC the ondataavailable
event listener's event.data.type
should carry the actual mimeType
used in the recording (which might be slightly different from the one specified):
for easier testing of issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=117681
and Safari support