Closed Bug 1420877 Opened 2 years ago Closed 1 year ago

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

Categories

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

enhancement

Tracking

()

RESOLVED INVALID
Tracking Status
firefox59 --- affected

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1423291, CID 1423288)

Attachments

(1 file)

Since the size of VideoSendStream::Config is roughly 320bytes I think we can pass it as const&.
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
Attachment #8932064 - Flags: review?(apehrson) → review-
Yes you're right, my patch didn't contain the modified header. Do you think I should report this upstream?
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
Flags: needinfo?(dminor)
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
Flags: needinfo?(dminor)
Maybe they are taking it by value to permit the usage of RValue?
Rank: 29
Priority: -- → P3

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

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.