Bug 234784 - MediaRecorder should support the bitsPerSecond option
Summary: MediaRecorder should support the bitsPerSecond option
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Safari 15
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 85851
  Show dependency treegraph
 
Reported: 2022-01-01 10:10 PST by maxcodefaster
Modified: 2022-01-05 03:29 PST (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description maxcodefaster 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.
Comment 1 youenn fablet 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.
Comment 2 youenn fablet 2022-01-04 05:06:46 PST
Created attachment 448284 [details]
Patch
Comment 3 youenn fablet 2022-01-04 07:40:26 PST
Created attachment 448295 [details]
Patch
Comment 4 Darin Adler 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?
Comment 5 youenn fablet 2022-01-05 01:40:10 PST
Created attachment 448369 [details]
Patch for landing
Comment 6 EWS 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].
Comment 7 Radar WebKit Bug Importer 2022-01-05 03:29:21 PST
<rdar://problem/87138299>