|
|
Created:
4 years ago by hta-webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman, hbos Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake ostream<< for enum class H264PacketizationMode
This makes it possible to use << and RTC_CHECK_EQ with this class.
BUG=none
Committed: https://crrev.com/ac382f3adcf64b31e594ec1c0ad427cfc031bf14
Cr-Commit-Position: refs/heads/master@{#15456}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added TODO and bug #Patch Set 3 : Swap RTC_CHECK_EQ arguments #
Total comments: 1
Patch Set 4 : Fix bug number #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== Make ostream<< for enum class H264PacketizationMode This makes it possible to use << and RTC_CHECK_EQ with this class. BUG=none ========== to ========== Make ostream<< for enum class H264PacketizationMode This makes it possible to use << and RTC_CHECK_EQ with this class. BUG=none ==========
hta@webrtc.org changed reviewers: + magjed@webrtc.org, sprang@webrtc.org
Review & CC to the people who commented on this in CL 2528343002.
hta@webrtc.org changed reviewers: + kwiberg@webrtc.org
Adding kwiberg for OWNER approval
https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:275: // .cc file it should belong to. Could you add a file-level TODO instead (and maybe even file a tracking bug), to add a source file for module_common_types? Looking around, there's an awful lot of inline code in this file, feels like it deserves a home of its own...
https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:275: // .cc file it should belong to. I'd really prefer to have this in a .cc file instead. This function has no business being inline. Now, webrtc/modules/include/module_common_types.h is #included by a large number of files, but it's not part of any GN build target. That's wrong, and it makes it impossible to move definitions such as this one out of the header. (And it looks like there are several other functions in this file that also should not be inline.) Ideally, I'd like you to put this function in a new .cc file, module_common_types.cc, and set up a build target for the new .h+.cc pair. I realize this is a bunch of cleanup work you may not have time for, though, so alternatively you could just post a bug saying that this should be done, and point to it from a comment in this file.
On 2016/12/06 11:40:19, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... > File webrtc/modules/include/module_common_types.h (right): > > https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... > webrtc/modules/include/module_common_types.h:275: // .cc file it should belong > to. > I'd really prefer to have this in a .cc file instead. This function has no > business being inline. > > Now, webrtc/modules/include/module_common_types.h is #included by a large number > of files, but it's not part of any GN build target. That's wrong, and it makes > it impossible to move definitions such as this one out of the header. (And it > looks like there are several other functions in this file that also should not > be inline.) > > Ideally, I'd like you to put this function in a new .cc file, > module_common_types.cc, and set up a build target for the new .h+.cc pair. I > realize this is a bunch of cleanup work you may not have time for, though, so > alternatively you could just post a bug saying that this should be done, and > point to it from a comment in this file. Checking: Do you think module_common_types.cc should live in webrtc/modules/source/ (a directory that currently doesn't exist)? Or somewhere else? There's a ton of stuff inline in this file that 1) shouldn't be inline, and 2) shoudldn't be in the common include file, since they're codec specific. Moving that around is also a reasonably-sized effort.
On 2016/12/06 12:01:01, hta-webrtc wrote: > On 2016/12/06 11:40:19, kwiberg-webrtc wrote: > > > https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... > > File webrtc/modules/include/module_common_types.h (right): > > > > > https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/include/module... > > webrtc/modules/include/module_common_types.h:275: // .cc file it should belong > > to. > > I'd really prefer to have this in a .cc file instead. This function has no > > business being inline. > > > > Now, webrtc/modules/include/module_common_types.h is #included by a large > number > > of files, but it's not part of any GN build target. That's wrong, and it makes > > it impossible to move definitions such as this one out of the header. (And it > > looks like there are several other functions in this file that also should not > > be inline.) > > > > Ideally, I'd like you to put this function in a new .cc file, > > module_common_types.cc, and set up a build target for the new .h+.cc pair. I > > realize this is a bunch of cleanup work you may not have time for, though, so > > alternatively you could just post a bug saying that this should be done, and > > point to it from a comment in this file. > > Checking: Do you think module_common_types.cc should live in > webrtc/modules/source/ (a directory that currently doesn't exist)? Or somewhere > else? In the same directory as the .h file, would be my preference. It's arguably "tidy" to have include/ directories with just .h files, but IMO it isn't worth the extra complexity (for WebRTC; other projects may be different). > There's a ton of stuff inline in this file that 1) shouldn't be inline, and 2) > shoudldn't be in the common include file, since they're codec specific. Moving > that around is also a reasonably-sized effort. Yes. So you may just want to do an inline method for now, and post a bug about (1) (de-inlining a bunch of stuff in this file) as I suggested. Posting a bug about (2) would probably be good to, but it has nothing to do with this CL.
lgtm https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2554003002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:272: RTC_CHECK_EQ(packetization_mode_, H264PacketizationMode::NonInterleaved); nit: You should swap the arguments, i.e. RTC_CHECK_EQ(H264PacketizationMode::NonInterleaved, packetization_mode_);
The CQ bit was checked by hta@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/...
The TODO for refactoring has been added, and the bug filed. Does this CL seem complete now?
lgtm, provided you point to the right bug https://codereview.webrtc.org/2554003002/diff/40001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2554003002/diff/40001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:276: // TODO(hta): Refactor. https://bugs.webrtc.org/6482 I'm pretty sure that's the wrong bug. "Replace Module ProcessThread in the RtcpModule", closed as duplicate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/06 22:16:10, kwiberg-webrtc wrote: > lgtm, provided you point to the right bug > > https://codereview.webrtc.org/2554003002/diff/40001/webrtc/modules/include/mo... > File webrtc/modules/include/module_common_types.h (right): > > https://codereview.webrtc.org/2554003002/diff/40001/webrtc/modules/include/mo... > webrtc/modules/include/module_common_types.h:276: // TODO(hta): Refactor. > https://bugs.webrtc.org/6482 > I'm pretty sure that's the wrong bug. "Replace Module ProcessThread in the > RtcpModule", closed as duplicate. Oops. Done.
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2554003002/#ps60001 (title: "Fix bug number")
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": 1481095237679950, "parent_rev": "e1ed457b7bd3e84aa34f5869143d6c3476c3067a", "commit_rev": "6121fbf925c61abefa6c26293f72e9be7e53a605"}
Message was sent while issue was closed.
Description was changed from ========== Make ostream<< for enum class H264PacketizationMode This makes it possible to use << and RTC_CHECK_EQ with this class. BUG=none ========== to ========== Make ostream<< for enum class H264PacketizationMode This makes it possible to use << and RTC_CHECK_EQ with this class. BUG=none ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make ostream<< for enum class H264PacketizationMode This makes it possible to use << and RTC_CHECK_EQ with this class. BUG=none ========== to ========== Make ostream<< for enum class H264PacketizationMode This makes it possible to use << and RTC_CHECK_EQ with this class. BUG=none Committed: https://crrev.com/ac382f3adcf64b31e594ec1c0ad427cfc031bf14 Cr-Commit-Position: refs/heads/master@{#15456} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ac382f3adcf64b31e594ec1c0ad427cfc031bf14 Cr-Commit-Position: refs/heads/master@{#15456}
Message was sent while issue was closed.
lgtm |