Closed
Bug 1404997
Opened 7 years ago
Closed 7 years ago
Move Opus decoding/NetEQ out of the MSG thread, it’s too expensive
Categories
(Core :: WebRTC: Audio/Video, enhancement, P2)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla59
| Tracking | Status | |
|---|---|---|
| firefox59 | --- | fixed |
People
(Reporter: padenot, Assigned: jya)
References
(Blocks 2 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
|
mozbugz
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mozbugz
:
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
|
mozbugz
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
Instruments profile showing this issue
This accounts for about 3 times all the rest of the MSG work combined. We should use lock-free queues here.
|
Reporter | |
Updated•7 years ago
|
Blocks: webrtc-call-quality
|
|
Reporter | |
Updated•7 years ago
|
Rank: 7
Priority: P2 → P1
|
|
Reporter | |
Comment 1•7 years ago
|
||
This is really critical to fix.
|
||
Comment 2•7 years ago
|
||
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...
|
||
Comment 3•7 years ago
|
||
@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?
|
||
Comment 4•7 years ago
|
||
Tech debt does not qualify as P1 under the new priority schema. Adjusting to top-rank P2.
Rank: 7 → 10
Priority: P1 → P2
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → jyavenard
|
|
Reporter | |
Comment 5•7 years ago
|
||
NI myself so I don't forget we want profiles from pre-57 update, and compare it with the profile above.
|
|
Reporter | |
Updated•7 years ago
|
Flags: needinfo?(padenot)
|
|
||
Comment 6•7 years ago
|
||
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.
|
|
Reporter | |
Comment 7•7 years ago
|
||
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.)
|
||
Comment 8•7 years ago
|
||
(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.
|
|
Reporter | |
Comment 9•7 years ago
|
||
> 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)
|
|
||
Comment 10•7 years ago
|
||
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.
|
||
Updated•7 years ago
|
Flags: needinfo?(ajones)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 45•7 years ago
|
||
| mozreview-review | ||
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 46•7 years ago
|
||
| mozreview-review | ||
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 47•7 years ago
|
||
| mozreview-review | ||
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 48•7 years ago
|
||
| mozreview-review | ||
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 49•7 years ago
|
||
| mozreview-review | ||
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 50•7 years ago
|
||
| mozreview-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 51•7 years ago
|
||
| mozreview-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 52•7 years ago
|
||
| mozreview-review | ||
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 53•7 years ago
|
||
| mozreview-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+
|
|
Assignee | |
Comment 54•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 55•7 years ago
|
||
| mozreview-review-reply | ||
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 56•7 years ago
|
||
| mozreview-review | ||
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 57•7 years ago
|
||
| mozreview-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-
|
|
Assignee | |
Comment 58•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935741 [details] Bug 1404997 - P7. Simplify played time calculations. https://reviewboard.mozilla.org/r/206644/#review212372
|
|
Assignee | |
Comment 59•7 years ago
|
||
| mozreview-review-reply | ||
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 60•7 years ago
|
||
| mozreview-review-reply | ||
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 61•7 years ago
|
||
| mozreview-review-reply | ||
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 62•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935736 [details] Bug 1404997 - P2. Use AutoTaskQueue in VideoFrameConverter. https://reviewboard.mozilla.org/r/206634/#review212394
Attachment #8935736 -
Flags: review+
|
|
||
Comment 63•7 years ago
|
||
| mozreview-review | ||
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)
|
|
||
Updated•7 years ago
|
Attachment #8935742 -
Flags: review?(drno)
|
|
||
Comment 64•7 years ago
|
||
| mozreview-review | ||
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 65•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935744 [details] Bug 1404997 - P11. More constness fixes. https://reviewboard.mozilla.org/r/206650/#review212406
Attachment #8935744 -
Flags: review?(apehrson) → review+
|
|
||
Comment 66•7 years ago
|
||
| mozreview-review-reply | ||
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 67•7 years ago
|
||
| mozreview-review | ||
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+
|
|
Assignee | |
Comment 68•7 years ago
|
||
| mozreview-review-reply | ||
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....
|
|
Assignee | |
Comment 69•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
||
Comment 70•7 years ago
|
||
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.
|
|
Assignee | |
Comment 71•7 years ago
|
||
| mozreview-review-reply | ||
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 72•7 years ago
|
||
| mozreview-review | ||
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+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 96•7 years ago
|
||
| mozreview-review | ||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 120•7 years ago
|
||
| mozreview-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 121•7 years ago
|
||
| mozreview-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 122•7 years ago
|
||
| mozreview-review | ||
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 123•7 years ago
|
||
| mozreview-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 124•7 years ago
|
||
| mozreview-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 125•7 years ago
|
||
| mozreview-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 126•7 years ago
|
||
| mozreview-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.
|
|
Assignee | |
Comment 127•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 128•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
Assignee | |
Comment 129•7 years ago
|
||
| mozreview-review-reply | ||
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
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8936190 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 152•7 years ago
|
||
| mozreview-review | ||
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+
|
|
Reporter | |
Comment 153•7 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 154•7 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 155•7 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 156•7 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 157•7 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 158•7 years ago
|
||
| mozreview-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+
|
|
Assignee | |
Comment 159•7 years ago
|
||
| mozreview-review-reply | ||
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.
|
|
||
Comment 160•7 years ago
|
||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 187•7 years ago
|
||
| mozreview-review | ||
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 188•7 years ago
|
||
| mozreview-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+
|
|
Assignee | |
Comment 189•7 years ago
|
||
| mozreview-review-reply | ||
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
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8935742 -
Flags: review?(gsquelart)
Attachment #8935742 -
Flags: review?(drno)
Attachment #8935742 -
Flags: review?(apehrson)
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8935742 -
Flags: review?(gsquelart) → review?(padenot)
|
|
Reporter | |
Comment 192•7 years ago
|
||
| mozreview-review | ||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 213•7 years ago
|
||
| mozreview-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 214•7 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 215•7 years ago
|
||
| mozreview-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+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8937140 -
Attachment is obsolete: true
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 269•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935742 [details] Bug 1404997 - P8. Follow coding style for members and methods. https://reviewboard.mozilla.org/r/206646/#review213042 For future coding style updates I would really prefer them to not contain code deletion/changes at the same time. And I would prefer them on separate bugs, so they don't block functional changes. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:86 (Diff revision 4) > - enum Direction > + enum DirectionType > { > TRANSMIT, > RECEIVE > }; > enum State Should this also be StateType?
Attachment #8935742 -
Flags: review+
|
|
Assignee | |
Comment 270•7 years ago
|
||
I'll split the bug, I had to change Direction into DirectionType as there was a conflict with a function of the same name. For consistency, I can also rename the State type, and make it an enum class while at it,
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8937290 -
Attachment is obsolete: true
Attachment #8937290 -
Flags: review?(padenot)
|
|
Assignee | |
Comment 292•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935743 [details] Bug 1404997 - P9. Remove unused member and accessor. https://reviewboard.mozilla.org/r/206648/#review213842 carrying on r+ from P8 comment
Attachment #8935743 -
Flags: review+
|
||
Comment 293•7 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a306e023eb1c P1. clang-format MediaPipeline.{cpp,h}. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/3f7963b69204 P2. Use AutoTaskQueue in VideoFrameConverter. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/844325b36ac8 P3. Rename some VideoFrameConverter members per coding style. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/749e749345cd P4. Make AudioProxyThread use AutoTaskQueue. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/40967b7fe56b P5. Fix constness and remove redundant virtual keyword. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/b299ba22a6a2 P6. Fix constness were applicable. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/e83401dbf349 P7. Simplify played time calculations. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/54fb95187c20 P8. Follow coding style for members and methods. r=drno,padenot https://hg.mozilla.org/integration/autoland/rev/df9c45f28536 P9. Remove unused member and accessor. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/3d3bfe24beae P11. More constness fixes. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/bafa97eff7a7 P12. Remove unused TrackAddedCallback class. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/169cd2016bab P13. Move ExtractPendingInput logic to SourceMediaStream. r=padenot https://hg.mozilla.org/integration/autoland/rev/f212c87ea003 P14. Rename members to clarify the finish meaning. r=padenot https://hg.mozilla.org/integration/autoland/rev/336263c26cd2 P15. Move MSG::FinishStream logic to MediaStream. r=padenot https://hg.mozilla.org/integration/autoland/rev/c67e47661a42 P16. Properly finish the SourceMediaStream during shutdown. r=padenot https://hg.mozilla.org/integration/autoland/rev/038600d1d2c8 P17. Split ExtractPendingInput into two methods. r=padenot https://hg.mozilla.org/integration/autoland/rev/737f227413b7 P18. Add Await convenience methods. r=gerald https://hg.mozilla.org/integration/autoland/rev/f9b7dc9f7e08 P19. Use new Await method with WebrtcMediaDataDecoder. r=gerald https://hg.mozilla.org/integration/autoland/rev/c27eaf506c73 P20. Make MediaStreamListener::NotifyPull asynchronous. r=padenot https://hg.mozilla.org/integration/autoland/rev/4b0c8c6ed4cc P21. Make MediaPipelineReceiveAudio listener asynchronous. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/09a1076a2a9f P22. Make mConduit release on main thread consistent. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/a695b9c49eda P23. Strongly enforced that our destination buffer is big enough. r=padenot https://hg.mozilla.org/integration/autoland/rev/32c31f7924af P24. Make AutoTaskQueue deliver runnables to nsIEventTarget. r=gerald https://hg.mozilla.org/integration/autoland/rev/5b0cffc026a4 P25. Add MSG and WebRTC MediaThreadType. r=gerald https://hg.mozilla.org/integration/autoland/rev/4ccc18cc509d P26. Give Await the threadpool to use. r=gerald https://hg.mozilla.org/integration/autoland/rev/2f1892facca9 P27. Name TaskQueue for debugging purposes. r=gerald
|
|
Assignee | |
Comment 294•7 years ago
|
||
hmmmm original "P10. Remove unnecessary const_cast. r?pehrsons" got squashed into P11 following a git rebase error...
|
||
Comment 295•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a306e023eb1c https://hg.mozilla.org/mozilla-central/rev/3f7963b69204 https://hg.mozilla.org/mozilla-central/rev/844325b36ac8 https://hg.mozilla.org/mozilla-central/rev/749e749345cd https://hg.mozilla.org/mozilla-central/rev/40967b7fe56b https://hg.mozilla.org/mozilla-central/rev/b299ba22a6a2 https://hg.mozilla.org/mozilla-central/rev/e83401dbf349 https://hg.mozilla.org/mozilla-central/rev/54fb95187c20 https://hg.mozilla.org/mozilla-central/rev/df9c45f28536 https://hg.mozilla.org/mozilla-central/rev/3d3bfe24beae https://hg.mozilla.org/mozilla-central/rev/bafa97eff7a7 https://hg.mozilla.org/mozilla-central/rev/169cd2016bab https://hg.mozilla.org/mozilla-central/rev/f212c87ea003 https://hg.mozilla.org/mozilla-central/rev/336263c26cd2 https://hg.mozilla.org/mozilla-central/rev/c67e47661a42 https://hg.mozilla.org/mozilla-central/rev/038600d1d2c8 https://hg.mozilla.org/mozilla-central/rev/737f227413b7 https://hg.mozilla.org/mozilla-central/rev/f9b7dc9f7e08 https://hg.mozilla.org/mozilla-central/rev/c27eaf506c73 https://hg.mozilla.org/mozilla-central/rev/4b0c8c6ed4cc https://hg.mozilla.org/mozilla-central/rev/09a1076a2a9f https://hg.mozilla.org/mozilla-central/rev/a695b9c49eda https://hg.mozilla.org/mozilla-central/rev/32c31f7924af https://hg.mozilla.org/mozilla-central/rev/5b0cffc026a4 https://hg.mozilla.org/mozilla-central/rev/4ccc18cc509d https://hg.mozilla.org/mozilla-central/rev/2f1892facca9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
|
|
||
Comment 296•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8935743 [details] Bug 1404997 - P9. Remove unused member and accessor. https://reviewboard.mozilla.org/r/206648/#review214084
Attachment #8935743 -
Flags: review?(drno) → review+
|
|
||
Updated•7 years ago
|
Flags: needinfo?(ajones)
You need to log in
before you can comment on or make changes to this bug.
Description
•