Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785713002:
Allow RtpPacket::SetPayloadSize to increase payload size (Closed)
DescriptionAllow RtpPacket::SetPayloadSize to increase payload size
Make SetPayloadSize return buffer to write to so that it can replace
AllocatePayload function.
BUG=None
Review-Url: https://codereview.webrtc.org/2785713002
Cr-Commit-Position: refs/heads/master@{#17450}
Committed: https://chromium.googlesource.com/external/webrtc/+/07a01b3357b220558310e85d460298bb6f5516b9
Patch Set 1 #
Total comments: 5
Patch Set 2 : Tweaked comment. #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== Allow RtpPacket::SetPayloadSize to increase payload Make SetPayloadSize return buffer to write to so that it can replace AllocatePayload function. BUG=None ========== to ========== Allow RtpPacket::SetPayloadSize to increase payload size Make SetPayloadSize return buffer to write to so that it can replace AllocatePayload function. BUG=None ==========
danilchap@webrtc.org changed reviewers: + nisse@webrtc.org
Inspired by Minyue (https://codereview.webrtc.org/2766323006/#msg13)
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
lgtm % comment https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:100: // Same as SetPayloadSize but doesn't guarantee to keep current payload. why not saying "but reset current payload"?
https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:100: // Same as SetPayloadSize but doesn't guarantee to keep current payload. On 2017/03/29 12:26:12, minyue-webrtc wrote: > why not saying "but reset current payload"? because it doesn't guarantee payload will be reset: it depends if CopyOnWriteBuffer used by the packet is shared or not. If it is shared, payload will be reset. If packet is the only owner - it will not be. AllocatePayload become not very usefull function because of this change and I'll consider removing it. There is one use case where I explicitly do not want to keep existent payload, but the optimization likely not worse keeping two functions that do almost the same.
https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:278: // reallocation and memcpy. Setting size to just headers reduces memcpy size. Last sentence of this comment is a bit confusing with the current code.
On 2017/03/29 12:38:38, danilchap wrote: > AllocatePayload become not very usefull function because of this change and I'll > consider removing it. > There is one use case where I explicitly do not want to keep existent payload, > but the optimization likely not worse keeping two functions that do almost the > same. If there's only one or a few places where you want AllocatePayload, you could add a SetPayloadSize(0) call there to avoid a useless copy operation.
https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:278: // reallocation and memcpy. Setting size to just headers reduces memcpy size. On 2017/03/29 12:47:07, nisse-webrtc wrote: > Last sentence of this comment is a bit confusing with the current code. Is new comment better?
On 2017/03/29 12:49:18, nisse-webrtc wrote: > On 2017/03/29 12:38:38, danilchap wrote: > > AllocatePayload become not very usefull function because of this change and > I'll > > consider removing it. > > There is one use case where I explicitly do not want to keep existent payload, > > but the optimization likely not worse keeping two functions that do almost the > > same. > > If there's only one or a few places where you want AllocatePayload, you could > add a SetPayloadSize(0) call there to avoid a useless copy operation. Prefer to postpone removing AllocatePayload to another CL (there are some non obvious decisions in that - do not want them to delay better SetPayloadSize function) SetPayloadSize(0) doesn't avoid useless copy operation, it only reduce size parameter in unavoidable memcpy.
https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/2785713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:278: // reallocation and memcpy. Setting size to just headers reduces memcpy size. On 2017/03/29 12:51:56, danilchap wrote: > On 2017/03/29 12:47:07, nisse-webrtc wrote: > > Last sentence of this comment is a bit confusing with the current code. > > Is new comment better? > It's good enough. lgtm.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2785713002/#ps20001 (title: "Tweaked comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm64_dbg/builds/4)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm64_dbg/builds/11) linux_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm64_rel/builds/13)
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": 20001, "attempt_start_ts": 1490797730199710,
"parent_rev": "e127e7a0edabaec02d34dd4a6b56bce54d7f5045", "commit_rev":
"07a01b3357b220558310e85d460298bb6f5516b9"}
Message was sent while issue was closed.
Description was changed from ========== Allow RtpPacket::SetPayloadSize to increase payload size Make SetPayloadSize return buffer to write to so that it can replace AllocatePayload function. BUG=None ========== to ========== Allow RtpPacket::SetPayloadSize to increase payload size Make SetPayloadSize return buffer to write to so that it can replace AllocatePayload function. BUG=None Review-Url: https://codereview.webrtc.org/2785713002 Cr-Commit-Position: refs/heads/master@{#17450} Committed: https://chromium.googlesource.com/external/webrtc/+/07a01b3357b220558310e85d4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/07a01b3357b220558310e85d4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
