New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 13 users
Status: Fixed
Owner:
Closed: Jul 20
Cc:
Components:
NextAction: ----
OS: ----
Pri: 2
Type: Enhancement

Blocking:
issue 5261



Sign in to add a comment
Refactor RTP header extensions.
Project Member Reported by stefan@webrtc.org, Jun 27 2013 Back to list
Currently the RTP header extensions in webrtc are implemented in a C-like way with a lot of switch statements and with strange methods for forcing values such as RTPSender::SetTransmissionTimeOffset and RTPSender::SetAbsoluteSendTime.

We should break this code out to its own set of classes.
 
Project Member Comment 1 by stefan@webrtc.org, Jun 27 2013
Cc: mflodman@webrtc.org pbos@webrtc.org
Project Member Comment 2 by kjellander@webrtc.org, Aug 28 2013
Labels: Dev-QAReview-NA
Project Member Comment 3 by solenberg@webrtc.org, Feb 7 2014
Cc: sprang@webrtc.org
This is related to the work sprang@ has started on, regarding RTPPacket class:

http://review.webrtc.org/2192004/

Comment 4 by vrk@webrtc.org, Oct 15 2014
Labels: Area-Network
Comment 5 by vrk@webrtc.org, Nov 3 2014
Labels: EngTriaged IceBox
Project Member Comment 6 by tnakamura@webrtc.org, Nov 4 2015
This bug hasn't been modified for more than a year. Is this still a valid open issue?
Project Member Comment 7 by stefan@webrtc.org, Nov 5 2015
Cc: solenberg@webrtc.org
Owner: danilchap@webrtc.org
Still valid and I think something that would be good to do. I think this will be done as part of danil's rtp class work.
Project Member Comment 8 by mflodman@webrtc.org, Nov 6 2015
Labels: -Area-Network -IceBox Area-Video Mstone-50
Project Member Comment 9 by sprang@webrtc.org, Nov 9 2015
Cc: danilchap@webrtc.org
Owner: sprang@webrtc.org
I'm cleaning up my old work on RtpPacket and aim to submit something this week, then hand over to danil again. Reassigning to me for now.
Project Member Comment 10 by mflodman@webrtc.org, Dec 10 2015
Erik,
Should this be kept open or merged with 5261?
Project Member Comment 11 by danilchap@webrtc.org, Feb 25 2016
Blocking: 5261
Cc: -danilchap@webrtc.org
Owner: danilchap@webrtc.org
Project Member Comment 12 by danilchap@webrtc.org, Mar 1 2016
Labels: -Mstone-50 Mstone-51
Project Member Comment 13 by danilchap@webrtc.org, Apr 6 2016
Labels: -Mstone-51 Mstone-52
Project Member Comment 14 by bugdroid1@chromium.org, Apr 20 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/1edb7ab7bd38318e039e99e0b1577de728e9a386

commit 1edb7ab7bd38318e039e99e0b1577de728e9a386
Author: danilchap <danilchap@webrtc.org>
Date: Wed Apr 20 12:25:10 2016

RtpPacket class introduced.

BUG= webrtc:1994 ,  webrtc:5261 

Review URL: https://codereview.webrtc.org/1841453004

