VLCLibrary: add API to log debug info to a file
Merge request reports
Activity
@caro There is currently no way to access the debug log because it is directly logged by NSLog, which is inaccessible when you remotely debug the app without Xcode attach (see the tvOS MR for reference). An alternative to the current approach of using libvlc to log to a file is to forward the messages through a callback to the client app and let that handle the further management. This is what I will implement in addition to the proposed API. Amended patch forthcoming later this week.
159 166 } 160 167 } 161 168 169 - (void)enableDebugLoggingToFile:(NSString *)filePath 170 { 171 if (!filePath) 172 return; 173 174 if (!_instance) 175 return; 176 177 if (_debugLogging) { 178 libvlc_log_unset(_instance); 179 } 180 181 _logFileStream = fopen([filePath UTF8String], "a"); - Resolved by Felix Paul Kühne
159 166 } 160 167 } 161 168 169 - (void)enableDebugLoggingToFile:(NSString *)filePath 170 { 171 if (!filePath) 172 return; 173 174 if (!_instance) 175 return; Can we make the filepath nonnull and remove the check? This method doesn't make sense with nil and instead of just returning for instance nil I'd add an assert. A developer would know immediately what's wrong if he hits it and that only happens in development
Edited by Carolachanged this line in version 6 of the diff
added 1 commit
- 53196630 - library: add API to log to a custom target using a defined protocol
added 1 commit
- 359e49dd - library: add API to log to a custom target using a defined protocol
added 1 commit
- 9ca1a07e - library: add API to log to a custom target using a defined protocol
222 288 } 289 290 static void HandleMessageForCustomTarget(void *data, 291 int level, 292 const libvlc_log_t *ctx, 293 const char *fmt, 294 va_list args) 295 { 296 VLCLibrary *libraryInstance = (__bridge VLCLibrary *)data; 297 id debugLoggingTarget = libraryInstance.debugLoggingTarget; 298 299 if (!debugLoggingTarget) { 300 return; 301 } 302 303 char *str; You can initialize this to NULL here...
Edit: Actually seems not really necessary unless it is not guaranteed that
vasprintf
modifiesstr
(either to NULL or a valid address), but I think it is.Edited by Marvin Scholzchanged this line in version 6 of the diff
290 static void HandleMessageForCustomTarget(void *data, 291 int level, 292 const libvlc_log_t *ctx, 293 const char *fmt, 294 va_list args) 295 { 296 VLCLibrary *libraryInstance = (__bridge VLCLibrary *)data; 297 id debugLoggingTarget = libraryInstance.debugLoggingTarget; 298 299 if (!debugLoggingTarget) { 300 return; 301 } 302 303 char *str; 304 if (vasprintf(&str, fmt, args) == -1) { 305 if (str) 303 char *str; 304 if (vasprintf(&str, fmt, args) == -1) { 305 if (str) 306 free(str); 307 str = NULL; 308 return; 309 } 310 311 if (str == NULL) 312 return; 313 314 [debugLoggingTarget handleMessage:[NSString stringWithUTF8String:str] debugLevel:level]; 315 316 free(str); 317 str = NULL; 318 } Why do you set
str
toNULL
in multiple places in the function when the function returns right after that? Nothing could accidentally usestr
anymore, or did I overlook something?Micro-optimization: You could use
initWithBytesNoCopy:length:encoding:freeWhenDone:
though likely not worth it as it only saves maybe one copy and only if the encoding matches what NSString natively uses on the system.
181 return; 182 183 if (!_instance) 184 return; 185 186 if (_debugLogging) { 187 libvlc_log_unset(_instance); 188 } 189 190 _logFileStream = fopen([filePath UTF8String], "a"); 191 192 if (_logFileStream) { 193 libvlc_log_set_file(_instance, _logFileStream); 194 _debugLogging = YES; 195 } 196 } Calling this method multiple times will lead to the previous
FILE
descriptor being leaked (missingfclose
), unless I am overlooking something.Additionally this method could have a
BOOL
return value to properly indicate failures, like whenfopen
fails.Another improvement, though thats more personal taste, would be to use NSURL instead of NSString for the file path. (Apple stated in Accessing Files and Directories that "The preferred way to specify the location of a file or directory is to use the NSURL class" and it seems overall more common in most APIs to use NSURLs for this.)
85 107 * \param readableName Human-readable application name, e.g. "FooBar player 1.2.3" 86 108 * \param userAgent HTTP User Agent, e.g. "FooBar/1.2.3 Python/2.6.0" 87 109 */ 88 - (void)setHumanReadableName:(NSString *)readableName withHTTPUserAgent:(NSString *)userAgent; 110 - (void)setHumanReadableName:(NSString * _Nullable)readableName withHTTPUserAgent:(NSString * _Nullable)userAgent; Is
readableName
actually nullable? In libvlc this directly is passed tovar_SetString
and I could not figure out if it's ok to call that with a new value ofNULL
as the documentation does not mention it, the code does not make it clear either. I guess it is fine though, just want to make sure as in countless other places I seevar_SetString(p_this, "something", (val != NULL) ? val : "");
.I don't know about NSString at all, but for var_SetString it can be NULL. However it must not be displayed as NULL, as
printf("%s", NULL)
is undefined behaviour. NULL is the default value for string variable, so is like "unset".That being said, they are places expecting a non-null "user-agent" (human readable name) and other checking before using it, and same for "http-user-agent". But nullable can also be used to define a default user-agent.
changed this line in version 6 of the diff
added 1 commit
- e8e28ec9 - library: add API to log to a custom target using a defined protocol