Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088233004:
Add RTX codecs for codecs only supported by external encoder. (Closed)
|
Created:
4 years, 6 months ago by Taylor Brandstetter Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd RTX codecs for codecs only supported by external encoder.
Previously we were only adding these RTX codecs if the codec was
internally supported.
R=pbos@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/6c3e788dcfe694e382fee4d662ca3ea6c1d8c463
Cr-Commit-Position: refs/heads/master@{#13328}
Patch Set 1 #
Total comments: 6
Patch Set 2 : For now, only add RTX codecs for recognized codecs, and use same payload types as before. #
Total comments: 4
Patch Set 3 : Removing now-unused constant. #
Messages
Total messages: 21 (8 generated)
deadbeef@webrtc.org changed reviewers: + pbos@webrtc.org, pthatcher@webrtc.org
https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:630: // need to make RTX codecs associated by name instead. Can we at least leave the RTX codecs with an unset payload type since nothing refers to them? That would make it more clear what's going on (a kPayloadTypeUnset or rtc::optional). And should we just make all calls to CreateRtxCodec in this class always have such an unset payload type? Are they not used in those other areas either?
https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:630: // need to make RTX codecs associated by name instead. On 2016/06/23 21:26:30, pthatcher1 wrote: > Can we at least leave the RTX codecs with an unset payload type since nothing > refers to them? That would make it more clear what's going on (a > kPayloadTypeUnset or rtc::optional). > > And should we just make all calls to CreateRtxCodec in this class always have > such an unset payload type? Are they not used in those other areas either? I agree about using rtc::Optional. But I'd rather just do all the refactoring at once, rather than in stages (stage 1 being RTX codecs, stage 2 being everything else). And I'd rather do it in a separate CL from this one. And to answer your question "Are they not used in those other areas either?": They are definitely used.
lgtm
https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... File webrtc/media/base/mediaconstants.h (right): https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... webrtc/media/base/mediaconstants.h:136: extern const int kDefaultRtxVp8PlType; Shouldn't you remove these RTX payload types then? https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:644: VideoCodec::CreateRtxCodec(kDynamicPayloadTypeMin, payload_type)); Can we apply this for all "real" video codecs outside this function (regardless of internal/external)? All VideoCodec::CODEC_VIDEO should have a RTX codec created, right?
https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... File webrtc/media/base/mediaconstants.h (right): https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... webrtc/media/base/mediaconstants.h:136: extern const int kDefaultRtxVp8PlType; On 2016/06/26 21:55:58, pbos-webrtc wrote: > Shouldn't you remove these RTX payload types then? I will later, but I don't know how many people depend on specific payload types (they shouldn't, but it's possible). If I remove the defaults and something breaks and I need to revert, I don't want to have to revert this bug fix as well. https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:644: VideoCodec::CreateRtxCodec(kDynamicPayloadTypeMin, payload_type)); On 2016/06/26 21:55:58, pbos-webrtc wrote: > Can we apply this for all "real" video codecs outside this function (regardless > of internal/external)? All VideoCodec::CODEC_VIDEO should have a RTX codec > created, right? You mean, just create all of the RTX codecs here, instead of in DefaultVideoCodecList()? I'd like to, but again I'm worried about something breaking if I muck with existing payload types, so I planned to do it in a separate CL. In that case, maybe in this CL I should do "if H.264, use H.264 rtx payload type, if VP8 use VP8 RTX payload type" etc.?
On 2016/06/27 17:46:45, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... > File webrtc/media/base/mediaconstants.h (right): > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... > webrtc/media/base/mediaconstants.h:136: extern const int kDefaultRtxVp8PlType; > On 2016/06/26 21:55:58, pbos-webrtc wrote: > > Shouldn't you remove these RTX payload types then? > > I will later, but I don't know how many people depend on specific payload types > (they shouldn't, but it's possible). If I remove the defaults and something > breaks and I need to revert, I don't want to have to revert this bug fix as > well. > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2.cc:644: > VideoCodec::CreateRtxCodec(kDynamicPayloadTypeMin, payload_type)); > On 2016/06/26 21:55:58, pbos-webrtc wrote: > > Can we apply this for all "real" video codecs outside this function > (regardless > > of internal/external)? All VideoCodec::CODEC_VIDEO should have a RTX codec > > created, right? > > You mean, just create all of the RTX codecs here, instead of in > DefaultVideoCodecList()? I'd like to, but again I'm worried about something > breaking if I muck with existing payload types, so I planned to do it in a > separate CL. In that case, maybe in this CL I should do "if H.264, use H.264 rtx > payload type, if VP8 use VP8 RTX payload type" etc.? Yeah I think that's preferable. Then in the follow-up you can add the dynamic type instead regardless.
On 2016/06/27 17:48:10, pbos-webrtc wrote: > On 2016/06/27 17:46:45, Taylor Brandstetter wrote: > > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... > > File webrtc/media/base/mediaconstants.h (right): > > > > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/base/mediaconsta... > > webrtc/media/base/mediaconstants.h:136: extern const int kDefaultRtxVp8PlType; > > On 2016/06/26 21:55:58, pbos-webrtc wrote: > > > Shouldn't you remove these RTX payload types then? > > > > I will later, but I don't know how many people depend on specific payload > types > > (they shouldn't, but it's possible). If I remove the defaults and something > > breaks and I need to revert, I don't want to have to revert this bug fix as > > well. > > > > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... > > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > > > > https://codereview.webrtc.org/2088233004/diff/1/webrtc/media/engine/webrtcvid... > > webrtc/media/engine/webrtcvideoengine2.cc:644: > > VideoCodec::CreateRtxCodec(kDynamicPayloadTypeMin, payload_type)); > > On 2016/06/26 21:55:58, pbos-webrtc wrote: > > > Can we apply this for all "real" video codecs outside this function > > (regardless > > > of internal/external)? All VideoCodec::CODEC_VIDEO should have a RTX codec > > > created, right? > > > > You mean, just create all of the RTX codecs here, instead of in > > DefaultVideoCodecList()? I'd like to, but again I'm worried about something > > breaking if I muck with existing payload types, so I planned to do it in a > > separate CL. In that case, maybe in this CL I should do "if H.264, use H.264 > rtx > > payload type, if VP8 use VP8 RTX payload type" etc.? > > Yeah I think that's preferable. Then in the follow-up you can add the dynamic > type instead regardless. Done. Take a look now.
Thanks, lgtm https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/base/mediaco... File webrtc/media/base/mediaconstants.h (right): https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/base/mediaco... webrtc/media/base/mediaconstants.h:130: extern const int kDynamicPayloadTypeMin; This looks unused now. https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:349: void AddCodecAndMaybeRtxCodec(const VideoCodec& codec, Swap this argument order imo.
https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/base/mediaco... File webrtc/media/base/mediaconstants.h (right): https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/base/mediaco... webrtc/media/base/mediaconstants.h:130: extern const int kDynamicPayloadTypeMin; On 2016/06/28 12:33:32, pbos-webrtc wrote: > This looks unused now. Removed. https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2088233004/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:349: void AddCodecAndMaybeRtxCodec(const VideoCodec& codec, On 2016/06/28 12:33:32, pbos-webrtc wrote: > Swap this argument order imo. Our style guide says input parameters should go before outputs, so I'm just going by that.
Description was changed from ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. ========== to ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. Excluding these trybots because the devices are currently disconnected: CQ_EXCLUDE_TRYBOTS=master.tryserver.webrtc:android_arm64_rel;master.tryserver.webrtc:android_dbg ==========
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2088233004/#ps40001 (title: "Removing now-unused constant.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. Excluding these trybots because the devices are currently disconnected: CQ_EXCLUDE_TRYBOTS=master.tryserver.webrtc:android_arm64_rel;master.tryserver.webrtc:android_dbg ========== to ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14626)
Description was changed from ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. ========== to ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. R=pbos@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6c3e788dcfe694e382fee4d66... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 6c3e788dcfe694e382fee4d662ca3ea6c1d8c463 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. R=pbos@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6c3e788dcfe694e382fee4d66... ========== to ========== Add RTX codecs for codecs only supported by external encoder. Previously we were only adding these RTX codecs if the codec was internally supported. R=pbos@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/6c3e788dcfe694e382fee4d662ca3ea6c1d8c463 Cr-Commit-Position: refs/heads/master@{#13328} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
