Closed Bug 1404997 Opened 2 years ago Closed 2 years ago

Move Opus decoding/NetEQ out of the MSG thread, it’s too expensive

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: padenot, Assigned: jya)

References

(Blocks 3 open bugs)

Details

Attachments

(27 files, 3 obsolete files)

769.91 KB, image/png
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
drno
: review+
Details
59 bytes, text/x-review-board-request
drno
: review+
jya
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
This accounts for about 3 times all the rest of the MSG work combined. We should use lock-free queues here.
Rank: 7
Priority: P2 → P1
This is really critical to fix.
So... it's important to get a clear periodic 'pull' signal to NetEq here (and as clean as possible, jitter-wise).  It bases the jitter buffer depth/etc decisions based on the incoming pulls versus jitter buffer state.  If we need data and it's not available due to missing packets, it has to do concealment at that point.

To avoid overhead in MSG pulls being delayed, you have to pre-decode and buffer (and ensure the buffer always (or almost always)) has enough data to satisfy a MSG pull.  The lock-free aspect is just an optimization here; if MSG needs data, and it's not in the buffer, we have to "make" the data -- and 0's won't cut it; we need real data or fake (concealment) data.

You can try to use signal or timer-based schemes to refill the buffer asynchronously from another thread (read N bytes of data, signal filler-thread, return to OS; filler-thread looks at buffer and starts decoding enough to satisfy the next MSG pull and hopefully finishes before MSG comes back), with (rare) blocking if you blow it and the data isn't available in time.  In most cases the data would be decoded before MSG comes for it.  However, the decoding now is off the high priority thread and so is actually *more* at risk of delays (at least on linux and mac; perhaps this can be side-stepped on windows). 

This will remove or mostly remove the overhead from the OS fetch; but it also will add circa 10ms to audio delays; maybe more.  Definitely not without cost!  And complex...
@padenot: which versions are affected? I'm guessing all versions up to release, but can you please set the flags?

And while I get that this might be a top priority to improve audio quality: is this really a P1 bug equivalent to a top crasher or sec bug?
Tech debt does not qualify as P1 under the new priority schema. Adjusting to top-rank P2.
Rank: 7 → 10
Priority: P1 → P2
Assignee: nobody → jyavenard
NI myself so I don't forget we want profiles from pre-57 update, and compare it with the profile above.
Flags: needinfo?(padenot)
so -- check encoding, and if there's a threading change that's landed - DirectListeners for example meant that audio got pushed into PeerConnections on the separate thread we spun up to insert into MSG.  If pehrsons turned them off, this can mean that audio gets pushed out of MSG on the callback, and into PeerConnections, increasing the overhead there.  That overhead may have changed in 57 as well, but a profile will help tell.

Also, I don't think we've cleanly regression-ranged this issue to the v57 update; just that "something has changed since this spring".

Any changes to NetEq should be coordinated with the upstream maintainer - we do NOT want to make fundamental changes here if upstream won't take them.  This means we should make the changes *to* the upstream code, and run them locally until we import a version from upstream with the changes.
Just to make things clearer, the profile screenshot in the attachment was made before pehrsons removed direct listener.

I'll re-do the profiles on a current build for two reasons:
- There has been some changes to the code since I profiled
- I want to be able to reliably compare profiles (same machines, same OS versions, same appear.in code running, etc.)
(In reply to Randell Jesup [:jesup] from comment #6)
> so -- check encoding, and if there's a threading change that's landed -
> DirectListeners for example meant that audio got pushed into PeerConnections
> on the separate thread we spun up to insert into MSG.  If pehrsons turned
> them off, this can mean that audio gets pushed out of MSG on the callback,
> and into PeerConnections, increasing the overhead there.  That overhead may
> have changed in 57 as well, but a profile will help tell.

There's an AudioProxyThread for pushing into PeerConnections so that overhead should be small.

With direct listeners and a microphone source, it was also the MSG thread pushing audio to peer connections, just not on every iteration. Well, at least with full duplex (which is the condition they are turned off under anyway).


(In reply to Paul Adenot (:padenot) from comment #7)
> Just to make things clearer, the profile screenshot in the attachment was
> made before pehrsons removed direct listener.

Really? I removed them from peer connections in 53, bug 1319445. That was in january.
> Really? I removed them from peer connections in 53, bug 1319445. That was in january.

I'm must have been really confused. Your recent work was on MediaRecorder, which was the last user, and then you removed the code, right ?
Flags: needinfo?(padenot)
I removed DirectMediaStreamListener yeah, but there's still DirectMediaStreamTrackListener :-)

So if we want direct audio back we can, we just turn off full duplex (or modify the code that reads the full duplex pref in MediaPipeline) and a track listener will be used.
Flags: needinfo?(ajones)
Depends on: 1331696
Depends on: 1423867
Comment on attachment 8935735 [details]
Bug 1404997 - P1. clang-format MediaPipeline.{cpp,h}.

https://reviewboard.mozilla.org/r/206632/#review212290


C/C++ static analysis found 14 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:625
(Diff revision 2)
>                               nsCOMPtr<nsIEventTarget> main_thread,
>                               nsCOMPtr<nsIEventTarget> sts_thread,
>                               RefPtr<MediaSessionConduit> conduit)
> -  : direction_(direction),
> -    level_(0),
> -    conduit_(conduit),
> +  : direction_(direction)
> +  , level_(0)
> +  , conduit_(conduit)

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , conduit_(conduit)
             ^
             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:628
(Diff revision 2)
> -  : direction_(direction),
> -    level_(0),
> -    conduit_(conduit),
> -    rtp_(nullptr, RTP),
> -    rtcp_(nullptr, RTCP),
> -    main_thread_(main_thread),
> +  : direction_(direction)
> +  , level_(0)
> +  , conduit_(conduit)
> +  , rtp_(nullptr, RTP)
> +  , rtcp_(nullptr, RTCP)
> +  , main_thread_(main_thread)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:629
(Diff revision 2)
> -    level_(0),
> -    conduit_(conduit),
> -    rtp_(nullptr, RTP),
> -    rtcp_(nullptr, RTCP),
> -    main_thread_(main_thread),
> -    sts_thread_(sts_thread),
> +  , level_(0)
> +  , conduit_(conduit)
> +  , rtp_(nullptr, RTP)
> +  , rtcp_(nullptr, RTCP)
> +  , main_thread_(main_thread)
> +  , sts_thread_(sts_thread)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , sts_thread_(sts_thread)
                ^
                std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1531
(Diff revision 2)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    bool is_video,
> +  bool is_video,
> -    dom::MediaStreamTrack* domtrack,
> +  dom::MediaStreamTrack* domtrack,
> -    RefPtr<MediaSessionConduit> conduit) :
> +  RefPtr<MediaSessionConduit> conduit)

