Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493893003:
Turn off error resilience for vp8 for no temporal layers if nack is enabled. (Closed)
|
Created:
4 years, 1 month ago by åsapersson Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman, marpan2 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTurn off error resilience for vp8 for no temporal layers if nack is enabled.
BUG=webrtc:6634
Committed: https://crrev.com/5f7226f8a3d5a177fd113aea865a925364bfee3a
Cr-Commit-Position: refs/heads/master@{#15240}
Patch Set 1 #Patch Set 2 : remove processing test #Patch Set 3 #
Total comments: 6
Patch Set 4 : address comments #Patch Set 5 : check temporal layers per stream #Patch Set 6 : rebase #Patch Set 7 : update after rebase #
Total comments: 6
Patch Set 8 : address comments #
Messages
Total messages: 60 (48 generated)
The CQ bit was checked by asapersson@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: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16096)
The CQ bit was checked by asapersson@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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/3071)
Description was changed from ========== Turn off error resilience for vp8 for single stream and no temporal layers if nack is enabled. BUG=webrtc:6634 ========== to ========== Turn off error resilience for vp8 for single stream and no temporal layers if nack is enabled. BUG=webrtc:6634 ==========
asapersson@webrtc.org changed reviewers: + mflodman@webrtc.org, stefan@webrtc.org
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by asapersson@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_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by asapersson@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.
https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:84: if (nack_enabled && streams.size() == 1 && Why only for single stream?
marpan@google.com changed reviewers: + marpan@google.com
https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:84: if (nack_enabled && streams.size() == 1 && On 2016/11/15 08:48:47, stefan-webrtc (holmer) wrote: > Why only for single stream? No need for single stream condition, can also be used for simulcast/stream.size() > 1. https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:90: case kVideoCodecVP9: { Same can also be done with VP9 (for #spatial_layers=1, #temporal_layers=1), maybe that will done in separate CL?
Description was changed from ========== Turn off error resilience for vp8 for single stream and no temporal layers if nack is enabled. BUG=webrtc:6634 ========== to ========== Turn off error resilience for vp8 for no temporal layers if nack is enabled. BUG=webrtc:6634 ==========
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by asapersson@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12603) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12622) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14968) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14834)
The CQ bit was checked by asapersson@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.
https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:84: if (nack_enabled && streams.size() == 1 && On 2016/11/15 17:38:23, marpan wrote: > On 2016/11/15 08:48:47, stefan-webrtc (holmer) wrote: > > Why only for single stream? > > No need for single stream condition, can also be used for > simulcast/stream.size() > 1. Done. https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:84: if (nack_enabled && streams.size() == 1 && On 2016/11/15 08:48:47, stefan-webrtc (holmer) wrote: > Why only for single stream? Removed single stream check. https://codereview.webrtc.org/2493893003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:90: case kVideoCodecVP9: { On 2016/11/15 17:38:23, marpan wrote: > Same can also be done with VP9 (for #spatial_layers=1, #temporal_layers=1), > maybe that will done in separate CL? Will do the same for VP9 in a separate CL.
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
lgtm
One question, LGTM with this answered. https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... File webrtc/modules/video_coding/video_codec_initializer.cc (right): https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... webrtc/modules/video_coding/video_codec_initializer.cc:124: if (streams[i].temporal_layer_thresholds_bps.size() > 0) What if size() == 1?
https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... File webrtc/modules/video_coding/video_codec_initializer.cc (right): https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... webrtc/modules/video_coding/video_codec_initializer.cc:124: if (streams[i].temporal_layer_thresholds_bps.size() > 0) On 2016/11/25 10:46:59, mflodman wrote: > What if size() == 1? It is the bitrate threshold for additional layers. size() == 1 gives two temporal layers.
The CQ bit was checked by asapersson@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/...
lgtm % comments https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... File webrtc/modules/video_coding/video_codec_initializer.cc (right): https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... webrtc/modules/video_coding/video_codec_initializer.cc:123: for (size_t i = 0; i < streams.size(); ++i) { It'd be nice to change this loop to for (const VideoStream& stream : streams) { ... } https://codereview.webrtc.org/2493893003/diff/260001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2493893003/diff/260001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:88: for (size_t i = 0; i < streams.size(); ++i) { for (VideoStream* stream : streams) { ... }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... File webrtc/modules/video_coding/video_codec_initializer.cc (right): https://codereview.webrtc.org/2493893003/diff/260001/webrtc/modules/video_cod... webrtc/modules/video_coding/video_codec_initializer.cc:123: for (size_t i = 0; i < streams.size(); ++i) { On 2016/11/25 10:58:35, stefan-webrtc (holmer) wrote: > It'd be nice to change this loop to > for (const VideoStream& stream : streams) { > ... > } Done. https://codereview.webrtc.org/2493893003/diff/260001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2493893003/diff/260001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:88: for (size_t i = 0; i < streams.size(); ++i) { On 2016/11/25 10:58:35, stefan-webrtc (holmer) wrote: > for (VideoStream* stream : streams) { > ... > } Done.
The CQ bit was checked by asapersson@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 asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, mflodman@webrtc.org, marpan@google.com Link to the patchset: https://codereview.webrtc.org/2493893003/#ps280001 (title: "address 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": 280001, "attempt_start_ts": 1480077314955690,
"parent_rev": "e518878c767e4310d06f4e7d341e28cf77bc6afc", "commit_rev":
"d8e95fd092f4f6c8e9b5fd80ea279a1eb143705f"}
Message was sent while issue was closed.
Description was changed from ========== Turn off error resilience for vp8 for no temporal layers if nack is enabled. BUG=webrtc:6634 ========== to ========== Turn off error resilience for vp8 for no temporal layers if nack is enabled. BUG=webrtc:6634 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Turn off error resilience for vp8 for no temporal layers if nack is enabled. BUG=webrtc:6634 ========== to ========== Turn off error resilience for vp8 for no temporal layers if nack is enabled. BUG=webrtc:6634 Committed: https://crrev.com/5f7226f8a3d5a177fd113aea865a925364bfee3a Cr-Commit-Position: refs/heads/master@{#15240} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5f7226f8a3d5a177fd113aea865a925364bfee3a Cr-Commit-Position: refs/heads/master@{#15240} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
