Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710683009:
Added support for changing the volume of AudioTrack as discussed in BUG=webrtc:6533 (Closed)
DescriptionAdded support for changing the volume of AudioTrack as discussed in BUG=webrtc:6533
This is a short term solution to change the volume of an AudioTrack until applyConstraints for MediaStreamTracks has been implemented.
This CL adds 1 new Java method & the relevant JNI file update:
AudioTrack.java:
public void setVolume(double volume);
BUG=webrtc:6533
Review-Url: https://codereview.webrtc.org/2710683009
Cr-Commit-Position: refs/heads/master@{#17682}
Committed: https://chromium.googlesource.com/external/webrtc/+/9d65f39d52d485109cd70ebab83cc1c4b9f2e893
Patch Set 1 #
Total comments: 5
Patch Set 2 : updated based on feedback from reviewers #
Total comments: 1
Patch Set 3 : alphabetical order as requested by reviewer #
Total comments: 1
Patch Set 4 : changed according to reviewers advice, simple setVolume on the audio track. Cleaned up AudioSource … #Patch Set 5 : one last fix for reviewers, lined up comments, and clarified 0-10 - tested this. #
Total comments: 1
Patch Set 6 : fixed volume capitalization #Patch Set 7 : added period at end of sentence #
Total comments: 2
Patch Set 8 : fixed silly mistake in JNI converting twice #
Messages
Total messages: 30 (10 generated)
Description was changed from ========== Added support for changing the volume of AudioSource as discussed in BUG=webrtc:6533 This is a short term solution to change the volume of an AudioTrack (which contains an getSource() AudioSource property) until applyConstraints for MediaStreamTracks has been implemented. This CL adds 2 new Java methods & their respective JNI files: AudioTrack.java: public AudioSource getSource(); AudioSource.java: public void setVolume(double volume); BUG=webrtc:6533 ========== to ========== Added support for changing the volume of AudioSource as discussed in BUG=webrtc:6533 This is a short term solution to change the volume of an AudioTrack (which contains an getSource() AudioSource property) until applyConstraints for MediaStreamTracks has been implemented. This CL adds 2 new Java methods & their respective JNI files: AudioTrack.java: public AudioSource getSource(); AudioSource.java: public void setVolume(double volume); BUG=webrtc:6533 ==========
henrika@webrtc.org changed reviewers: + solenberg@webrtc.org
henrika@webrtc.org changed reviewers: + pthatcher@webrtc.org
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
On 2017/03/03 06:57:38, pthatcher1 wrote: > mailto:pthatcher@webrtc.org changed reviewers: > + mailto:deadbeef@webrtc.org I think Taylor will be a better reviewer of this than me.
https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/AudioSource.java (right): https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/AudioSource.java:25: public void setVolume(double volume) { Could you add a comment mentioning that this volume is in the range 0 to 1 (assuming that's correct)? https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/AudioTrack.java:22: public AudioSource getSource() { There are some subtle caveats to this approach... If I create an AudioTrack from a source, then call getSource(), I'd get a different object than the one I passed in. Also, it looks like the JNI code doesn't add a reference, so the returned object shouldn't be dispose()'d. These things should be documented in a comment. Though, if this is only planned to be used for changing the volume, maybe a simple "setVolume" method would be better?
https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/AudioSource.java (right): https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/AudioSource.java:25: public void setVolume(double volume) { On 2017/03/04 00:23:19, Taylor Brandstetter wrote: > Could you add a comment mentioning that this volume is in the range 0 to 1 > (assuming that's correct)? In mediastreaminterface.h the range is documented as [0, 10], which is enforced with DCHECKs in remoteaudiosource.cc.
https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/AudioSource.java (right): https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/AudioSource.java:25: public void setVolume(double volume) { On 2017/03/07 12:04:16, the sun wrote: > On 2017/03/04 00:23:19, Taylor Brandstetter wrote: > > Could you add a comment mentioning that this volume is in the range 0 to 1 > > (assuming that's correct)? > > In mediastreaminterface.h the range is documented as [0, 10], which is enforced > with DCHECKs in remoteaudiosource.cc. Ok, then that should definitely be documented here as well.
On 2017/03/07 19:00:18, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... > File webrtc/sdk/android/api/org/webrtc/AudioSource.java (right): > > https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... > webrtc/sdk/android/api/org/webrtc/AudioSource.java:25: public void > setVolume(double volume) { > On 2017/03/07 12:04:16, the sun wrote: > > On 2017/03/04 00:23:19, Taylor Brandstetter wrote: > > > Could you add a comment mentioning that this volume is in the range 0 to 1 > > > (assuming that's correct)? > > > > In mediastreaminterface.h the range is documented as [0, 10], which is > enforced > > with DCHECKs in remoteaudiosource.cc. > > Ok, then that should definitely be documented here as well. Updated with comments & cleaned up some variables names.
lgtm % comment I'm not that familiar with the Java/JNI layer, though, so wait for the other reviewers. https://codereview.webrtc.org/2710683009/diff/20001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/classreferenceholder.cc (right): https://codereview.webrtc.org/2710683009/diff/20001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/classreferenceholder.cc:99: LoadClass(jni, "org/webrtc/AudioSource"); alphabetical order, please
On 2017/03/17 20:31:39, the sun wrote: > lgtm % comment > > I'm not that familiar with the Java/JNI layer, though, so wait for the other > reviewers. > > https://codereview.webrtc.org/2710683009/diff/20001/webrtc/sdk/android/src/jn... > File webrtc/sdk/android/src/jni/classreferenceholder.cc (right): > > https://codereview.webrtc.org/2710683009/diff/20001/webrtc/sdk/android/src/jn... > webrtc/sdk/android/src/jni/classreferenceholder.cc:99: LoadClass(jni, > "org/webrtc/AudioSource"); > alphabetical order, please alphabetical order as requested.
https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/AudioTrack.java:22: public AudioSource getSource() { On 2017/03/04 00:23:19, Taylor Brandstetter wrote: > There are some subtle caveats to this approach... If I create an AudioTrack from > a source, then call getSource(), I'd get a different object than the one I > passed in. Also, it looks like the JNI code doesn't add a reference, so the > returned object shouldn't be dispose()'d. These things should be documented in a > comment. > > Though, if this is only planned to be used for changing the volume, maybe a > simple "setVolume" method would be better? This comment doesn't seem addressed. https://codereview.webrtc.org/2710683009/diff/40001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/AudioSource.java (right): https://codereview.webrtc.org/2710683009/diff/40001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/AudioSource.java:26: * 0 to 1 nit: Line up comment lines, capitalize "Volume", add period to end. Also, as solenberg mentioned, isn't the range actually 0 to 10? It could be made [0, 1] at the Java level if it's just multiplied by 10 before it's passed to the C++ level.
On 2017/03/17 22:20:09, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... > File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): > > https://codereview.webrtc.org/2710683009/diff/1/webrtc/sdk/android/api/org/we... > webrtc/sdk/android/api/org/webrtc/AudioTrack.java:22: public AudioSource > getSource() { > On 2017/03/04 00:23:19, Taylor Brandstetter wrote: > > There are some subtle caveats to this approach... If I create an AudioTrack > from > > a source, then call getSource(), I'd get a different object than the one I > > passed in. Also, it looks like the JNI code doesn't add a reference, so the > > returned object shouldn't be dispose()'d. These things should be documented in > a > > comment. > > > > Though, if this is only planned to be used for changing the volume, maybe a > > simple "setVolume" method would be better? > > This comment doesn't seem addressed. > > https://codereview.webrtc.org/2710683009/diff/40001/webrtc/sdk/android/api/or... > File webrtc/sdk/android/api/org/webrtc/AudioSource.java (right): > > https://codereview.webrtc.org/2710683009/diff/40001/webrtc/sdk/android/api/or... > webrtc/sdk/android/api/org/webrtc/AudioSource.java:26: * 0 to 1 > nit: Line up comment lines, capitalize "Volume", add period to end. > > Also, as solenberg mentioned, isn't the range actually 0 to 10? It could be made > [0, 1] at the Java level if it's just multiplied by 10 before it's passed to the > C++ level. Hi there, I resolved the issues mentioned & I think it should meet the requirements now. Thanks, Dax
lgtm with nit. CL description should be updated. https://codereview.webrtc.org/2710683009/diff/80001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): https://codereview.webrtc.org/2710683009/diff/80001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/AudioTrack.java:22: /** Sets the volume for the underlying MediaSource. volume is a gain value in the range nit: Capitalize "volume", period at end of sentence.
Description was changed from ========== Added support for changing the volume of AudioSource as discussed in BUG=webrtc:6533 This is a short term solution to change the volume of an AudioTrack (which contains an getSource() AudioSource property) until applyConstraints for MediaStreamTracks has been implemented. This CL adds 2 new Java methods & their respective JNI files: AudioTrack.java: public AudioSource getSource(); AudioSource.java: public void setVolume(double volume); BUG=webrtc:6533 ========== to ========== Added support for changing the volume of AudioTrack as discussed in BUG=webrtc:6533 This is a short term solution to change the volume of an AudioTrack until applyConstraints for MediaStreamTracks has been implemented. This CL adds 1 new Java method & the relevant JNI file update: AudioTrack.java: public void setVolume(double volume); BUG=webrtc:6533 ==========
On 2017/04/05 04:45:46, Taylor Brandstetter wrote: > lgtm with nit. CL description should be updated. > > https://codereview.webrtc.org/2710683009/diff/80001/webrtc/sdk/android/api/or... > File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): > > https://codereview.webrtc.org/2710683009/diff/80001/webrtc/sdk/android/api/or... > webrtc/sdk/android/api/org/webrtc/AudioTrack.java:22: /** Sets the volume for > the underlying MediaSource. volume is a gain value in the range > nit: Capitalize "volume", period at end of sentence. Thank you for the feedback, I have updated the CL name & description - also fixed the captilization for comment of setVolume method in AudioTrack.java
deadbeef@webrtc.org changed reviewers: + magjed@webrtc.org
Still need a webrtc/sdk/android owner. magjed@, could you take a look?
https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/AudioTrack.java:15: final long nativeTrack; No need to introduce a new member variable, just use the nativeTrack in the super class MediaStreamTrack instead. https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/peerconnection_jni.cc:1207: jlong nativePointer = jlongFromPointer(source.get()); It looks like you are converting AudioSourceInterface to jlong and then back to AudioSourceInterface again. Can't you just replace the last two lines with: source->SetVolume(volume); ?
On 2017/04/05 19:14:00, magjed_webrtc wrote: > https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/api/o... > File webrtc/sdk/android/api/org/webrtc/AudioTrack.java (right): > > https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/api/o... > webrtc/sdk/android/api/org/webrtc/AudioTrack.java:15: final long nativeTrack; > No need to introduce a new member variable, just use the nativeTrack in the > super class MediaStreamTrack instead. > > https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/src/j... > File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): > > https://codereview.webrtc.org/2710683009/diff/120001/webrtc/sdk/android/src/j... > webrtc/sdk/android/src/jni/peerconnection_jni.cc:1207: jlong nativePointer = > jlongFromPointer(source.get()); > It looks like you are converting AudioSourceInterface to jlong and then back to > AudioSourceInterface again. Can't you just replace the last two lines with: > source->SetVolume(volume); > ? Silly me - I think I fixed all the above - thank you for pointing this out, I merged two functions into one after earlier reviewer pointed out it was unecessary.
lgtm
On 2017/04/06 11:28:27, magjed_webrtc wrote: > lgtm is there anything left for me to do here?
On 2017/04/12 15:03:50, dax frost wrote: > On 2017/04/06 11:28:27, magjed_webrtc wrote: > > lgtm > > is there anything left for me to do here? Just going through the commit queue by checking the "Commit" box; I'll go ahead and start that.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2710683009/#ps140001 (title: "fixed silly mistake in JNI converting twice")
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": 140001, "attempt_start_ts": 1492039600053740,
"parent_rev": "85512b33e6a8a32c104e8b33a2f82f73baaa58e4", "commit_rev":
"9d65f39d52d485109cd70ebab83cc1c4b9f2e893"}
Message was sent while issue was closed.
Description was changed from ========== Added support for changing the volume of AudioTrack as discussed in BUG=webrtc:6533 This is a short term solution to change the volume of an AudioTrack until applyConstraints for MediaStreamTracks has been implemented. This CL adds 1 new Java method & the relevant JNI file update: AudioTrack.java: public void setVolume(double volume); BUG=webrtc:6533 ========== to ========== Added support for changing the volume of AudioTrack as discussed in BUG=webrtc:6533 This is a short term solution to change the volume of an AudioTrack until applyConstraints for MediaStreamTracks has been implemented. This CL adds 1 new Java method & the relevant JNI file update: AudioTrack.java: public void setVolume(double volume); BUG=webrtc:6533 Review-Url: https://codereview.webrtc.org/2710683009 Cr-Commit-Position: refs/heads/master@{#17682} Committed: https://chromium.googlesource.com/external/webrtc/+/9d65f39d52d485109cd70ebab... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/9d65f39d52d485109cd70ebab... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
