Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516213002:
RTPPayloadRegistry: Stop using the rate to keep track of receive codecs (Closed)
|
Created:
4 years, 1 month ago by kwiberg-webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, 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 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRTPPayloadRegistry: Stop using the rate to keep track of receive codecs
It's not used for anything.
BUG=webrtc:5805
Committed: https://crrev.com/68d3213313d4cac9fcb06bfd5ba9aaaf7862f005
Cr-Commit-Position: refs/heads/master@{#15438}
Patch Set 1 #Patch Set 2 : rewrite #
Messages
Total messages: 25 (12 generated)
The CQ bit was checked by kwiberg@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: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/8460)
kwiberg@webrtc.org changed reviewers: + asapersson@webrtc.org, ossu@webrtc.org
This seems to keep all the rate-related code in RTPPayloadRegistry, but removes any way of setting it to something other than zero. Will you remove that in a separate CL? The call to UpdatePayloadRate is now, for example, extra useless.
On 2016/11/21 13:10:25, ossu wrote: > This seems to keep all the rate-related code in RTPPayloadRegistry, but removes > any way of setting it to something other than zero. Will you remove that in a > separate CL? The call to UpdatePayloadRate is now, for example, extra useless. No, the only change I make to RTPPayloadRegistry is to add overloads of RegisterReceivePayload and ReceivePayloadType that don't take a rate argument at all, implicitly setting the rate to zero (which was already special-cased to mean "any rate"). But all the old methods remain, because API backwards compatibility. I do stop calling the methods that specify rate, though, and they should probably be removed. I've added it to my todo list.
On 2016/11/24 21:02:43, kwiberg-webrtc wrote: > On 2016/11/21 13:10:25, ossu wrote: > > This seems to keep all the rate-related code in RTPPayloadRegistry, but > removes > > any way of setting it to something other than zero. Will you remove that in a > > separate CL? The call to UpdatePayloadRate is now, for example, extra useless. > > No, the only change I make to RTPPayloadRegistry is to add overloads of > RegisterReceivePayload and ReceivePayloadType that don't take a rate argument at > all, implicitly setting the rate to zero (which was already special-cased to > mean "any rate"). But all the old methods remain, because API backwards > compatibility. > > I do stop calling the methods that specify rate, though, and they should > probably be removed. I've added it to my todo list. FYI Magnus just did another refactoring to RtpPayloadRegistry touching same method but in more radical way: https://codereview.webrtc.org/2523843002/
On 2016/11/25 08:48:46, danilchap wrote: > On 2016/11/24 21:02:43, kwiberg-webrtc wrote: > > On 2016/11/21 13:10:25, ossu wrote: > > > This seems to keep all the rate-related code in RTPPayloadRegistry, but > > removes > > > any way of setting it to something other than zero. Will you remove that in > a > > > separate CL? The call to UpdatePayloadRate is now, for example, extra > useless. > > > > No, the only change I make to RTPPayloadRegistry is to add overloads of > > RegisterReceivePayload and ReceivePayloadType that don't take a rate argument > at > > all, implicitly setting the rate to zero (which was already special-cased to > > mean "any rate"). But all the old methods remain, because API backwards > > compatibility. > > > > I do stop calling the methods that specify rate, though, and they should > > probably be removed. I've added it to my todo list. > > FYI > Magnus just did another refactoring to RtpPayloadRegistry touching same method > but in more radical way: > https://codereview.webrtc.org/2523843002/ Thanks for the heads-up. I think that CL obsoletes this one---or I'll have to rewrite it completely, depending on your point of view.
On 2016/11/25 10:32:26, kwiberg-webrtc wrote: > On 2016/11/25 08:48:46, danilchap wrote: > > On 2016/11/24 21:02:43, kwiberg-webrtc wrote: > > > On 2016/11/21 13:10:25, ossu wrote: > > > > This seems to keep all the rate-related code in RTPPayloadRegistry, but > > > removes > > > > any way of setting it to something other than zero. Will you remove that > in > > a > > > > separate CL? The call to UpdatePayloadRate is now, for example, extra > > useless. > > > > > > No, the only change I make to RTPPayloadRegistry is to add overloads of > > > RegisterReceivePayload and ReceivePayloadType that don't take a rate > argument > > at > > > all, implicitly setting the rate to zero (which was already special-cased to > > > mean "any rate"). But all the old methods remain, because API backwards > > > compatibility. > > > > > > I do stop calling the methods that specify rate, though, and they should > > > probably be removed. I've added it to my todo list. > > > > FYI > > Magnus just did another refactoring to RtpPayloadRegistry touching same method > > but in more radical way: > > https://codereview.webrtc.org/2523843002/ > > Thanks for the heads-up. I think that CL obsoletes this one---or I'll have to > rewrite it completely, depending on your point of view. It looks like it's at cross purposes with what we're currently trying to do: instead of getting away from using CodecInst, it adds it to one more place. :/ Could we ignore the problem for the time being by just putting zeroes in for rate when calling RtpPayloadRegistry?
On 2016/11/25 12:55:24, ossu wrote: > On 2016/11/25 10:32:26, kwiberg-webrtc wrote: > > On 2016/11/25 08:48:46, danilchap wrote: > > > On 2016/11/24 21:02:43, kwiberg-webrtc wrote: > > > > On 2016/11/21 13:10:25, ossu wrote: > > > > > This seems to keep all the rate-related code in RTPPayloadRegistry, but > > > > removes > > > > > any way of setting it to something other than zero. Will you remove that > > in > > > a > > > > > separate CL? The call to UpdatePayloadRate is now, for example, extra > > > useless. > > > > > > > > No, the only change I make to RTPPayloadRegistry is to add overloads of > > > > RegisterReceivePayload and ReceivePayloadType that don't take a rate > > argument > > > at > > > > all, implicitly setting the rate to zero (which was already special-cased > to > > > > mean "any rate"). But all the old methods remain, because API backwards > > > > compatibility. > > > > > > > > I do stop calling the methods that specify rate, though, and they should > > > > probably be removed. I've added it to my todo list. > > > > > > FYI > > > Magnus just did another refactoring to RtpPayloadRegistry touching same > method > > > but in more radical way: > > > https://codereview.webrtc.org/2523843002/ > > > > Thanks for the heads-up. I think that CL obsoletes this one---or I'll have to > > rewrite it completely, depending on your point of view. > > It looks like it's at cross purposes with what we're currently trying to do: > instead of getting away from using CodecInst, it adds it to one more place. :/ > Could we ignore the problem for the time being by just putting zeroes in for > rate when calling RtpPayloadRegistry? Yes, that's what I was thinking, and probably what I'll do first. But there's also the option of replacing CodecInst with something else, like SdpAudioFormat. That should be easier now that we can change the audio code without wrecking the video code.
On 2016/11/25 12:59:41, kwiberg-webrtc wrote: > On 2016/11/25 12:55:24, ossu wrote: > > On 2016/11/25 10:32:26, kwiberg-webrtc wrote: > > > On 2016/11/25 08:48:46, danilchap wrote: > > > > On 2016/11/24 21:02:43, kwiberg-webrtc wrote: > > > > > On 2016/11/21 13:10:25, ossu wrote: > > > > > > This seems to keep all the rate-related code in RTPPayloadRegistry, > but > > > > > removes > > > > > > any way of setting it to something other than zero. Will you remove > that > > > in > > > > a > > > > > > separate CL? The call to UpdatePayloadRate is now, for example, extra > > > > useless. > > > > > > > > > > No, the only change I make to RTPPayloadRegistry is to add overloads of > > > > > RegisterReceivePayload and ReceivePayloadType that don't take a rate > > > argument > > > > at > > > > > all, implicitly setting the rate to zero (which was already > special-cased > > to > > > > > mean "any rate"). But all the old methods remain, because API backwards > > > > > compatibility. > > > > > > > > > > I do stop calling the methods that specify rate, though, and they should > > > > > probably be removed. I've added it to my todo list. > > > > > > > > FYI > > > > Magnus just did another refactoring to RtpPayloadRegistry touching same > > method > > > > but in more radical way: > > > > https://codereview.webrtc.org/2523843002/ > > > > > > Thanks for the heads-up. I think that CL obsoletes this one---or I'll have > to > > > rewrite it completely, depending on your point of view. > > > > It looks like it's at cross purposes with what we're currently trying to do: > > instead of getting away from using CodecInst, it adds it to one more place. :/ > > Could we ignore the problem for the time being by just putting zeroes in for > > rate when calling RtpPayloadRegistry? > > Yes, that's what I was thinking, and probably what I'll do first. But there's > also the option of replacing CodecInst with something else, like SdpAudioFormat. > That should be easier now that we can change the audio code without wrecking the > video code. Ah, that is true. Then we can filter based on parameters as well, which might be useful (i.e. same codec but with different fmtp strings).
The CQ bit was checked by kwiberg@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.
Hello reviewers, I did the re-write. Please take another look. (The diff against the previous patch set is unusable because rebase and rewrite, so just look at the diff to base.)
Short and to the point. LGTM!
The CQ bit was checked by kwiberg@webrtc.org
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": 20001, "attempt_start_ts": 1481025037688420,
"parent_rev": "310692bfbac8aedea5347eee22fc795557c79984", "commit_rev":
"305ea3d20874a282865b7df6f884b4e20af72b4e"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== RTPPayloadRegistry: Stop using the rate to keep track of receive codecs It's not used for anything. BUG=webrtc:5805 ========== to ========== RTPPayloadRegistry: Stop using the rate to keep track of receive codecs It's not used for anything. BUG=webrtc:5805 Committed: https://crrev.com/68d3213313d4cac9fcb06bfd5ba9aaaf7862f005 Cr-Commit-Position: refs/heads/master@{#15438} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/68d3213313d4cac9fcb06bfd5ba9aaaf7862f005 Cr-Commit-Position: refs/heads/master@{#15438} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
