Improve error handling throughout the library

[axxel]

@shoffmeister brought up the question of whether the habit of quoting class/function names in error strings adds value (https://github.com/darktable-org/rawspeed/pull/61#discussion_r99428209). There is a (inconsistently used) pattern that looks like this:

  if (frame.cps != soscps)
    ThrowRDE("LJpegDecompressor::parseSOS: Component number mismatch.");

This pattern has two issues:

• manually typed class/function name
• there are different ThrowXXX options available that have questionable value, expecially as they lead to other code patterns like the following:

  try {
  return checkSupportInternal(meta);
  } catch (TiffParserException &e) {
  ThrowRDE("%s", e.what());
  } catch (FileIOException &e) {
  ThrowRDE("%s", e.what());
  } catch (IOException &e) {
  ThrowRDE("%s", e.what());
  }

  And that is both verbose and error prone. Both the set of exception classes as well as the way they are used could be improved.

First question: what exception types are useful?

From a library users point of view, I see only two 'types' of problems: 1) unsupported file: he can't do anything about it, except maybe contact the developers and provide a sample 2) corrupted file (probably truncated, maybe otherwise 'randomly' broken): he can try to copy it again from his camera, there is no point in contacting the developers.

Unfortunately, the two cases can not reliably be distinguished as long as the library does not implement each and every spec completely (e.g. only one LJpeg predictor mode is currently implemented, so we bail out if predictorMode != 1. We can not say for sure whether that is a valid file with another predictor or a broken file with the right predictor). Even with only 95% accuracy, I'd say it would still be valuable to make that distinction.

From a developers point of view, we might need more diverse types internally, that would have to be determined by inspecting the different catch() sites and see what they actually make of the type. I'd expect to find quite a few inconsistent uses / bugs there.

Second question: what error messages are useful?

From a users point of view, there really is only the question of unsupported vs. corrupted. Information like Component number mismatch provides no value to him at all.

From a develops point of view, I see the need to identify the exact location in the source code if something goes wrong (the function name is only half way).

Before I continue this with some concrete suggestions about how we can better achieve the above goals, I'll wait for some feedback on this 'analysis'.

[axxel]

@LebedevRI @schenlap @LibRaw any feedback on this? (I guess a developer mailing list would have been the better place to post this, but since there is none...)

[LebedevRI]

I plan to try to make the exceptions automatically print from where they are thrown (filename.cpp without path, with line number, possibly with function name)

[axxel]

That is what I also wanted to suggest in the "how to get there" department after there has been some consensus on what the library should behave, as seen from the 'outside'.

I implemented something like that (automatic attachment of file/line/function info) for libgphoto2 a couple of years back (e.g. here and here).

So in case the above analysis makes sens for everyone I propose two things: 1) Introduce two new exceptions as sub classes of the new RawspeedException for the two errors a use needs to distinguish. Remove all other exceptions if they don't provide any added value for some internal error distinguishing. Make them a subclass of either one of the two. This gets rid of all the mentioned type-conversion code snippets. 2) Change the ThrowXXX functions to be macros, so we can access the file/line/function information at call site automatically. This should actually allow to completely remove the error strings, but they could be preserved if desired. The macro would also allow to automatically create an error message by 'stringifying' the expression. To give an example of what this could look like (replacement for the above if() Throw... line):

CHECK_VALID(frame.cps == soscps);

This would throw some InvalidRawFile exception that could have the following message: Invalid/broken raw image file: assertion "frame.cps == soscps" failed in LJpegDecompressor.cpp:245

This would remove lots of text from the code base and automatically produce some helpful, always up to date error information. The other macro would be something like CHECK_SUPP to tell the user that the file is unsupported instead of broken.

[LibRaw]

My 3 cents

• we use RawSpeed as decoder, not as metadata parser, so some 'unsupported' files (e.g. Nikons banned based on make/model/bit depth because of unknown black level) are really supported from our point.
• so: -- corrupted file should be reported -- unsupported format (unknown predictor) should be reported separately -- supported format, but unknown camera (not in cameras.xml) - separate reporting and possibility to disable this reporting, but 'just unpack it'.