Warning: The parameter 'conduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1532
(Diff revision 2)
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    bool is_video,
> +  bool is_video,
> -    dom::MediaStreamTrack* domtrack,
> +  dom::MediaStreamTrack* domtrack,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
> +  RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
                                ^
                                std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1532
(Diff revision 2)
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    bool is_video,
> +  bool is_video,
> -    dom::MediaStreamTrack* domtrack,
> +  dom::MediaStreamTrack* domtrack,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
> +  RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
                                             ^
                                             std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2189
(Diff revision 2)
> -MediaPipelineReceive::MediaPipelineReceive(
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -    const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +                                           nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> +                                           RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
                               ^
                               std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2189
(Diff revision 2)
> -MediaPipelineReceive::MediaPipelineReceive(
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -    const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +                                           nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> +                                           RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
                                            ^
                                            std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2189
(Diff revision 2)
> -MediaPipelineReceive::MediaPipelineReceive(
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -    const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +                                           nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> +                                           RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
                                                        ^
                                                        std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2194
(Diff revision 2)
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> -  segments_added_(0)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +  , segments_added_(0)
>  {
>  }
>  
> -MediaPipelineReceive::~MediaPipelineReceive()
> +MediaPipelineReceive::~MediaPipelineReceive() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

MediaPipelineReceive::~MediaPipelineReceive() {}
                      ^
                                              = default;

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2333
(Diff revision 2)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<AudioSessionConduit> conduit,
> +  RefPtr<AudioSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2333
(Diff revision 2)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<AudioSessionConduit> conduit,
> +  RefPtr<AudioSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                                          ^
                                          std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2515
(Diff revision 2)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<VideoSessionConduit> conduit,
> +  RefPtr<VideoSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2515
(Diff revision 2)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<VideoSessionConduit> conduit,
> +  RefPtr<VideoSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                                          ^
                                          std::move()
Comment on attachment 8935735 [details]
Bug 1404997 - P1. clang-format MediaPipeline.{cpp,h}.

https://reviewboard.mozilla.org/r/206632/#review212296


C/C++ static analysis found 14 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:625
(Diff revision 1)
>                               nsCOMPtr<nsIEventTarget> main_thread,
>                               nsCOMPtr<nsIEventTarget> sts_thread,
>                               RefPtr<MediaSessionConduit> conduit)
> -  : direction_(direction),
> -    level_(0),
> -    conduit_(conduit),
> +  : direction_(direction)
> +  , level_(0)
> +  , conduit_(conduit)

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , conduit_(conduit)
             ^
             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:628
(Diff revision 1)
> -  : direction_(direction),
> -    level_(0),
> -    conduit_(conduit),
> -    rtp_(nullptr, RTP),
> -    rtcp_(nullptr, RTCP),
> -    main_thread_(main_thread),
> +  : direction_(direction)
> +  , level_(0)
> +  , conduit_(conduit)
> +  , rtp_(nullptr, RTP)
> +  , rtcp_(nullptr, RTCP)
> +  , main_thread_(main_thread)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:629
(Diff revision 1)
> -    level_(0),
> -    conduit_(conduit),
> -    rtp_(nullptr, RTP),
> -    rtcp_(nullptr, RTCP),
> -    main_thread_(main_thread),
> -    sts_thread_(sts_thread),
> +  , level_(0)
> +  , conduit_(conduit)
> +  , rtp_(nullptr, RTP)
> +  , rtcp_(nullptr, RTCP)
> +  , main_thread_(main_thread)
> +  , sts_thread_(sts_thread)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , sts_thread_(sts_thread)
                ^
                std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1531
(Diff revision 1)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    bool is_video,
> +  bool is_video,
> -    dom::MediaStreamTrack* domtrack,
> +  dom::MediaStreamTrack* domtrack,
> -    RefPtr<MediaSessionConduit> conduit) :
> +  RefPtr<MediaSessionConduit> conduit)

Warning: The parameter 'conduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1532
(Diff revision 1)
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    bool is_video,
> +  bool is_video,
> -    dom::MediaStreamTrack* domtrack,
> +  dom::MediaStreamTrack* domtrack,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
> +  RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
                                ^
                                std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1532
(Diff revision 1)
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    bool is_video,
> +  bool is_video,
> -    dom::MediaStreamTrack* domtrack,
> +  dom::MediaStreamTrack* domtrack,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit),
> +  RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
                                             ^
                                             std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2189
(Diff revision 1)
> -MediaPipelineReceive::MediaPipelineReceive(
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -    const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +                                           nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> +                                           RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
                               ^
                               std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2189
(Diff revision 1)
> -MediaPipelineReceive::MediaPipelineReceive(
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -    const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +                                           nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> +                                           RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
                                            ^
                                            std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2189
(Diff revision 1)
> -MediaPipelineReceive::MediaPipelineReceive(
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -    const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +                                           nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<MediaSessionConduit> conduit) :
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> +                                           RefPtr<MediaSessionConduit> conduit)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)

Warning: Parameter 'conduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
                                                        ^
                                                        std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2194
(Diff revision 1)
> -  MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit),
> -  segments_added_(0)
> +  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +  , segments_added_(0)
>  {
>  }
>  
> -MediaPipelineReceive::~MediaPipelineReceive()
> +MediaPipelineReceive::~MediaPipelineReceive() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

MediaPipelineReceive::~MediaPipelineReceive() {}
                      ^
                                              = default;

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2333
(Diff revision 1)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<AudioSessionConduit> conduit,
> +  RefPtr<AudioSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2333
(Diff revision 1)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<AudioSessionConduit> conduit,
> +  RefPtr<AudioSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                                          ^
                                          std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2515
(Diff revision 1)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<VideoSessionConduit> conduit,
> +  RefPtr<VideoSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'main_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                             ^
                             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2515
(Diff revision 1)
> -    const std::string& pc,
> +  const std::string& pc,
> -    nsCOMPtr<nsIEventTarget> main_thread,
> +  nsCOMPtr<nsIEventTarget> main_thread,
> -    nsCOMPtr<nsIEventTarget> sts_thread,
> +  nsCOMPtr<nsIEventTarget> sts_thread,
> -    RefPtr<VideoSessionConduit> conduit,
> +  RefPtr<VideoSessionConduit> conduit,
> -    SourceMediaStream* aStream) :
> -  MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> +  SourceMediaStream* aStream)
> +  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)

Warning: Parameter 'sts_thread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
                                          ^
                                          std::move()
Comment on attachment 8935742 [details]
Bug 1404997 - P8. Follow coding style for members and methods.

https://reviewboard.mozilla.org/r/206646/#review212298


C/C++ static analysis found 17 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:614
(Diff revision 2)
> -                             nsCOMPtr<nsIEventTarget> main_thread,
> -                             nsCOMPtr<nsIEventTarget> sts_thread,
> -                             RefPtr<MediaSessionConduit> conduit)
> -  : direction_(direction)
> -  , level_(0)
> -  , conduit_(conduit)
> +                             nsCOMPtr<nsIEventTarget> aMainThread,
> +                             nsCOMPtr<nsIEventTarget> aStsThread,
> +                             RefPtr<MediaSessionConduit> aConduit)
> +  : mDirection(aDirection)
> +  , mLevel(0)
> +  , mConduit(aConduit)

