|
|
Created:
3 years, 9 months ago by jens.nielsen Modified:
3 years, 9 months ago Reviewers:
henrika_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, audio-team_agora.io, sdk-team_agora.io, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix memory leaks in Windows core audio
BUG=webrtc:7270
Review-Url: https://codereview.webrtc.org/2727273002
Cr-Commit-Position: refs/heads/master@{#17056}
Committed: https://chromium.googlesource.com/external/webrtc/+/5571ee94653536596f7c04fb1e20eb050b4ae385
Patch Set 1 #
Total comments: 3
Patch Set 2 : Replace WEBRTC_TRACE with LOG macro #Messages
Total messages: 15 (8 generated)
jens.nielsen@berotec.se changed reviewers: + henrika@webrtc.org
Hi Here's a couple of memory leak fixes. I don't think it's super easy to follow all the allocations in the Visual Studio tool but I can see the number of allocations dropping and just from code review I can see this should be needed. Best regards Jens https://codereview.webrtc.org/2727273002/diff/1/webrtc/modules/audio_device/w... File webrtc/modules/audio_device/win/audio_device_core_win.cc (left): https://codereview.webrtc.org/2727273002/diff/1/webrtc/modules/audio_device/w... webrtc/modules/audio_device/win/audio_device_core_win.cc:2341: } OK to remove? pWfxClosestMatch is logged in the loop now. AFAIK there's no scenario where this would be relevant (IsFormatSupported returns S_OK, but still allocates pWfxClosestMatch for some reason, and then Initialize fails for some reason where pWfxClosestMatch is relevant) https://codereview.webrtc.org/2727273002/diff/1/webrtc/modules/audio_device/w... File webrtc/modules/audio_device/win/audio_device_core_win.cc (right): https://codereview.webrtc.org/2727273002/diff/1/webrtc/modules/audio_device/w... webrtc/modules/audio_device/win/audio_device_core_win.cc:881: SAFE_RELEASE(_ptrCaptureVolume); "Bonus fix", I know it's not related to the bug report but I don't really know how to follow exactly which memory is allocated in the VS performance analyzer tool. I weeded this one out as well when debugging the original memory leak and it felt unnecessary to write a separate bug report for this... https://codereview.webrtc.org/2727273002/diff/1/webrtc/modules/audio_device/w... webrtc/modules/audio_device/win/audio_device_core_win.cc:2287: // Same in InitRecording(). This comment is still valid and relevant now because I'm not sure what to do with pWfxClosestMatch. Do we expect at some point to use it here? If the loop above fails to find a supported format we will currently try to initialize with the values from the last loop iteration
I can only give some short generic feedback this week. This class is super old and in a bad state. It would benefit from a complete rewrite but we can't prioritize that now. Fixing crashes and other severe issues is fine. But it is difficult to phase in a new and better style. In your case, using the old WEBRTC_TRACE macros bloats the code. We always uses new macros in webrtc/base/logging.h. Can you try them in your CL to avoid overhead. Also, I would like to avoid making "nice to have changes" in this old code. Real fixes of real problems are fine of course ;-) Is your problem a real problem that you see now given 4-channel support?
On 2017/03/02 15:07:57, henrika_webrtc wrote: > I can only give some short generic feedback this week. > > This class is super old and in a bad state. It would benefit from a complete > rewrite but we can't prioritize that now. > Fixing crashes and other severe issues is fine. But it is difficult to phase in > a new and better style. > > In your case, using the old WEBRTC_TRACE macros bloats the code. > We always uses new macros in webrtc/base/logging.h. Can you try them in your CL > to avoid overhead. > > Also, I would like to avoid making "nice to have changes" in this old code. > Real fixes of real problems are fine of course ;-) > > Is your problem a real problem that you see now given 4-channel support? I agree it's difficult to follow what the strategy is in this class, it does feel like a very big patchwork. I'll refrain from making further cleanups :) Sure, I can try to replace the trace macros. - The _ptrCaptureVolume leak should apply to everybody, InitMicrophone() is called multiple times without Terminate() in between as far as I can see. I've only verified with audio_device_tests though. - The pWfxClosestMatch leak in InitRecording() is observed with my 4 channel microphone, the mono and stereo IsFormatSupported() calls return S_FALSE with pWfxClosestMatch set. I can also reproduce it with my laptop built-in stereo microphone if I select any other sampling frequency than 48kHz as its default format so it should be a general problem. - The pWfxClosestMatch leak in InitPlayout() I can also reproduce by setting my playback device default format to anything other than 48kHz. I'll add this information to the bug report as well
Thanks. Might be slow with feedback. But appreciate your work.
Your patch seems to resolve the issue. As mentioned, the state of this code is "not good" and there are several things I would like to improve and refactor. The main issue is that it contains several old (Windows specific) parts that are not unit tested and not really needed any longer. LGTM
The CQ bit was checked by jens.nielsen@berotec.se 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.
The CQ bit was checked by jens.nielsen@berotec.se
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": 20001, "attempt_start_ts": 1488795732496830, "parent_rev": "3b2fb203fd11eeccc9039c3381ea443848cc6198", "commit_rev": "5571ee94653536596f7c04fb1e20eb050b4ae385"}
Message was sent while issue was closed.
Description was changed from ========== Fix memory leaks in Windows core audio BUG=webrtc:7270 ========== to ========== Fix memory leaks in Windows core audio BUG=webrtc:7270 Review-Url: https://codereview.webrtc.org/2727273002 Cr-Commit-Position: refs/heads/master@{#17056} Committed: https://chromium.googlesource.com/external/webrtc/+/5571ee94653536596f7c04fb1... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/5571ee94653536596f7c04fb1... |