Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753253002:
Re-enable ULPFEC end-to-end test. (Closed)
DescriptionRe-enable ULPFEC end-to-end test.
- Reduced flakyness by switching to a packetization format that has
PictureID.
- Removed the explicit send-side BWE enabling from ULPFEC and FlexFEC
tests, since that is now on by default.
BUG=webrtc:7047
Review-Url: https://codereview.webrtc.org/2753253002
Cr-Commit-Position: refs/heads/master@{#17310}
Committed: https://chromium.googlesource.com/external/webrtc/+/c55f27afa4530501aa96dfee839e092723858935
Patch Set 1 #
Total comments: 3
Messages
Total messages: 19 (11 generated)
The CQ bit was checked by brandtr@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.
brandtr@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
I don't fully understand why this works, but I guess it has to do with the logic in the jitter buffer. I noticed that switching to VP8 helped when printf debugging this E2E test with the FAKE codec. Very few images were being rendered after packet losses, and it took a couple of seconds after a packet loss until a new image was rendered. I didn't check that it was a key frame, but assume so. Perhaps the old jitter buffer was more liberal in sending key frame requests?
Seems like good change. lgtm
https://codereview.webrtc.org/2753253002/diff/1/webrtc/video/end_to_end_tests.cc File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2753253002/diff/1/webrtc/video/end_to_end_tests... webrtc/video/end_to_end_tests.cc:644: // in the packetization headers. I can't see why picture id matters in the case of FEC?
brandtr@google.com changed reviewers: + brandtr@google.com
https://codereview.webrtc.org/2753253002/diff/1/webrtc/video/end_to_end_tests.cc File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2753253002/diff/1/webrtc/video/end_to_end_tests... webrtc/video/end_to_end_tests.cc:644: // in the packetization headers. On 2017/03/20 12:09:07, philipel wrote: > I can't see why picture id matters in the case of FEC? I believe it has something to do with key frame requests from the JB, which seem to be important for this particular test. When using the FAKE codec, a single lost packet leads to a very long delay (~ seconds) until the next frame is rendered. This doesn't seem to happen with VP8 packetization. The only difference I can think of is that the FAKE encoder does not have PictureIDs. For VP8, PictureID is enabled by default, right?
https://codereview.webrtc.org/2753253002/diff/1/webrtc/video/end_to_end_tests.cc File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2753253002/diff/1/webrtc/video/end_to_end_tests... webrtc/video/end_to_end_tests.cc:644: // in the packetization headers. On 2017/03/20 12:12:09, brandtr_google wrote: > On 2017/03/20 12:09:07, philipel wrote: > > I can't see why picture id matters in the case of FEC? > > I believe it has something to do with key frame requests from the JB, which seem > to be important for this particular test. When using the FAKE codec, a single > lost packet leads to a very long delay (~ seconds) until the next frame is > rendered. This doesn't seem to happen with VP8 packetization. The only > difference I can think of is that the FAKE encoder does not have PictureIDs. For > VP8, PictureID is enabled by default, right? I guess the main point is that switching to VP8 seems to make this test less flaky. Since this is just a correctness test, I haven't studied the reason for this in depth :)
Description was changed from ========== Re-enable ULPFEC end-to-end test. - Reduced flakyness by switching to a packetization format that has PictureID. - Removed the explicit send-side BWE enabling from ULPFEC and FlexFEC tests, since that is now on by default. BUG=webrtc:7047 ========== to ========== Re-enable ULPFEC end-to-end test. - Reduced flakyness by switching to a packetization format that has PictureID. - Removed the explicit send-side BWE enabling from ULPFEC and FlexFEC tests, since that is now on by default. BUG=webrtc:7047 ==========
brandtr@webrtc.org changed reviewers: - brandtr@google.com
LGTM after some offline discussion.
The CQ bit was checked by brandtr@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": 1, "attempt_start_ts": 1490015521457250, "parent_rev":
"ab980d0cb1bacab7c234a634b6a9a962c96db076", "commit_rev":
"c55f27afa4530501aa96dfee839e092723858935"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable ULPFEC end-to-end test. - Reduced flakyness by switching to a packetization format that has PictureID. - Removed the explicit send-side BWE enabling from ULPFEC and FlexFEC tests, since that is now on by default. BUG=webrtc:7047 ========== to ========== Re-enable ULPFEC end-to-end test. - Reduced flakyness by switching to a packetization format that has PictureID. - Removed the explicit send-side BWE enabling from ULPFEC and FlexFEC tests, since that is now on by default. BUG=webrtc:7047 Review-Url: https://codereview.webrtc.org/2753253002 Cr-Commit-Position: refs/heads/master@{#17310} Committed: https://chromium.googlesource.com/external/webrtc/+/c55f27afa4530501aa96dfee8... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/c55f27afa4530501aa96dfee8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