[LebedevRI]

Remove all other exceptions if they don't provide any added value for some internal error distinguishing.

I will not merge this. I do not know at the moment whether the current exceptions make sense or not, so i can only make an educated guess. And if i turn out to be wrong, guess who will have to change it back?

[axxel]

@LibRaw thanks for you feedback. Completely fits into my picture. The possibility to process 'unknown' cameras would not be affected. And the error thrown if the check is enabled can of course be a 3rd type.

@LebedevRI I don't have full picture myself of every catch() at the moment but I noticed two things: Klaus seemed to be eager to make sure only one type of exceptions ever reaches client code so there is no external code depending on different internal exceptions and going over a couple of try/catch sites you'll not find any rocket science there.

And to answer your question: you will have to change it, because no one else seems to have write access to the repository ;).

[LebedevRI]

no one else seems to have write access to the repository ;).

Everyone in the @darktable-org has write access

[axxel]

Everyone in the @darktable-org has write access

Fair enough :). Then my problem is that I've never seen anyone touch the repo except you. But this comment of mine has to be interpreted in the context of my promise to fix anything that my efforts on the code base might break.

Then again: all this is just a proposal, trying to improve the maintainability and usability of the code. And we are discussing it, good enough for me.

[LebedevRI]

True, not many even touch the darktable-org/darktable repo Camera support has always been an outcast, with very few contributors

[axxel]

There is one more aspect to this subject that I did not mention before: There are lots of places where some error is caught, added as string to the error-log of the raw image and then ignored, so maybe half a frame was successfully decoded but the file was truncated. Idea: still perform following processing steps like scaling or bad pixel correction as half a frame might be better than nothing. This approach, however, is inconsistently applied. So if valuable, it should be improved.

For my application domain, this has no value. If the file is corrupt, the content is useless to me. Question @LebedevRI and @LibRaw: is dt or LibRaw using this feature? Are they looking at the error log and showing it to the user?

EDIT: Some stats: There are 84 catch sites in the code and they fall in 4 categories:

• completely ignore the exception
• log the error then ignore the exception
• throw some other type of exception
• do something more 'interesting' (there less than 10 of those and most of them have been touched/modified by me recently (e.g. Tiff parsing)).

With the new RawspeedExpection base class, at least the type conversion catches can be removed and the multiple copies of catching different exceptions and then doing the same logging for all of them can be reduced to one (or completely removed, see above).

[LebedevRI]

Are they looking at the error log and showing it to the user?

Yes, absolutely.

[LibRaw]

In LibRaw - we catch RawSpeed exceptions and raise internal warning flag and also try to unpack same file by (slower) LibRaw unpacker.

In calling applications (RawDigger, FastRawViewer) - we check this flag and result of LibRaw processing and may warn user if he select this option in preferences.

[axxel]

In LibRaw - we catch RawSpeed exceptions and raise internal warning flag and also try to unpack same file by (slower) LibRaw unpacker.

But what about the RawImage::errors vector of strings that contain this 'log' that I was referring to? It may very well be the decodeRaw call went through without throwing but this error log explains why you get a completely black frame. So you would have to catch and check this variable to be sure everything is fine.

Little idea: similar to the 'failOnUnknown' flag, we could add a symmetric 'failOnCorrupt' flag and if that is true, you get an exception if the error log is non-empty. So in the LibRaw use case you'd set the first to false but the second to true and in dt the other way around.

Yes, absolutely.

So you also consider it a valid use case to decode a half-broken file? (I totally see this as potentially useful, just not for my application and I wanted to hear what others have to say about it.)

[LebedevRI]

just not for my application

You keep saying that, yet never name it. Is that information secret?

[axxel]

You keep saying that, yet never name it. Is that information secret?

Not at all: www.lizardq.com. I did not mention it explicitly before but mentioned that I'm running a small company. And I believe google(my name) points you there without detour. :)

[LibRaw]

But what about the RawImage::errors vector of strings that contain this 'log' that I was referring to

We check size of this log, not contents.