Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127763002:
Removed the memory pool from the mixer. (Closed)
|
Created:
4 years, 5 months ago by aleloi Modified:
4 years, 4 months ago Reviewers:
ossu CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@removed_time_scheduler Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemoved the memory pool from the mixer.
Memory frames are now expected to be owned by the mixing participants.
Committed: https://crrev.com/a0db81f83a9dc1fa80efb4e9bce13e9b4eca7e45
Cr-Commit-Position: refs/heads/master@{#13554}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changes from reviewer comments. #
Total comments: 1
Patch Set 3 : Described life time of audio frame pointer. #Patch Set 4 : Removed '_pointer'. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 57 (36 generated)
Description was changed from ========== Removed the memory pool from the mixer. Memory frames are now owned by the mixing participants. ========== to ========== Removed the memory pool from the mixer. Memory frames are now owned by the mixing participants. ==========
aleloi@webrtc.org changed reviewers: + ossu@webrtc.org
PTAL! I think this is short and relatively uncontroversial. There is an existing test in //webrtc/modules/audio_mixer/test/ that covers the code in these changes. There is one potential thing however: what if a participant is destroyed during mixing? If it happens at precisely the right time, the mixer can have a pointer into a deleted audio frame. Can it happen? Can something go wrong? Is eventual 'wrongness' detected by running all tests? WDYT?
The CQ bit was checked by aleloi@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/...
I thought a little on my previous message. I don't think it's relevant: every participant must unregister itself from the mixer before destruction. If it is destroyed but not unregistered, the old mixer would crash as well.
On 2016/07/06 11:38:02, aleloi wrote: > PTAL! > > I think this is short and relatively uncontroversial. There is an existing test > in //webrtc/modules/audio_mixer/test/ that covers the code in these changes. > > There is one potential thing however: what if a participant is destroyed during > mixing? If it happens at precisely the right time, the mixer can have a pointer > into a deleted audio frame. Can it happen? Can something go wrong? Is eventual > 'wrongness' detected by running all tests? WDYT? Well, if the participant gets destroyed while used, anything can happen, right? So that's probably nothing we need to worry about here.
https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... File webrtc/modules/audio_mixer/include/audio_mixer_defines.h (right): https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:48: virtual AudioFrameInfo GetAudioFrameWithMuted(int32_t id, So, why does this function need an AudioFrame as input if it's going to return one of its own anyways? https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:75: AudioFrame _audio_frame; Seems to me the frame should be put in the implementing class, not in this one. Also it should be spelled audio_frame_. :)
How about modifying the return value of GetAudioFrameWithMuted to contain a frame pointer? Something like AudioFrameWithInfo get...(id) ? And also implement it in Channel later? I could get rid of GetAudioFrame at the same time. https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... File webrtc/modules/audio_mixer/include/audio_mixer_defines.h (right): https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:48: virtual AudioFrameInfo GetAudioFrameWithMuted(int32_t id, On 2016/07/06 11:49:37, ossu wrote: > So, why does this function need an AudioFrame as input if it's going to return > one of its own anyways? It's public API (although only defined in voe::Channel, one test and something called Buzz). I could change it, but then I would have to change the code everywhere else as well. https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:75: AudioFrame _audio_frame; On 2016/07/06 11:49:37, ossu wrote: > Seems to me the frame should be put in the implementing class, not in this one. > Also it should be spelled audio_frame_. :) You are right. I think I wanted to be consistent with the surrounding code or something.
I followed your suggestion and changed the return type of GetAudioFrameWithMuted. I also removed GetAudioFrame and updated the tests.
The CQ bit was checked by aleloi@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/...
Description was changed from ========== Removed the memory pool from the mixer. Memory frames are now owned by the mixing participants. ========== to ========== Removed the memory pool from the mixer. Memory frames are now expected to be owned by the mixing participants. ==========
LGTM with a small naming nit. https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... File webrtc/modules/audio_mixer/include/audio_mixer_defines.h (right): https://codereview.webrtc.org/2127763002/diff/1/webrtc/modules/audio_mixer/in... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:48: virtual AudioFrameInfo GetAudioFrameWithMuted(int32_t id, On 2016/07/06 11:59:38, aleloi wrote: > On 2016/07/06 11:49:37, ossu wrote: > > So, why does this function need an AudioFrame as input if it's going to return > > one of its own anyways? > > It's public API (although only defined in voe::Channel, one test and something > called Buzz). I could change it, but then I would have to change the code > everywhere else as well. Well it's not public yet, since you just forked the old mixer code and started on your own. :) https://codereview.webrtc.org/2127763002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/include/audio_mixer_defines.h (right): https://codereview.webrtc.org/2127763002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/include/audio_mixer_defines.h:34: AudioFrame* audio_frame_pointer; I think this should just be called audio_frame (without the _pointer). I can't find us explicitly calling out a thing is a pointer like this almost anywhere else in the source.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/2127763002/#ps60001 (title: "Removed '_pointer'.")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2109333006 Patch 240001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Wohoo! Thanks for the comments :)
The CQ bit was checked by aleloi@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2109333006 Patch 240001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by aleloi@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: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1075) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1076) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15021) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/4534) mac_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10746)
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by aleloi@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.
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by aleloi@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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/2127763002/#ps200001 (title: "Removed '_pointer'.")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2109333006 Patch 400001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by aleloi@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2109333006 Patch 400001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by aleloi@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2109333006 Patch 400001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by aleloi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Removed the memory pool from the mixer. Memory frames are now expected to be owned by the mixing participants. ========== to ========== Removed the memory pool from the mixer. Memory frames are now expected to be owned by the mixing participants. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Removed the memory pool from the mixer. Memory frames are now expected to be owned by the mixing participants. ========== to ========== Removed the memory pool from the mixer. Memory frames are now expected to be owned by the mixing participants. Committed: https://crrev.com/a0db81f83a9dc1fa80efb4e9bce13e9b4eca7e45 Cr-Commit-Position: refs/heads/master@{#13554} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a0db81f83a9dc1fa80efb4e9bce13e9b4eca7e45 Cr-Commit-Position: refs/heads/master@{#13554} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
