Add diagnostic error information for Non-CLI applications about openjph HOT 6 CLOSED

Hi Aous,
I have successfully tested the 0.14.0 release and have confirmed that it resolves this issue. Many thanks.
Best Regards,
John Fralinger

from openjph.

Hi Fralinjh,

Thank you for looking at my code.
Before any modifications, I can see an error in message_error::operator() function.
It is currently,

if (error_stream == NULL)
  return;

fprintf(error_stream, "ojph error 0x%08X at %s:%d: ",
  error_code, file_name, line_num);
va_list args;
va_start(args, fmt);
vfprintf(error_stream, fmt, args);
fprintf(error_stream, "\n");
va_end(args);

throw std::runtime_error("ojph error");

but I think it should be -- not super sure, and need to test.

if (error_stream != NULL)
  fprintf(error_stream, "ojph error 0x%08X at %s:%d: ",
    error_code, file_name, line_num);
va_list args;
va_start(args, fmt);
if (error_stream != NULL) {
  vfprintf(error_stream, fmt, args);
  fprintf(error_stream, "\n");
}
va_end(args);

throw std::runtime_error("ojph error");

I am still learning, and this runtime_error is not an easy part.

Going back to your suggestion -- thank you.

1. Isn't the currently available printing to std_err enough? Is this a necessary feature, or a good to have?
2. How will the buffer char *buf0 be freed? I think it should be freed, otherwise an implementer can catch the exception and retry to use the library, causing a memory leak.
3. Throughout the code I tried to minimize, almost eliminate all new/delete or malloc/free from the code -- the code might under large revamp in a couple of months time.

Long discussion:
I think there is no clean solution, but if we really want this, then I can see a couple of options.

1. Copy buf0 contents to std::string, and then free it before throwing the exception -- too many allocations/de-allocations.
2. Create a class to hold the buffer with a destructor, freeing the memory -- not sure if this is a good idea.

Please let me know what you think.

Kind regards,
Aous.

PS:
Notes on https://en.cppreference.com/w/cpp/error/runtime_error says

Because copying std::runtime_error is not permitted to throw exceptions, this message is typically stored internally as a separately-allocated reference-counted string. 

It looks like the message is internally stored as std::string.

from openjph.

Hi Aous,
First let me apologize because the diffs I previously sent were applied to OpenJPH release 0.10.5. I should have referenced the trunk or specifically stated the OpenJPH version 0.10.5 when I opened the issue. I also agree that it is good to check error_stream for null and that the corrections you propose make sense. The use of varargs from C does add some alloc/dealloc annoyances that I tried to avoid but was unsuccessful even after searching the internet.
Like you said, error processing is always more difficult than it should be. Presenting errors and warnings etc. to the user via GUI applications seldom works using log files, then alone multiple log files that lack context synchronization. We prefer Error, Warning and Info callbacks as was done in OpenJPEG if that is possible. The info/warning/error strings in the callbacks can be logged with context, presented in a dialog or even just ignored.

Until then getting error diagnostics to the user via the code below is helpful.

  ////////////////////////////////////////////////////////////////////////////
  void message_error::operator()(int error_code, const char* file_name,
    int line_num, const char *fmt, ...)
  {
    size_t sz = snprintf(nullptr, 0, "ojph error 0x%08X at %s:%d: ", error_code, file_name, line_num);
    size_t bufsize = sz + 1;
    char *buf0 = static_cast<char*>(malloc(bufsize));
    snprintf(buf0, bufsize,"ojph error 0x%08X at %s:%d: ", error_code, file_name, line_num);
    std::string str0 = buf0;
    free(buf0);
    if (error_stream)
        fprintf(error_stream, str0.c_str()); 

    va_list args;
    va_start(args, fmt);
    sz = vsnprintf(nullptr, 0, fmt, args);
    bufsize = sz + 1;
    char *buf = static_cast<char*>(malloc(bufsize));
    assert(buf);
    vsnprintf(buf, bufsize, fmt, args);

    if (error_stream)
        vfprintf(error_stream, fmt, args);
    if (error_stream)
        fprintf(error_stream, "\n");
    va_end(args);
    std::string str = buf;
    free(buf);

    throw std::runtime_error(str0 + str);
  }
}

Once again,
Thank you for your contributions,
The code is useful and impressively fast.
Best Regards,
John Fralinger

from openjph.

Hi John,

Thank you for spending the time looking into this.
I guess this is the best currently available option.
Leave it with me; I will add a variation of this in the next version.

Kind regards,
Aous.

from openjph.

Hi John,

I am sorry I changed my mind. However, now you have a way to whatever you want on your side.

After reading the code, trying to remember what I did, I realized that there was, by design, a method that allows the end user to basically do whatever they want, but the code to achieve that was buggy. This has been fixed now.

So to support your use case, derive a new class from ojph::message_error, and define your own operator(), which should throw an exception before exiting. Then, tell the library to use this class by calling configure_error().

Here is a version of the code that you can use, part of which was to be used before changing my mind.

////////////////////////////////////////////////////////////////////////////
class my_error : public ojph::message_error
{
  public:
    void operator() (int warn_code, const char* file_name,
                     int line_num, const char *fmt, ...) override
      {
        int t = snprintf(NULL, 0, "ojph error 0x%08X at %s:%d: ", 
                        error_code, file_name, line_num);
        int total_bytes = t;
        int max_bytes = t;
        
        va_list args, args_copy;
        va_start(args, fmt);
        va_copy(args_copy, args);   // create a copy of args.
        t = vsnprintf(NULL, 0, fmt, args);
        total_bytes += t;
        max_bytes = ojph_max(max_bytes, t);
        t = snprintf(NULL, 0, "\n");
        total_bytes += t;
        max_bytes = ojph_max(max_bytes, t);
        va_end(args);
        ++max_bytes; // one extra
        ++total_bytes;
    
        constexpr int max_store = 256; 
        char tmp[max_store]; // on the stack, hopefully enough 
        char *p = tmp;
        if (max_bytes > max_store) // if more is needed then malloc
          p = (char*)malloc(max_bytes);
    
        std::string s(total_bytes, ' '); // create an string large enough
        snprintf(p, max_bytes, "ojph error 0x%08X at %s:%d: ", 
                 error_code, file_name, line_num);
        s = p;
        va_start(args_copy, fmt);
        vsnprintf(p, max_bytes, fmt, args_copy);
        va_end(args_copy);
        s += p;
        s += '\n';
    
        if (p != tmp)
          free(p);
    
        if (error_stream != NULL)
          fprintf(error_stream, "%s\n", s.c_str());
    
        throw std::runtime_error(s);
      }
};

Then you instantiate the object and call configure_error().

my_error e;
ojph::configure_error(&e);

Then any error will call your code.

Hope this helps.

Kind regards,
Aous.

from openjph.

Great to hear this.
Thank you for letting me know.
I am closing this issue.

Kind regards,
Aous.

from openjph.