Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669583002:
Added a flag to AudioCodecSpec to indicate adaptive bitrate support. (Closed)
|
Created:
3 years, 10 months ago by ossu Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded a flag to AudioCodecSpec to indicate adaptive bitrate support.
It's currently only used to ensure transport-cc is enabled for the format in question. It might be used to toggle more things in future.
BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2669583002
Cr-Commit-Position: refs/heads/master@{#16514}
Committed: https://chromium.googlesource.com/external/webrtc/+/9def800b7055de678bd5bf464f06194bb94ccd2a
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed strange extra space. #Patch Set 3 : Rebase #Patch Set 4 : Fixed CN and DTMF codecs not getting generated properly. #Patch Set 5 : int -> size_t to avoid unsigned/signed mismatch warnings on Windows. #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 36 (23 generated)
Description was changed from ========== Added a flag to AudioCodecSpec to indicate adaptive bitrate support. BUG=webrtc:5806 ========== to ========== Added a flag to AudioCodecSpec to indicate adaptive bitrate support. It's currently only used to ensure transport-cc is enabled for the format in question. It might be used to toggle more things in future. BUG=webrtc:5806 ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org, stefan@webrtc.org
stefan: although we didn't come to any massive conclusion in our meeting last week, it did at least give me a better understanding of transport-cc. This change lets the codec implementation choose if they want it or not, rather than filtering explicitly on Opus. PTAL that I didn't completely misunderstand it. :) kwiberg: PTAL at the actual implementation. I went with the simplest form we discussed. It seems fine to me!
Drive-by nit-picking. https://codereview.webrtc.org/2669583002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2669583002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:185: { "useinbandfec", "1" } What's going on with the spaces here?
https://codereview.webrtc.org/2669583002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2669583002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:185: { "useinbandfec", "1" } On 2017/01/31 15:46:33, hlundin-webrtc wrote: > What's going on with the spaces here? Very good question. I clang-formatted it but I must've confused it. Will fix!
lgtm
Thanks, kwiberg. Getting ready to land. ping stefan - Any comments on this one? It's mostly an FYI.
The CQ bit was checked by ossu@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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21010)
I think it looks good to me. I assume "network adaptation" is used because it includes both BWE and ANA? Or should we otherwise just call it "supports bitrate adaptation" or something like that?
On 2017/02/07 15:23:14, stefan-webrtc wrote: > I think it looks good to me. I assume "network adaptation" is used because it > includes both BWE and ANA? Or should we otherwise just call it "supports bitrate > adaptation" or something like that? I tried to use a slightly wider term to try to capture that it's not just bitrate, but other parameters as well. From the network side, it's packet loss etc. and for the encoder could include other parameters as well, like FEC, packet size and ... I guess the sky's the limit. :)
ok, lgtm
The Android bots picked up an error no other tests managed to catch. It turns out no CN or DTMF codecs were generated. I've fixed that and added a test to webrtcvoiceengine_unittests. kwiberg@: PTAL
The CQ bit was checked by ossu@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/18433)
The CQ bit was checked by ossu@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_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/22438)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by ossu@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.
lgtm https://codereview.webrtc.org/2669583002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2669583002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3735: return static_cast<int>(i); This is a test, so checked_cast? Not that it's likely to trigger, but still...
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2669583002/#ps100001 (title: "int -> size_t to avoid unsigned/signed mismatch warnings on Windows.")
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": 100001, "attempt_start_ts": 1486645912024080,
"parent_rev": "0289364abc38c2bf8299edb434529af23d44e9dc", "commit_rev":
"9def800b7055de678bd5bf464f06194bb94ccd2a"}
Message was sent while issue was closed.
Description was changed from ========== Added a flag to AudioCodecSpec to indicate adaptive bitrate support. It's currently only used to ensure transport-cc is enabled for the format in question. It might be used to toggle more things in future. BUG=webrtc:5806 ========== to ========== Added a flag to AudioCodecSpec to indicate adaptive bitrate support. It's currently only used to ensure transport-cc is enabled for the format in question. It might be used to toggle more things in future. BUG=webrtc:5806 Review-Url: https://codereview.webrtc.org/2669583002 Cr-Commit-Position: refs/heads/master@{#16514} Committed: https://chromium.googlesource.com/external/webrtc/+/9def800b7055de678bd5bf464... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/9def800b7055de678bd5bf464...
Message was sent while issue was closed.
I'll follow up with a tiny patch to fix these issues. https://codereview.webrtc.org/2669583002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2669583002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3727: // supplementary codecs are orderd after the general codecs. There's also a spelling error here. https://codereview.webrtc.org/2669583002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3735: return static_cast<int>(i); On 2017/02/09 05:26:44, kwiberg-webrtc wrote: > This is a test, so checked_cast? Not that it's likely to trigger, but still... Hmm, now that you mention it, it probably should be checked_cast here. Since the list iterated over is the one that gets generated by the code under test, it could be of any size. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
