Skip to content

core(logger): strip opencv's modules base path#22924

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
alalek:logger_strip_base_dir
Dec 13, 2022
Merged

core(logger): strip opencv's modules base path#22924
asmorkalov merged 1 commit intoopencv:4.xfrom
alalek:logger_strip_base_dir

Conversation

@alalek
Copy link
Copy Markdown
Member

@alalek alalek commented Dec 6, 2022

  • Strip path from logger, keep file name only.
  • Reverted CV_Error() changes.

@asmorkalov
Copy link
Copy Markdown
Contributor

Full path to code indicates if OpenCV is build with CI (OpenCV Python, Android SDK, etc) or it's own build of OpenCV. I treat it as useful feature and prefer to stay as is.

@mshabunin
Copy link
Copy Markdown
Contributor

Logs with this PR:

[ WARN:0@0.092] global videoio/src/cap_gstreamer.cpp (2074) open OpenCV | GStreamer warning: OpenCV backend does not support passed FOURCC value
[ WARN:0@0.092] global videoio/src/cap_gstreamer.cpp (1820) close_ OpenCV | GStreamer warning: No source in GStreamer pipeline. Ignore

I vote for even shorter logs 😄

[ WARN:0@0.059] global cap_gstreamer.cpp:2074:open OpenCV | GStreamer warning: OpenCV backend does not support passed FOURCC value
[ WARN:0@0.059] global cap_gstreamer.cpp:1820:close_ OpenCV | GStreamer warning: No source in GStreamer pipeline. Ignore

Same with cleaner log messages and log tag set:

[ WARN:0@0.059] videoio cap_gstreamer.cpp:2074:open OpenCV backend does not support passed FOURCC value
[ WARN:0@0.059] videoio cap_gstreamer.cpp:1820:close_ No source in GStreamer pipeline. Ignore

For reference - GStreamer logs (timestamp - pid? - address? - level - domain - file:line:function: message)

0:00:00.006281520 59922 0x5613a42f7800 INFO              GST_STATES gstelement.c:2806:gst_element_continue_state:<pipeline0> completed state change to NULL
0:00:00.006287489 59922 0x5613a42f7800 INFO         GST_REFCOUNTING gstelement.c:3382:gst_element_dispose:<pipeline0> 0x5613a4572180 dispose

Full path to code indicates if OpenCV is build with CI
I treat it as useful feature

I agree only partially, yes it helps to distinguish official opencv-python from manually built package, but that is all. Without version/platform/env details it is useless, we still need cmake/build logs and getBuildInformation output to be able to help in remote psychic debugging 🙂

@alalek
Copy link
Copy Markdown
Member Author

alalek commented Dec 7, 2022

Currently there are 2 changes:

  • logger messages
  • error messages from CV_Error()

The last one could be reverted to provide full path with error messages.

@asmorkalov
Copy link
Copy Markdown
Contributor

Vote for the last proposal from alalek with CV_Error exception.

Comment on lines +238 to +250
std::string file_s(file);
size_t pos1 = file_s.rfind('/');
size_t pos2 = file_s.rfind('\\');
size_t pos = 0;
if (pos2 == std::string::npos)
pos = pos1;
else if (pos1 == std::string::npos)
pos = pos2;
else
pos = std::max(pos1, pos2);
if (pos == std::string::npos || pos >= file_s.length() - 1)
return file;
return file + pos + 1;
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we use C for this function? Something like:

const char * pos = std::max(strrchr(file, '/'), strrchr(file, '\\'));
if (!pos)
    return file;
return pos + 1;

It will be inlined and evaluated at compile time unlike C++ (https://godbolt.org/z/8dxj8j46x).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced C++ verbose code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, for some reason only clang (5+) is able to evaluate this function at compile time, even the latest GCC adds an inlined function call: https://godbolt.org/z/v8PE7K85h While the variant with strrchr is consistently replaced with a precomputed pointer by older compilers such as GCC 4.6.4 and clang 3.6.

Also I just thought that in order to enable compile time evaluation we need to directly wrap __FILE__ macro here:

On the other hand if we do so then in builds without optimization (-O0) each log message will add this function call.

Perhaps I'm digging too deep here and the simple readable code is fine in the end.

Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Works well for me. Tested manually with Ubuntu 18.04.

@asmorkalov asmorkalov merged commit 1788c93 into opencv:4.x Dec 13, 2022
@alalek alalek mentioned this pull request Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants