Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532433002:
Add overhead to audio bwe min, max. (Closed)
DescriptionAdd overhead to audio bwe min, max.
BUG=webrtc:6762
Review-Url: https://codereview.webrtc.org/2532433002
Cr-Commit-Position: refs/heads/master@{#16014}
Committed: https://chromium.googlesource.com/external/webrtc/+/6672b26d02e16c9190d146bd7f6bb63d73787ab5
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added a todo. #
Total comments: 3
Patch Set 3 : Respond to comment #Patch Set 4 : Respond to offline discussion #
Total comments: 4
Patch Set 5 : Respond to comments #
Total comments: 2
Patch Set 6 : Response to comments #
Total comments: 2
Patch Set 7 : Responde to comments #Patch Set 8 : Respond to offline discussion. #
Total comments: 4
Patch Set 9 : Response to comments. #
Messages
Total messages: 47 (18 generated)
Description was changed from ========== Add overhead to audio bwe min, max. BUG=webrtc:6762 ========== to ========== Add overhead to audio bwe min, max. BUG=webrtc:6762 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1427: // kMaxOverheadBps = OverheadPerPacket * 8 * 1000ms / frame_length(20ms) use 10ms I think due to https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvo...
Should add a unittest for this. https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1430: constexpr int kMinOverheadBps = 7733; This will be hard to maintain. can't we compute them instead based on https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvo...
https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1427: // kMaxOverheadBps = OverheadPerPacket * 8 * 1000ms / frame_length(20ms) I think we should not us 10ms frames since they are not used in ANA. Setting here 10ms as min frame length would lead to a higher max encoding rate which would be a waste of bandwidth.
https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1427: // kMaxOverheadBps = OverheadPerPacket * 8 * 1000ms / frame_length(20ms) On 2016/11/24 10:51:24, michaelt wrote: > I think we should not us 10ms frames since they are not used in ANA. Setting > here 10ms as min frame length would lead to a higher max encoding rate which > would be a waste of bandwidth. > > It does not sound convincing to me. Are we only defining a lower/upper bounds for the overall bitrate here. Or are you saying that the limit is a function of frame length? https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1430: constexpr int kMinOverheadBps = 7733; On 2016/11/24 10:48:27, stefan-webrtc (holmer) wrote: > This will be hard to maintain. can't we compute them instead based on > https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvo... It looks like that the problem is that the min frame length in webrtcvoiceengine.cc is one thing and the min frame length that can ever happen in audio network adaptation is another. But since we are defining lower/upper bound now, it is supposed to work if go choose global min and max (Opus min and max sounds good to me, too), unless we made error somewhere.
After some offline discussion's I came to the conclusion the it would be best to let the audio encoder decide which min or max frame bit rate he would prefer. This could even change during the call => changing frame length. As discussed the impl. of this idea will be done in another CL.
On 2016/12/01 14:06:19, michaelt wrote: > After some offline discussion's I came to the conclusion the it would be best to > let the audio encoder decide which min or max frame bit rate he would prefer. > This could even change during the call => changing frame length. As discussed > the impl. of this idea will be done in another CL. Hi Stefan, After some discussion with Michael, we plan to do the TODO he wrote in the last PS now/soon. So please do not spend any big time on this CL.
On 2016/12/06 08:09:35, minyue-webrtc wrote:
> On 2016/12/01 14:06:19, michaelt wrote:
> > After some offline discussion's I came to the conclusion the it would be
best
> to
> > let the audio encoder decide which min or max frame bit rate he would
prefer.
> > This could even change during the call => changing frame length. As
discussed
> > the impl. of this idea will be done in another CL.
>
> Hi Stefan,
>
> After some discussion with Michael, we plan to do the TODO he wrote in the
last
> PS now/soon. So please do not spend any big time on this CL.
Hi Stefan,
It looks like adding a feedback of min/max bitrate from audio encoder to the
bitrate allocator is a big change. So we would need to go for this CL first,
i.e., making a meaningful guess of min/max bitrate+overhead, in the world of a
changing frame length. I like the idea of
config_.min_bitrate_bps = kOpusMinBitrateBps + kMinOverheadBps;
config_.max_bitrate_bps = kOpusBitrateFbBps + kMaxOverheadBps;
which is in this CL.
WDYT?
On 2016/12/21 13:46:58, minyue-webrtc wrote: > On 2016/12/06 08:09:35, minyue-webrtc wrote: > > On 2016/12/01 14:06:19, michaelt wrote: > > > After some offline discussion's I came to the conclusion the it would be > best > > to > > > let the audio encoder decide which min or max frame bit rate he would > prefer. > > > This could even change during the call => changing frame length. As > discussed > > > the impl. of this idea will be done in another CL. > > > > Hi Stefan, > > > > After some discussion with Michael, we plan to do the TODO he wrote in the > last > > PS now/soon. So please do not spend any big time on this CL. > > Hi Stefan, > > It looks like adding a feedback of min/max bitrate from audio encoder to the > bitrate allocator is a big change. So we would need to go for this CL first, Could you make it more clear why this feedback is needed? The max doesn't necessarily have to be that strict. > i.e., making a meaningful guess of min/max bitrate+overhead, in the world of a > changing frame length. I like the idea of > config_.min_bitrate_bps = kOpusMinBitrateBps + kMinOverheadBps; > config_.max_bitrate_bps = kOpusBitrateFbBps + kMaxOverheadBps; > which is in this CL. > > WDYT? Looks reasonable to me.
https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1430: constexpr int kMinOverheadBps = 7733; On 2016/11/28 15:27:06, minyue-webrtc wrote: > On 2016/11/24 10:48:27, stefan-webrtc (holmer) wrote: > > This will be hard to maintain. can't we compute them instead based on > > > https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvo... > > It looks like that the problem is that the min frame length in > webrtcvoiceengine.cc is one thing and the min frame length that can ever happen > in audio network adaptation is another. > > But since we are defining lower/upper bound now, it is supposed to work if go > choose global min and max (Opus min and max sounds good to me, too), unless we > made error somewhere. I still don't really follow. How are they different? I think that must be made clear here, otherwise it will be hard for others to change this code. Can the adaptive configuration override the frame lengths configured here: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvo... ? Seems like we should be restricted by what frame lengths are negotiated?
This CL will just be a temporary solution. I solution i have in mind is to let the audio encoder (opus) decides his min/max bit rate by it self. Since the audio encoder is the only place where we have all the necessary information to make good decision about the min / max value for audio bwe.
On 2017/01/09 11:02:28, michaelt wrote: > This CL will just be a temporary solution. > I solution i have in mind is to let the audio encoder (opus) decides his min/max > bit rate by it self. > Since the audio encoder is the only place where we have all the necessary > information to make good decision about the min / max value for audio bwe. But isn't it reasonable to at least make it possible to configure an overall max/min, even though the encoder may have its own max/min internally (which should be within the overall max/min configured)?
Sounds ok to me. There is one disadvantage to this approach. We would then use 10ms for max BW. Which means we may allocate 24kbps to much for audio which will never be used. If you think that this is no problem. Then I will change this cl to your global audio BW approach.
https://codereview.webrtc.org/2532433002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1430: // kMaxOverheadBps = OverheadPerPacket * 8 * 1000ms / frame_length(20ms) nit: make the order min max throughout the block.
https://codereview.webrtc.org/2532433002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1430: // kMaxOverheadBps = OverheadPerPacket * 8 * 1000ms / frame_length(20ms) On 2017/01/09 15:03:55, the sun wrote: > nit: make the order > min > max > throughout the block. Done.
The CQ bit was checked by michaelt@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.
Update the CL after a offline discussion with stefan.
lgtm % comment https://codereview.webrtc.org/2532433002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:408: std::distance( Hm, isn't this easier to write with a for loop? Pretty hard to read... https://codereview.webrtc.org/2532433002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1458: std::max(*min_packet_size_ms, kOpusDefaultPTime); Add a comment about this max(), so that it's clear that this code was added because the encoder never uses frames shorter than 20 ms.
https://codereview.webrtc.org/2532433002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:408: std::distance( I use a loop now, but i'm not sure if its really better to read. What do you think ? https://codereview.webrtc.org/2532433002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1458: std::max(*min_packet_size_ms, kOpusDefaultPTime); On 2017/01/10 12:21:17, stefan-webrtc wrote: > Add a comment about this max(), so that it's clear that this code was added > because the encoder never uses frames shorter than 20 ms. Done.
https://codereview.webrtc.org/2532433002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:406: int packet_sizes_ms_size = [&]() { I'd simply write it like this: size_t num_packet_sizes = kMaxNumPacketSize; for (int index = 0; index < kMaxNumPacketSize; index++) { if (packet_size == 0) num_packet_sizes = index; } return rtc::ArrayView<const int>(kCodecPrefs[i].packet_sizes_ms, num_packet_sizes);
https://codereview.webrtc.org/2532433002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:406: int packet_sizes_ms_size = [&]() { On 2017/01/10 13:42:04, stefan-webrtc wrote: > I'd simply write it like this: > > size_t num_packet_sizes = kMaxNumPacketSize; > for (int index = 0; index < kMaxNumPacketSize; index++) { > if (packet_size == 0) > num_packet_sizes = index; > } > return rtc::ArrayView<const int>(kCodecPrefs[i].packet_sizes_ms, > num_packet_sizes); Done.
lgtm
https://codereview.webrtc.org/2532433002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1426: // bitrate he would prefer. he -> it
https://codereview.webrtc.org/2532433002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1457: // Opus will not use a shorter frame length than kOpusDefaultPTime. where can I prove this fact?
https://codereview.webrtc.org/2532433002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1457: // Opus will not use a shorter frame length than kOpusDefaultPTime. I rephrased the comment a bit. Since Opus without ANA could actually do 10ms frames.
The CQ bit was checked by michaelt@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 michaelt@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/...
Added change after offline discussion with minyue.
https://codereview.webrtc.org/2532433002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1445: if (packet_sizes_ms.size() > 0) { !packet_sizes_ms.empty() instead https://codereview.webrtc.org/2532433002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1452: IsCodec(config_.send_codec_spec.codec_inst, kOpusCodecName)) { I still think we should have a comment here. To someone who is new to this code this looks very strange, and it's hard to understand why the codec configuration is ignored.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2532433002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2532433002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1445: if (packet_sizes_ms.size() > 0) { On 2017/01/11 14:23:26, stefan-webrtc wrote: > !packet_sizes_ms.empty() instead Done. https://codereview.webrtc.org/2532433002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1452: IsCodec(config_.send_codec_spec.codec_inst, kOpusCodecName)) { On 2017/01/11 14:23:26, stefan-webrtc wrote: > I still think we should have a comment here. To someone who is new to this code > this looks very strange, and it's hard to understand why the codec configuration > is ignored. Done.
The CQ bit was checked by michaelt@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/2532433002/#ps160001 (title: "Response to comments.")
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": 1484154392018910,
"parent_rev": "e8084706c34161bbdb72040aa8e398ef86c17e5a", "commit_rev":
"6672b26d02e16c9190d146bd7f6bb63d73787ab5"}
Message was sent while issue was closed.
Description was changed from ========== Add overhead to audio bwe min, max. BUG=webrtc:6762 ========== to ========== Add overhead to audio bwe min, max. BUG=webrtc:6762 Review-Url: https://codereview.webrtc.org/2532433002 Cr-Commit-Position: refs/heads/master@{#16014} Committed: https://chromium.googlesource.com/external/webrtc/+/6672b26d02e16c9190d146bd7... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/6672b26d02e16c9190d146bd7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
