From 5abd78b40a4ddad0d7d33c10f522e63749a22ce9 Mon Sep 17 00:00:00 2001 From: Alex Glaznev Date: Fri, 1 Jun 2018 19:12:58 +0000 Subject: [PATCH] Revert "Reland "Injectable logging"" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 21219a0e43446701810236fb9fdd59be072c12df. Reason for revert: No tags are written when routing logs to a file - b/86953692 Original change's description: > Reland "Injectable logging" > > 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} TBR=magjed@webrtc.org,sakal@webrtc.org,kwiberg@webrtc.org,phensman@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:9225 Change-Id: I4d0a5990b5f742cc1a96afde3ca97fad9143d2d4 Reviewed-on: https://webrtc-review.googlesource.com/80641 Reviewed-by: Alex Glaznev Commit-Queue: Alex Glaznev Cr-Commit-Position: refs/heads/master@{#23498} --- 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, 11 insertions(+), 292 deletions(-) delete mode 100644 rtc_base/java/src/org/webrtc/Loggable.java delete mode 100644 sdk/android/src/java/org/webrtc/JNILogging.java delete mode 100644 sdk/android/src/jni/logging/logsink.cc delete mode 100644 sdk/android/src/jni/logging/logsink.h diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index b49d00a392..22182b5473 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -1315,7 +1315,6 @@ 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 deleted file mode 100644 index cd66aa1214..0000000000 --- a/rtc_base/java/src/org/webrtc/Loggable.java +++ /dev/null @@ -1,22 +0,0 @@ -/* - * 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 eb3d464603..f143d2f8b2 100644 --- a/rtc_base/java/src/org/webrtc/Logging.java +++ b/rtc_base/java/src/org/webrtc/Logging.java @@ -15,35 +15,21 @@ 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 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: + * 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: * - Logging.enableLogThreads * - Logging.enableLogTimeStamps * - Logging.enableLogToDebugOutput * - * 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. + * Using these APIs 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"); @@ -51,17 +37,6 @@ 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 { @@ -108,26 +83,11 @@ 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 0681d8c872..8b3d9f1180 100644 --- a/rtc_base/logging.cc +++ b/rtc_base/logging.cc @@ -81,11 +81,6 @@ std::ostream& GetNoopStream() { CriticalSection g_log_crit; } // namespace -void LogSink::OnLogMessage(const std::string& msg, - LoggingSeverity severity, - const char* tag) { - OnLogMessage(msg); -} ///////////////////////////////////////////////////////////////////////////// // LogMessage @@ -132,14 +127,8 @@ LogMessage::LogMessage(const char* file, print_stream_ << "[" << std::dec << id << "] "; } - if (file != nullptr) { -#if defined(WEBRTC_ANDROID) - tag_ = FilenameFromPath(file); - print_stream_ << "(line " << line << "): "; -#else + if (file != nullptr) print_stream_ << "(" << FilenameFromPath(file) << ":" << line << "): "; -#endif - } if (err_ctx != ERRCTX_NONE) { char tmp_buf[1024]; @@ -192,6 +181,7 @@ LogMessage::LogMessage(const char* file, 0 /* err */) { if (!is_noop_) { tag_ = tag; + print_stream_ << tag << ": "; } } #endif @@ -229,11 +219,7 @@ 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 f16aca6243..8f9d440fc2 100644 --- a/rtc_base/logging.h +++ b/rtc_base/logging.h @@ -116,9 +116,6 @@ 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; }; @@ -496,7 +493,7 @@ class LogMessage { LoggingSeverity severity_; #if defined(WEBRTC_ANDROID) - // The default Android debug output tag. + // The Android debug output tag. const char* tag_ = "libjingle"; #endif diff --git a/rtc_base/logging_unittest.cc b/rtc_base/logging_unittest.cc index 1b7a6fe0b8..1be0b24b87 100644 --- a/rtc_base/logging_unittest.cc +++ b/rtc_base/logging_unittest.cc @@ -47,9 +47,6 @@ 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 @@ -189,13 +186,7 @@ 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 a66e1a1a69..4286ef71fe 100644 --- a/sdk/android/BUILD.gn +++ b/sdk/android/BUILD.gn @@ -90,7 +90,6 @@ 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", ] @@ -520,38 +519,6 @@ 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 @@ -568,6 +535,7 @@ 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", @@ -625,7 +593,6 @@ 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", @@ -752,7 +719,6 @@ dist_jar("libwebrtc") { ":java_audio_device_module_java", ":libjingle_peerconnection_java", ":libjingle_peerconnection_metrics_default_java", - ":logging_java", ":peerconnection_java", ":screencapturer_java", ":surfaceviewrenderer_java", @@ -1047,7 +1013,6 @@ 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 c7a53f2531..07afac68f6 100644 --- a/sdk/android/api/org/webrtc/PeerConnectionFactory.java +++ b/sdk/android/api/org/webrtc/PeerConnectionFactory.java @@ -13,7 +13,6 @@ 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; @@ -42,20 +41,15 @@ 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, @Nullable Loggable loggable, - @Nullable Severity loggableSeverity) { + NativeLibraryLoader nativeLibraryLoader) { 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) { @@ -68,8 +62,6 @@ 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; @@ -95,16 +87,9 @@ 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, loggable, - loggableSeverity); + enableInternalTracer, enableVideoHwAcceleration, nativeLibraryLoader); } } } @@ -214,16 +199,6 @@ 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() { @@ -499,6 +474,4 @@ 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 deleted file mode 100644 index f391db61a1..0000000000 --- a/sdk/android/src/java/org/webrtc/JNILogging.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * 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 deleted file mode 100644 index cfa3c60abb..0000000000 --- a/sdk/android/src/jni/logging/logsink.cc +++ /dev/null @@ -1,35 +0,0 @@ -/* - * 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 deleted file mode 100644 index bac51a06da..0000000000 --- a/sdk/android/src/jni/logging/logsink.h +++ /dev/null @@ -1,39 +0,0 @@ -/* - * 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 804b07db73..105be31a16 100644 --- a/sdk/android/src/jni/pc/peerconnectionfactory.cc +++ b/sdk/android/src/jni/pc/peerconnectionfactory.cc @@ -27,7 +27,6 @@ #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" @@ -82,9 +81,6 @@ 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(); @@ -507,29 +503,5 @@ 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