[webkit-reviews] review granted: [Bug 234784] MediaRecorder should support the bitsPerSecond option : [Attachment 448295] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 4 21:20:35 PST 2022


Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 234784: MediaRecorder should support the bitsPerSecond option
https://bugs.webkit.org/show_bug.cgi?id=234784

Attachment 448295: Patch

https://bugs.webkit.org/attachment.cgi?id=448295&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 448295
  --> https://bugs.webkit.org/attachment.cgi?id=448295
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?


More information about the webkit-reviews mailing list