Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775033002:
Changed defaults for CreateAnswer in non-constraint mode (Closed)
|
Created:
4 years, 9 months ago by hta-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChanged defaults for CreateAnswer in non-constraint mode
This CL also adds control flag in webrtcsession_unittests
that says whether to prefer constraints APIs or non-constraints APIs, and uses it in the test that was needed
to uncover the bug.
BUG=webrtc:4906
Committed: https://crrev.com/aac2dea76541b70a54f77b6b7939f4c92b7e2d6e
Cr-Commit-Position: refs/heads/master@{#11947}
Patch Set 1 : First landable version of patch #
Total comments: 8
Patch Set 2 : Review comments, bug fix #
Messages
Total messages: 37 (20 generated)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775033002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775033002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/9352) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/9439) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5788) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5780) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8118) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8033) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13311) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11944) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/13061) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/9217) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/13329) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/10462) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/347) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12975) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13347) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3984)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775033002/20001
Description was changed from ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds a new test that is entirely non-constraint, but is very like an existing test. Once we have made the decision to deprecate constraint APIs, the other test can be deleted. BUG=webrtc:4906 ========== to ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds a new test that is entirely non-constraint, but is very like an existing test. Once we have made the decision to deprecate constraint APIs, the other test can be deleted. BUG=webrtc:4906 ==========
hta@webrtc.org changed reviewers: + tommi@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775033002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
Description was changed from ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds a new test that is entirely non-constraint, but is very like an existing test. Once we have made the decision to deprecate constraint APIs, the other test can be deleted. BUG=webrtc:4906 ========== to ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds control flag in webrtcsession_unittests that says whether to prefer constraints APIs or non-constraints APIs, and uses it in the test that was needed to uncover the bug. BUG=webrtc:4906 ==========
hta@webrtc.org changed reviewers: + perkj@webrtc.org
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775033002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775033002/60001
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If you're awake early, your early review is appreciated...
One bug. Does this only affect non-constaint mode and not publicly used versions? If not - you should probably add a defect. https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:417: return session_description_options_.offer_to_receive_audio > 0 || video https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:418: session_description_options_.offer_to_receive_audio == video https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:970: PeerConnectionInterface::RTCOfferAnswerOptions session_description_options_; suggest change name to match the type. what about offer_answer_options_; https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:521: ASSERT_TRUE(ExtractMediaSessionOptions(rtc_options, true, session_options)); Hum. weird that ExtractMediaSessionOptions has been moved to peerconnection.cc but not the tests.... Looks like all these tests are in the wrong place after a refactoring... Anyway - not in this cl.....
I discovered the bug when I first tried to use the new API in Chromium (3 out of 100-or-so content_browsertests failed). I don't think anyone else uses it at the moment, so did not file a bug on it. It's just part of the implementation cycle. https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:417: return session_description_options_.offer_to_receive_audio > 0 || On 2016/03/10 15:35:24, perkj_webrtc wrote: > video Ouch. Makes me feel I should write tests for my tests :-( https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:418: session_description_options_.offer_to_receive_audio == On 2016/03/10 15:35:24, perkj_webrtc wrote: > video Done. https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:970: PeerConnectionInterface::RTCOfferAnswerOptions session_description_options_; On 2016/03/10 15:35:24, perkj_webrtc wrote: > suggest change name to match the type. what about offer_answer_options_; Good idea. The "session_description" moniker was no good. Changed both variables' names (session_description_constraints -> offer_answer_constraints too). https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1775033002/diff/60001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:521: ASSERT_TRUE(ExtractMediaSessionOptions(rtc_options, true, session_options)); On 2016/03/10 15:35:24, perkj_webrtc wrote: > Hum. weird that ExtractMediaSessionOptions has been moved to peerconnection.cc > but not the tests.... Looks like all these tests are in the wrong place after a > refactoring... Anyway - not in this cl..... Yep, I wondered if I could stop exporting it at all, then this happened... it "smells" like an internal helper function over there...
lgtm
The CQ bit was checked by hta@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775033002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hta@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775033002/80001
Message was sent while issue was closed.
Description was changed from ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds control flag in webrtcsession_unittests that says whether to prefer constraints APIs or non-constraints APIs, and uses it in the test that was needed to uncover the bug. BUG=webrtc:4906 ========== to ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds control flag in webrtcsession_unittests that says whether to prefer constraints APIs or non-constraints APIs, and uses it in the test that was needed to uncover the bug. BUG=webrtc:4906 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds control flag in webrtcsession_unittests that says whether to prefer constraints APIs or non-constraints APIs, and uses it in the test that was needed to uncover the bug. BUG=webrtc:4906 ========== to ========== Changed defaults for CreateAnswer in non-constraint mode This CL also adds control flag in webrtcsession_unittests that says whether to prefer constraints APIs or non-constraints APIs, and uses it in the test that was needed to uncover the bug. BUG=webrtc:4906 Committed: https://crrev.com/aac2dea76541b70a54f77b6b7939f4c92b7e2d6e Cr-Commit-Position: refs/heads/master@{#11947} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/aac2dea76541b70a54f77b6b7939f4c92b7e2d6e Cr-Commit-Position: refs/heads/master@{#11947} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
