Closed Bug 1343640 Opened 3 years ago Closed 3 years ago

Add hex dumping of RTP and RTCP to mtransport

Categories

(Core :: WebRTC: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(1 file)

We should have a special logging which dumps incoming and/or outgoing packets right before encryption/after decryption in hex format so it can be converted into PCAPs by text2pcap.
Should be fairly straight forward to add this to the media pipelines.
Actually we should probably dump RTCP completely in hex, but omit the RTP payload (or replace it with fake data) and only dump the real RTP header content.
Assignee: nobody → drno
the first... 10 bytes of the RTP payload would actually be useful so it includes things like the VP8 payload descriptor (https://tools.ietf.org/html/rfc7741#section-4.2).

This would allow identifying things like whether the frame is a keyframe. The use case is being able to answer "I sent a NACK, did I receive a keyframe?"
Fair enough. Although that looks more like 5 bytes to me (except if lots of the optional stuff is set, but I'm not going to parse the header to find out).
Comment on attachment 8843447 [details]
Bug 1343640: dump RT(C)P as raw hex into log files.

https://reviewboard.mozilla.org/r/117198/#review119614

I think this code should be simplified some.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1642
(Diff revision 1)
>  
>    // libsrtp enciphers in place, so we need a big enough buffer.
>    MOZ_ASSERT(data->capacity() >= data->len() + SRTP_MAX_EXPANSION);
>  
> +  webrtc::RtpHeaderParser *parser = nullptr;
> +  if (pipeline_) {

We bail earlier if this isn't set.

::: media/webrtc/signaling/src/mediapipeline/RtpLogger.cpp:24
(Diff revision 1)
> +void RtpLogger::LogPacket(const unsigned char *data, int len, bool input,
> +                          bool isRtp, int payloadOffset,
> +                          webrtc::RtpHeaderParser *rtpParser) {

I think we would have much simpler code if this took a header length, instead of an offset and an optional parser. This would let us put all of this offset calculation logic in one place (here).

::: media/webrtc/signaling/src/mediapipeline/RtpLogger.cpp:47
(Diff revision 1)
> +#else
> +    struct timeval tv;
> +    gettimeofday(&tv, NULL);
> +    ss << "." << (tv.tv_usec) << " ";
> +#endif
> +    ss << " 000000";

What is this for?

::: media/webrtc/signaling/src/mediapipeline/RtpLogger.cpp:50
(Diff revision 1)
> +    if (isRtp && payloadOffset == -1) {
> +      webrtc::RTPHeader header;
> +      if (!rtpParser || !rtpParser->Parse(data, len, &header)) {

Let's assume that if the caller has not specified a header size, we won't go to the trouble of trying to parse stuff.

::: media/webrtc/signaling/src/mediapipeline/RtpLogger.cpp:54
(Diff revision 1)
> +    int offset_ = payloadOffset;
> +    if (isRtp && payloadOffset == -1) {
> +      webrtc::RTPHeader header;
> +      if (!rtpParser || !rtpParser->Parse(data, len, &header)) {
> +          // use the minimum RTP header size as fallback
> +          offset_ = 12;

Everywhere else seems to be using this as if it is the position of the last byte we want in the clear. With a header of size 12, isn't that 11? Maybe it would be better to use the position of the first byte we want obscured (ie; the header size, plus some more if we think we can get some of the codec header)? I would have an easier time thinking about this if we did it that way, but that's just me.
Attachment #8843447 - Flags: review?(docfaraday) → review-
Comment on attachment 8843447 [details]
Bug 1343640: dump RT(C)P as raw hex into log files.

https://reviewboard.mozilla.org/r/117198/#review119614

> I think we would have much simpler code if this took a header length, instead of an offset and an optional parser. This would let us put all of this offset calculation logic in one place (here).

For outgoing packets we don't run the RTP parser. And I wanted to avoid running the RTP parser in case the RTP logging is not turned on.
While I agree that your proposal would simplify the API, I'm not sure how much overhead running the RTP parser all the time would create.

Maybe the right thing here is to add an API to check if RTP logging is turned on and then run parser depending on that.

> What is this for?

The whole output format is text2pcap compliant. And this is the line counter. But since lines appear to have no length limit it's just hard coded to always be zero.

> Let's assume that if the caller has not specified a header size, we won't go to the trouble of trying to parse stuff.

I'm not sure I like the idea to blindly assume something about the length. With more and more RTP header extensions getting used it will get pretty hard to guess right.
Comment on attachment 8843447 [details]
Bug 1343640: dump RT(C)P as raw hex into log files.

https://reviewboard.mozilla.org/r/117198/#review120556

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1641
(Diff revision 2)
> +  if (RtpLogger::IsPacketLoggingOn()) {
> +    int header_len = 12;
> +    webrtc::RTPHeader header;
> +    if (pipeline_->rtp_parser_ &&
> +        pipeline_->rtp_parser_->Parse(data->data(), data->len(), &header)) {
> +        // use the minimum RTP header size as fallback

This comment probably belongs where we assign 12.
Attachment #8843447 - Flags: review?(docfaraday) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/e51ffbba045f
dump RT(C)P as raw hex into log files. r=bwc
https://hg.mozilla.org/mozilla-central/rev/e51ffbba045f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.