Cr-Commit-Position: refs/heads/master@{#12444}

[modify] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/modules.gyp
[modify] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/BUILD.gn
[modify] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/rtp_rtcp.gypi
[modify] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc
[modify] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_packet.cc
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_packet.h
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_packet_received.h
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc
[modify] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/test/fuzzers/BUILD.gn
[add] https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386/webrtc/test/fuzzers/rtp_packet_fuzzer.cc

Project Member Comment 15 by danilchap@webrtc.org, Jul 20 2016
Labels: -Mstone-52
Project Member Comment 16 by bugdroid1@chromium.org, Aug 26 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/a897f26f1729699628426d47f6615f77f2399b94

commit a897f26f1729699628426d47f6615f77f2399b94
Author: danilchap <danilchap@webrtc.org>
Date: Fri Aug 26 12:42:41 2016

AbsoluteSendTime rtp header extension publish MsTo24Bit conversion
Since this conversion is used in multiple place and extension seems
right place to keep it in.

BUG= webrtc:1994 
NOTRY=true

Review-Url: https://codereview.webrtc.org/2272563010
Cr-Commit-Position: refs/heads/master@{#13934}

[modify] https://crrev.com/a897f26f1729699628426d47f6615f77f2399b94/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
[modify] https://crrev.com/a897f26f1729699628426d47f6615f77f2399b94/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h

Project Member Comment 18 by anatolid@webrtc.org, Oct 7 2016
What's the status of this? Is any more work planned on this issue?
Project Member Comment 19 by danilchap@webrtc.org, Oct 11 2016
I still have some CL in mind on this topic.
like few renames and reducing redundancy with constants in some root files.
Project Member Comment 20 by anatolid@webrtc.org, Oct 12 2016
Cc: anatolid@webrtc.org
Project Member Comment 21 by bugdroid1@chromium.org, Oct 25 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/cc348338098133849063f2cab8023946a5cf42cb

commit cc348338098133849063f2cab8023946a5cf42cb
Author: danilchap <danilchap@webrtc.org>
Date: Tue Oct 25 10:12:28 2016

Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565,  webrtc:1994 

Review-Url: https://codereview.webrtc.org/2431253003
Cr-Commit-Position: refs/heads/master@{#14763}

[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc
[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h
[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc
[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_sender.h
[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
[modify] https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc

Project Member Comment 22 by bugdroid1@chromium.org, Oct 28 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/e2a017725570ead5946a4ca8235af27470ca0df9

commit e2a017725570ead5946a4ca8235af27470ca0df9
Author: danilchap <danilchap@webrtc.org>
Date: Fri Oct 28 14:08:58 2016

Style cleanups in rtp header extension traits:
renamed kName to kUri and make it more const.
remove IsSupportedBy to reduce header dependency.

BUG= webrtc:1994 

Review-Url: https://codereview.webrtc.org/2457783005
Cr-Commit-Position: refs/heads/master@{#14825}

[modify] https://crrev.com/e2a017725570ead5946a4ca8235af27470ca0df9/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
[modify] https://crrev.com/e2a017725570ead5946a4ca8235af27470ca0df9/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h

Project Member Comment 23 by bugdroid1@chromium.org, Oct 31 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/42ca68ab7264154e16291c99f1de7338abc56af7

commit 42ca68ab7264154e16291c99f1de7338abc56af7
Author: danilchap <danilchap@webrtc.org>
Date: Mon Oct 31 10:34:40 2016

Ensure one does not register same rtp header extension with different id
Added assert to RtpHeaderExtensionMap
Altered tests that did.

BUG= webrtc:1994 

Review-Url: https://codereview.webrtc.org/2462663002
Cr-Commit-Position: refs/heads/master@{#14840}

[modify] https://crrev.com/42ca68ab7264154e16291c99f1de7338abc56af7/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc
[modify] https://crrev.com/42ca68ab7264154e16291c99f1de7338abc56af7/webrtc/video/video_send_stream_tests.cc

Project Member Comment 25 by bugdroid1@chromium.org, Nov 8 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6

commit bc38b4d450a80bf45efbe5b0a452e0374cdae1e6
Author: danilchap <danilchap@webrtc.org>
Date: Tue Nov 08 21:36:37 2016

Revert of Simplify and extend RtpHeaderExtensionMap (patchset #12 id:260001 of https://codereview.webrtc.org/2452293004/ )

Reason for revert:
breaks downstream project

Original issue's description:
> Simplify and extend RtpHeaderExtensionMap
> Add register functions for various codepaths.
> Add initialize-list constructor to create usable const RtpHeaderExtensionMap
> Optimize implementation for GetId/GetType.
>
> BUG= webrtc:1994 
>
> Committed: https://crrev.com/d1d26fbeb37a69471a34004c6ac2d3fafde5d404
> Cr-Commit-Position: refs/heads/master@{#14986}

TBR=sprang@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= webrtc:1994 

Review-Url: https://codereview.webrtc.org/2484863007
Cr-Commit-Position: refs/heads/master@{#14988}

[modify] https://crrev.com/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc
[modify] https://crrev.com/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h
[modify] https://crrev.com/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc
[modify] https://crrev.com/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
[modify] https://crrev.com/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h

Project Member Comment 26 by bugdroid1@chromium.org, Nov 9 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/6ba58d67ceb34882ffa6d731c9b29f4effb68f5d

commit 6ba58d67ceb34882ffa6d731c9b29f4effb68f5d
Author: danilchap <danilchap@webrtc.org>
Date: Wed Nov 09 13:46:39 2016

Reland of Simplify and extend RtpHeaderExtensionMap (patchset #1 id:1 of https://codereview.webrtc.org/2484863007/ )

Reason for revert:
dependent project adjusted

Original issue's description:
> Revert of Simplify and extend RtpHeaderExtensionMap (patchset #12 id:260001 of https://codereview.webrtc.org/2452293004/ )
>
> Reason for revert:
> breaks downstream project
>
> Original issue's description:
> > Simplify and extend RtpHeaderExtensionMap
> > Add register functions for various codepaths.
> > Add initialize-list constructor to create usable const RtpHeaderExtensionMap
> > Optimize implementation for GetId/GetType.
> >
> > BUG= webrtc:1994 
> >
> > Committed: https://crrev.com/d1d26fbeb37a69471a34004c6ac2d3fafde5d404
> > Cr-Commit-Position: refs/heads/master@{#14986}
>
> TBR=sprang@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= webrtc:1994 
>
> Committed: https://crrev.com/bc38b4d450a80bf45efbe5b0a452e0374cdae1e6
> Cr-Commit-Position: refs/heads/master@{#14988}

TBR=sprang@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= webrtc:1994 

Review-Url: https://codereview.webrtc.org/2492443002
Cr-Commit-Position: refs/heads/master@{#15000}

[modify] https://crrev.com/6ba58d67ceb34882ffa6d731c9b29f4effb68f5d/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc
[modify] https://crrev.com/6ba58d67ceb34882ffa6d731c9b29f4effb68f5d/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h
[modify] https://crrev.com/6ba58d67ceb34882ffa6d731c9b29f4effb68f5d/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc
[modify] https://crrev.com/6ba58d67ceb34882ffa6d731c9b29f4effb68f5d/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
[modify] https://crrev.com/6ba58d67ceb34882ffa6d731c9b29f4effb68f5d/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h

Project Member Comment 29 by bugdroid1@chromium.org, Apr 4 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/fab482b32684b65f2ad9c6c7a967a4802dacff82

commit fab482b32684b65f2ad9c6c7a967a4802dacff82
Author: danilchap <danilchap@webrtc.org>
Date: Tue Apr 04 09:33:48 2017

Simplify RTPSender::RegisterRtpHeaderExtension
DCHECK instead of run-time check extension type is correct.

BUG= webrtc:1994 

Review-Url: https://codereview.webrtc.org/2795723002
Cr-Commit-Position: refs/heads/master@{#17524}

[modify] https://crrev.com/fab482b32684b65f2ad9c6c7a967a4802dacff82/webrtc/modules/rtp_rtcp/source/rtp_sender.cc

Project Member Comment 30 by danilchap@webrtc.org, Apr 21 2017
Components: -Video Network>RTP
Project Member Comment 31 by anatolid@chromium.org, Apr 21 2017
Cc: -anatolid@webrtc.org
Project Member Comment 32 by danilchap@webrtc.org, Jul 20
Status: Fixed
Sign in to add a comment