Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523843002:
Send audio and video codecs to RTPPayloadRegistry (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, danilchap, yujie_mao (webrtc), zhuangzesen_agora.io, Andrew MacDonald, zhengzhonghou_agora.io, henrika_webrtc, stefan-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, the sun, mflodman, hta-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSend audio and video codecs to RTPPayloadRegistry
The purpose with this CL is to be able to send video codec specific
information down to RTPPayloadRegistry. We already do this for audio
with explicit arguments for e.g. number of channels. Instead of
extracting the arguments from webrtc::CodecInst (audio) and
webrtc::VideoCodec, this CL sends the types unmodified all the way down
to RTPPayloadRegistry.
This CL does not contain any functional changes, and is just a
preparation for future CL:s.
In the dependent CL https://codereview.webrtc.org/2524923002/,
RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled
audio/video specific aspects of payload handling. After this CL, we will
know if we get audio or video codecs without any dependency injection,
since we have different functions with different signatures for audio
vs video.
BUG=webrtc:6743
TBR=mflodman
Committed: https://crrev.com/56124bd158f371e39aeae3dcff176cbbad75dbcd
Cr-Commit-Position: refs/heads/master@{#15231}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments and add video test for RtpPayloadRegistry #
Total comments: 8
Patch Set 3 : Address more comments. #
Total comments: 1
Patch Set 4 : Keep old interface public to allow external code to migrate. #Patch Set 5 : Change strcpy to strncpy #Dependent Patchsets: Messages
Total messages: 68 (53 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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12725) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15095) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19016)
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_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10546)
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/10549)
Description was changed from ========== Send audio and video codecs down to RTPPayloadRegistry This CL is the first in a series to split RTPPayloadRegistry into different classes for audio and video. BUG=webrtc:6743 ========== to ========== Send audio and video codecs to RTPPayloadRegistry This CL is the first in a series to split RTPPayloadRegistry into different classes for audio and video. BUG=webrtc:6743 ==========
Description was changed from ========== Send audio and video codecs to RTPPayloadRegistry This CL is the first in a series to split RTPPayloadRegistry into different classes for audio and video. BUG=webrtc:6743 ========== to ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
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
https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:24: forward declare CodecInst/VideoCodec and include webrtc/common_types.h in the .cc file. [or explicitly include webrtc/common_types.h here] https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_receiver.h (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_receiver.h:20: same for CodecInst/VideoCodec declaration in this file https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:61: CreatePayloadType(testing::StrEq(kTypicalPayloadName), payload_type, probably nicer to add using ::testing::StrEq above, and use just StrEq here https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:69: CodecInst typical_audio_codec_; may be use constant outside of the fixture instead of member to stress it never change: const CodecInst kTypicalAudioCodec = {-1, "name", kTypicalFrequency, 0, kTypicalChannels, kTypicalRate}; or factory: CodecInst TypicalAudioCodecWithPayloadType(int payload_type) { CodecInst codec; codec.pltype = payload_type; ... return codec; } https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:281: audio_codec, &ignored)); // dummy values, except for payload_type May be fix comment as well: add dot in the end, start with uppercase. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:113: if (rtp_media_receiver_->OnNewPayloadTypeCreated( look like noone interested in new Video payload type created. Do you plan to remove this in follow up CL?
hta@webrtc.org changed reviewers: + hta@webrtc.org
lgtm Looks like a simplification all over the place. I like it. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:385: } // namespace webrtc There's a new function added that takes a VideoCodec. I don't see tests for that function here. (Today it's just a very small shim. Still, test that it continues to behave.)
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/10574)
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/10577)
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/...
Patchset #2 (id:60001) has been deleted
https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:24: On 2016/11/23 15:19:23, danilchap wrote: > forward declare CodecInst/VideoCodec and include webrtc/common_types.h in the > .cc file. > [or explicitly include webrtc/common_types.h here] Done. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_receiver.h (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_receiver.h:20: On 2016/11/23 15:19:23, danilchap wrote: > same for CodecInst/VideoCodec declaration in this file Done. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:61: CreatePayloadType(testing::StrEq(kTypicalPayloadName), payload_type, On 2016/11/23 15:19:23, danilchap wrote: > probably nicer to add using ::testing::StrEq above, and use just StrEq here Done. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:69: CodecInst typical_audio_codec_; On 2016/11/23 15:19:23, danilchap wrote: > may be use constant outside of the fixture instead of member to stress it never > change: > > const CodecInst kTypicalAudioCodec = {-1, "name", kTypicalFrequency, 0, > kTypicalChannels, kTypicalRate}; > or factory: > CodecInst TypicalAudioCodecWithPayloadType(int payload_type) { > CodecInst codec; > codec.pltype = payload_type; > ... > return codec; > } Thanks, much better. I tried to do this but used kTypicalPayloadName to initialize instead of duplicating "name" and then I got compile errors because the plname in CodecInst is a char array. But with your suggestion it works :) https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:281: audio_codec, &ignored)); // dummy values, except for payload_type On 2016/11/23 15:19:23, danilchap wrote: > May be fix comment as well: add dot in the end, start with uppercase. Done. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:385: } // namespace webrtc On 2016/11/23 16:08:24, hta-webrtc wrote: > There's a new function added that takes a VideoCodec. I don't see tests for that > function here. > > (Today it's just a very small shim. Still, test that it continues to behave.) Yeah, you are right. I added a default test for video similar to the existing RegistersAndRemembersPayloadsUntilDeregistered. I will add more tests once I add the video H264 profile specific logic into RtpPayloadRegistry. https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2523843002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:113: if (rtp_media_receiver_->OnNewPayloadTypeCreated( On 2016/11/23 15:19:23, danilchap wrote: > look like noone interested in new Video payload type created. > Do you plan to remove this in follow up CL? Thanks, you are right! I didn't notice when I wrote this. I removed this whole function now and updated the call sites to call the registry directly instead.
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/10580)
lgtm functions with 1-2 parameters are way easier to read than function with 6 parameters. thank you. https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_receiver.h (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_receiver.h:20: class VideoCodec; maybe put VideoCodec after RTPPayloadRegistry for alphabetical order. https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:303: const uint8_t payload_type = static_cast<uint8_t>(GetParam()); better to leave payload_type as int, should also be ok to use implicit conversion: audio_codec.pltype = GetParam(); (pltype may be not as descriptive as payload_type, but still give explicit name to the parameter) https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:48: // Ignore, this function will never be called. use RTC_NOTREACHABLE() instead of the comment to be more sure
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:70: int32_t RegisterReceivePayload(const VideoCodec& video_codec); While I do agree that this CL is an improvement over the previous state, I don't really like that media-specific types are seeping down the way it is here. What audio/video specific knowledge does RTPPayloadRegistry really need to have? Can we use a generic type in both cases and have that knowledge somewhere else?
solenberg@webrtc.org changed reviewers: - solenberg@webrtc.org
https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h:70: int32_t RegisterReceivePayload(const VideoCodec& video_codec); On 2016/11/23 19:29:35, the sun wrote: > While I do agree that this CL is an improvement over the previous state, I don't > really like that media-specific types are seeping down the way it is here. What > audio/video specific knowledge does RTPPayloadRegistry really need to have? The codec knowledge RTPPayloadRegistry needs to have is: Audio: payload name, frequency, channels, rate (same as the arguments we have now). Video: payload name + profile. We currently only have payload name for video. So when we register more than one H264 video codec (typically Constrained Baseline and Constrained High profile), and later call ReceivePayloadType to look up the payload type and only have "H264" as information, we will get the wrong payload type back. Audio codecs have the same ambiguity with payload names and I guess that's why we already pass in frequency, channels, and rate. I see adding video profile information as analogous to the audio specific information. > Can we use a generic type in both cases and have that knowledge somewhere else? Yeah, maybe. We already have this generic RtpUtility::Payload tagged union. Maybe we could send in RtpUtility::Payload as argument and have a function 'bool IsPayloadCompatible(const RtpUtility::Payload&, const RtpUtility::Payload&)'. I don't really like the tagged union approach though. The only reason we use it in RTPPayloadRegistry is because we are trying to be audio/video generic. In reality, RTPPayloadRegistry is either used for audio codecs only (from webrtc/voice_engine/channel.cc) or for video codecs only (from webrtc/video/rtp_stream_receiver.cc). That's why I think it would be better to subclass RTPPayloadRegistry; one for audio and one for video. The common code would be kept in the RTPPayloadRegistry base class and the audio/video specific logic would go in the subclasses. webrtc/voice_engine/channel.cc would use the audio subclass directly and webrtc/video/rtp_stream_receiver.cc would use the video subclass directly. That way we can get rid of RTPPayloadStrategy and the unions. Another approach would be to make RTPPayloadRegistry a template template<typename T> RTPPayloadRegistry; and require the existence of IsPayloadCompatible(const T&, const T&). That might be problematic, because there is some audio/video specific logic not related to the payload type, e.g. the DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType function. Regardless, I think this CL and the dependent CL https://codereview.webrtc.org/2524923002/ is an improvement over the existing code, and it will unblock H264 High profile in the near future. Maybe we can file an issue and discuss how RTPPayloadRegistry should be designed in the long run? https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_receiver.h (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_receiver.h:20: class VideoCodec; On 2016/11/23 17:12:50, danilchap wrote: > maybe put VideoCodec after RTPPayloadRegistry for alphabetical order. Done. https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc:303: const uint8_t payload_type = static_cast<uint8_t>(GetParam()); On 2016/11/23 17:12:50, danilchap wrote: > better to leave payload_type as int, > should also be ok to use implicit conversion: > audio_codec.pltype = GetParam(); > (pltype may be not as descriptive as payload_type, but still give explicit name > to the parameter) Done. https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://codereview.webrtc.org/2523843002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:48: // Ignore, this function will never be called. On 2016/11/23 17:12:50, danilchap wrote: > use RTC_NOTREACHABLE() instead of the comment to be more sure Done.
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/...
magjed@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman - Please review webrtc/video/rtp_stream_receiver.cc.
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/10597)
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
lgtm for voice_engine/channel.cc https://codereview.webrtc.org/2523843002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2523843002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1038: (rtp_receiver_->RegisterReceivePayload(codec) == -1)) { nice to get rid of this duplication
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/...
Patchset #4 (id:120001) has been deleted
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/10614)
Description was changed from ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 ========== to ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 TBR=mflodman ==========
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, danilchap@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2523843002/#ps140001 (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/...
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/10618)
LGTM for webrtc/video/rtp_stream_receiver.cc
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, hta@webrtc.org, danilchap@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2523843002/#ps160001 (title: "Change strcpy to strncpy")
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": 160001, "attempt_start_ts": 1480007990315840,
"parent_rev": "843f983235cfca5e13012c49f991f7da54e9e560", "commit_rev":
"c6b4977824909024b98718a61d8a75283188719f"}
Message was sent while issue was closed.
Description was changed from ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 TBR=mflodman ========== to ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 TBR=mflodman ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 TBR=mflodman ========== to ========== Send audio and video codecs to RTPPayloadRegistry The purpose with this CL is to be able to send video codec specific information down to RTPPayloadRegistry. We already do this for audio with explicit arguments for e.g. number of channels. Instead of extracting the arguments from webrtc::CodecInst (audio) and webrtc::VideoCodec, this CL sends the types unmodified all the way down to RTPPayloadRegistry. This CL does not contain any functional changes, and is just a preparation for future CL:s. In the dependent CL https://codereview.webrtc.org/2524923002/, RTPPayloadStrategy is removed. RTPPayloadStrategy previously handled audio/video specific aspects of payload handling. After this CL, we will know if we get audio or video codecs without any dependency injection, since we have different functions with different signatures for audio vs video. BUG=webrtc:6743 TBR=mflodman Committed: https://crrev.com/56124bd158f371e39aeae3dcff176cbbad75dbcd Cr-Commit-Position: refs/heads/master@{#15231} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/56124bd158f371e39aeae3dcff176cbbad75dbcd Cr-Commit-Position: refs/heads/master@{#15231} |
