From 21219a0e43446701810236fb9fdd59be072c12df Mon Sep 17 00:00:00 2001 From: Paulina Hensman Date: Fri, 18 May 2018 14:32:50 +0200 Subject: [PATCH] Reland "Injectable logging" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any injected loggable or NativeLogger would be deleted if PCFactory was reinitialized without calling setInjectableLogger. Now native logging is not implemented as a Loggable, so it will remain active unless a Loggable is injected. This is a reland of 59216ec4a4151b1ba5478c8f2b5c9f01f4683d7f Original change's description: > Injectable logging > > Allows passing a Loggable to PCFactory.initializationOptions, which > is then injected to Logging.java and logging.h. Future log messages > in both Java and native will then be passed to this Loggable. > > Bug: webrtc:9225 > Change-Id: I2ff693380639448301a78a93dc11d3a0106f0967 > Reviewed-on: https://webrtc-review.googlesource.com/73243 > Commit-Queue: Paulina Hensman > Reviewed-by: Karl Wiberg > Reviewed-by: Sami Kalliomäki > Cr-Commit-Position: refs/heads/master@{#23241} Bug: webrtc:9225 Change-Id: I2fe3fbc8c323814284bb62e43fe1870bdab581ee TBR: kwiberg Reviewed-on: https://webrtc-review.googlesource.com/77140 Commit-Queue: Paulina Hensman Reviewed-by: Sami Kalliomäki Cr-Commit-Position: refs/heads/master@{#23310} --- rtc_base/BUILD.gn | 1 + rtc_base/java/src/org/webrtc/Loggable.java | 22 ++++++++ rtc_base/java/src/org/webrtc/Logging.java | 50 +++++++++++++++++-- rtc_base/logging.cc | 18 ++++++- rtc_base/logging.h | 5 +- rtc_base/logging_unittest.cc | 9 ++++ sdk/android/BUILD.gn | 37 +++++++++++++- .../api/org/webrtc/PeerConnectionFactory.java | 31 +++++++++++- .../src/java/org/webrtc/JNILogging.java | 28 +++++++++++ sdk/android/src/jni/logging/logsink.cc | 35 +++++++++++++ sdk/android/src/jni/logging/logsink.h | 39 +++++++++++++++ .../src/jni/pc/peerconnectionfactory.cc | 28 +++++++++++ 12 files changed, 292 insertions(+), 11 deletions(-) create mode 100644 rtc_base/java/src/org/webrtc/Loggable.java create mode 100644 sdk/android/src/java/org/webrtc/JNILogging.java create mode 100644 sdk/android/src/jni/logging/logsink.cc create mode 100644 sdk/android/src/jni/logging/logsink.h diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index e518ad6539..0ef33fdf77 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -1331,6 +1331,7 @@ if (is_android) { rtc_android_library("base_java") { java_files = [ "java/src/org/webrtc/ContextUtils.java", + "java/src/org/webrtc/Loggable.java", "java/src/org/webrtc/Logging.java", "java/src/org/webrtc/Size.java", "java/src/org/webrtc/ThreadUtils.java", diff --git a/rtc_base/java/src/org/webrtc/Loggable.java b/rtc_base/java/src/org/webrtc/Loggable.java new file mode 100644 index 0000000000..cd66aa1214 --- /dev/null +++ b/rtc_base/java/src/org/webrtc/Loggable.java @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +package org.webrtc; + +import org.webrtc.Logging.Severity; + +/** + * Java interface for WebRTC logging. The default implementation uses webrtc.Logging. + * + * When injected, the Loggable will receive logging from both Java and native. + */ +public interface Loggable { + public void onLogMessage(String message, Severity severity, String tag); +} diff --git a/rtc_base/java/src/org/webrtc/Logging.java b/rtc_base/java/src/org/webrtc/Logging.java index f143d2f8b2..eb3d464603 100644 --- a/rtc_base/java/src/org/webrtc/Logging.java +++ b/rtc_base/java/src/org/webrtc/Logging.java @@ -15,21 +15,35 @@ import java.io.StringWriter; import java.util.EnumSet; import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.Nullable; +import org.webrtc.Loggable; /** - * Java wrapper for WebRTC logging. Logging defaults to java.util.logging.Logger, but will switch to - * native logging (rtc::LogMessage) if one of the following static functions are called from the - * app: + * Java wrapper for WebRTC logging. Logging defaults to java.util.logging.Logger, but a custom + * logger implementing the Loggable interface can be injected along with a Severity. All subsequent + * log messages will then be redirected to the injected Loggable, except those with a severity lower + * than the specified severity, which will be discarded. + * + * It is also possible to switch to native logging (rtc::LogMessage) if one of the following static + * functions are called from the app: * - Logging.enableLogThreads * - Logging.enableLogTimeStamps * - Logging.enableLogToDebugOutput * - * Using these APIs requires that the native library is loaded, using - * PeerConnectionFactory.initialize. + * The priority goes: + * 1. Injected loggable + * 2. Native logging + * 3. Fallback logging. + * Only one method will be used at a time. + * + * Injecting a Loggable or using any of the enable... methods requires that the native library is + * loaded, using PeerConnectionFactory.initialize. */ public class Logging { private static final Logger fallbackLogger = createFallbackLogger(); private static volatile boolean loggingEnabled; + @Nullable private static Loggable loggable; + private static Severity loggableSeverity; private static Logger createFallbackLogger() { final Logger fallbackLogger = Logger.getLogger("org.webrtc.Logging"); @@ -37,6 +51,17 @@ public class Logging { return fallbackLogger; } + static void injectLoggable(Loggable injectedLoggable, Severity severity) { + if (injectedLoggable != null) { + loggable = injectedLoggable; + loggableSeverity = severity; + } + } + + static void deleteInjectedLoggable() { + loggable = null; + } + // TODO(solenberg): Remove once dependent projects updated. @Deprecated public enum TraceLevel { @@ -83,11 +108,26 @@ public class Logging { // TODO(bugs.webrtc.org/8491): Remove NoSynchronizedMethodCheck suppression. @SuppressWarnings("NoSynchronizedMethodCheck") public static synchronized void enableLogToDebugOutput(Severity severity) { + if (loggable != null) { + throw new IllegalStateException( + "Logging to native debug output not supported while Loggable is injected. " + + "Delete the Loggable before calling this method."); + } nativeEnableLogToDebugOutput(severity.ordinal()); loggingEnabled = true; } public static void log(Severity severity, String tag, String message) { + if (loggable != null) { + // Filter log messages below loggableSeverity. + if (severity.ordinal() < loggableSeverity.ordinal()) { + return; + } + loggable.onLogMessage(message, severity, tag); + return; + } + + // Try native logging if no loggable is injected. if (loggingEnabled) { nativeLog(severity.ordinal(), tag, message); return; diff --git a/rtc_base/logging.cc b/rtc_base/logging.cc index 8eae4b7330..8411c21158 100644 --- a/rtc_base/logging.cc +++ b/rtc_base/logging.cc @@ -80,6 +80,11 @@ std::ostream& GetNoopStream() { CriticalSection g_log_crit; } // namespace +void LogSink::OnLogMessage(const std::string& msg, + LoggingSeverity severity, + const char* tag) { + OnLogMessage(msg); +} ///////////////////////////////////////////////////////////////////////////// // LogMessage @@ -126,8 +131,14 @@ LogMessage::LogMessage(const char* file, print_stream_ << "[" << std::dec << id << "] "; } - if (file != nullptr) + if (file != nullptr) { +#if defined(WEBRTC_ANDROID) + tag_ = FilenameFromPath(file); + print_stream_ << "(line " << line << "): "; +#else print_stream_ << "(" << FilenameFromPath(file) << ":" << line << "): "; +#endif + } if (err_ctx != ERRCTX_NONE) { char tmp_buf[1024]; @@ -180,7 +191,6 @@ LogMessage::LogMessage(const char* file, 0 /* err */) { if (!is_noop_) { tag_ = tag; - print_stream_ << tag << ": "; } } #endif @@ -218,7 +228,11 @@ LogMessage::~LogMessage() { CritScope cs(&g_log_crit); for (auto& kv : streams_) { if (severity_ >= kv.second) { +#if defined(WEBRTC_ANDROID) + kv.first->OnLogMessage(str, severity_, tag_); +#else kv.first->OnLogMessage(str); +#endif } } } diff --git a/rtc_base/logging.h b/rtc_base/logging.h index e25d56e935..1df8cd63f9 100644 --- a/rtc_base/logging.h +++ b/rtc_base/logging.h @@ -117,6 +117,9 @@ class LogSink { public: LogSink() {} virtual ~LogSink() {} + virtual void OnLogMessage(const std::string& msg, + LoggingSeverity severity, + const char* tag); virtual void OnLogMessage(const std::string& message) = 0; }; @@ -243,7 +246,7 @@ class LogMessage { LoggingSeverity severity_; #if defined(WEBRTC_ANDROID) - // The Android debug output tag. + // The default Android debug output tag. const char* tag_ = "libjingle"; #endif diff --git a/rtc_base/logging_unittest.cc b/rtc_base/logging_unittest.cc index 1be0b24b87..1b7a6fe0b8 100644 --- a/rtc_base/logging_unittest.cc +++ b/rtc_base/logging_unittest.cc @@ -47,6 +47,9 @@ class LogMessageForTesting : public LogMessage { const std::string& get_extra() const { return extra_; } bool is_noop() const { return is_noop_; } +#if defined(WEBRTC_ANDROID) + const char* get_tag() const { return tag_; } +#endif // Returns the contents of the internal log stream. // Note that parts of the stream won't (as is) be available until *after* the @@ -186,7 +189,13 @@ TEST(LogTest, CheckFilePathParsed) { log_msg.stream() << "<- Does this look right?"; const std::string stream = log_msg.GetPrintStream(); +#if defined(WEBRTC_ANDROID) + const char* tag = log_msg.get_tag(); + EXPECT_NE(nullptr, strstr(tag, "myfile.cc")); + EXPECT_NE(std::string::npos, stream.find("100")); +#else EXPECT_NE(std::string::npos, stream.find("(myfile.cc:100)")); +#endif } TEST(LogTest, CheckNoopLogEntry) { diff --git a/sdk/android/BUILD.gn b/sdk/android/BUILD.gn index 4286ef71fe..a66e1a1a69 100644 --- a/sdk/android/BUILD.gn +++ b/sdk/android/BUILD.gn @@ -90,6 +90,7 @@ rtc_source_set("base_jni") { "src/jni/jni_helpers.cc", "src/jni/jni_helpers.h", "src/jni/pc/audio.h", + "src/jni/pc/logging.cc", "src/jni/pc/media.h", "src/jni/pc/video.h", ] @@ -519,6 +520,38 @@ generate_jni("generated_peerconnection_jni") { jni_generator_include = "//sdk/android/src/jni/jni_generator_helper.h" } +rtc_android_library("logging_java") { + java_files = [ "src/java/org/webrtc/JNILogging.java" ] + + deps = [ + ":base_java", + "//rtc_base:base_java", + ] +} + +generate_jni("generated_logging_jni") { + sources = [ + "src/java/org/webrtc/JNILogging.java", + ] + jni_package = "" + jni_generator_include = "//sdk/android/src/jni/jni_generator_helper.h" +} + +rtc_static_library("logging_jni") { + visibility = [ "*" ] + sources = [ + "src/jni/logging/logsink.cc", + "src/jni/logging/logsink.h", + ] + + deps = [ + ":base_jni", + ":generated_logging_jni", + ":native_api_jni", + "../../rtc_base:rtc_base", + ] +} + rtc_static_library("peerconnection_jni") { # Do not depend on this target externally unless you absolute have to. It is # made public because we don't have a proper NDK yet. Header APIs here are not @@ -535,7 +568,6 @@ rtc_static_library("peerconnection_jni") { "src/jni/pc/dtmfsender.cc", "src/jni/pc/icecandidate.cc", "src/jni/pc/icecandidate.h", - "src/jni/pc/logging.cc", "src/jni/pc/mediaconstraints.cc", "src/jni/pc/mediaconstraints.h", "src/jni/pc/mediasource.cc", @@ -593,6 +625,7 @@ rtc_static_library("peerconnection_jni") { ":base_jni", ":generated_external_classes_jni", ":generated_peerconnection_jni", + ":logging_jni", ":native_api_jni", "../..:webrtc_common", "../../api:libjingle_peerconnection_api", @@ -719,6 +752,7 @@ dist_jar("libwebrtc") { ":java_audio_device_module_java", ":libjingle_peerconnection_java", ":libjingle_peerconnection_metrics_default_java", + ":logging_java", ":peerconnection_java", ":screencapturer_java", ":surfaceviewrenderer_java", @@ -1013,6 +1047,7 @@ rtc_android_library("peerconnection_java") { deps = [ ":audio_api_java", ":base_java", + ":logging_java", ":video_api_java", ":video_java", "//modules/audio_device:audio_device_java", diff --git a/sdk/android/api/org/webrtc/PeerConnectionFactory.java b/sdk/android/api/org/webrtc/PeerConnectionFactory.java index 07afac68f6..c7a53f2531 100644 --- a/sdk/android/api/org/webrtc/PeerConnectionFactory.java +++ b/sdk/android/api/org/webrtc/PeerConnectionFactory.java @@ -13,6 +13,7 @@ package org.webrtc; import android.content.Context; import java.util.List; import javax.annotation.Nullable; +import org.webrtc.Logging.Severity; import org.webrtc.audio.AudioDeviceModule; import org.webrtc.audio.LegacyAudioDeviceModule; @@ -41,15 +42,20 @@ public class PeerConnectionFactory { final boolean enableInternalTracer; final boolean enableVideoHwAcceleration; final NativeLibraryLoader nativeLibraryLoader; + @Nullable Loggable loggable; + @Nullable Severity loggableSeverity; private InitializationOptions(Context applicationContext, String fieldTrials, boolean enableInternalTracer, boolean enableVideoHwAcceleration, - NativeLibraryLoader nativeLibraryLoader) { + NativeLibraryLoader nativeLibraryLoader, @Nullable Loggable loggable, + @Nullable Severity loggableSeverity) { this.applicationContext = applicationContext; this.fieldTrials = fieldTrials; this.enableInternalTracer = enableInternalTracer; this.enableVideoHwAcceleration = enableVideoHwAcceleration; this.nativeLibraryLoader = nativeLibraryLoader; + this.loggable = loggable; + this.loggableSeverity = loggableSeverity; } public static Builder builder(Context applicationContext) { @@ -62,6 +68,8 @@ public class PeerConnectionFactory { private boolean enableInternalTracer = false; private boolean enableVideoHwAcceleration = true; private NativeLibraryLoader nativeLibraryLoader = new NativeLibrary.DefaultLoader(); + @Nullable private Loggable loggable = null; + @Nullable private Severity loggableSeverity = null; Builder(Context applicationContext) { this.applicationContext = applicationContext; @@ -87,9 +95,16 @@ public class PeerConnectionFactory { return this; } + public Builder setInjectableLogger(Loggable loggable, Severity severity) { + this.loggable = loggable; + this.loggableSeverity = severity; + return this; + } + public PeerConnectionFactory.InitializationOptions createInitializationOptions() { return new PeerConnectionFactory.InitializationOptions(applicationContext, fieldTrials, - enableInternalTracer, enableVideoHwAcceleration, nativeLibraryLoader); + enableInternalTracer, enableVideoHwAcceleration, nativeLibraryLoader, loggable, + loggableSeverity); } } } @@ -199,6 +214,16 @@ public class PeerConnectionFactory { if (options.enableInternalTracer && !internalTracerInitialized) { initializeInternalTracer(); } + if (options.loggable != null) { + Logging.injectLoggable(options.loggable, options.loggableSeverity); + nativeInjectLoggable(new JNILogging(options.loggable), options.loggableSeverity.ordinal()); + } else { + Logging.d(TAG, + "PeerConnectionFactory was initialized without an injected Loggable. " + + "Any existing Loggable will be deleted."); + Logging.deleteInjectedLoggable(); + nativeDeleteLoggable(); + } } private void checkInitializeHasBeenCalled() { @@ -474,4 +499,6 @@ public class PeerConnectionFactory { private static native void nativeInvokeThreadsCallbacks(long factory); private static native void nativeFreeFactory(long factory); private static native long nativeGetNativePeerConnectionFactory(long factory); + private static native void nativeInjectLoggable(JNILogging jniLogging, int severity); + private static native void nativeDeleteLoggable(); } diff --git a/sdk/android/src/java/org/webrtc/JNILogging.java b/sdk/android/src/java/org/webrtc/JNILogging.java new file mode 100644 index 0000000000..f391db61a1 --- /dev/null +++ b/sdk/android/src/java/org/webrtc/JNILogging.java @@ -0,0 +1,28 @@ +/* + * Copyright 2018 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +package org.webrtc; + +import org.webrtc.CalledByNative; +import org.webrtc.Loggable; +import org.webrtc.Logging.Severity; + +class JNILogging { + private final Loggable loggable; + + public JNILogging(Loggable loggable) { + this.loggable = loggable; + } + + @CalledByNative + public void logToInjectable(String message, Integer severity, String tag) { + loggable.onLogMessage(message, Severity.values()[severity], tag); + } +} diff --git a/sdk/android/src/jni/logging/logsink.cc b/sdk/android/src/jni/logging/logsink.cc new file mode 100644 index 0000000000..cfa3c60abb --- /dev/null +++ b/sdk/android/src/jni/logging/logsink.cc @@ -0,0 +1,35 @@ +/* + * Copyright 2018 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#include "sdk/android/src/jni/logging/logsink.h" + +#include "sdk/android/generated_logging_jni/jni/JNILogging_jni.h" + +namespace webrtc { +namespace jni { + +JNILogSink::JNILogSink(JNIEnv* env, const JavaRef& j_logging) + : j_logging_(env, j_logging) {} +JNILogSink::~JNILogSink() = default; + +void JNILogSink::OnLogMessage(const std::string& msg, + rtc::LoggingSeverity severity, + const char* tag) { + JNIEnv* env = AttachCurrentThreadIfNeeded(); + Java_JNILogging_logToInjectable(env, j_logging_, NativeToJavaString(env, msg), + NativeToJavaInteger(env, severity), + NativeToJavaString(env, tag)); +} + +void JNILogSink::OnLogMessage(const std::string& msg) { + RTC_NOTREACHED(); +} + +} // namespace jni +} // namespace webrtc diff --git a/sdk/android/src/jni/logging/logsink.h b/sdk/android/src/jni/logging/logsink.h new file mode 100644 index 0000000000..bac51a06da --- /dev/null +++ b/sdk/android/src/jni/logging/logsink.h @@ -0,0 +1,39 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#ifndef SDK_ANDROID_SRC_JNI_LOGGING_LOGSINK_H_ +#define SDK_ANDROID_SRC_JNI_LOGGING_LOGSINK_H_ + +#include + +#include "rtc_base/logging.h" +#include "sdk/android/native_api/jni/java_types.h" +#include "sdk/android/src/jni/jni_helpers.h" + +namespace webrtc { +namespace jni { + +class JNILogSink : public rtc::LogSink { + public: + JNILogSink(JNIEnv* env, const JavaRef& j_logging); + ~JNILogSink() override; + + void OnLogMessage(const std::string& msg, + rtc::LoggingSeverity severity, + const char* tag) override; + void OnLogMessage(const std::string& msg) override; + + private: + const ScopedJavaGlobalRef j_logging_; +}; + +} // namespace jni +} // namespace webrtc + +#endif // SDK_ANDROID_SRC_JNI_LOGGING_LOGSINK_H_ diff --git a/sdk/android/src/jni/pc/peerconnectionfactory.cc b/sdk/android/src/jni/pc/peerconnectionfactory.cc index 105be31a16..804b07db73 100644 --- a/sdk/android/src/jni/pc/peerconnectionfactory.cc +++ b/sdk/android/src/jni/pc/peerconnectionfactory.cc @@ -27,6 +27,7 @@ #include "sdk/android/generated_peerconnection_jni/jni/PeerConnectionFactory_jni.h" #include "sdk/android/native_api/jni/java_types.h" #include "sdk/android/src/jni/jni_helpers.h" +#include "sdk/android/src/jni/logging/logsink.h" #include "sdk/android/src/jni/pc/androidnetworkmonitor.h" #include "sdk/android/src/jni/pc/audio.h" #include "sdk/android/src/jni/pc/icecandidate.h" @@ -81,6 +82,9 @@ static char* field_trials_init_string = nullptr; static bool factory_static_initialized = false; static bool video_hw_acceleration_enabled = true; +// Set in PeerConnectionFactory_InjectLoggable(). +static std::unique_ptr jni_log_sink; + void PeerConnectionFactoryNetworkThreadReady() { RTC_LOG(LS_INFO) << "Network thread JavaCallback"; JNIEnv* env = AttachCurrentThreadIfNeeded(); @@ -503,5 +507,29 @@ static jlong JNI_PeerConnectionFactory_GetNativePeerConnectionFactory( return jlongFromPointer(factoryFromJava(native_factory)); } +static void JNI_PeerConnectionFactory_InjectLoggable( + JNIEnv* jni, + const JavaParamRef&, + const JavaParamRef& j_logging, + jint nativeSeverity) { + // If there is already a LogSink, remove it from LogMessage. + if (jni_log_sink) { + rtc::LogMessage::RemoveLogToStream(jni_log_sink.get()); + } + jni_log_sink = rtc::MakeUnique(jni, j_logging); + rtc::LogMessage::AddLogToStream( + jni_log_sink.get(), static_cast(nativeSeverity)); + rtc::LogMessage::LogToDebug(rtc::LS_NONE); +} + +static void JNI_PeerConnectionFactory_DeleteLoggable( + JNIEnv* jni, + const JavaParamRef&) { + if (jni_log_sink) { + rtc::LogMessage::RemoveLogToStream(jni_log_sink.get()); + jni_log_sink.reset(); + } +} + } // namespace jni } // namespace webrtc