Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2307693002:
Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime (Closed)
|
Created:
4 years, 3 months ago by skvlad Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, perkj_webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime
This test failed on the memcheck bot:
https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6704/steps/video_engine_tests/logs/stdio
The test assumed that the absolute send time header extension can never
be zero. It's a timestamp truncated to 24 bits, and zero is not a
special value - so it can very rarely end up being precisely zero.
The fix makes the test wait for at least one packet having a non-zero send time.
I've considered changing the test to use a fake clock instead to ensure
that not only the value is non-zero, but that it indeed reflects the
system timestamp - but that involves changing a very large number of
files. Besides, other tests in this file don't verify values for header
extensions where zeroes are allowed.
NOTRY=true
Committed: https://crrev.com/c3f3515f8ea78c0a8a5026639e605a9bdfda4add
Cr-Commit-Position: refs/heads/master@{#14056}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Keep sending packets until one has non-zero absSendTime. #Patch Set 3 : Log a warning when we get a zero absSendTime #
Total comments: 1
Patch Set 4 : "absoluteSendTime" #Patch Set 5 : formatting #Messages
Total messages: 34 (18 generated)
Description was changed from ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. BUG= ========== to ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix removes the "greater than zero" check from the test. We still check for the presence of the header extension. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. BUG= ==========
skvlad@webrtc.org changed reviewers: + pbos@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2307693002/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream_tests.cc (left): https://codereview.webrtc.org/2307693002/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream_tests.cc:139: EXPECT_GT(header.extension.absoluteSendTime, 0u); How about returning SEND_PACKET if absoluteSendTime is non-zero and wait for the next one to arrive instead (and log a warning, so that you can see why (through repeated logging) if the test times out)?
https://codereview.webrtc.org/2307693002/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream_tests.cc (left): https://codereview.webrtc.org/2307693002/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream_tests.cc:139: EXPECT_GT(header.extension.absoluteSendTime, 0u); On 2016/09/01 23:06:22, pbos-webrtc wrote: > How about returning SEND_PACKET if absoluteSendTime is non-zero and wait for the > next one to arrive instead (and log a warning, so that you can see why (through > repeated logging) if the test times out)? Sorry, return SEND_PACKET early if it's zero, not non-zero.
Description was changed from ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix removes the "greater than zero" check from the test. We still check for the presence of the header extension. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. BUG= ========== to ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. BUG= ==========
https://codereview.webrtc.org/2307693002/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream_tests.cc (left): https://codereview.webrtc.org/2307693002/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream_tests.cc:139: EXPECT_GT(header.extension.absoluteSendTime, 0u); On 2016/09/01 23:06:49, pbos-webrtc wrote: > On 2016/09/01 23:06:22, pbos-webrtc wrote: > > How about returning SEND_PACKET if absoluteSendTime is non-zero and wait for > the > > next one to arrive instead (and log a warning, so that you can see why > (through > > repeated logging) if the test times out)? > > Sorry, return SEND_PACKET early if it's zero, not non-zero. Done.
lgtm
The CQ bit was checked by skvlad@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2307693002/#ps40001 (title: "Log a warning when we get a zero absSendTime")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2307693002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2307693002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:146: LOG(LS_WARNING) << "Got a packet with zero absSendTime; waiting for" absoluteSendTime, waiting + formatting after that
The CQ bit was unchecked by skvlad@webrtc.org
The CQ bit was checked by skvlad@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2307693002/#ps60001 (title: ""absoluteSendTime"")
The CQ bit was unchecked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by skvlad@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2307693002/#ps80001 (title: "formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm again, thanks
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by skvlad@webrtc.org
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
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. BUG= ========== to ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. NOTRY=true ==========
The CQ bit was checked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. NOTRY=true ========== to ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. NOTRY=true ========== to ========== Fixed flaky VideoSendStreamTest::SupportsAbsoluteSendTime This test failed on the memcheck bot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/6... The test assumed that the absolute send time header extension can never be zero. It's a timestamp truncated to 24 bits, and zero is not a special value - so it can very rarely end up being precisely zero. The fix makes the test wait for at least one packet having a non-zero send time. I've considered changing the test to use a fake clock instead to ensure that not only the value is non-zero, but that it indeed reflects the system timestamp - but that involves changing a very large number of files. Besides, other tests in this file don't verify values for header extensions where zeroes are allowed. NOTRY=true Committed: https://crrev.com/c3f3515f8ea78c0a8a5026639e605a9bdfda4add Cr-Commit-Position: refs/heads/master@{#14056} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c3f3515f8ea78c0a8a5026639e605a9bdfda4add Cr-Commit-Position: refs/heads/master@{#14056} |
