Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803203002:
Android Logging.java: Load native library only when needed (Closed)
|
Created:
3 years, 8 months ago by magjed_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid Logging.java: Load native library only when needed
Logging.java currently always tries to load jingle_peerconnection_so in
the static section, but some clients don't want to use it. This CL loads
jingle_peerconnection_so only when a client requests it by calling one
of:
* Logging.enableLogThreads
* Logging.enableLogTimeStamps
* Logging.enableTracing
* Logging.enableLogToDebugOutput
BUG=b/36410678
Review-Url: https://codereview.webrtc.org/2803203002
Cr-Commit-Position: refs/heads/master@{#17647}
Committed: https://chromium.googlesource.com/external/webrtc/+/dee5eb14e1d4e5bd894772448cc9418e94b3a967
Patch Set 1 #Patch Set 2 : Load jingle_peerconnection_so in FileVideoCapturer and VideoFileRenderer #
Total comments: 4
Patch Set 3 : Add class comment to Logging.java, and always set fallbackLogger.setLevel(Level.ALL). #
Messages
Total messages: 27 (16 generated)
Description was changed from ========== Android Logging.java: Load native library lazily BUG=b/36410678 ========== to ========== Android Logging.java: Load native library only when needed Logging.java currently always tries to load jingle_peerconnection_so in the static section, but some clients don't want to use it. This CL loads jingle_peerconnection_so only when a client requests it by calling one of: * Logging.enableLogThreads * Logging.enableLogTimeStamps * Logging.enableTracing * Logging.enableLogToDebugOutput BUG=b/36410678 ==========
magjed@webrtc.org changed reviewers: + sakal@webrtc.org
Sami - please take a look.
The CQ bit was checked by magjed@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/...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/23061)
The CQ bit was checked by magjed@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.
magjed@webrtc.org changed reviewers: + glaznev@webrtc.org
Alex - please take a look.
noahric@chromium.org changed reviewers: + noahric@chromium.org
LGTM modulo comments. Thanks for this! Long term, it'd still be nice if the name of the assembly wasn't hardcoded, maybe in the build somehow. https://codereview.webrtc.org/2803203002/diff/20001/webrtc/base/java/src/org/... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/2803203002/diff/20001/webrtc/base/java/src/org/... webrtc/base/java/src/org/webrtc/Logging.java:19: /** Java wrapper for WebRTC logging. */ Can you update this comment to describe the loading behavior? It probably won't be clear to readers later why log doesn't try to load the .so https://codereview.webrtc.org/2803203002/diff/20001/webrtc/base/java/src/org/... webrtc/base/java/src/org/webrtc/Logging.java:35: fallbackLogger.setLevel(Level.ALL); Should this run somewhere outside loadNativeLibrary?
lgtm
There are multiple calls of System.loadLibrary("jingle_peerconnection_so")
throughout WebRTC code.
May be in future we could consolidate them into separate helper class, which
will handle native library loading once for all requests from different WebRTC
places?
+Taylor
On 2017/04/10 21:23:02, AlexG wrote:
> lgtm
>
> There are multiple calls of System.loadLibrary("jingle_peerconnection_so")
> throughout WebRTC code.
>
> May be in future we could consolidate them into separate helper class, which
> will handle native library loading once for all requests from different WebRTC
> places?
>
> +Taylor
Sounds good to me.
https://codereview.webrtc.org/2803203002/diff/20001/webrtc/base/java/src/org/... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/2803203002/diff/20001/webrtc/base/java/src/org/... webrtc/base/java/src/org/webrtc/Logging.java:19: /** Java wrapper for WebRTC logging. */ On 2017/04/10 16:56:49, noahric wrote: > Can you update this comment to describe the loading behavior? It probably won't > be clear to readers later why log doesn't try to load the .so Done. https://codereview.webrtc.org/2803203002/diff/20001/webrtc/base/java/src/org/... webrtc/base/java/src/org/webrtc/Logging.java:35: fallbackLogger.setLevel(Level.ALL); On 2017/04/10 16:56:49, noahric wrote: > Should this run somewhere outside loadNativeLibrary? Good catch, thanks.
On 2017/04/10 21:23:02, AlexG wrote:
> lgtm
>
> There are multiple calls of System.loadLibrary("jingle_peerconnection_so")
> throughout WebRTC code.
>
> May be in future we could consolidate them into separate helper class, which
> will handle native library loading once for all requests from different WebRTC
> places?
>
> +Taylor
I agree. I filed a bug for it in:
https://bugs.chromium.org/p/webrtc/issues/detail?id=7474.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org, glaznev@webrtc.org, noahric@chromium.org Link to the patchset: https://codereview.webrtc.org/2803203002/#ps40001 (title: "Add class comment to Logging.java, and always set fallbackLogger.setLevel(Level.ALL).")
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": 40001, "attempt_start_ts": 1491908354208250,
"parent_rev": "382f2b2c456e87a06df678b9a52986529e637300", "commit_rev":
"dee5eb14e1d4e5bd894772448cc9418e94b3a967"}
Message was sent while issue was closed.
Description was changed from ========== Android Logging.java: Load native library only when needed Logging.java currently always tries to load jingle_peerconnection_so in the static section, but some clients don't want to use it. This CL loads jingle_peerconnection_so only when a client requests it by calling one of: * Logging.enableLogThreads * Logging.enableLogTimeStamps * Logging.enableTracing * Logging.enableLogToDebugOutput BUG=b/36410678 ========== to ========== Android Logging.java: Load native library only when needed Logging.java currently always tries to load jingle_peerconnection_so in the static section, but some clients don't want to use it. This CL loads jingle_peerconnection_so only when a client requests it by calling one of: * Logging.enableLogThreads * Logging.enableLogTimeStamps * Logging.enableTracing * Logging.enableLogToDebugOutput BUG=b/36410678 Review-Url: https://codereview.webrtc.org/2803203002 Cr-Commit-Position: refs/heads/master@{#17647} Committed: https://chromium.googlesource.com/external/webrtc/+/dee5eb14e1d4e5bd894772448... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/dee5eb14e1d4e5bd894772448...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2816753002/ by brandtr@webrtc.org. The reason for reverting is: Breaks C++ logs in Java apps.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
