Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521393004:
Don't cache video codec list in VideoEngine2. (Closed)
DescriptionDon't cache video codec list in VideoEngine2.
A WebRtcVideoEngine2 object seems to be reused between PeerConnections,
which means that the field trial added in
https://codereview.webrtc.org/2511703002/ may not activate/deactivate
as intended between calls. This CL removes the caching of video codecs,
which gets rid of this problem.
BUG=webrtc:5654
Committed: https://crrev.com/ffc61181d860d9686ab769b6d68e1833309d0944
Cr-Commit-Position: refs/heads/master@{#15265}
Patch Set 1 #Patch Set 2 : Add more unit tests for codecs() call. #
Total comments: 6
Patch Set 3 : Add the requested unit test instead... #
Total comments: 12
Patch Set 4 : count() instead of find() #Patch Set 5 : Remove tests of questionable value. Simplify remaining test. #Patch Set 6 : Remove unnecessary test fixture and minor fixes. #
Messages
Total messages: 33 (17 generated)
brandtr@webrtc.org changed reviewers: + magjed@webrtc.org, perkj@webrtc.org
Please have a look :)
On 2016/11/23 16:34:29, brandtr wrote: > Please have a look :) I'm not sure if you are aware, but I made an almost identical CL 2 weeks ago: "Stop caching supported codecs in WebRtcVideoEngine2" https://codereview.webrtc.org/2492473002/. I had to revert that CL because it broke some Android Chromium FYI bots, and I haven't had time to investigate. Can you try running the Chromium FYI bots?
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 brandtr@webrtc.org
On 2016/11/23 16:52:45, magjed_webrtc wrote: > On 2016/11/23 16:34:29, brandtr wrote: > > Please have a look :) > > I'm not sure if you are aware, but I made an almost identical CL 2 weeks ago: > "Stop caching supported codecs in WebRtcVideoEngine2" > https://codereview.webrtc.org/2492473002/. I had to revert that CL because it > broke some Android Chromium FYI bots, and I haven't had time to investigate. Can > you try running the Chromium FYI bots? That's funny! I did see the revert in the changelog, but never looked into what the CL was about. So I was not aware that you had tried to do the same thing :) I tried to find the failures that you were talking about in the buildbot history on the Chromium FYI waterfall, but I'm not able to go back that far in time. (I only see ~20 builds here: https://build.chromium.org/p/chromium.webrtc.fyi/console). Do you happen to know a way to find the bots that failed on you? I've run this CL on the Android Chromium bots, and it seems to look good. I don't think the two failures are due to this CL: the android_coverage fail is due to something in third_party/icu and the android_blink_rel fail happens both with and without the patch from this CL. The android_blink_rel bot also seems pretty flakey: https://build.chromium.org/p/tryserver.chromium.android/builders/android_blin... I also started some try runs on the standard win, mac, and linux Chromium bots. They are slow to finish, but so far none of them have failed.
On 2016/11/24 08:48:50, brandtr wrote: > On 2016/11/23 16:52:45, magjed_webrtc wrote: > > On 2016/11/23 16:34:29, brandtr wrote: > > > Please have a look :) > > > > I'm not sure if you are aware, but I made an almost identical CL 2 weeks ago: > > "Stop caching supported codecs in WebRtcVideoEngine2" > > https://codereview.webrtc.org/2492473002/. I had to revert that CL because it > > broke some Android Chromium FYI bots, and I haven't had time to investigate. > Can > > you try running the Chromium FYI bots? > > That's funny! I did see the revert in the changelog, but never looked into what > the CL was about. So I was not aware that you had tried to do the same thing :) > > I tried to find the failures that you were talking about in the buildbot history > on the Chromium FYI waterfall, but I'm not able to go back that far in time. (I > only see ~20 builds here: > https://build.chromium.org/p/chromium.webrtc.fyi/console). Do you happen to know > a way to find the bots that failed on you? > > I've run this CL on the Android Chromium bots, and it seems to look good. I > don't think the two failures are due to this CL: the android_coverage fail is > due to something in third_party/icu and the android_blink_rel fail happens both > with and without the patch from this CL. The android_blink_rel bot also seems > pretty flakey: > https://build.chromium.org/p/tryserver.chromium.android/builders/android_blin... > > I also started some try runs on the standard win, mac, and linux Chromium bots. > They are slow to finish, but so far none of them have failed. It was this this built bot: https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%... The logs are gone now, but these were the failing tests: content_browsertests failures: WebRtcGetUserMediaBrowserTest.GetUserMediaWithMandatorySourceID WebRtcGetUserMediaBrowserTest.GetUserMediaWithInvalidOptionalSourceID WebRtcGetUserMediaBrowserTest.GetUserMediaWithInvalidMandatorySourceID This CL lgtm. I did some other changes than you have done, so maybe it was a bug in that code.
On 2016/11/24 12:44:26, magjed_webrtc wrote: > On 2016/11/24 08:48:50, brandtr wrote: > > On 2016/11/23 16:52:45, magjed_webrtc wrote: > > > On 2016/11/23 16:34:29, brandtr wrote: > > > > Please have a look :) > > > > > > I'm not sure if you are aware, but I made an almost identical CL 2 weeks > ago: > > > "Stop caching supported codecs in WebRtcVideoEngine2" > > > https://codereview.webrtc.org/2492473002/. I had to revert that CL because > it > > > broke some Android Chromium FYI bots, and I haven't had time to investigate. > > Can > > > you try running the Chromium FYI bots? > > > > That's funny! I did see the revert in the changelog, but never looked into > what > > the CL was about. So I was not aware that you had tried to do the same thing > :) > > > > I tried to find the failures that you were talking about in the buildbot > history > > on the Chromium FYI waterfall, but I'm not able to go back that far in time. > (I > > only see ~20 builds here: > > https://build.chromium.org/p/chromium.webrtc.fyi/console). Do you happen to > know > > a way to find the bots that failed on you? > > > > I've run this CL on the Android Chromium bots, and it seems to look good. I > > don't think the two failures are due to this CL: the android_coverage fail is > > due to something in third_party/icu and the android_blink_rel fail happens > both > > with and without the patch from this CL. The android_blink_rel bot also seems > > pretty flakey: > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_blin... > > > > I also started some try runs on the standard win, mac, and linux Chromium > bots. > > They are slow to finish, but so far none of them have failed. > > It was this this built bot: > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%... > The logs are gone now, but these were the failing tests: > content_browsertests > failures: > WebRtcGetUserMediaBrowserTest.GetUserMediaWithMandatorySourceID > WebRtcGetUserMediaBrowserTest.GetUserMediaWithInvalidOptionalSourceID > WebRtcGetUserMediaBrowserTest.GetUserMediaWithInvalidMandatorySourceID > > This CL lgtm. I did some other changes than you have done, so maybe it was a bug > in that code. Thanks for digging it up. Did you find it in your email, or is there some page that I haven't found yet where you can look at old buildbot results? Your other changes look innocuous enough to me, so I can't really see what the problem would be. I'll keep a close eye on the waterfalls when landing.
looks like this can need a unit test. Otherwise looks good.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks for taking a look! The functionality for external codecs is already tested here: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... For the internal codecs, I added two tests however.
https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:84: static rtc::scoped_refptr<webrtc::VideoFrameBuffer> CreateBlackFrameBuffer( can we remove static here. (not your cl I know) https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:765: TEST_F(WebRtcVideoEngine2Test, ReportSupportedInternalCodecs) { The fix is for different codecs when the engine has been reset right? So can we write a test that Gets the codecs and then reinitialise the engine with the experiment set and reinitialise the engine and get the codecs again.
Patchset #3 (id:80001) has been deleted
Still lgtm https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:770: EXPECT_NE(codec_names.find("VP8"), codec_names.end()); nit: I think it would be slightly cleaner to write it like: EXPECT_EQ(1u, codec_names.count("VP8"));
Patchset #3 (id:100001) has been deleted
Addressed comments :) https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:84: static rtc::scoped_refptr<webrtc::VideoFrameBuffer> CreateBlackFrameBuffer( On 2016/11/28 09:20:06, perkj_webrtc wrote: > can we remove static here. (not your cl I know) Done. https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:765: TEST_F(WebRtcVideoEngine2Test, ReportSupportedInternalCodecs) { On 2016/11/28 09:20:06, perkj_webrtc wrote: > The fix is for different codecs when the engine has been reset right? > So can we write a test that Gets the codecs and then reinitialise the engine > with the experiment set and reinitialise the engine and get the codecs again. Done. I did not reinitialize the engine in the test however, as the field trial change is effective without reinitializing the engine. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:808: Flexfec03SupportedAsInternalCodecBehindFieldTrial) { Had I written this test before, the problem addressed in this CL would have been uncovered earlier... https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:839: // Set up external encoder factory with first codec, and initialize engine. This test fails before the change in this CL.
https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:770: EXPECT_NE(codec_names.find("VP8"), codec_names.end()); On 2016/11/28 10:31:31, magjed_webrtc wrote: > nit: I think it would be slightly cleaner to write it like: > EXPECT_EQ(1u, codec_names.count("VP8")); Agree. I'm used to the find()/end() style after working with std::set's, but since this is a hash table, count() is nicer.
lgtm % below addressed. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:15: #include <unordered_set> remove https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:30: #include "webrtc/modules/video_coding/codecs/h264/include/h264.h" remove https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:31: #include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h" remove https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:808: Flexfec03SupportedAsInternalCodecBehindFieldTrial) { On 2016/11/28 10:40:15, brandtr wrote: > Had I written this test before, the problem addressed in this CL would have been > uncovered earlier... great. I think you can remove the first two tests and just keep this. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:810: std::unordered_set<std::string> codec_names_before = GetCodecNames(engine_); please remove GetCodecNames and directy use std::find_if on the returned vector from engine_.GetCodecs()
Patchset #6 (id:170001) has been deleted
https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:15: #include <unordered_set> On 2016/11/28 10:56:05, perkj_webrtc wrote: > remove Done. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:30: #include "webrtc/modules/video_coding/codecs/h264/include/h264.h" On 2016/11/28 10:56:05, perkj_webrtc wrote: > remove Done. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:31: #include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h" On 2016/11/28 10:56:05, perkj_webrtc wrote: > remove Done. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:808: Flexfec03SupportedAsInternalCodecBehindFieldTrial) { On 2016/11/28 10:56:05, perkj_webrtc wrote: > On 2016/11/28 10:40:15, brandtr wrote: > > Had I written this test before, the problem addressed in this CL would have > been > > uncovered earlier... > > great. I think you can remove the first two tests and just keep this. Done. https://codereview.webrtc.org/2521393004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:810: std::unordered_set<std::string> codec_names_before = GetCodecNames(engine_); On 2016/11/28 10:56:05, perkj_webrtc wrote: > please remove GetCodecNames and directy use std::find_if on the returned vector > from engine_.GetCodecs() Done.
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: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17372)
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2521393004/#ps190001 (title: "Remove unnecessary test fixture and minor fixes.")
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": 190001, "attempt_start_ts": 1480341633926780,
"parent_rev": "ce3961bdbf02546716eb57d01e21f170158313ea", "commit_rev":
"dd419841a6563070c7df080449b9521b931be02b"}
Message was sent while issue was closed.
Committed patchset #6 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Don't cache video codec list in VideoEngine2. A WebRtcVideoEngine2 object seems to be reused between PeerConnections, which means that the field trial added in https://codereview.webrtc.org/2511703002/ may not activate/deactivate as intended between calls. This CL removes the caching of video codecs, which gets rid of this problem. BUG=webrtc:5654 ========== to ========== Don't cache video codec list in VideoEngine2. A WebRtcVideoEngine2 object seems to be reused between PeerConnections, which means that the field trial added in https://codereview.webrtc.org/2511703002/ may not activate/deactivate as intended between calls. This CL removes the caching of video codecs, which gets rid of this problem. BUG=webrtc:5654 Committed: https://crrev.com/ffc61181d860d9686ab769b6d68e1833309d0944 Cr-Commit-Position: refs/heads/master@{#15265} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ffc61181d860d9686ab769b6d68e1833309d0944 Cr-Commit-Position: refs/heads/master@{#15265} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
