Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3014463002:
Break rtp_rtcp_format out of rtp_rtcp, to resolve circular dependencies (Closed)
DescriptionBreak rtp_rtcp_format out of rtp_rtcp, to resolve circular dependencies
BUG=webrtc:8111
patch from issue 3011233002 at patchset 1 (http://crrev.com/3011233002#ps1)
Review-Url: https://codereview.webrtc.org/3014463002
Cr-Commit-Position: refs/heads/master@{#19801}
Committed: https://chromium.googlesource.com/external/webrtc/+/09f4481173c9bf7a96e8e7a190cf685d7fea0178
Patch Set 1 #Patch Set 2 : add few more less trivial missing dependencies, and git cl format #Patch Set 3 : include stddef for size_t #
Total comments: 7
Patch Set 4 : copied a warning supressor #
Total comments: 2
Messages
Total messages: 21 (13 generated)
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/...
danilchap@webrtc.org changed reviewers: + eladalon@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/1...) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/29007)
If it works, lgtm. https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/B... File webrtc/modules/rtp_rtcp/BUILD.gn (right): https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/B... webrtc/modules/rtp_rtcp/BUILD.gn:16: "source/byte_io.h", Not in this CL, but I wonder if such a utility module belongs in rtp_rtcp_format? ByteReader, etc., don't sound to me RTP-specific.
https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/byte_io.h (right): https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/byte_io.h:39: #include <stddef.h> By the way, for my information, don't we prefer to include webrtc/typedefs.h? https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/byte_io.h:410: return (size + 3) & ~0x3; Is this the safest way to do this? 1. There are some implicit conversions in play, and I am not 100% comfortable with them. Given that size_t is always unsigned, perhaps we could take the maximum value of it and subtract three instead? You get the same value us ~0x3, but without any conversions. 2. Speaking of maximum values, what if (size+3) overflows?
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/3014463002/diff/40001/webrtc/modules/rtp_rtcp/B... File webrtc/modules/rtp_rtcp/BUILD.gn (right): https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/B... webrtc/modules/rtp_rtcp/BUILD.gn:16: "source/byte_io.h", On 2017/09/12 14:41:03, eladalon wrote: > Not in this CL, but I wonder if such a utility module belongs in > rtp_rtcp_format? ByteReader, etc., don't sound to me RTP-specific. probably should be merged with rtc_base/bytebuffer eventually. no, it is not rtp specific at all. https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/byte_io.h (right): https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/byte_io.h:39: #include <stddef.h> On 2017/09/12 14:45:07, eladalon wrote: > By the way, for my information, don't we prefer to include webrtc/typedefs.h? I've reverted this file alltogether, but generally I think we do not: typedefs.h defines macros like NO_RETURN/FALLTHROUGH etc. this file doesn't need those macros so it shouldn't include it instead of c header that it actually need (for uintX_t types) Can't find rule that explicitly direct one way or another. So following generic guidance 'include as little as you actually need' https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/byte_io.h:410: return (size + 3) & ~0x3; On 2017/09/12 14:45:07, eladalon wrote: > Is this the safest way to do this? > 1. There are some implicit conversions in play, and I am not 100% comfortable > with them. Given that size_t is always unsigned, perhaps we could take the > maximum value of it and subtract three instead? You get the same value us ~0x3, > but without any conversions. > 2. Speaking of maximum values, what if (size+3) overflows? 2. Moved inside rtp_header_extension_map where size is obviously way below maximum value. 1. Changed to safer + and % from treating size_t as bitmask.
https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/byte_io.h (right): https://codereview.webrtc.org/3014463002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/byte_io.h:39: #include <stddef.h> On 2017/09/12 15:17:18, danilchap wrote: > On 2017/09/12 14:45:07, eladalon wrote: > > By the way, for my information, don't we prefer to include webrtc/typedefs.h? > > I've reverted this file alltogether, > but generally I think we do not: > typedefs.h defines macros like NO_RETURN/FALLTHROUGH etc. > this file doesn't need those macros so it shouldn't include it instead of c > header that it actually need (for uintX_t types) > > Can't find rule that explicitly direct one way or another. So following generic > guidance 'include as little as you actually need' Acknowledged. https://codereview.webrtc.org/3014463002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extension_map.cc (right): https://codereview.webrtc.org/3014463002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension_map.cc:105: return size + 3 - (size + 3) % 4; I still think this would be safer, clearer and more efficient: return size & (std::numeric_limits<size_t>::max() - 3); 1. If I am not mistaken, bitwise-and will always be more efficient than modulo. 2. The left side of the binary-and operator is compile-time, not runtime, computable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/3014463002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extension_map.cc (right): https://codereview.webrtc.org/3014463002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension_map.cc:105: return size + 3 - (size + 3) % 4; On 2017/09/12 15:25:24, eladalon wrote: > I still think this would be safer, clearer and more efficient: > return size & (std::numeric_limits<size_t>::max() - 3); > 1. If I am not mistaken, bitwise-and will always be more efficient than modulo. > 2. The left side of the binary-and operator is compile-time, not runtime, > computable. For any reasonable use (there are up to 16 types of extension each of up to 16 bytes long, so size is always very small) I find both solutions equally safe. personally I find rounding with reminder clearer than rounding with bitwise operator (last one rely on knowledge how integers are layed out in memory, first one rely on a bit of math) bitwise is more efficient, but extra efficiency I fine negligible.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from eladalon@webrtc.org Link to the patchset: https://codereview.webrtc.org/3014463002/#ps60001 (title: "copied a warning supressor")
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": 60001, "attempt_start_ts": 1505233216339660,
"parent_rev": "3fe3e3b8fdc4539d4c17142dda74536244b52d3f", "commit_rev":
"09f4481173c9bf7a96e8e7a190cf685d7fea0178"}
Message was sent while issue was closed.
Description was changed from ========== Break rtp_rtcp_format out of rtp_rtcp, to resolve circular dependencies BUG=webrtc:8111 patch from issue 3011233002 at patchset 1 (http://crrev.com/3011233002#ps1) ========== to ========== Break rtp_rtcp_format out of rtp_rtcp, to resolve circular dependencies BUG=webrtc:8111 patch from issue 3011233002 at patchset 1 (http://crrev.com/3011233002#ps1) Review-Url: https://codereview.webrtc.org/3014463002 Cr-Commit-Position: refs/heads/master@{#19801} Committed: https://chromium.googlesource.com/external/webrtc/+/09f4481173c9bf7a96e8e7a19... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/09f4481173c9bf7a96e8e7a19... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