Warning: Parameter 'aconduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mConduit(aConduit)
             ^
             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:617
(Diff revision 2)
> -  : direction_(direction)
> -  , level_(0)
> -  , conduit_(conduit)
> -  , rtp_(nullptr, RTP)
> -  , rtcp_(nullptr, RTCP)
> -  , main_thread_(main_thread)
> +  : mDirection(aDirection)
> +  , mLevel(0)
> +  , mConduit(aConduit)
> +  , mRtp(nullptr, RTP)
> +  , mRtcp(nullptr, RTCP)
> +  , mMainThread(aMainThread)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:618
(Diff revision 2)
> -  , level_(0)
> -  , conduit_(conduit)
> -  , rtp_(nullptr, RTP)
> -  , rtcp_(nullptr, RTCP)
> -  , main_thread_(main_thread)
> -  , sts_thread_(sts_thread)
> +  , mLevel(0)
> +  , mConduit(aConduit)
> +  , mRtp(nullptr, RTP)
> +  , mRtcp(nullptr, RTCP)
> +  , mMainThread(aMainThread)
> +  , mStsThread(aStsThread)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mStsThread(aStsThread)
               ^
               std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:715
(Diff revision 2)
> -                             filter),
> +                             aFilter),
>                  NS_DISPATCH_NORMAL);
>  }
>  
>  void
> -MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> rtp_transport,
> +MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> aRtpTransport,

Warning: The parameter 'artptransport' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1520
(Diff revision 2)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  bool is_video,
> -  dom::MediaStreamTrack* domtrack,
> -  RefPtr<MediaSessionConduit> conduit)
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  bool aIsVideo,
> +  dom::MediaStreamTrack* aDomTrack,
> +  RefPtr<MediaSessionConduit> aConduit)

Warning: The parameter 'aconduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1521
(Diff revision 2)
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  bool is_video,
> -  dom::MediaStreamTrack* domtrack,
> -  RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  bool aIsVideo,
> +  dom::MediaStreamTrack* aDomTrack,
> +  RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)
                                 ^
                                 std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1521
(Diff revision 2)
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  bool is_video,
> -  dom::MediaStreamTrack* domtrack,
> -  RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  bool aIsVideo,
> +  dom::MediaStreamTrack* aDomTrack,
> +  RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)
                                              ^
                                              std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1797
(Diff revision 2)
>    return NS_OK;
>  }
>  
>  nsresult
>  MediaPipeline::PipelineTransport::SendRtpRtcpPacket_s(
> -  nsAutoPtr<DataBuffer> data,
> +  nsAutoPtr<DataBuffer> aData,

Warning: The parameter 'adata' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2176
(Diff revision 2)
>  
> -MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -                                           nsCOMPtr<nsIEventTarget> main_thread,
> -                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -                                           RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& aPc,
> +                                           nsCOMPtr<nsIEventTarget> aMainThread,
> +                                           nsCOMPtr<nsIEventTarget> aStsThread,
> +                                           RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)
                                ^
                                std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2176
(Diff revision 2)
>  
> -MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -                                           nsCOMPtr<nsIEventTarget> main_thread,
> -                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -                                           RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& aPc,
> +                                           nsCOMPtr<nsIEventTarget> aMainThread,
> +                                           nsCOMPtr<nsIEventTarget> aStsThread,
> +                                           RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)
                                             ^
                                             std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2176
(Diff revision 2)
>  
> -MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -                                           nsCOMPtr<nsIEventTarget> main_thread,
> -                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -                                           RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& aPc,
> +                                           nsCOMPtr<nsIEventTarget> aMainThread,
> +                                           nsCOMPtr<nsIEventTarget> aStsThread,
> +                                           RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aconduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)
                                                         ^
                                                         std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2322
(Diff revision 2)
>  
>  MediaPipelineReceiveAudio::MediaPipelineReceiveAudio(
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<AudioSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<AudioSessionConduit> aConduit,

Warning: The parameter 'aconduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2324
(Diff revision 2)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<AudioSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<AudioSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2324
(Diff revision 2)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<AudioSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<AudioSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                                           ^
                                           std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2504
(Diff revision 2)
>  
>  MediaPipelineReceiveVideo::MediaPipelineReceiveVideo(
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<VideoSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<VideoSessionConduit> aConduit,

Warning: The parameter 'aconduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2506
(Diff revision 2)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<VideoSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<VideoSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2506
(Diff revision 2)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<VideoSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<VideoSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                                           ^
                                           std::move()
Comment on attachment 8935742 [details]
Bug 1404997 - P8. Follow coding style for members and methods.

https://reviewboard.mozilla.org/r/206646/#review212306


C/C++ static analysis found 17 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:614
(Diff revision 1)
> -                             nsCOMPtr<nsIEventTarget> main_thread,
> -                             nsCOMPtr<nsIEventTarget> sts_thread,
> -                             RefPtr<MediaSessionConduit> conduit)
> -  : direction_(direction)
> -  , level_(0)
> -  , conduit_(conduit)
> +                             nsCOMPtr<nsIEventTarget> aMainThread,
> +                             nsCOMPtr<nsIEventTarget> aStsThread,
> +                             RefPtr<MediaSessionConduit> aConduit)
> +  : mDirection(aDirection)
> +  , mLevel(0)
> +  , mConduit(aConduit)

Warning: Parameter 'aconduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mConduit(aConduit)
             ^
             std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:617
(Diff revision 1)
> -  : direction_(direction)
> -  , level_(0)
> -  , conduit_(conduit)
> -  , rtp_(nullptr, RTP)
> -  , rtcp_(nullptr, RTCP)
> -  , main_thread_(main_thread)
> +  : mDirection(aDirection)
> +  , mLevel(0)
> +  , mConduit(aConduit)
> +  , mRtp(nullptr, RTP)
> +  , mRtcp(nullptr, RTCP)
> +  , mMainThread(aMainThread)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:618
(Diff revision 1)
> -  , level_(0)
> -  , conduit_(conduit)
> -  , rtp_(nullptr, RTP)
> -  , rtcp_(nullptr, RTCP)
> -  , main_thread_(main_thread)
> -  , sts_thread_(sts_thread)
> +  , mLevel(0)
> +  , mConduit(aConduit)
> +  , mRtp(nullptr, RTP)
> +  , mRtcp(nullptr, RTCP)
> +  , mMainThread(aMainThread)
> +  , mStsThread(aStsThread)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mStsThread(aStsThread)
               ^
               std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:715
