Refactored logging API to make use of libvlcLogInterop #15
Conversation
There was a problem hiding this comment.
👍
A few points:
-
videolan.libvlcloginterop naming: I think you should drop the
videolanpart. I only used it for libvlc native packages, and I'm still wondering whether that was a good idea. If the namespace is there, adding it to the package name might be redundant. What do you think? -
If I'm correct, right now loaded the logging binary is loaded by default. Any reason why? How about loading it only on demand?
-
Tests: I need to start a
contributingguide, but I'm gonna start asking for tests with PRs. What are your thoughts on testing this particular thing? -
build libvlcloginterop for other platforms: If I believe your
build.sh, you currently build successfully the logging module for these platforms:
build x86_64-linux-gnu
build arm-linux-gnueabihf
build x86_64-w64-mingw32
build i686-w64-mingw32
build x86_64-apple-darwin
Anything missing?
Should we upload a nuget with all of these builds so we can test this PR properly? As they depend on each others.
Last but not least: How's the perf?
I might have more feedback when I start testing the code on Android/iOS, but good job :)
mfkl
added
enhancement
| } | ||
| } | ||
|
|
||
| internal static T GetLibraryFunction<T>(IntPtr libraryHandle, string functionName) |
There was a problem hiding this comment.
I think these definitions belong to MarshalUtils
There was a problem hiding this comment.
then PreloadNativeLibrary too.
I think the loading architecture could be done in a separate class if you want, but maybe it's another PR? Maybe that other PR could be done before this one?
I've mixed a few things here, the linux .so loading and the real implementation of the logging API.
I'm just rebasing for now, will see in the future what to do with that.
LibVLCSharp/Shared/LibVLC.cs
Outdated
| GetLogContext(logContext, out var module, out var file, out var line); | ||
|
|
||
| // Do the notification on another thread, so that VLC is not interrupted by the logging | ||
| Task.Run(() => this._logEventHandler?.Invoke(new LogMessage(logLevel, message, module, file, line))); |
There was a problem hiding this comment.
Spawn a thread with a Blocking queue instead, so we guarantee the order of the log message and don't depend on the ThreadPool's load.
This is the second implementation, rebased on master.
jeremyVignelles
force-pushed
the
feature/logging
branch
from
e8fee90
to
125c586
Compare
Done
Right, will be included in a next commit here. I just tried to rebase on master for now.
I replaced your tests with the new way of doing
Yeah, that's a discussion for the repo. I built for some platforms but didn't test all of them. We will probably need to think about it...
For now :
We need to publish a nuget package before merging, but for now, I'm trying with a beta package I built and put in a local nuget folder on my machine.
Didn't test yet, but hopefully better than Vlc.DotNet |
Remove the current Log API which only (kinda) works on Windows and not other platforms, in preparation of 3.0 stable release. Proper logging mechanism will come back soon with #15
|
This seems to work as expected with #57 , closing this as no longer required. |
This new API lets .net user receive logs from callbacks, thanks to https://github.com/jeremyVignelles/libvlcLogInterop .
Things left to do:
Tested on :