(In reply to bpantazhs from comment #26) > Thanks Nils! Here are some questions so I am sure I follow you before I > commit anything: > > > In sdp_unittests.cpp you added two new asserts. I think we should have > > another test where we assert that the parser in fact correctly parses > > 'usedtx=1' and some random number for maxaveragebitrate. We should have in > > the end two places where we do these checks: one where we verify that these > > values are in fact zero (that exists already in your patchset) and another > > place where we check for non-null values. > > So the assert comparisons to 0 remain and there needs to be added something > like: > > ASSERT_EQ(48000U, opus_parameters->maxaveragebitrate); > ASSERT_EQ(1U, opus_parameters->useDTX); > > It is not clear to me where to place these as only in TEST_P(NewSdpTest, > CheckFormatParameters) > it seems to check for the parameters' values, could you shed some light on > this please? You could either modify the SDP that |CheckFormatParameters| uses, by adding maxaveragebitrate and usedtx. Then you can check that the expected values are parsed. Or, you could write a standalone test similar to one of the other test-cases that looks at fmtp (eg; CheckTelephoneEventWithDefaultEvents). > > And then we need a test to verify that we in fact will answer with usedtx=1 > > if it was offered. The best place to do that would be in > > jsep_session_unittest.cpp. If you take a look at the test where you added > > two new asserts it create the offer here > > https://searchfox.org/mozilla-central/rev/ > > 38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/ > > jsep_session_unittest.cpp#3357 I would create a new test which does exactly > > the same thing up to and include the CreateOffer() call. Then I would search > > and replace in that resulting string 'usedtx=0' with 'usedtx=1'. For example > > something like here > > https://searchfox.org/mozilla-central/rev/ > > 38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/ > > jsep_session_unittest.cpp#3437 you also then need to SetLocalOffer() and > > SetRemoteOffer() with your modified offer. Then call CreateAnswer() and > > assert that the answer contains 'usedtx=1'. > > > I think I follow this but I am not entirely sure so as to commit a patch. If > I understand correctly you would like to see a new test that looks something > like this: > > > > TEST_F(JsepSessionTest, ValidateOfferedDTXParameter) > { > types.push_back(SdpMediaSection::kAudio); > types.push_back(SdpMediaSection::kVideo); > > RefPtr<JsepTransceiver> audio(new > JsepTransceiver(SdpMediaSection::kAudio)); > audio->mSendTrack.UpdateTrackIds(std::vector<std::string>(1, > "offerer_stream"), "a1"); > mSessionOff->AddTransceiver(audio); > > RefPtr<JsepTransceiver> video(new > JsepTransceiver(SdpMediaSection::kVideo)); > video->mSendTrack.UpdateTrackIds(std::vector<std::string>(1, > "offerer_stream"), "v1"); > mSessionOff->AddTransceiver(video); > > std::string offer = CreateOffer(); > > size_t start = offer.find("usedtx=0"); > size_t end = offer.find("\r\n", start); > offer.replace(start, end+2-start, "usedtx=1"); > > SetLocalOffer(offer); > SetRemoteOffer(offer); > > ASSERT_NE(std::string::npos,answer.find("usedtx=1")); > } > > > Thanks for all your help! Sort of, but it could be simpler. See this as an example, since you are only doing audio: https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4960 Also, you would rewrite the offer only for the remote side, between the SetLocalOffer and SetRemoteOffer calls. Lastly, you will need to create an answer.
Bug 1418804 Comment 31 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to bpantazhs from comment #26) > Thanks Nils! Here are some questions so I am sure I follow you before I > commit anything: > > > In sdp_unittests.cpp you added two new asserts. I think we should have > > another test where we assert that the parser in fact correctly parses > > 'usedtx=1' and some random number for maxaveragebitrate. We should have in > > the end two places where we do these checks: one where we verify that these > > values are in fact zero (that exists already in your patchset) and another > > place where we check for non-null values. > > So the assert comparisons to 0 remain and there needs to be added something > like: > > ASSERT_EQ(48000U, opus_parameters->maxaveragebitrate); > ASSERT_EQ(1U, opus_parameters->useDTX); > > It is not clear to me where to place these as only in TEST_P(NewSdpTest, > CheckFormatParameters) > it seems to check for the parameters' values, could you shed some light on > this please? You could modify the SDP that |CheckFormatParameters| uses, by adding maxaveragebitrate and usedtx. Then you can check that the expected values are parsed. Or, you could write a standalone test similar to one of the other test-cases that looks at fmtp (eg; CheckTelephoneEventWithDefaultEvents). > > And then we need a test to verify that we in fact will answer with usedtx=1 > > if it was offered. The best place to do that would be in > > jsep_session_unittest.cpp. If you take a look at the test where you added > > two new asserts it create the offer here > > https://searchfox.org/mozilla-central/rev/ > > 38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/ > > jsep_session_unittest.cpp#3357 I would create a new test which does exactly > > the same thing up to and include the CreateOffer() call. Then I would search > > and replace in that resulting string 'usedtx=0' with 'usedtx=1'. For example > > something like here > > https://searchfox.org/mozilla-central/rev/ > > 38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/ > > jsep_session_unittest.cpp#3437 you also then need to SetLocalOffer() and > > SetRemoteOffer() with your modified offer. Then call CreateAnswer() and > > assert that the answer contains 'usedtx=1'. > > > I think I follow this but I am not entirely sure so as to commit a patch. If > I understand correctly you would like to see a new test that looks something > like this: > > > > TEST_F(JsepSessionTest, ValidateOfferedDTXParameter) > { > types.push_back(SdpMediaSection::kAudio); > types.push_back(SdpMediaSection::kVideo); > > RefPtr<JsepTransceiver> audio(new > JsepTransceiver(SdpMediaSection::kAudio)); > audio->mSendTrack.UpdateTrackIds(std::vector<std::string>(1, > "offerer_stream"), "a1"); > mSessionOff->AddTransceiver(audio); > > RefPtr<JsepTransceiver> video(new > JsepTransceiver(SdpMediaSection::kVideo)); > video->mSendTrack.UpdateTrackIds(std::vector<std::string>(1, > "offerer_stream"), "v1"); > mSessionOff->AddTransceiver(video); > > std::string offer = CreateOffer(); > > size_t start = offer.find("usedtx=0"); > size_t end = offer.find("\r\n", start); > offer.replace(start, end+2-start, "usedtx=1"); > > SetLocalOffer(offer); > SetRemoteOffer(offer); > > ASSERT_NE(std::string::npos,answer.find("usedtx=1")); > } > > > Thanks for all your help! Sort of, but it could be simpler. See this as an example, since you are only doing audio: https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4960 Also, you would rewrite the offer only for the remote side, between the SetLocalOffer and SetRemoteOffer calls. Lastly, you will need to create an answer.