(Diff revision 1)
> -                             filter),
> +                             aFilter),
>                  NS_DISPATCH_NORMAL);
>  }
>  
>  void
> -MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> rtp_transport,
> +MediaPipeline::UpdateTransport_s(RefPtr<TransportFlow> aRtpTransport,

Warning: The parameter 'artptransport' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1520
(Diff revision 1)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  bool is_video,
> -  dom::MediaStreamTrack* domtrack,
> -  RefPtr<MediaSessionConduit> conduit)
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  bool aIsVideo,
> +  dom::MediaStreamTrack* aDomTrack,
> +  RefPtr<MediaSessionConduit> aConduit)

Warning: The parameter 'aconduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1521
(Diff revision 1)
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  bool is_video,
> -  dom::MediaStreamTrack* domtrack,
> -  RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  bool aIsVideo,
> +  dom::MediaStreamTrack* aDomTrack,
> +  RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)
                                 ^
                                 std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1521
(Diff revision 1)
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  bool is_video,
> -  dom::MediaStreamTrack* domtrack,
> -  RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, TRANSMIT, main_thread, sts_thread, conduit)
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  bool aIsVideo,
> +  dom::MediaStreamTrack* aDomTrack,
> +  RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, TRANSMIT, aMainThread, aStsThread, aConduit)
                                              ^
                                              std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1797
(Diff revision 1)
>    return NS_OK;
>  }
>  
>  nsresult
>  MediaPipeline::PipelineTransport::SendRtpRtcpPacket_s(
> -  nsAutoPtr<DataBuffer> data,
> +  nsAutoPtr<DataBuffer> aData,

Warning: The parameter 'adata' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2176
(Diff revision 1)
>  
> -MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -                                           nsCOMPtr<nsIEventTarget> main_thread,
> -                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -                                           RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& aPc,
> +                                           nsCOMPtr<nsIEventTarget> aMainThread,
> +                                           nsCOMPtr<nsIEventTarget> aStsThread,
> +                                           RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)
                                ^
                                std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2176
(Diff revision 1)
>  
> -MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -                                           nsCOMPtr<nsIEventTarget> main_thread,
> -                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -                                           RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& aPc,
> +                                           nsCOMPtr<nsIEventTarget> aMainThread,
> +                                           nsCOMPtr<nsIEventTarget> aStsThread,
> +                                           RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)
                                             ^
                                             std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2176
(Diff revision 1)
>  
> -MediaPipelineReceive::MediaPipelineReceive(const std::string& pc,
> -                                           nsCOMPtr<nsIEventTarget> main_thread,
> -                                           nsCOMPtr<nsIEventTarget> sts_thread,
> -                                           RefPtr<MediaSessionConduit> conduit)
> -  : MediaPipeline(pc, RECEIVE, main_thread, sts_thread, conduit)
> +MediaPipelineReceive::MediaPipelineReceive(const std::string& aPc,
> +                                           nsCOMPtr<nsIEventTarget> aMainThread,
> +                                           nsCOMPtr<nsIEventTarget> aStsThread,
> +                                           RefPtr<MediaSessionConduit> aConduit)
> +  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aconduit' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipeline(aPc, RECEIVE, aMainThread, aStsThread, aConduit)
                                                         ^
                                                         std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2322
(Diff revision 1)
>  
>  MediaPipelineReceiveAudio::MediaPipelineReceiveAudio(
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<AudioSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<AudioSessionConduit> aConduit,

Warning: The parameter 'aconduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2324
(Diff revision 1)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<AudioSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<AudioSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2324
(Diff revision 1)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<AudioSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<AudioSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                                           ^
                                           std::move()

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2504
(Diff revision 1)
>  
>  MediaPipelineReceiveVideo::MediaPipelineReceiveVideo(
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<VideoSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<VideoSessionConduit> aConduit,

Warning: The parameter 'aconduit' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2506
(Diff revision 1)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<VideoSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<VideoSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'amainthread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                              ^
                              std::move( )

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2506
(Diff revision 1)
> -  const std::string& pc,
> -  nsCOMPtr<nsIEventTarget> main_thread,
> -  nsCOMPtr<nsIEventTarget> sts_thread,
> -  RefPtr<VideoSessionConduit> conduit,
> +  const std::string& aPc,
> +  nsCOMPtr<nsIEventTarget> aMainThread,
> +  nsCOMPtr<nsIEventTarget> aStsThread,
> +  RefPtr<VideoSessionConduit> aConduit,
>    SourceMediaStream* aStream)
> -  : MediaPipelineReceive(pc, main_thread, sts_thread, conduit)
> +  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)

Warning: Parameter 'aststhread' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : MediaPipelineReceive(aPc, aMainThread, aStsThread, aConduit)
                                           ^
                                           std::move()
Comment on attachment 8935751 [details]
Bug 1404997 - P18. Add Await convenience methods.

https://reviewboard.mozilla.org/r/206664/#review212288

LGTM, but I'd feel better if you had tests, or at least some code that uses this!
r+ with fixes below, and assuming you exercize Await.

::: commit-message-30069:1
(Diff revision 2)
> +Bug 1404997 - P17. Add Await convience methods. r?gerald

'convience' -> 'convenience'

::: commit-message-30069:3
(Diff revision 2)
> +Bug 1404997 - P17. Add Await convience methods. r?gerald
> +
> +Takes either a MozPromise or a AllPromiseType and will execute the resolve/reject functions synchronously once the promise has resolved/rejected.

'a AllPromiseType' -> 'an AllPromiseType'
'has' -> 'is'

