Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571463002:
Fix for video protection_bitrate in BWE with overhead. (Closed)
DescriptionFix for video protection_bitrate in BWE with overhead.
BUG=webrtc:6876, webrtc:6638, webrtc:6886
Review-Url: https://codereview.webrtc.org/2571463002
Cr-Commit-Position: refs/heads/master@{#16305}
Committed: https://chromium.googlesource.com/external/webrtc/+/192132ef04407fe0b8c7fe97ce60d46c9084d249
Patch Set 1 #
Total comments: 10
Patch Set 2 : Response to comments. #Patch Set 3 : Response to comments #
Total comments: 7
Patch Set 4 : Response to comments #
Total comments: 4
Patch Set 5 : Respond to comments #
Total comments: 6
Patch Set 6 : Respond to comments. #Patch Set 7 : Rebased #Patch Set 8 : Made unittest better and fixed a bug in the impl. #
Total comments: 6
Patch Set 9 : Add cast to fix windows build #
Total comments: 2
Patch Set 10 : Response to comments #
Messages
Total messages: 56 (33 generated)
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: 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/20860)
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.
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6638 ========== to ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6638 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
Here a Cl which fixes the problem with "video protection_bitrate" in BWE with overhead.
could you explain the problem in some details?
Description was changed from ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6638 ========== to ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6876, webrtc:6638 ==========
https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:280: size_t packet_size_byte, packet_size_bytes https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:281: size_t overhead_byte_per_packet) { overhead_bytes_per_packet or packet_overhead_bytes? https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1224: overhead_bytes_per_packet_); The overhead for FEC isn't the same as the overhead for media packets. Maybe it's not worth taking that into account here though...
https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1224: overhead_bytes_per_packet_); Do you think it's not worth to take the overhead into account or the differences in overhead between FEC and media packets?
https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1224: overhead_bytes_per_packet_); On 2016/12/13 08:07:09, michaelt wrote: > Do you think it's not worth to take the overhead into account or the differences > in overhead between FEC and media packets? The difference in overhead. Do you agree?
https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:280: size_t packet_size_byte, On 2016/12/12 16:19:55, stefan-webrtc (holmer) wrote: > packet_size_bytes Done. https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:281: size_t overhead_byte_per_packet) { On 2016/12/12 16:19:55, stefan-webrtc (holmer) wrote: > overhead_bytes_per_packet or packet_overhead_bytes? Done. https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1224: overhead_bytes_per_packet_); I don't know how the video FEC packets are build. If you think the difference is not big between FEC and media overhead, I'm OK with it.
https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1224: overhead_bytes_per_packet_); Instead of doing this, maybe we should do: protection_bitrate = bitrate_bps - CalculateOverheadRateBps(encoder_target_rate_bps_, ...); and remove line 1206?
https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1224: overhead_bytes_per_packet_); Did something similar to your idea pleas take a look.
https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:289: return overhead_bps > max_overhead_bps ? 0 : overhead_bps; Why do we have to return 0 if the overhead is greater than some max overhead? Seems like returning max_overhead_bps would make more sense in that case. https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1218: CalculateOverheadRateBps( Put this on its own line and assign to temp so that we this becomes more readable. Give it a good name, because currently I'm not sure what it is. https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1228: // Add overhead to the protection_bitrate Is this a todo?
https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:289: return overhead_bps > max_overhead_bps ? 0 : overhead_bps; True, that would make more sense :) https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1218: CalculateOverheadRateBps( On 2016/12/20 15:25:12, stefan-webrtc (holmer) wrote: > Put this on its own line and assign to temp so that we this becomes more > readable. Give it a good name, because currently I'm not sure what it is. Done. https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1228: // Add overhead to the protection_bitrate Not removed the comment.
https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1228: // Add overhead to the protection_bitrate Should be: No, I removed the comment. :)
lgtm https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1222: uint32_t protection_bitrate = So this is the protection bitrate including overhead? Maybe make that clear.
https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1222: uint32_t protection_bitrate = I'm not sure about this since it would just include overhead under the condition that we are in the "bwe with overhead" field trial. bitrate_bps may include overhead as well. And I did not marked it with "may include overhead". Do you think we should add a comment or change the names ?
https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1222: uint32_t protection_bitrate = On 2016/12/22 14:14:14, michaelt wrote: > I'm not sure about this since it would just include overhead under the condition > that we are in the "bwe with overhead" field trial. > bitrate_bps may include overhead as well. And I did not marked it with "may > include overhead". Do you think we should add a comment or change the names ? > Yes, please add a comment in that case.
https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1222: uint32_t protection_bitrate = On 2017/01/10 15:28:13, stefan-webrtc wrote: > On 2016/12/22 14:14:14, michaelt wrote: > > I'm not sure about this since it would just include overhead under the > condition > > that we are in the "bwe with overhead" field trial. > > bitrate_bps may include overhead as well. And I did not marked it with "may > > include overhead". Do you think we should add a comment or change the names ? > > > > Yes, please add a comment in that case. Done.
Hi Michael, consider my comments but do not wait for my further approval. https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:279: uint32_t CalculateOverheadRateBps(uint32_t bitrate_bps, not totally obvious to me that the bitrate_bps and packet_size_bytes can be either 1. both are with overhead or 2. both are without overhead I think we may better change this function to uint32_t CalculateOverheadRateBps(int packets_per_second, size_t overhead_bytes_per_packet, uint32_t max_overhead_bps) We may move "int packets_per_second = std::ceil(bitrate_bps / (8 * packet_size_bytes));" outside the function, (Note that my other comment suggests that the calculation may be wrong). You may move it to another utility function, e.g., CalculatePacketRate() https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:286: int packets_per_second = std::ceil(bitrate_bps / (8 * packet_size_bytes)); I don't ceil can do anything. it only makes sense when its argument is float. you may want to do integer operations like. (bitrate_bps + (8 * packet_size_bytes) - 1) / (8 * packet_size_bytes) if you do it, add a comment that this is to do the ceil. https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:289: return overhead_bps > max_overhead_bps ? max_overhead_bps : overhead_bps; I'd prefer std::min(xxx, xxx)
https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:279: uint32_t CalculateOverheadRateBps(uint32_t bitrate_bps, On 2017/01/10 22:17:31, minyue-webrtc(OOOtoJan16) wrote: > not totally obvious to me that the bitrate_bps and packet_size_bytes can be > either > 1. both are with overhead > or > 2. both are without overhead > > I think we may better change this function to > > uint32_t CalculateOverheadRateBps(int packets_per_second, size_t > overhead_bytes_per_packet, uint32_t max_overhead_bps) > > We may move "int packets_per_second = std::ceil(bitrate_bps / (8 * > packet_size_bytes));" outside the function, (Note that my other comment suggests > that the calculation may be wrong). You may move it to another utility function, > e.g., CalculatePacketRate() Done. https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:286: int packets_per_second = std::ceil(bitrate_bps / (8 * packet_size_bytes)); Good catch. Thanks. https://codereview.webrtc.org/2571463002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:289: return overhead_bps > max_overhead_bps ? max_overhead_bps : overhead_bps; On 2017/01/10 22:17:31, minyue-webrtc(OOOtoJan16) wrote: > I'd prefer std::min(xxx, xxx) 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: Try jobs failed on following builders: linux_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/17652) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12073)
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: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/8505)
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/...
While trying to make the unittest better i realized i had mad a mistake with the calculations. So in the last patch we should have a correct impl. and a better unittest (No 30sec wait time, and should not be flaky anymore).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5574)
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.
nice! and see some small comments. https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3217: // Wait for the first sent packet so that videosendstream know's knows And does this comment belong to line 3196 https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3231: bitrate_config.min_bitrate_bps = kMinBitrateBps; how did it work when no min was used. https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3248: bool first_packet_send_ GUARDED_BY(&crit_); send -> sent https://codereview.webrtc.org/2571463002/diff/180001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2571463002/diff/180001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3175: TEST_F(VideoSendStreamTest, RemoveOverheadFromBandwidth) { This solves 6886? Then put webrtc:6886 under BUG.
Description was changed from ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6876, webrtc:6638 ========== to ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6876, webrtc:6638, webrtc:6886 ==========
https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3217: // Wait for the first sent packet so that videosendstream know's Yeap, it would fit better to 3196. Moved it an fixed "knows". https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3231: bitrate_config.min_bitrate_bps = kMinBitrateBps; When i see this right, then default min was used in the case it not defined. But I think this way its cleaner. https://codereview.webrtc.org/2571463002/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3248: bool first_packet_send_ GUARDED_BY(&crit_); On 2017/01/26 09:28:50, minyue-webrtc wrote: > send -> sent Done. https://codereview.webrtc.org/2571463002/diff/180001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2571463002/diff/180001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:3175: TEST_F(VideoSendStreamTest, RemoveOverheadFromBandwidth) { At least i hope it does :) Done
lgtm
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/2571463002/#ps200001 (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": 200001, "attempt_start_ts": 1485445480613910,
"parent_rev": "04bd836ca1a2f3276b472b5a3cddfe7ffda49b88", "commit_rev":
"192132ef04407fe0b8c7fe97ce60d46c9084d249"}
Message was sent while issue was closed.
Description was changed from ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6876, webrtc:6638, webrtc:6886 ========== to ========== Fix for video protection_bitrate in BWE with overhead. BUG=webrtc:6876, webrtc:6638, webrtc:6886 Review-Url: https://codereview.webrtc.org/2571463002 Cr-Commit-Position: refs/heads/master@{#16305} Committed: https://chromium.googlesource.com/external/webrtc/+/192132ef04407fe0b8c7fe97c... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/192132ef04407fe0b8c7fe97c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
