DeadSix27 / waifu2x-converter-cpp

Improved fork of Waifu2X C++ using OpenCL and OpenCV
MIT License
792 stars 86 forks source link

Log Handler Support #146

Closed DeadSix27 closed 4 years ago

DeadSix27 commented 5 years ago

This is to be done after: #144

Instead of using --silent we should be adding --log-level and have the many options, for starters:

--log-level 1 to 3

1 = equal to --silent 2 = only show process block()... 3 = show everything (same as without --silent)

DeadSix27 commented 5 years ago

@YukihoAA See https://github.com/DeadSix27/waifu2x-converter-cpp/tree/log_level_support

Question: I reached a point where I figured it would be smarter to have a class that handles logging.. as now I have no idea how to otherwise access functions from main.cpp in w2xconv.cpp

So I thought that with creating a new logging Class, we can pass a reference to that class to w2xconv.

So that w2xconv can call logger->whatever()

Same as I do currently with for example: print_processing_file()

Only that print_processing_file() is not in a class.

But my c++ skills are limited, I am not so sure how to implement a basic logger class and pass it to w2xconv.

EDIT: my concern being other programs that depend on libw2x

Those most likely won't be having a logger class themselves. Unless they implement one too..

So I am not sure how to handle this.. in C# or Python I would handle this via Events which you can subscribe to optionally or not.

YukihoAA commented 5 years ago

I don't recommend to implement logger class because..

In fact, our libw2x is not in c++, it is C in C, there is no classes and it is procedural programming language so.. It is not good idea to use external classes in lib and it is more capable to control functions by argument parameter not by calling some classes or something.

If we do that.. there is no meaning that we uses lib. lib cannot standalone any more so.. It will better to combine in an exe...

DeadSix27 commented 5 years ago

Then I'm not sure how to expose functions into w2xconv.cpp defined in main.cpp

And if we can't do that I would have to double up a lot of code, I was going to create the whole format (e.g json/human-readable) etc in main.cpp and have w2xconv only call some functions there..

If we can't do that I would have to define the json logic in w2xconv and main.

Unless theres another way?

EDIT: is it enough to pass function pointers?

And can we combine many functions into 1 pointer?

Maybe a pointer to a struct of functions?

-> https://stackoverflow.com/a/17052566 ? but idk

Kinda like building a simple event system? If so is it possible to check if the pointer is nullptr from within wx2conv.

EDIT2: I just dont see having a ton of function arguments as a good solution:

w2xconv_init(enum W2XConvGPUMode gpu, int nJob, int log_level, function1,function2,function3,function4)

Looks very hacky to me.

YukihoAA commented 5 years ago

You cannot use functions in main.cpp inside of dll directly because it compiled separately.

if you declare some event system class by using struct in w2xconv.cpp and calling it from main.cpp is working but it is not in "C" way. (but actually you can use it anyway... who cares?)

but.. This project is created in C because original creator wanted to extremely speed up. JSON parsing is quite slow to support.. I doubt it has value to loosing dependence and speed..

I think bypass cout and cerr to string buffer in main.cpp, parse it, and make format for that is more in "C" way and it won't make slowdown if we made toggle function and there is no dependence problem.

and.. if you need real-time support, you can use json parse thread to do that.

if you want to print json in cout, you need to bypass default output or create custom pipeline to other program anyway so.. i think it is better to implement

YukihoAA commented 5 years ago

you can make function pointers in a struct and pass it. it works. (but not recommended due to dependence)

DeadSix27 commented 5 years ago

So, catching stderr and stdout in main.cpp, implement a whole new parser just to parse my own stdout and then stdout it in a different format to make it the most "C" ?

That said, since I'd be capturing stdout from w2xlib the original format can be simplified right, comma seperated lists of values, easier to parse.

But still, sounds annoying, I was hoping it's possible to have libw2x announce its logs, and everyone using libw2x can either subscribe to it or ignore it and get nothing to stdout.

And in main.cpp we just don't ignore it.

YukihoAA commented 5 years ago

Yes C language is not comfortable to programmers..

I read CMakeLists.txt, and i found out w2xconv.h, .cpp is "C" but the others does not (c++).

You can create some logger class out side of w2xconv.cpp (*but inside of lib), but i have no idea that works in w2xconv.cpp or not.

EDIT: it works because i declared W2Mat in "Class" but it works fine. (of course it is not "C" way but It works so.. whatever)

Implement Logger class inside of libw2x but outside of w2xconv, and declare getJSON() function in w2xconv.h is could be solution? we can handover setting like stdout or toggle funcions in a struct anyway.

EDIT2: I'd recommend creating new cpp like w2xlog.cpp ?

EDIT3: I think there was some error handling function like setPathError()..

DeadSix27 commented 5 years ago

Ye, setPathError handles errors from w2xconv to main, however its just passing strings/ints around at single points in the code.

E.g when main.cpp calls check_for_errors().

We'd need a thread to constantly do check_for_logs() which sounds inefficient to me..

however maybe that fixes the log slowing down the processing, not sure.. what would stop w2xconv from just continuing and not waiting for check_for_logs() to finish displaying, nothing.. unless we make it a buffer/queue and check_for_logs() has to dequeue log lines until its empty.. which in many cases will result in the log taking longer than the conversion but won't slow it down anymore.. ?

I'll prob do the w2xlog.cpp and take w2mat as example.. but this will take me some time.. Im not the biggest fan of C/cpp 🙃

maybe... _waifu2x-converter-rust_ 🙄