::: dom/media/systemservices/MediaUtils.h:436
(Diff revision 2)
> +
> +  bool done = false;
> +
> +  aPromise->Then(thread,
> +                 __func__,
> +                 [&](ResolveValueType aResolveValue) {

If you will only use Await with exclusive promises, you'd receive an rvalue-ref here, saving a construction.

Alternatively, Await could be implemented inside MozPromise itself (because it's really similar to 'Then', except it blocks the current thread), and there it would have access to MaybeMove for maximum efficiency. This could be done in a separate bug, possibly by JW.

Another suggestion while I'm on a roll: Another useful 'Await' function could take only the promise (no extra resolve/reject functions), and return the final MozPromise<...>::ResolveOrRejectValue.

::: dom/media/systemservices/MediaUtils.h:438
(Diff revision 2)
> +
> +  aPromise->Then(thread,
> +                 __func__,
> +                 [&](ResolveValueType aResolveValue) {
> +                   MonitorAutoLock lock(mon);
> +                   aResolveFunction(aResolveValue);

You can Move(aResolveValue), since it's the last use of our local copy.

::: dom/media/systemservices/MediaUtils.h:444
(Diff revision 2)
> +                   done = true;
> +                   mon.Notify();
> +                 },
> +                 [&](RejectValueType aRejectValue) {
> +                   MonitorAutoLock lock(mon);
> +                   aRejectFunction(aRejectValue);

same

::: dom/media/systemservices/MediaUtils.h:461
(Diff revision 2)
> +Await(RefPtr<typename MozPromise<ResolveValueType, RejectValueType, Excl>::
> +               AllPromiseType> aAllPromise,

An AllPromiseType *is* a MozPromise itself, so I don't think you need this separate function.

::: dom/media/systemservices/MediaUtils.h:473
(Diff revision 2)
> +    SharedThreadPool::Get(NS_LITERAL_CSTRING("AwaitMozPromise"));
> +  bool done = false;
> +
> +  aAllPromise->Then(thread,
> +                    __func__,
> +                    [&](ResolveValueType aResolveValue) {

In case you really need to keep this separate Await-all-promise function, I believe the parameter type for the resolve function should be `nsTArray<ResolveValueType>`.
Attachment #8935751 - Flags: review?(gsquelart) → review+
Comment on attachment 8935735 [details]
Bug 1404997 - P1. clang-format MediaPipeline.{cpp,h}.

https://reviewboard.mozilla.org/r/206632/#review212316

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:620
(Diff revision 2)
>                               nsCOMPtr<nsIEventTarget> main_thread,
>                               nsCOMPtr<nsIEventTarget> sts_thread,
>                               RefPtr<MediaSessionConduit> conduit)

Makes sense to make these refs like clangbot suggests.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2573
(Diff revision 2)
> +{
>    return webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds();
>  }
>  
>  DOMHighResTimeStamp
> -MediaPipeline::RtpCSRCStats::GetExpiryFromTime(
> +MediaPipeline::RtpCSRCStats::GetExpiryFromTime(const DOMHighResTimeStamp aTime)

A const copy. Funky.
Attachment #8935735 - Flags: review?(apehrson) → review+
Comment on attachment 8935736 [details]
Bug 1404997 - P2. Use AutoTaskQueue in VideoFrameConverter.

https://reviewboard.mozilla.org/r/206634/#review212328

I'll give r+ for the switch in general but I'd like to know how we get to the UAF first. Or if we don't, removing Shutdown() would be fine.

::: commit-message-4bf81:4
(Diff revision 2)
> +Bug 1404997 - P2. Use AutoTaskQueue in VideoFrameConverter. r?pehrsons
> +
> +It removes the need to explicitly shut down the taskqueue and wait on the taskqueue to have run all dispatched task.
> +We do want to enforce that no listeners are being called once the VideoFrameConverter's owner has been destroyed as it could potentially lead to a UAF.

I don't see how this could lead to UAF as all the pointers are RefPtr. Am I missing something?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:240
(Diff revision 2)
>    void Shutdown()
>    {
> -    mTaskQueue->BeginShutdown();
> -    mTaskQueue->AwaitShutdownAndIdle();
> +    MutexAutoLock lock(mMutex);
> +    mListeners.Clear();
>    }

The way I read the code, Shutdown() shouldn't be needed at all.
Attachment #8935736 - Flags: review?(apehrson)
Comment on attachment 8935737 [details]
Bug 1404997 - P3. Rename some VideoFrameConverter members per coding style.

https://reviewboard.mozilla.org/r/206636/#review212332
Attachment #8935737 - Flags: review?(apehrson) → review+
Comment on attachment 8935738 [details]
Bug 1404997 - P4. Make AudioProxyThread use AutoTaskQueue.

https://reviewboard.mozilla.org/r/206638/#review212334

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
(Diff revision 2)
> -    // Use only 1 thread; also forces FIFO operation
> -    // We could use multiple threads, but that may be dicier with the webrtc.org
> -    // code.  If so we'd need to use TaskQueues like the videoframe converter

The reasoning behind one thread in the pool is worth keeping I think.
Attachment #8935738 - Flags: review?(apehrson) → review+
Comment on attachment 8935738 [details]
Bug 1404997 - P4. Make AudioProxyThread use AutoTaskQueue.

https://reviewboard.mozilla.org/r/206638/#review212334

> The reasoning behind one thread in the pool is worth keeping I think.

I don't see the value in the original reasoning tbh.

If having multi-threaded conversion could cause a problem at shutdown, it would be the same earlier on.
Comment on attachment 8935736 [details]
Bug 1404997 - P2. Use AutoTaskQueue in VideoFrameConverter.

https://reviewboard.mozilla.org/r/206634/#review212328

> I don't see how this could lead to UAF as all the pointers are RefPtr. Am I missing something?

it's the re-entrancy in the SourceMediaStream / MSG that I worry about.
Right now it's fine because it's all accessed on the same thread.
But this will change in follow up patches.
The SourceMediaStream keeps a raw pointer to the MSG. SourceMediaStream::mGraph isn't accessed in a thread-safe fashion.

> The way I read the code, Shutdown() shouldn't be needed at all.

I agree..

However, the original shutdown was synchronous in waiting that any tasks of the taskqueue this keeps the same flow...
Shutdown() will ensure that once it has been called, re-entrancy can no longer occur.
Comment on attachment 8935740 [details]
Bug 1404997 - P6. Fix constness were applicable.

https://reviewboard.mozilla.org/r/206642/#review212348
Attachment #8935740 - Flags: review?(apehrson) → review+
Comment on attachment 8935741 [details]
Bug 1404997 - P7. Simplify played time calculations.

https://reviewboard.mozilla.org/r/206644/#review212358

::: dom/media/MediaStreamGraph.cpp:189
(Diff revision 2)
>      if (aStream->mPullEnabled && !aStream->mFinished &&
>          !aStream->mListeners.IsEmpty()) {
>        // Compute how much stream time we'll need assuming we don't block
>        // the stream at all.
>        StreamTime t = aStream->GraphTimeToStreamTime(aDesiredUpToTime);
> +      StreamTime current = aStream->mTracks.GetEnd();

If `aStream` has two tracks that have received different amounts of data, `current` will be the end of the track ending earliest (the comment in StreamTracks.h appears to be wrong, compared to the impl).

Then we risk appending too much data to the other track.

I think the risk of this happening is pretty low, but I could see it in cases where we feed two tracks to the same SourceMediaStream, like MediaManager.

To land this you'll need to assert that all live tracks in `aStream` are indeed of the same duration.

Important to note that you'll also have to account for data that has already been pushed since the last ExtractPendingInput(). That would be the segments in `mUpdateTracks`. `MediaEngineWebRTCMicrophoneSource` is an example of a track source that has pull enabled but pushes outside of NotifyPull().
Attachment #8935741 - Flags: review?(apehrson) → review-
Comment on attachment 8935741 [details]
Bug 1404997 - P7. Simplify played time calculations.

https://reviewboard.mozilla.org/r/206644/#review212372
Comment on attachment 8935741 [details]
Bug 1404997 - P7. Simplify played time calculations.

https://reviewboard.mozilla.org/r/206644/#review212358

> If `aStream` has two tracks that have received different amounts of data, `current` will be the end of the track ending earliest (the comment in StreamTracks.h appears to be wrong, compared to the impl).
> 
> Then we risk appending too much data to the other track.
> 
> I think the risk of this happening is pretty low, but I could see it in cases where we feed two tracks to the same SourceMediaStream, like MediaManager.
> 
> To land this you'll need to assert that all live tracks in `aStream` are indeed of the same duration.
> 
> Important to note that you'll also have to account for data that has already been pushed since the last ExtractPendingInput(). That would be the segments in `mUpdateTracks`. `MediaEngineWebRTCMicrophoneSource` is an example of a track source that has pull enabled but pushes outside of NotifyPull().

Need a StreamTracksListener rather than having multiple listener for the same stream :(
it's a bit silly right now to have those..
Comment on attachment 8935738 [details]
Bug 1404997 - P4. Make AudioProxyThread use AutoTaskQueue.

https://reviewboard.mozilla.org/r/206638/#review212334

> I don't see the value in the original reasoning tbh.
> 
> If having multi-threaded conversion could cause a problem at shutdown, it would be the same earlier on.

Shutdown?

I think it's more about not breaking assumptions in webrtc.org code since it's an external module. Like when you have tasks running in sequence but webrtc.org may barf on the threads being different.
Comment on attachment 8935736 [details]
Bug 1404997 - P2. Use AutoTaskQueue in VideoFrameConverter.

https://reviewboard.mozilla.org/r/206634/#review212328

> it's the re-entrancy in the SourceMediaStream / MSG that I worry about.
> Right now it's fine because it's all accessed on the same thread.
> But this will change in follow up patches.
> The SourceMediaStream keeps a raw pointer to the MSG. SourceMediaStream::mGraph isn't accessed in a thread-safe fashion.

Ok, please put a short explanation of this in the commit message.
Comment on attachment 8935742 [details]
Bug 1404997 - P8. Follow coding style for members and methods.

https://reviewboard.mozilla.org/r/206646/#review212398

I'm myself not sure why our WebRTC classes are not following our coding style. Drno would be a more suitable reviewer.
Attachment #8935742 - Flags: review?(apehrson)
Comment on attachment 8935743 [details]
Bug 1404997 - P9. Remove unused member and accessor.

https://reviewboard.mozilla.org/r/206648/#review212402
Attachment #8935743 - Flags: review?(apehrson) → review+
Comment on attachment 8935741 [details]
Bug 1404997 - P7. Simplify played time calculations.

https://reviewboard.mozilla.org/r/206644/#review212358

> Need a StreamTracksListener rather than having multiple listener for the same stream :(
> it's a bit silly right now to have those..

Bug 1423241
Comment on attachment 8935745 [details]
Bug 1404997 - P12. Remove unused TrackAddedCallback class.

https://reviewboard.mozilla.org/r/206652/#review212416

Oh. I guess this is no longer needed because since Transceivers landed we use one SourceMediaStream per track.
Attachment #8935745 - Flags: review?(apehrson) → review+
Comment on attachment 8935742 [details]
Bug 1404997 - P8. Follow coding style for members and methods.

https://reviewboard.mozilla.org/r/206646/#review212398

And there's no valid reason that it shouldn't be...

Especially, as there's just no consistent style in this file. It varied depending on who added a line...

As I mentioned in the commit message, at least now, you can more easily tell what is webrtc and what isn't....
Comment on attachment 8935751 [details]
Bug 1404997 - P18. Add Await convenience methods.

https://reviewboard.mozilla.org/r/206664/#review212288

> If you will only use Await with exclusive promises, you'd receive an rvalue-ref here, saving a construction.
> 
> Alternatively, Await could be implemented inside MozPromise itself (because it's really similar to 'Then', except it blocks the current thread), and there it would have access to MaybeMove for maximum efficiency. This could be done in a separate bug, possibly by JW.
> 
> Another suggestion while I'm on a roll: Another useful 'Await' function could take only the promise (no extra resolve/reject functions), and return the final MozPromise<...>::ResolveOrRejectValue.

good idea.. will do that.
In general all code in media/webrtc/signaling/* is ours and it's probably a good idea to format it. But it will take me some time to review this. I'm also wondering if it would make more sense to land such reformats in a spearate bug to allow for easier tracking/bisecting etc.
Comment on attachment 8935741 [details]
Bug 1404997 - P7. Simplify played time calculations.

https://reviewboard.mozilla.org/r/206644/#review212358

> Bug 1423241

ok, will revisit once that bug is closed...
Comment on attachment 8935739 [details]
Bug 1404997 - P5. Fix constness and remove redundant virtual keyword.

https://reviewboard.mozilla.org/r/206640/#review212626
Attachment #8935739 - Flags: review?(matt.woodrow) → review+
Depends on: 1424647
Blocks: 1424653
Comment on attachment 8936185 [details]
Bug 1404997 - P19. Use new Await method with WebrtcMediaDataDecoder.

https://reviewboard.mozilla.org/r/206940/#review212762
Attachment #8936185 - Flags: review?(gsquelart) → review+
Comment on attachment 8935741 [details]
Bug 1404997 - P7. Simplify played time calculations.

https://reviewboard.mozilla.org/r/206644/#review212812

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2244
(Diff revision 4)
>  
>      TrackRate rate = graph->GraphRate();
> -    uint32_t samples_per_10ms = rate/100;
> +    uint32_t samples_per_10ms = rate / 100;
> +    // Determine how many frames we need.
> +    // As we get frames from conduit_ at the same rate as the graph's rate,
> +    // the number of frames needed straightfully determined.

Missing an "is"?
Attachment #8935741 - Flags: review?(apehrson) → review+
Comment on attachment 8935742 [details]
Bug 1404997 - P8. Follow coding style for members and methods.

https://reviewboard.mozilla.org/r/206646/#review212816

::: commit-message-4c3cc:1
(Diff revision 4)
> +Bug 1404997 - P8. Follow coding style for members and methods. r?pehrsons,r?drno

r?drno would be sufficient here, I'll cancel, thanks.
Attachment #8935742 - Flags: review?(apehrson)
Comment on attachment 8936188 [details]
Bug 1404997 - P22. Make mConduit release on main thread consistent.

https://reviewboard.mozilla.org/r/206946/#review212818
Attachment #8936188 - Flags: review?(apehrson) → review+
Comment on attachment 8936189 [details]
Bug 1404997 - P23. Strongly enforced that our destination buffer is big enough.

https://reviewboard.mozilla.org/r/206948/#review212820

I'm also fine if you leave these as out of scope of this bug, like you mentioned when clangbot brought them up last time.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:94
(Diff revision 2)
> -                nsCOMPtr<nsIEventTarget> aMainThread,
> -                nsCOMPtr<nsIEventTarget> aStsThread,
> -                RefPtr<MediaSessionConduit> aConduit);
> +                already_AddRefed<nsIEventTarget> aMainThread,
> +                already_AddRefed<nsIEventTarget> aStsThread,
> +                already_AddRefed<MediaSessionConduit> aConduit);

Since these are shared and we're not taking ownership I'd rather see them being `const RefPtr<>&`.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1324
(Diff revision 2)
>  class MediaPipelineTransmit::PipelineListener : public MediaStreamVideoSink
>  {
>    friend class MediaPipelineTransmit;
>  
>  public:
> -  explicit PipelineListener(const RefPtr<MediaSessionConduit>& aConduit)
> +  explicit PipelineListener(RefPtr<MediaSessionConduit> aConduit)

Why the need for copy here?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1511
(Diff revision 2)
> +                  aMainThread.forget(),
> +                  aStsThread.forget(),
> +                  aConduit.forget())
>    , mIsVideo(aIsVideo)
> -  , mListener(new PipelineListener(aConduit))
> -  , mFeeder(aIsVideo ? MakeAndAddRef<VideoFrameFeeder>(mListener)
> +  , mListener(new PipelineListener(mConduit))
> +  , mFeeder(aIsVideo ? new VideoFrameFeeder(mListener)

In general we seem to be moving away from raw pointers, not towards them.
Attachment #8936189 - Flags: review?(apehrson) → review-
Comment on attachment 8936190 [details]
Bug 1404997 - P23. Make clang-tidy happier.

https://reviewboard.mozilla.org/r/206950/#review212822

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:95
(Diff revision 2)
>                                       uint64_t aCaptureTime) = 0;
>  
>    virtual void OnVideoFrameConverted(const webrtc::VideoFrame& aVideoFrame) = 0;
>  
>  protected:
> -  virtual ~VideoConverterListener() {}
> +  virtual ~VideoConverterListener()= default;

nit: s/=/ =/
Attachment #8936190 - Flags: review?(apehrson) → review+
Comment on attachment 8936187 [details]
Bug 1404997 - P21. Make MediaPipelineReceiveAudio listener asynchronous.

https://reviewboard.mozilla.org/r/206944/#review212824

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:55
(Diff revision 2)
> -#include "CSFLog.h"
> +#include <inttypes.h>
> +#include <math.h>

Coding style says:

Includes are split into three blocks and are sorted alphabetically in each block:
- The main header: #include "Foo.h" in Foo.cpp
- Standard library includes: #include <map>
- Mozilla includes: #include "mozilla/dom/Element.h"

So at the very least these should go up one block.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2174
(Diff revision 2)
>                     const RefPtr<MediaSessionConduit>& aConduit)
>      : GenericReceiveListener(aTrack)
>      , mConduit(aConduit)
> +    , mSource(mTrack->GetInputStream()->AsSourceStream())
> +    , mTrackId(mTrack->GetInputTrackId())
> +    , mRate(mSource ? mSource->GraphRate() : 0)

We need an invariant (which we seem to have) that checks that `mSource` is always non-null instead of all this handling of when it is null.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2177
(Diff revision 2)
> +    , mSource(mTrack->GetInputStream()->AsSourceStream())
> +    , mTrackId(mTrack->GetInputTrackId())
> +    , mRate(mSource ? mSource->GraphRate() : 0)
> +    , mTaskQueue(new AutoTaskQueue(
> +        SharedThreadPool::Get(NS_LITERAL_CSTRING("PipelineAudioListener"))))
>      , mLastLog(0)

This is now already initialized to 0.
Attachment #8936187 - Flags: review?(apehrson) → review+
Comment on attachment 8936186 [details]
Bug 1404997 - P20. Make MediaStreamListener::NotifyPull asynchronous.

https://reviewboard.mozilla.org/r/206942/#review212826

::: dom/media/MediaStreamGraph.cpp:2748
(Diff revision 2)
>  #endif
> -      for (uint32_t j = 0; j < mListeners.Length(); ++j) {
> +  for (uint32_t j = 0; j < mListeners.Length(); ++j) {
> -        MediaStreamListener* l = mListeners[j];
> +    MediaStreamListener* l = mListeners[j];
> -        {
> +    {
> -          MutexAutoUnlock unlock(mMutex);
> +      MutexAutoUnlock unlock(mMutex);
> -          l->NotifyPull(GraphImpl(), t);
> +      promises.AppendElement(l->AsyncNotifyPull(GraphImpl(), t));

Having a pref to turn off async pulling if needed would be useful IMO.
Comment on attachment 8936189 [details]
Bug 1404997 - P23. Strongly enforced that our destination buffer is big enough.

https://reviewboard.mozilla.org/r/206948/#review212820

Well, we can remove unnecessary

> Since these are shared and we're not taking ownership I'd rather see them being `const RefPtr<>&`.

As they are passed, it allows move semantics and avoids the addRef. By passing const references you will need an extra addRef which is what this change aims to

> Why the need for copy here?

So we can use move semantics, something you can't do with const references. There will be no copy here as per calling points.

> In general we seem to be moving away from raw pointers, not towards them.

MakeAndAddRef is utterly useless. It serves no purpose of any kind. It doesn't even do what its name says. This is all done within the RefPtr constructor. We aren't dealing with raw pointers per say.
Comment on attachment 8936187 [details]
Bug 1404997 - P21. Make MediaPipelineReceiveAudio listener asynchronous.

https://reviewboard.mozilla.org/r/206944/#review212824

> We need an invariant (which we seem to have) that checks that `mSource` is always non-null instead of all this handling of when it is null.

Good point, I'll make use of it.
Comment on attachment 8936186 [details]
Bug 1404997 - P20. Make MediaStreamListener::NotifyPull asynchronous.

https://reviewboard.mozilla.org/r/206942/#review212826

> Having a pref to turn off async pulling if needed would be useful IMO.

this will be done in bug 1424653
Blocks: 1425023
Attachment #8936190 - Attachment is obsolete: true
Comment on attachment 8935746 [details]
Bug 1404997 - P13. Move ExtractPendingInput logic to SourceMediaStream.

https://reviewboard.mozilla.org/r/206654/#review213556
Attachment #8935746 - Flags: review?(padenot) → review+
Comment on attachment 8935747 [details]
Bug 1404997 - P14. Rename members to clarify the finish meaning.

https://reviewboard.mozilla.org/r/206656/#review213558

::: commit-message-96caa:3
(Diff revision 5)
> +Bug 1404997 - P13. Rename members to clarify the finish meaning. r?padenot
> +
> +We have different concept of "finish" between the base class and its hierarchie.

hierarchy

::: dom/media/MediaStreamGraph.h:1172
(Diff revision 5)
>    /**
> -   * Force this stream into the finished state.
> +   * Queue a message to force this stream into the finished state.
>     */
> -  void Finish();
> +  void QueueFinish();
>    /**
> -   * Set the autofinish flag on this stream (defaults to false). When this flag
> +   * Queoe a message to set the autofinish flag on this stream (defaults to

s/Queuoe/Queue/
Attachment #8935747 - Flags: review?(padenot) → review+
Comment on attachment 8935748 [details]
Bug 1404997 - P15. Move MSG::FinishStream logic to MediaStream.

https://reviewboard.mozilla.org/r/206658/#review213560
Attachment #8935748 - Flags: review?(padenot) → review+
Comment on attachment 8935750 [details]
Bug 1404997 - P17. Split ExtractPendingInput into two methods.

https://reviewboard.mozilla.org/r/206662/#review213564
Attachment #8935750 - Flags: review?(padenot) → review+
Comment on attachment 8935749 [details]
Bug 1404997 - P16. Properly finish the SourceMediaStream during shutdown.

https://reviewboard.mozilla.org/r/206660/#review213562
Attachment #8935749 - Flags: review?(padenot) → review+
Comment on attachment 8936186 [details]
Bug 1404997 - P20. Make MediaStreamListener::NotifyPull asynchronous.

https://reviewboard.mozilla.org/r/206942/#review213566

r+ with comment answered or addressed.

::: dom/media/MediaStreamGraph.cpp:1153
(Diff revision 3)
>  
>    bool ensureNextIteration = false;
>  
>    // Grab pending stream input and compute blocking time
> +  // TODO: Ensure that heap memory allocations isn't going to be a problem.
> +  // Maybe modify code to use nsAutoTArray as out parameters.

It would be better to use AutoTArray yeah, with a large number of slots.

::: dom/media/MediaStreamGraph.cpp:2715
(Diff revision 3)
> -void
> +nsTArray<RefPtr<SourceMediaStream::NotifyPullPromise>>
>  SourceMediaStream::PullNewData(StreamTime aDesiredUpToTime,
>                                 bool* aEnsureNextIteration)
>  {
> +  // 2 is the average number of listeners per SourceMediaStream.
> +  nsTArray<RefPtr<SourceMediaStream::NotifyPullPromise>> promises(2);

This also allocates. Can we find a strategy to avoid it? I think we can determine how many listeners we have on the call site and pass in a stack-based array.
Attachment #8936186 - Flags: review?(padenot) → review+
Comment on attachment 8936189 [details]
Bug 1404997 - P23. Strongly enforced that our destination buffer is big enough.

https://reviewboard.mozilla.org/r/206948/#review213572
Attachment #8936189 - Flags: review?(padenot) → review+
Blocks: 1425473
Comment on attachment 8936186 [details]
Bug 1404997 - P20. Make MediaStreamListener::NotifyPull asynchronous.

https://reviewboard.mozilla.org/r/206942/#review213566

> This also allocates. Can we find a strategy to avoid it? I think we can determine how many listeners we have on the call site and pass in a stack-based array.

There's plenty of calls to "new" across the line, so unless this is a proven issue I will leave it as is... I will create another bug to change that.
I would love to limit or even eliminate allocations within the MSG hot path, but I doubt we can get it near 0.  Still, avoiding ones we can is good.  I'd set the default size to well above the "average" number of listeners, especially as we're talking about a very limited amount of memory in any case - doubly-so if it's on the stack.  And an AutoTArray makes sense
Comment on attachment 8937137 [details]
Bug 1404997 - P24. Make AutoTaskQueue deliver runnables to nsIEventTarget.

https://reviewboard.mozilla.org/r/207838/#review213716
Attachment #8937137 - Flags: review?(gsquelart) → review+
Comment on attachment 8937139 [details]
Bug 1404997 - P26. Give Await the threadpool to use.

https://reviewboard.mozilla.org/r/207842/#review213720

::: dom/media/systemservices/MediaUtils.h:440
(Diff revision 1)
> -  RefPtr<AutoTaskQueue> taskQueue = new AutoTaskQueue(
> -    SharedThreadPool::Get(NS_LITERAL_CSTRING("AwaitMozPromise")));
> +  RefPtr<nsIEventTarget> pool = aPool;
> +  RefPtr<AutoTaskQueue> taskQueue = new AutoTaskQueue(pool.forget());

It's the only use of `aPool` in this function, so you don't need to store it into a RefPtr and then immediately forget it, instead just pass it straight through to AutoTaskQueue.

::: dom/media/systemservices/MediaUtils.h:472
(Diff revision 1)
> -  RefPtr<AutoTaskQueue> taskQueue = new AutoTaskQueue(
> -    SharedThreadPool::Get(NS_LITERAL_CSTRING("AwaitMozPromise")));
> +  RefPtr<nsIEventTarget> pool = aPool;
> +  RefPtr<AutoTaskQueue> taskQueue = new AutoTaskQueue(pool.forget());

same

::: dom/media/systemservices/MediaUtils.h:534
(Diff revision 1)
> +         nsTArray<RefPtr<MozPromise<ResolveValueType, RejectValueType, true>>>&
>             aPromises)
>  {
>    typedef MozPromise<ResolveValueType, RejectValueType, true> Promise;
> -  RefPtr<AutoTaskQueue> taskQueue = new AutoTaskQueue(
> -    SharedThreadPool::Get(NS_LITERAL_CSTRING("AwaitMozPromise")));
> +  RefPtr<nsIEventTarget> pool = aPool;
> +  RefPtr<TaskQueue> t = new TaskQueue(do_AddRef(pool));

This line should probably be removed (I don't see `t` being used.)
Attachment #8937139 - Flags: review?(gsquelart) → review+
Comment on attachment 8937139 [details]
Bug 1404997 - P26. Give Await the threadpool to use.

https://reviewboard.mozilla.org/r/207842/#review213720

> It's the only use of `aPool` in this function, so you don't need to store it into a RefPtr and then immediately forget it, instead just pass it straight through to AutoTaskQueue.

yes I do...

otherwise I get a compilation error that already_AddRef copy constructor is deleted.

> This line should probably be removed (I don't see `t` being used.)

oops
Attachment #8935742 - Flags: review?(gsquelart)
Attachment #8935742 - Flags: review?(drno)
Attachment #8935742 - Flags: review?(apehrson)
Attachment #8935742 - Flags: review?(gsquelart) → review?(padenot)
Comment on attachment 8935742 [details]
Bug 1404997 - P8. Follow coding style for members and methods.

https://reviewboard.mozilla.org/r/206646/#review213766
Attachment #8935742 - Flags: review?(padenot) → review+
Comment on attachment 8937138 [details]
Bug 1404997 - P25. Add MSG and WebRTC MediaThreadType.

https://reviewboard.mozilla.org/r/207840/#review213770
Attachment #8937138 - Flags: review?(gsquelart) → review+
Comment on attachment 8937198 [details]
Bug 1404997 - P27. Name TaskQueue for debugging purposes.

https://reviewboard.mozilla.org/r/207914/#review213772
Attachment #8937198 - Flags: review?(gsquelart) → review+
Comment on attachment 8937140 [details]
Bug 1404997 - P26. Reduce global threads count.

https://reviewboard.mozilla.org/r/207844/#review213782
Attachment #8937140 - Flags: review?(padenot) → review+
Blocks: 1425631
Attachment #8937140 - Attachment is obsolete: true