Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by skvlad Modified:
4 years, 2 months ago Reviewers:
stefan-webrtc, mflodman, perkj_webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, tterriberry_mozilla.com, perkj_webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixed flaky VideoSendStreamTests after ViEEncoder changes
After https://codereview.webrtc.org/2386573002 changed where resolution
changes are handled, a few VideoSendStreamTests became flaky. They were
waiting for an InitEncode call they triggered, but sometimes were
getting one triggered by the resolution change when the first frame was
generated.
The fix was to make the tests wait for two InitEncode calls first -
one when the stream is created, and the second when the first frame was
encoded.
BUG=webrtc:6467, webrtc:6371
R=perkj@webrtc.org, stefan@webrtc.org
Committed: https://crrev.com/cf33d9c9d3fb57cca28861845d673934993e83df
Cr-Commit-Position: refs/heads/master@{#14490}
Patch Set 1 #Patch Set 2 : Fixed another test #Patch Set 3 : Wait inside InitEncode to avoid data races. #
Created: 4 years, 2 months ago
Messages
Total messages: 14 (7 generated)
Description was changed from ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=None ========== to ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=None ==========
skvlad@webrtc.org changed reviewers: + mflodman@webrtc.org, perkj@webrtc.org, stefan@webrtc.org
PTAL. The MSan and DrMemory bots have been recently flaky because of this: https://build.chromium.org/p/client.webrtc/builders/Linux%20MSan
Description was changed from ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=None ========== to ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467 ==========
On 2016/10/04 01:06:54, skvlad wrote: > PTAL. The MSan and DrMemory bots have been recently flaky because of this: > https://build.chromium.org/p/client.webrtc/builders/Linux%20MSan Thanks. I added BUG=webrtc:6467 for this
Per, are we now encoding a frame in QVGA resolution in the beginning that we didn't do before? Is that expected and something we really want?
lgtm thanks for fixing. Stefan, the behaviour is moved from webrtcvideoengine2 to ViEEncoder. The qcif initialization happens before the first frame is received from the capturer and is something pbos did to speed up the encoder initialization to my understanding. I am not sure if its worth it but I did not want to change the behaviour in my cl.
Description was changed from ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467 ========== to ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467, webrtc:6371 ==========
On 2016/10/04 07:42:43, perkj_webrtc wrote: > lgtm > thanks for fixing. > > Stefan, the behaviour is moved from webrtcvideoengine2 to ViEEncoder. The qcif > initialization happens before the first frame is received from the capturer and > is something pbos did to speed up the encoder initialization to my > understanding. I am not sure if its worth it but I did not want to change the > behaviour in my cl. Ok, so it's the QCIF initialization that is new and not the QVGA? And the third InitEncode is for something not related to resolution. lgtm in that case. (unrelated, it may have made more sense to do the first initialization at QVGA rather than QCIF as we're way more likely to pick that resolution as our final one...)
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/...
Description was changed from ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467, webrtc:6371 ========== to ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467, webrtc:6371 R=perkj@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/cf33d9c9d3fb57cca28861845... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as cf33d9c9d3fb57cca28861845d673934993e83df (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467, webrtc:6371 R=perkj@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/cf33d9c9d3fb57cca28861845... ========== to ========== Fixed flaky VideoSendStreamTests after ViEEncoder changes After https://codereview.webrtc.org/2386573002 changed where resolution changes are handled, a few VideoSendStreamTests became flaky. They were waiting for an InitEncode call they triggered, but sometimes were getting one triggered by the resolution change when the first frame was generated. The fix was to make the tests wait for two InitEncode calls first - one when the stream is created, and the second when the first frame was encoded. BUG=webrtc:6467, webrtc:6371 R=perkj@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/cf33d9c9d3fb57cca28861845d673934993e83df Cr-Commit-Position: refs/heads/master@{#14490} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
