Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873803002:
Make AudioSinkInterface::Data hold a const pointer to the audio buffer. (Closed)
DescriptionMake AudioSinkInterface::Data hold a const pointer to the audio buffer.
This is in preparation for https://codereview.webrtc.org/2750783004/, where
requiring a non-const pointer for AudioSinkInterface would force an unmuting
and zeroing of every frame.
BUG=webrtc:7343
Review-Url: https://codereview.webrtc.org/2873803002
Cr-Commit-Position: refs/heads/master@{#18107}
Committed: https://chromium.googlesource.com/external/webrtc/+/38605965bd68ad1e376c530883041b9c12f26d3b
Patch Set 1 #
Total comments: 2
Messages
Total messages: 19 (10 generated)
The CQ bit was checked by yujo@chromium.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.
yujo@chromium.org changed reviewers: + henrik.lundin@webrtc.org, solenberg@webrtc.org
https://codereview.webrtc.org/2873803002/diff/1/webrtc/api/call/audio_sink.h File webrtc/api/call/audio_sink.h (right): https://codereview.webrtc.org/2873803002/diff/1/webrtc/api/call/audio_sink.h#... webrtc/api/call/audio_sink.h:41: const int16_t* data; // The actual 16bit audio data. Seems like a good change to me---it can't break callers, and it'll only break AudioSinkInterface implementations that mutate the input buffer, which seems like a questionable practice... (Two points that are out f scope for this CL: First, |data| should probably be an ArrayView instead of a raw pointer; the array length is already implicitly specified by all the other Data members, but an ArrayView would make things easier to reason about. Second, what's the point of passing all the arguments in a struct with named fields instead of as five separate arguments, and provide a Data constructor that means callers won't use all those field names anyway?)
LGTM, but I have no powers here.
lgtm https://codereview.webrtc.org/2873803002/diff/1/webrtc/api/call/audio_sink.h File webrtc/api/call/audio_sink.h (right): https://codereview.webrtc.org/2873803002/diff/1/webrtc/api/call/audio_sink.h#... webrtc/api/call/audio_sink.h:41: const int16_t* data; // The actual 16bit audio data. On 2017/05/10 08:19:51, kwiberg-webrtc wrote: > Seems like a good change to me---it can't break callers, and it'll only break > AudioSinkInterface implementations that mutate the input buffer, which seems > like a questionable practice... > > (Two points that are out f scope for this CL: First, |data| should probably be > an ArrayView instead of a raw pointer; the array length is already implicitly > specified by all the other Data members, but an ArrayView would make things > easier to reason about. Second, what's the point of passing all the arguments in > a struct with named fields instead of as five separate arguments, and provide a > Data constructor that means callers won't use all those field names anyway?) Agree with Karl on all points (let's not address the out-of-scope issue here and now).
Thanks, can someone commit for me?
The CQ bit was checked by yujo@chromium.org
The CQ bit was unchecked by yujo@chromium.org
The CQ bit was checked by yujo@chromium.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": 1, "attempt_start_ts": 1494533740444620, "parent_rev":
"f512c92ab2ee1700da1cf2cd0a7f17d087789c93", "commit_rev":
"38605965bd68ad1e376c530883041b9c12f26d3b"}
Message was sent while issue was closed.
Description was changed from ========== Make AudioSinkInterface::Data hold a const pointer to the audio buffer. This is in preparation for https://codereview.webrtc.org/2750783004/, where requiring a non-const pointer for AudioSinkInterface would force an unmuting and zeroing of every frame. BUG=webrtc:7343 ========== to ========== Make AudioSinkInterface::Data hold a const pointer to the audio buffer. This is in preparation for https://codereview.webrtc.org/2750783004/, where requiring a non-const pointer for AudioSinkInterface would force an unmuting and zeroing of every frame. BUG=webrtc:7343 Review-Url: https://codereview.webrtc.org/2873803002 Cr-Commit-Position: refs/heads/master@{#18107} Committed: https://chromium.googlesource.com/external/webrtc/+/38605965bd68ad1e376c53088... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/38605965bd68ad1e376c53088...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.webrtc.org/2877013002/ by zhihuang@webrtc.org. The reason for reverting is: Reverted because it possibly breaks the internal project..
Message was sent while issue was closed.
On 2017/05/11 20:35:34, commit-bot: I haz the power wrote: > Committed patchset #1 (id:1) as > https://chromium.googlesource.com/external/webrtc/+/38605965bd68ad1e376c53088... An internal project is broken after this CL landed but I'm not 100% percent sure that this is the root cause. Since I don't have a easy way to reproduce it locally, I reverted this. I'll reland this If it doesn't help. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
