Closed Bug 1377299 Opened 7 years ago Closed 7 years ago

Add [ChromeOnly] packet dump hooks to RTCPeerConnection

Categories

(Core :: WebRTC: Signaling, enhancement, P3)

Product:
Core
Component:
WebRTC: Signaling
Version:
54 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed
Blocking Flags:
backlog tech-debt

People

(Reporter: bwc, Assigned: bwc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1377299: Add packet dump hooks r+drno
59 bytes, text/x-review-board-request
smaug
: review+
drno
: review+
Details 
It would be very useful to be able to perform packet inspection in mochitest, to verify things like RTP/RTCP, DTLS handshake, ICE stuff, etc. It would also make it possible to log packet dumps automatically when a failure occurs, making debugging easier.
Summary: Add ChromeOnly packet dump hooks to RTCPeerConnection → Add [ChromeOnly] packet dump hooks to RTCPeerConnection
backlog: --- → tech-debt
Rank: 28
Priority: -- → P2
Assignee: nobody → docfaraday
Attachment #8882448 - Flags: review?(drno)
Attachment #8882448 - Flags: review?(bugs)
Comment on attachment 8882448 [details]
Bug 1377299: Add packet dump hooks r+drno

https://reviewboard.mozilla.org/r/153576/#review160960

r+ for .webidl which seems to be all chromeonly.
Attachment #8882448 - Flags: review?(bugs) → review+
Comment on attachment 8882448 [details]
Bug 1377299: Add packet dump hooks r+drno

https://reviewboard.mozilla.org/r/153576/#review161792

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:358
(Diff revision 5)
>      audio_stream_track_ = new FakeAudioStreamTrack();
>    }
>  
>    virtual void CreatePipelines_s(bool aIsRtcpMux) {
>  
> -    std::string test_pc("PC");
> +    std::string test_pc;

Out of curiosity: what is this changed needed for?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2321
(Diff revision 5)
>  
> +bool
> +PeerConnectionImpl::ShouldDumpPacket(size_t level, dom::mozPacketDumpType type,
> +                                     bool sending) const
> +{
> +  MutexAutoLock lock(mPacketDumpFlagsMutex);

I'm a little concerned that we are locking now twice on every send and received packet in here.
The default use case for 99% of our PeerConnection is going to never touch this and skip over. Thus I would feel better if we had some early exit criteria like a bool, which gets flipped on the very first call to EnablePacketDump().
Attachment #8882448 - Flags: review?(drno) → review+
Comment on attachment 8882448 [details]
Bug 1377299: Add packet dump hooks r+drno

https://reviewboard.mozilla.org/r/153576/#review161792

> Out of curiosity: what is this changed needed for?

PacketDumper (really, PeerConnectionWrapper) gets asserty when it can't find the PC. I'm using an empty handle to indicate there is no PC, and to disable the packet dump functionality.

> I'm a little concerned that we are locking now twice on every send and received packet in here.
> The default use case for 99% of our PeerConnection is going to never touch this and skip over. Thus I would feel better if we had some early exit criteria like a bool, which gets flipped on the very first call to EnablePacketDump().

Well, it is just one lock (since we do not dispatch if the first check fails), with no contention. Last I checked, that would not impose a detectable performance penalty for something like network packets. But an atomic bool would be a simple optimization.
Blocks: 1382855
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0fdec80a50c
Add packet dump hooks r+drno r=drno,smaug
https://hg.mozilla.org/mozilla-central/rev/e0fdec80a50c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: