WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
234784
MediaRecorder should support the bitsPerSecond option
https://bugs.webkit.org/show_bug.cgi?id=234784
Summary
MediaRecorder should support the bitsPerSecond option
maxcodefaster
Reported
2022-01-01 10:10:18 PST
Referencing
https://bugs.webkit.org/show_bug.cgi?id=85851#c92
audioBitesPerSecond, videoBitsPerSecond, bitsPerSecond are not respected by WebKit. This results in huge blobs produced by MediaRecorder, which can not be uploaded.
Attachments
Patch
(13.45 KB, patch)
2022-01-04 05:06 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(16.20 KB, patch)
2022-01-04 07:40 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(16.19 KB, patch)
2022-01-05 01:40 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2022-01-04 04:56:33 PST
(In reply to maxcodefaster from
comment #0
)
> Referencing
https://bugs.webkit.org/show_bug.cgi?id=85851#c92
> > audioBitesPerSecond, videoBitsPerSecond, bitsPerSecond are not respected by > WebKit. This results in huge blobs produced by MediaRecorder, which can not > be uploaded.
audioBitsPerSecond and videoBitsPerSecond should be respected by WebKit. bitsPerSecond is not supported yet though.
youenn fablet
Comment 2
2022-01-04 05:06:46 PST
Created
attachment 448284
[details]
Patch
youenn fablet
Comment 3
2022-01-04 07:40:26 PST
Created
attachment 448295
[details]
Patch
Darin Adler
Comment 4
2022-01-04 21:20:35 PST
Comment on
attachment 448295
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=448295&action=review
> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:408 > + auto rates = MediaRecorderPrivate::computeBitRates(m_options, updateWithStream == UpdateWithStream::Yes ? &m_stream->privateStream() : nullptr); > + m_audioBitsPerSecond = rates.first; > + m_videoBitsPerSecond = rates.second;
Could do it like this: std::tie(m_audioBitsPerSecond, m_videoBitsPerSecond) = MediaRecorderPrivate::computeBitRates(m_options, privateStream);
> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:122 > + enum class UpdateWithStream { No, Yes }; > + void updateBitRates(UpdateWithStream);
In a case like this, consider two separate named functions. They can call a internal function that takes a stream pointer. Could even have one take a stream pointer that defaults to null if you want since this is all private. This gives us the flexibility to come up with two clean names rather than having to name both a function and an enumeration.
> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:109 > + bool hasAudio = stream ? stream->hasAudio() : true; > + bool hasVideo = stream ? stream->hasVideo() : true;
I think I personally might find the Boolean logic easier to read than the ? : bool hasAudio = !stream || stream->hasAudio(); Not sure which you think is clearer.
> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:113 > + auto audioBitsPerSecond = std::min(LargeAudioBitRate, std::max(SmallAudioBitRate, totalBitsPerSecond / 10));
Extra space here before the "std::min". The "/ 10" here seems like a magic number. I don’t understand why it’s there!
> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:125 > + return std::make_pair(options.audioBitsPerSecond.value_or(LargeAudioBitRate), options.videoBitsPerSecond.value_or(LargeVideoBitRate));
Consider the { } syntax instead of std::make_pair.
> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:76 > + static std::pair<unsigned, unsigned> computeBitRates(const MediaRecorderPrivateOptions&, const MediaStreamPrivate* = nullptr);
Do you think it’s obvious what the two halves of the pair are?
youenn fablet
Comment 5
2022-01-05 01:40:10 PST
Created
attachment 448369
[details]
Patch for landing
EWS
Comment 6
2022-01-05 03:28:00 PST
Committed
r287613
(
245740@main
): <
https://commits.webkit.org/245740@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 448369
[details]
.
Radar WebKit Bug Importer
Comment 7
2022-01-05 03:29:21 PST
<
rdar://problem/87138299
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug