Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409323009:
Add JNI interface for functions to start and stop recording AEC dumps and RTC event logs. (Closed)
DescriptionAdd JNI interface for functions to start and stop recording AEC dumps and RTC event logs.
BUG=webrtc:4741
Committed: https://crrev.com/b2514725a9a2e09da15f77a2ab9a6446a4a616f7
Cr-Commit-Position: refs/heads/master@{#10776}
Patch Set 1 : Initial version. #
Total comments: 5
Patch Set 2 : Added compile guards and updated PeerConnectionFactory.java #
Total comments: 5
Patch Set 3 : Added java documentation and changed first letter of function names to lowercase. #
Total comments: 1
Patch Set 4 : Fixed indentation in java code. #
Messages
Total messages: 31 (8 generated)
Description was changed from ========== Add JNI interface for functions to start and stop recording AEC dumps and RTC event logs. BUG=webrtc:4741 ========== to ========== Add JNI interface for functions to start and stop recording AEC dumps and RTC event logs. BUG=webrtc:4741 ==========
ivoc@webrtc.org changed reviewers: + magjed@webrtc.org, terelius@webrtc.org
Hi guys, Please have a look at this small CL to provide a JNI wrapper for the functions to start and stop AEC dumps and RTC event logs. Thanks!
Adding jiayl@ to CC. https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... talk/app/webrtc/java/jni/peerconnection_jni.cc:1255: JNIEnv* jni, jclass, jlong native_factory, jint file) { Can the file handle safely be cast to a jint and back?
jiayl@google.com changed reviewers: + jiayl@google.com
1. How to pass the file size limit? 2. You also need to add the java declaration in PeerConnectionFactory.java. https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... talk/app/webrtc/java/jni/peerconnection_jni.cc:1255: JNIEnv* jni, jclass, jlong native_factory, jint file) { On 2015/11/10 16:40:00, terelius wrote: > Can the file handle safely be cast to a jint and back? Seems OK with http://developer.android.com/reference/android/os/ParcelFileDescriptor.html#d... Will the native code close the handle?
https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... talk/app/webrtc/java/jni/peerconnection_jni.cc:1255: JNIEnv* jni, jclass, jlong native_factory, jint file) { On 2015/11/10 17:00:04, jiayl1 wrote: > On 2015/11/10 16:40:00, terelius wrote: > > Can the file handle safely be cast to a jint and back? > > Seems OK with > http://developer.android.com/reference/android/os/ParcelFileDescriptor.html#d... > > Will the native code close the handle? Ok. The RtcEventLog calls FILE* file_stream = rtc::FdopenPlatformFileForWriting(platform_file_); file_->OpenFromFileHandle(file_stream, true, false); in StartLogging, and file_->CloseFile(); rtc::ClosePlatformFile(platform_file_); in StopLogging. Ivo, do the AECdump do the same?
Thanks for the quick reviews! I added compilation guards so the JNI code is only compiled on android (since the functions are only useful on android). I added the missing code in PeerConnectionFactory.java. Support for limiting the number of bytes for both AEC dump and RTC event log has not landed yet, so we will update these functions with an extra argument when that happens. https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... talk/app/webrtc/java/jni/peerconnection_jni.cc:1255: JNIEnv* jni, jclass, jlong native_factory, jint file) { On 2015/11/10 17:00:04, jiayl1 wrote: > On 2015/11/10 16:40:00, terelius wrote: > > Can the file handle safely be cast to a jint and back? > > Seems OK with > http://developer.android.com/reference/android/os/ParcelFileDescriptor.html#d... > > Will the native code close the handle? @terelius: Good question, I think I need to add an #if defined(ANDROID) around these function calls. On linux, the platform file is an int, but on windows it is a HANDLE, which seems to be a pointer type (and therefore 64-bit), so this conversion will probably cause compiler problems. As far as I'm aware, there is no way to get access to a file descriptor (as int) in plain (non-Android) java code anyway, so this restriction shouldn't matter much. @jiayl: Yes, native code will close the handle. https://codereview.webrtc.org/1409323009/diff/1/talk/app/webrtc/java/jni/peer... talk/app/webrtc/java/jni/peerconnection_jni.cc:1255: JNIEnv* jni, jclass, jlong native_factory, jint file) { On 2015/11/10 17:15:11, terelius wrote: > On 2015/11/10 17:00:04, jiayl1 wrote: > > On 2015/11/10 16:40:00, terelius wrote: > > > Can the file handle safely be cast to a jint and back? > > > > Seems OK with > > > http://developer.android.com/reference/android/os/ParcelFileDescriptor.html#d... > > > > Will the native code close the handle? > > Ok. > > The RtcEventLog calls > > FILE* file_stream = rtc::FdopenPlatformFileForWriting(platform_file_); > file_->OpenFromFileHandle(file_stream, true, false); > > in StartLogging, and > > file_->CloseFile(); > rtc::ClosePlatformFile(platform_file_); > > in StopLogging. Ivo, do the AECdump do the same? @terelius: Good question, I think I need to add an #if defined(ANDROID) around these function calls. On linux, the platform file is an int, but on windows it is a HANDLE, which seems to be a pointer type (and therefore 64-bit), so this conversion will probably cause compiler problems. As far as I'm aware, there is no way to get access to a file descriptor (as int) in plain (non-Android) java code anyway, so this restriction shouldn't matter much. @terelius2: Yes, AEC dump does the same thing. @jiayl: Yes, native code will close the handle.
https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:134: public boolean StartAecDump(int file_descriptor) { First letter of method name should be lower case. https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public void StopAecDump() { Please document these APIs as comments. In particular, what happens if 1. startX is called repeatedly? 2. stopX is called without calling startX? 3. stopX is called repeatedly? Any thread expectation?
https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public void StopAecDump() { On 2015/11/10 17:52:07, jiayl1 wrote: > Please document these APIs as comments. > In particular, what happens if > 1. startX is called repeatedly? > 2. stopX is called without calling startX? > 3. stopX is called repeatedly? > > Any thread expectation? At the moment the log is single threaded and has no expectations on the calling thread. However, I am currently rewriting some of the logging internals and it could be useful to add an expectation that StopLogging be called from the same thread as StartLogging.
On 2015/11/10 17:58:21, terelius wrote: > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public void > StopAecDump() { > On 2015/11/10 17:52:07, jiayl1 wrote: > > Please document these APIs as comments. > > In particular, what happens if > > 1. startX is called repeatedly? > > 2. stopX is called without calling startX? > > 3. stopX is called repeatedly? > > > > Any thread expectation? > > At the moment the log is single threaded and has no expectations on the calling > thread. However, I am currently rewriting some of the logging internals and it > could be useful to add an expectation that StopLogging be called from the same > thread as StartLogging. Do you mean the calls must be made on a single thread, or the APIs are thread safe (can be called on any thread)?
On 2015/11/10 18:01:50, jiayl1 wrote: > On 2015/11/10 17:58:21, terelius wrote: > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): > > > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public > void > > StopAecDump() { > > On 2015/11/10 17:52:07, jiayl1 wrote: > > > Please document these APIs as comments. > > > In particular, what happens if > > > 1. startX is called repeatedly? > > > 2. stopX is called without calling startX? > > > 3. stopX is called repeatedly? > > > > > > Any thread expectation? > > > > At the moment the log is single threaded and has no expectations on the > calling > > thread. However, I am currently rewriting some of the logging internals and it > > could be useful to add an expectation that StopLogging be called from the same > > thread as StartLogging. > > Do you mean the calls must be made on a single thread, or the APIs are thread > safe (can be called on any thread)? The API is thread safe. However, if it isn't an inconvenience for you, it might be useful to make a restriction that Start and Stop has to be called from the same thread. (There are two thread classes in WebRTC; thread.h and thread_wrapper.h. I'm trying both, but thread_wrapper.h says "The implementation must be assumed to be single threaded, meaning that all methods of the class, must be called from the same thread, including instantiation." Thus, I'd get more flexibility in the design if we can add the assuption that StartLogging and StopLogging are called from the same thread.)
On 2015/11/10 18:21:36, terelius wrote: > On 2015/11/10 18:01:50, jiayl1 wrote: > > On 2015/11/10 17:58:21, terelius wrote: > > > > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > > File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): > > > > > > > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public > > void > > > StopAecDump() { > > > On 2015/11/10 17:52:07, jiayl1 wrote: > > > > Please document these APIs as comments. > > > > In particular, what happens if > > > > 1. startX is called repeatedly? > > > > 2. stopX is called without calling startX? > > > > 3. stopX is called repeatedly? > > > > > > > > Any thread expectation? > > > > > > At the moment the log is single threaded and has no expectations on the > > calling > > > thread. However, I am currently rewriting some of the logging internals and > it > > > could be useful to add an expectation that StopLogging be called from the > same > > > thread as StartLogging. > > > > Do you mean the calls must be made on a single thread, or the APIs are thread > > safe (can be called on any thread)? > > The API is thread safe. However, if it isn't an inconvenience for you, it might > be > useful to make a restriction that Start and Stop has to be called from the same > thread. > > (There are two thread classes in WebRTC; thread.h and thread_wrapper.h. > I'm trying both, but thread_wrapper.h says "The implementation must be assumed > to be single threaded, meaning that all methods of the class, must be > called from the same thread, including instantiation." Thus, I'd get more > flexibility in the design if we can add the assuption that StartLogging and > StopLogging are called from the same thread.) OK, please document that. Another question: is the file immediately ready when the stopX call returns?
I added documentation to the java code and changed the first letter of the functions to lowercase. @jiayl: Yes, the file will be ready immediately when the StopX function returns. When we implement the byte-size logging limit, we will have to think about how to signal from WebRTC to the application that the logging has completed. Do you have any ideas about how to implement that? Maybe using a callback function? https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:134: public boolean StartAecDump(int file_descriptor) { On 2015/11/10 17:52:07, jiayl1 wrote: > First letter of method name should be lower case. Done. https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public void StopAecDump() { On 2015/11/10 17:58:21, terelius wrote: > On 2015/11/10 17:52:07, jiayl1 wrote: > > Please document these APIs as comments. > > In particular, what happens if > > 1. startX is called repeatedly? > > 2. stopX is called without calling startX? > > 3. stopX is called repeatedly? > > > > Any thread expectation? > > At the moment the log is single threaded and has no expectations on the calling > thread. However, I am currently rewriting some of the logging internals and it > could be useful to add an expectation that StopLogging be called from the same > thread as StartLogging. I added some documentation, please have a look to see if it's sufficient. I think it is not necessary for StartX and StopX to be called from the same java thread, because the code in libjingle involves a thread-hop to the webrtc worker thread anyway (see talk/session/media/channelmanager.cc).
On 2015/11/11 09:31:11, ivoc wrote: > I added documentation to the java code and changed the first letter of the > functions to lowercase. > > @jiayl: Yes, the file will be ready immediately when the StopX function returns. > When we implement the byte-size logging limit, we will have to think about how > to signal from WebRTC to the application that the logging has completed. Do you > have any ideas about how to implement that? Maybe using a callback function? > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:134: public > boolean StartAecDump(int file_descriptor) { > On 2015/11/10 17:52:07, jiayl1 wrote: > > First letter of method name should be lower case. > > Done. > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public void > StopAecDump() { > On 2015/11/10 17:58:21, terelius wrote: > > On 2015/11/10 17:52:07, jiayl1 wrote: > > > Please document these APIs as comments. > > > In particular, what happens if > > > 1. startX is called repeatedly? > > > 2. stopX is called without calling startX? > > > 3. stopX is called repeatedly? > > > > > > Any thread expectation? > > > > At the moment the log is single threaded and has no expectations on the > calling > > thread. However, I am currently rewriting some of the logging internals and it > > could be useful to add an expectation that StopLogging be called from the same > > thread as StartLogging. > > I added some documentation, please have a look to see if it's sufficient. > > I think it is not necessary for StartX and StopX to be called from the same java > thread, because the code in libjingle involves a thread-hop to the webrtc worker > thread anyway (see talk/session/media/channelmanager.cc). Just to confirm: There is a thread hop in StopX, but the file does not need to be properly closed by the worker thread to be usable? The callback can be implemented in a similar way to PeerConnection.createOffer: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
On 2015/11/12 16:59:12, jiayl1 wrote: > On 2015/11/11 09:31:11, ivoc wrote: > > I added documentation to the java code and changed the first letter of the > > functions to lowercase. > > > > @jiayl: Yes, the file will be ready immediately when the StopX function > returns. > > When we implement the byte-size logging limit, we will have to think about how > > to signal from WebRTC to the application that the logging has completed. Do > you > > have any ideas about how to implement that? Maybe using a callback function? > > > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): > > > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:134: public > > boolean StartAecDump(int file_descriptor) { > > On 2015/11/10 17:52:07, jiayl1 wrote: > > > First letter of method name should be lower case. > > > > Done. > > > > > https://codereview.webrtc.org/1409323009/diff/20001/talk/app/webrtc/java/src/... > > talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: public > void > > StopAecDump() { > > On 2015/11/10 17:58:21, terelius wrote: > > > On 2015/11/10 17:52:07, jiayl1 wrote: > > > > Please document these APIs as comments. > > > > In particular, what happens if > > > > 1. startX is called repeatedly? > > > > 2. stopX is called without calling startX? > > > > 3. stopX is called repeatedly? > > > > > > > > Any thread expectation? > > > > > > At the moment the log is single threaded and has no expectations on the > > calling > > > thread. However, I am currently rewriting some of the logging internals and > it > > > could be useful to add an expectation that StopLogging be called from the > same > > > thread as StartLogging. > > > > I added some documentation, please have a look to see if it's sufficient. > > > > I think it is not necessary for StartX and StopX to be called from the same > java > > thread, because the code in libjingle involves a thread-hop to the webrtc > worker > > thread anyway (see talk/session/media/channelmanager.cc). > > Just to confirm: There is a thread hop in StopX, but the file does not need to > be properly > closed by the worker thread to be usable? > > The callback can be implemented in a similar way to PeerConnection.createOffer: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... There is a thread hop in both StartX and StopX, so the java code shouldn't have to care about which thread the functions are called from. The java code is blocked until the StartX or StopX function finishes executing on the worker thread. The file will be closed automatically in the StopX function before it returns, so there is no need to close it on the java side. When StopX returns, the file will be already closed and written to storage. In case of errors, the WebRTC code will take responsibility for closing the file. I will have a look at adding a callback to the StartX functions.
lgtm
lgtm
lgtm % nit https://codereview.webrtc.org/1409323009/diff/40001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java (right): https://codereview.webrtc.org/1409323009/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/PeerConnectionFactory.java:138: return nativeStartAecDump(nativeFactory, file_descriptor); nit: I think you should use 2 space indentation like the rest of the file.
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, terelius@webrtc.org, jiayl@google.com Link to the patchset: https://codereview.webrtc.org/1409323009/#ps60001 (title: "Fixed indentation in java code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409323009/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409323009/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1930)
ivoc@webrtc.org changed reviewers: + jiayl@webrtc.org
@jiayl: Could you LGTM this CL from your webrtc account as well? It is needed to get owner permission for the java file. Thanks!
lgtm
The CQ bit was checked by ivoc@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409323009/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409323009/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b2514725a9a2e09da15f77a2ab9a6446a4a616f7 Cr-Commit-Position: refs/heads/master@{#10776} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
