Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524923002:
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry (Closed)
|
Created:
4 years ago by magjed_webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman, hta-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
Committed: https://crrev.com/b881254dc86d2cc80a52e08155433458be002166
Cr-Commit-Position: refs/heads/master@{#15232}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 28
Patch Set 3 : Address comments. #
Total comments: 10
Patch Set 4 : Address comments #2 #Patch Set 5 : Rebase #Patch Set 6 : Keep old interface public to allow external code to migrate. #Patch Set 7 : Keep old interface public to allow external code to migrate. #Messages
Total messages: 59 (47 generated)
The CQ bit was checked by magjed@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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10554)
The CQ bit was checked by magjed@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: Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...) android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18664)
The CQ bit was checked by magjed@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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10564)
Description was changed from
==========
Refactor RTP Registry more
BUG=None
==========
to
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
==========
to
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
==========
magjed@webrtc.org changed reviewers: + danilchap@webrtc.org
Please take a look. danilchap: Owner of webrtc/modules/rtp_rtcp hta: Mostly FYI, but review if you want
The CQ bit was checked by magjed@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: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18862)
https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (left): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:63: ~RTPPayloadRegistry(); restore destructor: it is default, but not trivial, so it's definition better be in the .cc file. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:118: std::map<int8_t, RtpUtility::Payload> payload_type_map_; bette use int for the payload type https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (left): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:219: if (rate == 0) { if (stored->Audio.rate == 0 && received->audio_codec.rate != 0) this expresion would be false but IsCompatible() will be true. Better check with someone familiar with audio if current change fix or introduce a bug. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:44: assert(audio_codec.plfreq >= 1000); RTC_DCHECK_GE(audio_codec.plfreq, 1000); (and #include "webrtc/base/checks.h" for that) https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:116: const uint32_t rate = std::max(0, audio_codec.rate); while you recalculate rate, you do not set it back to audio_codec, as a result, when calling PayloadIsCompatible, audio_codec.rate is used even if it is negative. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:30: static const uint32_t kTypicalFrequency = 44000; what is the reason for using fixed size integer type? (generally it is discouraged by style guide). https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:36: class RtpPayloadRegistryTest : public ::testing::Test { This fixture become so simple that in one CL it is possible to remove it completely. Nice! https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:63: EXPECT_EQ(false, retrieved_payload->audio); EXPECT_FALSE(retrieved_payload->audio); https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:88: EXPECT_EQ(true, retrieved_payload->audio); EXPECT_TRUE(retrieved_payload->audio) https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:105: rtp_payload_registry_.reset(new RTPPayloadRegistry()); without strategies look like the line can be removed completly https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.h (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.h:39: typedef std::map<int8_t, Payload*> PayloadTypeMap; likely can remove this typedef now https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.h:41: bool StringCompare(const char* str1, const char* str2, const uint32_t length); these two function are not specific to rtp. May be rather than adding missing function here, add it to /base/stringutils.h"
The CQ bit was checked by magjed@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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10602)
magjed@webrtc.org changed reviewers: + mflodman@webrtc.org, solenberg@webrtc.org
Please take a look. solenberg: webrtc/voice_engine/channel.cc (and the rest of the CL as well if you are interested). mflodman: webrtc/modules/video_coding/test/rtp_player.cc and webrtc/video/rtp_stream_receiver.cc. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (left): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:63: ~RTPPayloadRegistry(); On 2016/11/23 19:06:17, danilchap wrote: > restore destructor: it is default, but not trivial, so it's definition better be > in the .cc file. True, done. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:118: std::map<int8_t, RtpUtility::Payload> payload_type_map_; On 2016/11/23 19:06:17, danilchap wrote: > bette use int for the payload type Done. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (left): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:219: if (rate == 0) { On 2016/11/23 19:06:18, danilchap wrote: > if (stored->Audio.rate == 0 && received->audio_codec.rate != 0) > this expresion would be false but IsCompatible() will be true. > Better check with someone familiar with audio if current change fix or introduce > a bug. Yes, this is true, thanks for reviewing so carefully. I also noticed this discrepancy, but I assumed this was just old code, and that the real intent is to use the same IsCompatible() logic that is used in the other functions RegisterReceivePayload and DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType. I added solenberg as a reviewer and maybe he knows. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:44: assert(audio_codec.plfreq >= 1000); On 2016/11/23 19:06:18, danilchap wrote: > RTC_DCHECK_GE(audio_codec.plfreq, 1000); > (and #include "webrtc/base/checks.h" for that) Done. This logic is copy-pasted from line 427 if you wonder where it comes from. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:116: const uint32_t rate = std::max(0, audio_codec.rate); On 2016/11/23 19:06:18, danilchap wrote: > while you recalculate rate, you do not set it back to audio_codec, > as a result, when calling PayloadIsCompatible, audio_codec.rate is used even if > it is negative. Yes, that's why I recalculate the rate from audio_codec.rate in PayloadIsCompatible as well. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:30: static const uint32_t kTypicalFrequency = 44000; On 2016/11/23 19:06:18, danilchap wrote: > what is the reason for using fixed size integer type? > (generally it is discouraged by style guide). I had to change them in order to avoid compiler warnings/errors about comparing integers with different signs. They will be compared against members from this class: struct AudioPayload { uint32_t frequency; size_t channels; uint32_t rate; }; so I changed the types to match exactly. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:36: class RtpPayloadRegistryTest : public ::testing::Test { On 2016/11/23 19:06:18, danilchap wrote: > This fixture become so simple that in one CL it is possible to remove it > completely. Nice! Yeah, you're right. I went ahead and removed the fixture in this CL. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:63: EXPECT_EQ(false, retrieved_payload->audio); On 2016/11/23 19:06:18, danilchap wrote: > EXPECT_FALSE(retrieved_payload->audio); Done. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:88: EXPECT_EQ(true, retrieved_payload->audio); On 2016/11/23 19:06:18, danilchap wrote: > EXPECT_TRUE(retrieved_payload->audio) Done. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:105: rtp_payload_registry_.reset(new RTPPayloadRegistry()); On 2016/11/23 19:06:18, danilchap wrote: > without strategies look like the line can be removed completly Done. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.h (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.h:39: typedef std::map<int8_t, Payload*> PayloadTypeMap; On 2016/11/23 19:06:18, danilchap wrote: > likely can remove this typedef now I tried, but it was used in PossiblyRemoveExistingPayloadType in webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.h. I see now that PossiblyRemoveExistingPayloadType is a dead function and not used. I removed it. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.h:41: bool StringCompare(const char* str1, const char* str2, const uint32_t length); On 2016/11/23 19:06:18, danilchap wrote: > these two function are not specific to rtp. > May be rather than adding missing function here, add it to /base/stringutils.h" I asked tommi@ about this. He thinks it's best to just remove these helper functions and write '_stricmp(str1, str2) == 0' directly, i.e. no helper function for doing the '== 0' part. I will do that in a separate CL to not pollute this CL.
https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:116: const uint32_t rate = std::max(0, audio_codec.rate); On 2016/11/24 12:20:39, magjed_webrtc wrote: > On 2016/11/23 19:06:18, danilchap wrote: > > while you recalculate rate, you do not set it back to audio_codec, > > as a result, when calling PayloadIsCompatible, audio_codec.rate is used even > if > > it is negative. > > Yes, that's why I recalculate the rate from audio_codec.rate in > PayloadIsCompatible as well. oops, missed that, not a problem then. may be then clean rate only where it used since it is used once in this function. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.h (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.h:41: bool StringCompare(const char* str1, const char* str2, const uint32_t length); On 2016/11/24 12:20:39, magjed_webrtc wrote: > On 2016/11/23 19:06:18, danilchap wrote: > > these two function are not specific to rtp. > > May be rather than adding missing function here, add it to > /base/stringutils.h" > > I asked tommi@ about this. He thinks it's best to just remove these helper > functions and write '_stricmp(str1, str2) == 0' directly, i.e. no helper > function for doing the '== 0' part. I will do that in a separate CL to not > pollute this CL. using _stricmp directly instead of introducing a new function in this CL feel less polluting. (removing already existent StringCompare function agree better leave for another CL) https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right): https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:12: #include <algorithm> for std::max https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:79: } // anonymous namespace remove 'anonymous ': } // namespace https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:90: bool IsPayloadTypeValid(int8_t payload_type) { move this function into unnamed namespace too. https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:152: EXPECT_EQ(true, retrieved_payload->audio); EXPECT_TRUE https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:161: EXPECT_EQ(true, retrieved_payload->audio); EXPECT_TRUE
lgtm lgtm for voice_engine/channel.cc
The CQ bit was checked by magjed@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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10607)
https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:116: const uint32_t rate = std::max(0, audio_codec.rate); On 2016/11/24 13:24:36, danilchap wrote: > On 2016/11/24 12:20:39, magjed_webrtc wrote: > > On 2016/11/23 19:06:18, danilchap wrote: > > > while you recalculate rate, you do not set it back to audio_codec, > > > as a result, when calling PayloadIsCompatible, audio_codec.rate is used even > > if > > > it is negative. > > > > Yes, that's why I recalculate the rate from audio_codec.rate in > > PayloadIsCompatible as well. > > oops, missed that, not a problem then. > may be then clean rate only where it used since it is used once in this > function. Done. https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.h (right): https://codereview.webrtc.org/2524923002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.h:41: bool StringCompare(const char* str1, const char* str2, const uint32_t length); On 2016/11/24 13:24:36, danilchap wrote: > On 2016/11/24 12:20:39, magjed_webrtc wrote: > > On 2016/11/23 19:06:18, danilchap wrote: > > > these two function are not specific to rtp. > > > May be rather than adding missing function here, add it to > > /base/stringutils.h" > > > > I asked tommi@ about this. He thinks it's best to just remove these helper > > functions and write '_stricmp(str1, str2) == 0' directly, i.e. no helper > > function for doing the '== 0' part. I will do that in a separate CL to not > > pollute this CL. > > using _stricmp directly instead of introducing a new function in this CL feel > less polluting. > (removing already existent StringCompare function agree better leave for another > CL) True, done. https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right): https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:12: On 2016/11/24 13:24:36, danilchap wrote: > #include <algorithm> > for std::max Done. https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:79: } // anonymous namespace On 2016/11/24 13:24:36, danilchap wrote: > remove 'anonymous ': > } // namespace Done. https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:90: bool IsPayloadTypeValid(int8_t payload_type) { On 2016/11/24 13:24:36, danilchap wrote: > move this function into unnamed namespace too. Done. https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:152: EXPECT_EQ(true, retrieved_payload->audio); On 2016/11/24 13:24:36, danilchap wrote: > EXPECT_TRUE Done. https://codereview.webrtc.org/2524923002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:161: EXPECT_EQ(true, retrieved_payload->audio); On 2016/11/24 13:24:36, danilchap wrote: > EXPECT_TRUE Done.
lgtm!
The CQ bit was checked by magjed@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/...
LGTM for webrtc/video/rtp_stream_receiver.cc
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 magjed@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 magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, danilchap@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2524923002/#ps240001 (title: "Keep old interface public to allow external code to migrate.")
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": 240001, "attempt_start_ts": 1480012206507350,
"parent_rev": "c6b4977824909024b98718a61d8a75283188719f", "commit_rev":
"cb697d94b84bd2cbdb4f9638c136d6184a0268f1"}
Message was sent while issue was closed.
Description was changed from
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
==========
to
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
==========
to
==========
Remove RTPPayloadStrategy and simplify RTPPayloadRegistry
This CL removes RTPPayloadStrategy that is currently used to handle
audio/video specific aspects of payload handling. Instead, the audio and
video specific aspects will now have different functions, with linear
code flow.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
The main purpose with this CL is to add this function:
bool PayloadIsCompatible(const RtpUtility::Payload& payload,
const webrtc::VideoCodec& video_codec);
that can easily be extended in a future CL to look at video codec
specific information.
BUG=webrtc:6743
Committed: https://crrev.com/b881254dc86d2cc80a52e08155433458be002166
Cr-Commit-Position: refs/heads/master@{#15232}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b881254dc86d2cc80a52e08155433458be002166 Cr-Commit-Position: refs/heads/master@{#15232}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:240001) has been created in https://codereview.webrtc.org/2528993002/ by magjed@webrtc.org. The reason for reverting is: Breaks downstream projects.. |
