Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2789773004:
Add functions to get/set rtp header extension by id. (Closed)
|
Created:
3 years, 8 months ago by danilchap Modified:
3 years, 8 months ago Reviewers:
nisse-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman, Dr Alex Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd functions to get/set rtp header extension by id.
This helps with dynamic size extensions.
BUG=webrtc:7433
Review-Url: https://codereview.webrtc.org/2789773004
Cr-Commit-Position: refs/heads/master@{#17505}
Committed: https://chromium.googlesource.com/external/webrtc/+/653063f6fd8b5406da6a6117c41470e0a47b1eb8
Patch Set 1 : . #
Total comments: 12
Patch Set 2 : Simplify rtp::Packet::AllocateExtension signature #Patch Set 3 : nits #
Messages
Total messages: 28 (20 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Add functions to get/set rtp header extension to rtp::Packet without registering them BUG=webrtc:7433 ========== to ========== Add functions to get/set rtp header extension by id. This allow to use header extension without registering them, in paricualar helps to use dynamic size extensions. BUG=webrtc:7433 ==========
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Add functions to get/set rtp header extension by id. This allow to use header extension without registering them, in paricualar helps to use dynamic size extensions. BUG=webrtc:7433 ========== to ========== Add functions to get/set rtp header extension by id. This helps using dynamic size extensions. BUG=webrtc:7433 ==========
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add functions to get/set rtp header extension by id. This helps using dynamic size extensions. BUG=webrtc:7433 ========== to ========== Add functions to get/set rtp header extension by id. This helps with dynamic size extensions. BUG=webrtc:7433 ==========
danilchap@webrtc.org changed reviewers: + nisse@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:34: constexpr size_t Packet::kMaxExtensionHeaders; Why is this needed? (And not part of this cl, but isn't the usual convention that upper limits like this is a limit on the size, i.e., maximum index value + 1?) https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:280: RTC_DCHECK_GT(id, 0); I think it's unusual and confusing with a exclusive lower bound and inclusive upper bound. I'd suggest RTC_DCHECK_GT(id, 1), or maybe even use a symbolic constant, similar to RtpExtension::kMinId. https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:557: bool Packet::AllocateExtension(ExtensionType type, For consistency, add an AllocateExtension method which returns an ArrayView, and deprecate this method. (TODO-item plus followup cl would be nice). https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.h:98: bool HasRawExtension(int id) const; |id| is the numerical value used on the wire (and associated with an uri via the extensions map)? Doesn't seem entirely obvious from context, so I think it deserves either a comment or a more verbose argument name. In other places, in particular, RtpHeaderExtensionMap, we use the uint8_t for the id. But RtpExtension uses an int. Can we get any consistency? https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.h:101: // Returns empty arrayview if extension is not present. The return convention makes sense, assuming that no valid extension can have zero length. Is ArrayView used in this way elsewhere in the code?
https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:34: constexpr size_t Packet::kMaxExtensionHeaders; On 2017/04/03 09:38:00, nisse-webrtc wrote: > Why is this needed? > In theory - to make program well-formed. In practice - to allow use of the constant in CHECK_EQ macros. https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#the-c... > (And not part of this cl, but isn't the usual convention that upper limits like > this is a limit on the size, i.e., maximum index value + 1?) 0 is not a valid id, extensions ids are enumerated from 1, so in this particular case max index and max number of supported extensions are same.... doesn't mean same constant should be used. https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:280: RTC_DCHECK_GT(id, 0); On 2017/04/03 09:38:00, nisse-webrtc wrote: > I think it's unusual and confusing with a exclusive lower bound and inclusive > upper bound. I'd suggest RTC_DCHECK_GT(id, 1), or maybe even use a symbolic > constant, similar to RtpExtension::kMinId. Good point. It is also bad idea to use kMaxExtensionHeader as kMaxExtensionId (following your other comment). Fixed. https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:557: bool Packet::AllocateExtension(ExtensionType type, On 2017/04/03 09:38:00, nisse-webrtc wrote: > For consistency, add an AllocateExtension method which returns an ArrayView, and > deprecate this method. (TODO-item plus followup cl would be nice). this is a private function, so can just replace it. Done. https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.h:98: bool HasRawExtension(int id) const; On 2017/04/03 09:38:00, nisse-webrtc wrote: > |id| is the numerical value used on the wire (and associated with an uri via the > extensions map)? Doesn't seem entirely obvious from context, so I think it > deserves either a comment or a more verbose argument name. Failed to find verbose enough argument name, so added a comment. > > In other places, in particular, RtpHeaderExtensionMap, we use the uint8_t for > the id. But RtpExtension uses an int. Can we get any consistency? Moving towards |int|, specially for parameters. uint8_t is not a good type for this value since it is not one-byte value on the wire (it takes half-byte). Style recommend int for this kind of generic case with run-time range checks. https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.h:101: // Returns empty arrayview if extension is not present. On 2017/04/03 09:38:00, nisse-webrtc wrote: > The return convention makes sense, assuming that no valid extension can have > zero length. Yes, extension is not allowed to have 0 length. > Is ArrayView used in this way elsewhere in the code? Not right now: I try to use ArrayView more often though. When return value can't be empty, then plan to use same convention, (i.e. treating empty arrayview as a nullptr pointer.)
lgtm https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:34: constexpr size_t Packet::kMaxExtensionHeaders; On 2017/04/03 10:57:00, danilchap wrote: > On 2017/04/03 09:38:00, nisse-webrtc wrote: > > Why is this needed? > > > > In theory - to make program well-formed. > In practice - to allow use of the constant in CHECK_EQ macros. The FAQ entry talks about static const, but here we have constexpr, which I thought was intended to make things more sane...
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:34: constexpr size_t Packet::kMaxExtensionHeaders; On 2017/04/03 11:21:19, nisse-webrtc wrote: > On 2017/04/03 10:57:00, danilchap wrote: > > On 2017/04/03 09:38:00, nisse-webrtc wrote: > > > Why is this needed? > > > > > > > In theory - to make program well-formed. > > In practice - to allow use of the constant in CHECK_EQ macros. > > The FAQ entry talks about static const, but here we have constexpr, which I > thought was intended to make things more sane... longer story: you can get away without this construct if you only use the constant by value, but if you try to take address of the variable, then it should be defined somewhere too. That what this line do. CHECK_EQ and similar macroses take parameters by const&, i.e. they need address of the variable.
On 2017/04/03 12:10:53, danilchap wrote: > https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): > > https://codereview.webrtc.org/2789773004/diff/100001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtp_packet.cc:34: constexpr size_t > Packet::kMaxExtensionHeaders; > On 2017/04/03 11:21:19, nisse-webrtc wrote: > > On 2017/04/03 10:57:00, danilchap wrote: > > > On 2017/04/03 09:38:00, nisse-webrtc wrote: > > > > Why is this needed? > > > > > > > > > > In theory - to make program well-formed. > > > In practice - to allow use of the constant in CHECK_EQ macros. > > > > The FAQ entry talks about static const, but here we have constexpr, which I > > thought was intended to make things more sane... > > longer story: > you can get away without this construct if you only use the constant by value, > but if you try to take address of the variable, then it should be defined > somewhere too. That what this line do. > > CHECK_EQ and similar macroses take parameters by const&, i.e. they need address > of the variable. I see. Thanks for the explanation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by danilchap@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491225189247170,
"parent_rev": "23425f9068aed6ba93d947f0ace17e55f2e1247d", "commit_rev":
"653063f6fd8b5406da6a6117c41470e0a47b1eb8"}
Message was sent while issue was closed.
Description was changed from ========== Add functions to get/set rtp header extension by id. This helps with dynamic size extensions. BUG=webrtc:7433 ========== to ========== Add functions to get/set rtp header extension by id. This helps with dynamic size extensions. BUG=webrtc:7433 Review-Url: https://codereview.webrtc.org/2789773004 Cr-Commit-Position: refs/heads/master@{#17505} Committed: https://chromium.googlesource.com/external/webrtc/+/653063f6fd8b5406da6a6117c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/653063f6fd8b5406da6a6117c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
