1420877 - [Static Analysis][Big Parameter Passed as Value] VideoReceiveStream::Config should be passed as const T&

Status: RESOLVED INVALID

(Core :: WebRTC: Audio/Video, enhancement, P3)

Description

[Andi [:andi]]

Since the size of VideoSendStream::Config is roughly 320bytes I think we can pass it as const&.

Comment 1

[Andi [:andi]]

Attachment: Bug 1420877 - pass VideoReceiveStream::Config as const&.

Review commit: https://reviewboard.mozilla.org/r/203124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203124/

Comment 2

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8932064 [details]
Bug 1420877 - pass VideoReceiveStream::Config as const&.

https://reviewboard.mozilla.org/r/203124/#review208490

::: media/webrtc/signaling/gtest/MockCall.h:143
(Diff revision 1)
> -  VideoSendStream* CreateVideoSendStream(VideoSendStream::Config config,
> +  VideoSendStream* CreateVideoSendStream(const VideoSendStream::Config& config,
>                                           VideoEncoderConfig encoder_config) override {

It's overriding the upstream [1] though. Does this even compile?

[1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/media/webrtc/trunk/webrtc/call/call.h#112

Comment 3

[Andi [:andi]]

Yes you're right, my patch didn't contain the modified header. Do you think I should report this upstream?

Comment 4

[Andreas Pehrson [:pehrsons]]

This was actually fixed upstream 3 and a half years ago, see [1].

Hmm, Dan, how come we haven't picked this up? 57 is not that old, is it?


[1] https://chromium.googlesource.com/external/webrtc/+/6ae48c660934784b4df56ab1ac99402ce3745e9f%5E%21/webrtc/call.h

Comment 5

[Andreas Pehrson [:pehrsons]]

I'll answer myself. The upstream version of this code is at [1]. A couple of blames show [2] as the offending commit that reverted this. It added Move semantics for those config objects. And indeed, the callsites of these functions in chromium use std::move. But then why do they take the arg by-value and not as an RRef? What am I missing?

Filing something on that could make sense at least.

[1] https://chromium.googlesource.com/external/webrtc/+/52b6562a10b495cf771d8388ee51990d56074059/webrtc/call/call.h#113
[2] https://chromium.googlesource.com/external/webrtc/+/26091b1118e37b5cbbfe0de9869127fd5c1b01d0%5E%21/#F3

Comment 6

[Andi [:andi]]

Maybe they are taking it by value to permit the usage of RValue?

Comment 7

[Andi [:andi]]

This should be closed as it's no longer present in our code.