Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972423002:
Fix video_replay tool to respect RTX stream and fix default parameters. (Closed)
DescriptionFix video_replay tool to respect RTX stream and fix default parameters.
Defaults are consistent with these used in CallTest.
BUG=none
Review-Url: https://codereview.webrtc.org/2972423002
Cr-Commit-Position: refs/heads/master@{#18961}
Committed: https://chromium.googlesource.com/external/webrtc/+/863f03ba38f397c078931d2bd27d40a4d60efc02
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use call test constants as defaults values #
Total comments: 9
Patch Set 3 : Fix types of moved constants #Patch Set 4 : Fix format #
Messages
Total messages: 33 (13 generated)
ilnik@webrtc.org changed reviewers: + philipel@webrtc.org, sprang@webrtc.org
nice! one comment though https://codereview.webrtc.org/2972423002/diff/1/webrtc/video/replay.cc File webrtc/video/replay.cc (right): https://codereview.webrtc.org/2972423002/diff/1/webrtc/video/replay.cc#newcode46 webrtc/video/replay.cc:46: DEFINE_int32(payload_type, 123, "Payload type"); The new default values, do they need to be literals? Otherwise it might be nice to reference the code from whence they came. Egg CallTest for ssrcs and video_quality_test.cc for the payload types.
https://codereview.webrtc.org/2972423002/diff/1/webrtc/video/replay.cc File webrtc/video/replay.cc (right): https://codereview.webrtc.org/2972423002/diff/1/webrtc/video/replay.cc#newcode46 webrtc/video/replay.cc:46: DEFINE_int32(payload_type, 123, "Payload type"); On 2017/07/10 16:00:48, sprang_webrtc wrote: > The new default values, do they need to be literals? > Otherwise it might be nice to reference the code from whence they came. Egg > CallTest for ssrcs and video_quality_test.cc for the payload types. Done.
lgtm
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:450: const int CallTest::kPayloadTypeH264 = 122; Codec payload types are dynamically negotiated, right?
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:450: const int CallTest::kPayloadTypeH264 = 122; On 2017/07/10 17:00:50, philipel wrote: > Codec payload types are dynamically negotiated, right? Not in the Call test. I think. I am certain, that full stack tests use these hardcoded values.
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:450: const int CallTest::kPayloadTypeH264 = 122; On 2017/07/10 17:06:19, ilnik wrote: > On 2017/07/10 17:00:50, philipel wrote: > > Codec payload types are dynamically negotiated, right? > > Not in the Call test. I think. I am certain, that full stack tests use these > hardcoded values. Exactly, these are the constants used in tests, for ease of use. In prod they are dynamically allocated (so in practice probably just hardcoded on some higher abstraction layer). Anyway, these are only defaults in the replay tool, rather than being like before set to 0 (which is disallowed).
lgtm
The CQ bit was checked by philipel@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 CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ilnik@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19104)
Description was changed from ========== Fix video_replay tool to respect RTX stream and fix default parameters. Defaults are consistent with these used in CallTest. BUG=none ========== to ========== Fix video_replay tool to respect RTX stream and fix default parameters. Defaults are consistent with these used in CallTest. BUG=none ==========
ilnik@webrtc.org changed reviewers: + tommi@webrtc.org
On 2017/07/11 08:01:54, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19104) +Tommi please approve test/call_test.cc and test/call_test.h Change is trivial - just added 3 constants (moved them here from the inherit test)
rs lgtm https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; out of curiosity - should these be enum values? alternatively, should they be int32 if a particular size is needed?
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; On 2017/07/11 08:34:47, tommi (wëbrtc) wrote: > out of curiosity - should these be enum values? alternatively, should they be > int32 if a particular size is needed? Actually, type really should be uint8_t. As for enum, I feel it's better like that, similarly to all the constants at lines 422-429.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, sprang@webrtc.org, tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2972423002/#ps60001 (title: "Fix format")
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": 1499762699966040,
"parent_rev": "d8cf08f1663c195cc4ed7ba60b3fa6cfec82e091", "commit_rev":
"863f03ba38f397c078931d2bd27d40a4d60efc02"}
Message was sent while issue was closed.
Description was changed from ========== Fix video_replay tool to respect RTX stream and fix default parameters. Defaults are consistent with these used in CallTest. BUG=none ========== to ========== Fix video_replay tool to respect RTX stream and fix default parameters. Defaults are consistent with these used in CallTest. BUG=none Review-Url: https://codereview.webrtc.org/2972423002 Cr-Commit-Position: refs/heads/master@{#18961} Committed: https://chromium.googlesource.com/external/webrtc/+/863f03ba38f397c078931d2bd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/863f03ba38f397c078931d2bd...
Message was sent while issue was closed.
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; On 2017/07/11 08:43:43, ilnik wrote: > On 2017/07/11 08:34:47, tommi (wëbrtc) wrote: > > out of curiosity - should these be enum values? alternatively, should they be > > int32 if a particular size is needed? > Actually, type really should be uint8_t. As for enum, I feel it's better like > that, similarly to all the constants at lines 422-429. Assuming they're all mutually exclusive, it would make sense to me to group them all together in a single enum like so: enum class PayloadType : uint8_t { ... H264 = 122, VP8 = 123, VP9 = 124, ... };
Message was sent while issue was closed.
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; On 2017/07/11 10:14:45, tommi (wëbrtc) wrote: > On 2017/07/11 08:43:43, ilnik wrote: > > On 2017/07/11 08:34:47, tommi (wëbrtc) wrote: > > > out of curiosity - should these be enum values? alternatively, should they > be > > > int32 if a particular size is needed? > > Actually, type really should be uint8_t. As for enum, I feel it's better like > > that, similarly to all the constants at lines 422-429. > > Assuming they're all mutually exclusive, it would make sense to me to group them > all together in a single enum like so: > > enum class PayloadType : uint8_t { > ... > H264 = 122, > VP8 = 123, > VP9 = 124, > ... > }; In fact, most of them are not mutually exclusive: E.g. kFlexfecPayloadType, kSendRtxPayloadType and kVideoSendPayloadType may be used in the same send config at the same time. These 3 moved are exclusive, but it will look worse if half of these constants would be in enums, and half would be plain constants.
Message was sent while issue was closed.
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; On 2017/07/11 10:20:59, ilnik wrote: > On 2017/07/11 10:14:45, tommi (wëbrtc) wrote: > > On 2017/07/11 08:43:43, ilnik wrote: > > > On 2017/07/11 08:34:47, tommi (wëbrtc) wrote: > > > > out of curiosity - should these be enum values? alternatively, should they > > be > > > > int32 if a particular size is needed? > > > Actually, type really should be uint8_t. As for enum, I feel it's better > like > > > that, similarly to all the constants at lines 422-429. > > > > Assuming they're all mutually exclusive, it would make sense to me to group > them > > all together in a single enum like so: > > > > enum class PayloadType : uint8_t { > > ... > > H264 = 122, > > VP8 = 123, > > VP9 = 124, > > ... > > }; > > In fact, most of them are not mutually exclusive: E.g. kFlexfecPayloadType, > kSendRtxPayloadType and kVideoSendPayloadType may be used in the same send > config at the same time. These 3 moved are exclusive, but it will look worse if > half of these constants would be in enums, and half would be plain constants. Oh I was thinking mutually exclusive values. Not usage. Seems to me that grouping payload types into an enum would be a good thing. Also making sure their value is limited to the underlying primitive type (8 bits instead of 32 or 64), would be good.
Message was sent while issue was closed.
https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; On 2017/07/11 11:41:36, tommi (wëbrtc) wrote: > On 2017/07/11 10:20:59, ilnik wrote: > > On 2017/07/11 10:14:45, tommi (wëbrtc) wrote: > > > On 2017/07/11 08:43:43, ilnik wrote: > > > > On 2017/07/11 08:34:47, tommi (wëbrtc) wrote: > > > > > out of curiosity - should these be enum values? alternatively, should > they > > > be > > > > > int32 if a particular size is needed? > > > > Actually, type really should be uint8_t. As for enum, I feel it's better > > like > > > > that, similarly to all the constants at lines 422-429. > > > > > > Assuming they're all mutually exclusive, it would make sense to me to group > > them > > > all together in a single enum like so: > > > > > > enum class PayloadType : uint8_t { > > > ... > > > H264 = 122, > > > VP8 = 123, > > > VP9 = 124, > > > ... > > > }; > > > > In fact, most of them are not mutually exclusive: E.g. kFlexfecPayloadType, > > kSendRtxPayloadType and kVideoSendPayloadType may be used in the same send > > config at the same time. These 3 moved are exclusive, but it will look worse > if > > half of these constants would be in enums, and half would be plain constants. > > Oh I was thinking mutually exclusive values. Not usage. Seems to me that > grouping payload types into an enum would be a good thing. Also making sure > their value is limited to the underlying primitive type (8 bits instead of 32 or > 64), would be good. Makes sense. I am making a tracking bug for that refactoring. Will do it when I have time.
Message was sent while issue was closed.
On 2017/07/11 12:20:30, ilnik wrote: > https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc > File webrtc/test/call_test.cc (right): > > https://codereview.webrtc.org/2972423002/diff/20001/webrtc/test/call_test.cc#... > webrtc/test/call_test.cc:452: const int CallTest::kPayloadTypeVP9 = 124; > On 2017/07/11 11:41:36, tommi (wëbrtc) wrote: > > On 2017/07/11 10:20:59, ilnik wrote: > > > On 2017/07/11 10:14:45, tommi (wëbrtc) wrote: > > > > On 2017/07/11 08:43:43, ilnik wrote: > > > > > On 2017/07/11 08:34:47, tommi (wëbrtc) wrote: > > > > > > out of curiosity - should these be enum values? alternatively, should > > they > > > > be > > > > > > int32 if a particular size is needed? > > > > > Actually, type really should be uint8_t. As for enum, I feel it's better > > > like > > > > > that, similarly to all the constants at lines 422-429. > > > > > > > > Assuming they're all mutually exclusive, it would make sense to me to > group > > > them > > > > all together in a single enum like so: > > > > > > > > enum class PayloadType : uint8_t { > > > > ... > > > > H264 = 122, > > > > VP8 = 123, > > > > VP9 = 124, > > > > ... > > > > }; > > > > > > In fact, most of them are not mutually exclusive: E.g. kFlexfecPayloadType, > > > kSendRtxPayloadType and kVideoSendPayloadType may be used in the same send > > > config at the same time. These 3 moved are exclusive, but it will look worse > > if > > > half of these constants would be in enums, and half would be plain > constants. > > > > Oh I was thinking mutually exclusive values. Not usage. Seems to me that > > grouping payload types into an enum would be a good thing. Also making sure > > their value is limited to the underlying primitive type (8 bits instead of 32 > or > > 64), would be good. > > Makes sense. I am making a tracking bug for that refactoring. Will do it when I > have time. thanks, there's no rush :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
