Conversation
|
This patch changes key behavior of imwrite - it returns boolean flag to indicate success of operation. Nobody expect exceptions here. So as the solution mentioned issue we expected documentation improvement regarding it. |
4cb93dd to
5538b70
Compare
|
@dkurt Output of imwrite will still be a boolean, however, permission denied error message will simply be printed on stdcerr in case of exception as it is being catched in try { } catch { } block. This patch simply describes the permission denied error so it will be easier to debug for end user. I do notice that imwrite does not always return a boolean i.e. if invalid file extension is specified: cv2.imwrite('test.abc',img), it will throw an exception and exit code. I'm sorry if I misunderstood your comment, I'm open to suggestions. |
5538b70 to
789d048
Compare
alalek
left a comment
There was a problem hiding this comment.
It is better to avoid trying file before decoding.
Avoid changes in "successful path".
Perhaps it make sense to try:
- moving this check in encoders itself (some of them don't open files directly - uses 3rdparty libraries which we should not change)
- and/or trying permission denied diagnostic only if encoding call was failed.
modules/imgcodecs/src/loadsave.cpp
Outdated
| { | ||
| if (errno == EACCES) | ||
| { | ||
| CV_Error( Error::StsError, "permission denied" ); |
There was a problem hiding this comment.
Please use logging:
CV_LOG_WARNING(NULL, "imwrite(): Can't open file for writing: " << filename);
return false;
There was a problem hiding this comment.
I have updated my code accordingly. I did initially consider trying for file access in all encoders however, as you've said, the implementations of encoder.write are different. This could potentially raise more issues in the process and may not be so elegant. Thus I've opted for second suggestion.
modules/imgcodecs/src/loadsave.cpp
Outdated
| else | ||
| { | ||
| fclose(f); | ||
| remove(filename.c_str()); |
There was a problem hiding this comment.
There is some chance that we may get problem like this again: #12342
See general note.
There was a problem hiding this comment.
I believe this issue is not something caused by OpenCV itself but rather an inherent could-be problem that can occur in some OS's like Windows which don't allow mutual sharing of file. In the unlikely scenario that the program lands on this else condition, it will at worse create a temporary file that is not removed, imwrite will still not crash as remove will simply return non zero. I think this could occur if another process from OS accesses the temporary file in between fclose() and remove() function. I will try to generate this occurrence by building opencv without the condition imposed by if( !f ) and share my findings. Additionally, I could further add warning log if fclose() and remove() do not return 0.
|
@omasaht, thanks, got it! With code example it's clear now. |
…write_ function when user does not have write permission
789d048 to
8dd0fd8
Compare
resolves #15155
This pull request adds changes to modules/imgcodecs/src/loadsave.cpp
imwrite_ now describes permission denied error when a user does not have write permission in directory.
Example:
Code Output: