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&


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




Tracking Status
firefox59 --- affected


(Reporter: andi, Assigned: andi)


(Blocks 1 open bug, )


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


(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&.

::: 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?

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?

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.

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.

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