Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973083002:
TransmitMixer: Check GetSendCodec return value. (Closed)
DescriptionTransmitMixer: Check GetSendCodec return value.
BUG=b/62909493
Review-Url: https://codereview.webrtc.org/2973083002
Cr-Commit-Position: refs/heads/master@{#18975}
Committed: https://chromium.googlesource.com/external/webrtc/+/950c1c908c46d10f3de6081e52718a304a199b61
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add DCHECKs on the result of GetSendCodec. #
Total comments: 2
Patch Set 3 : More checking in channel.cc and added TODOs. #
Messages
Total messages: 37 (20 generated)
The CQ bit was checked by ossu@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: This issue passed the CQ dry run.
ossu@webrtc.org changed reviewers: + minyue@webrtc.org
I've added a simple check and logging to the GetSendCodec call. I'm a bit worried that this may paper over some deeper issue, but I've not been able to reproduce any such issue, so it may be fine. We could at least try and land this and see if the crashes eventually go away. :) It at least shouldn't break anything that's already working. I had the idea that this may happen if encoder creation fails, somehow, but I tried that and it didn't crash on the CHECKs I added.
https://codereview.webrtc.org/2973083002/diff/1/webrtc/voice_engine/transmit_... File webrtc/voice_engine/transmit_mixer.cc (right): https://codereview.webrtc.org/2973083002/diff/1/webrtc/voice_engine/transmit_... webrtc/voice_engine/transmit_mixer.cc:250: if (channel->GetSendCodec(codec) == 0) { I found another usage of this, should that be taken care of as well. https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel.... https://codereview.webrtc.org/2973083002/diff/1/webrtc/voice_engine/transmit_... webrtc/voice_engine/transmit_mixer.cc:254: LOG(LS_WARNING) << "Unable to get send codec for channel " If I understand correctly, this is not supposed to happen because Sending() returns true. I think we'd understand why it is possible to reach here. Code wise, I'd add RTC_NOTREACHED() in here.
can we try to land this today. Doing the following will look good to me, even if we cannot find now why Sending() is conflicting with GetSendCodec(). 1. adding RTC_NOTREACHED 2. adding DCHECK and safeguard for the other caller of GetSendCodec
On 2017/07/11 07:28:33, minyue-webrtc wrote: > can we try to land this today. > > Doing the following will look good to me, even if we cannot find now why > Sending() is conflicting with GetSendCodec(). > > 1. adding RTC_NOTREACHED > 2. adding DCHECK and safeguard for the other caller of GetSendCodec Sure, I'll put up a new patch set ASAP. https://codereview.webrtc.org/2973083002/diff/1/webrtc/voice_engine/transmit_... File webrtc/voice_engine/transmit_mixer.cc (right): https://codereview.webrtc.org/2973083002/diff/1/webrtc/voice_engine/transmit_... webrtc/voice_engine/transmit_mixer.cc:250: if (channel->GetSendCodec(codec) == 0) { On 2017/07/10 09:30:32, minyue-webrtc wrote: > I found another usage of this, should that be taken care of as well. > > https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel.... We could add a DCHECK there. From what I can see, it's not actually being called - rather the other overload of ProcessAndEncodeAudio is being used. https://codereview.webrtc.org/2973083002/diff/1/webrtc/voice_engine/transmit_... webrtc/voice_engine/transmit_mixer.cc:254: LOG(LS_WARNING) << "Unable to get send codec for channel " On 2017/07/10 09:30:32, minyue-webrtc wrote: > If I understand correctly, this is not supposed to happen because Sending() > returns true. > > I think we'd understand why it is possible to reach here. > > Code wise, I'd add RTC_NOTREACHED() in here. NOTREACHED after the logging, I presume? So we'll be able to spot this happening in logs and also have it crash if run in debug mode.
The CQ bit was checked by ossu@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/...
New patchset up!
https://codereview.webrtc.org/2973083002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2973083002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2762: const int result = GetSendCodec(codec); will this be safe enough in release build?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ossu@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/...
https://codereview.webrtc.org/2973083002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2973083002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2762: const int result = GetSendCodec(codec); On 2017/07/11 11:18:09, minyue-webrtc wrote: > will this be safe enough in release build? Not sure. Added a fallback route in case this returns != 0. In that case, the data should make it into AudioCodingModuleImpl::Add10MsDataInternal and be rejected by HaveValidEncoder() if there's no encoder set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by ossu@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19136)
ossu@webrtc.org changed reviewers: + henrika@webrtc.org
Adding henrika@ for owner review.
ossu@webrtc.org changed reviewers: + henrikg@webrtc.org
Adding henrikg@ as well, since we're close to quitting time
rs lgtm - deferring to minyue@.
The CQ bit was checked by ossu@webrtc.org
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": 40001, "attempt_start_ts": 1499786156316250,
"parent_rev": "29d0840b5c79df20fab70e5552f1bceb43a8bddb", "commit_rev":
"950c1c908c46d10f3de6081e52718a304a199b61"}
Message was sent while issue was closed.
Description was changed from ========== TransmitMixer: Check GetSendCodec return value. BUG=b/62909493 ========== to ========== TransmitMixer: Check GetSendCodec return value. BUG=b/62909493 Review-Url: https://codereview.webrtc.org/2973083002 Cr-Commit-Position: refs/heads/master@{#18975} Committed: https://chromium.googlesource.com/external/webrtc/+/950c1c908c46d10f3de6081e5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/950c1c908c46d10f3de6081e5...
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
