implement lock guards

[brabebhin]

Done.

I've been thinking - should we introduce result classes for the various public methods that we expose? For example, when we create the media sources, we sometimes may return null, or some other exception when it is not possible to create the souce. As we have seen, it isn't trivial to pass meaningful error messages via exceptions through C++/winRT, and returning null is sometimes a trap for the caller. We could introduce a generic Result and wrap exceptions, ffmpeg error messages and other stuff like that through it.

[lukasf]

I don't know. It would be a pretty huge breaking change if we'd modify the result type of the existing public methods. I understand that lib users would like more detailed error messages. Return classes would allow for that, theoretically. But fact is, if a file or stream cannot be opened, the only way to really find out why it failed is to run it with debugger attached and check the debug output. FFmpeg has return values, but most of the time they are more or less generic or meaningless, and come from somewhere deep within the lib. You only know if a call worked or not, but you don't know why it did not work in case of error. So if we cannot provide detailed error messages anyways, we should not start breaking APIs when in the end, we would just replace one generic error message with a different generic error message.

There are quite a lot of MediaFoundation HResult error codes, which show meaningful messages when used. We might find some which are better than the generic E_FAIL, and use them for the important cases. This way, we could improve error messages to some degree, without breaking our public API.

Also, using return classes would enforce a result checking workflow, which is something that C# devs are not really used to (and personally I am also not a fan of that style). Our Create methods are supposed to always return a value or throw an exception. If there are cases where we return null, I'd consider that a bug, so if you find such a case, please open an issue.

Accessing a disposed class is a different topic. Frankly, I never really thought about how we should react on calls after being disposed. We could also consider adding checks in every public method call (including properties), and always throw a specifc error (E_UNEXPECTED?). Then we'd have a clear policy on that, and no "surprise" null values.

[lukasf]

I just found that RO_E_CLOSED seems to map to ObjectDisposedException. This code is also explicitly mentioned in the IClosable interface. So this is probably what we should use, if we want to error out on member access after closed.

[brabebhin]

Personally, I am not a fan of exceptions, and I find exceptions in C# are abused (not as much as java, but still abused). The general problem of exceptions is that they affect the predictibility of your code. The way I see it, there's 2 usable approaches to exceptions: you either wrap the place where the dangerous call occurs with try-catch, which is essentially equivalent to checking error codes or return values, or you use a global try-catch around your message loop and simply discard every single invalid operation, no matter the cost. The problem with this second apporach is that in today's multithreaded environment, it isn't always obvious where the exception will end up. And it also absoluteley destroies performance because of stack unwinding.

Any other usage of exceptions beyond these 2 is simply asking for trouble. In the majority of cases, the place where the dangerous calls occurs is the only place where you can take meaningful action on the exceptions.

Another magor disadvantaje of exception usage is performance: they absolutely destroy performance if you get more than 1 or 2. One of the reasons windows.storage APIs is so slow is that they throw exceptions for file not found, it was so ridiculous that Microsoft had to provide a try-get method. There are many other try-get methods in the .net framework, and they are specifically turned for performance.

At my project at work we took the decission to simply follow method 1 (we still have a global try-catch but that is to help us catch unexpected errors and put them in place using method 1). The only methods allowed to throw exceptions are third party libraries or the .net framework itself. No authored method throws exceptions, we only use result classes. (this lib comes in handy: https://github.com/altmann/FluentResults) Needless to say, our web API is the most stable and most performant out of all others in the company :)

I had a bit more thought on the matter and it probably is fine for us to throw exceptions instead of returning null. The reason is we can't really catch all the errors in ffmpeg and media foundation anyway (cause of multithreaded environment), so in a way the choice was already made for us by others.

[lukasf]

Exceptions should only occur in exceptional situations. If someone uses exceptions for normal control flow, they are abusing the concept (and they will pay a performance penalty for that). But for 99.9% of method calls, you expect them to work all the time. Nothing wrong with throwing exceptions in the very rare cases where they do not work (due to circumstances usually out of your control - or due to bugs in your code or lib code). Like, if there is 1 in 1000 video files that cannot be opened, I see nothing wrong in throwing an exception. There is nothing you can do about that anyways.

In my experience there are very few exceptional situations where you could really recover the problem. In these very few places, you can have a narrow try/catch (or even better, try to use APIs to detect the error beforehand and avoid the exception to occur). For all other cases you have high level try/catch. This saves you results checking for 99% of method calls, and still makes sure you can recover from the very few recoverable situations, plus makes sure that no other error slips through.

The problem with result checking approach is that it is very tedious and people are lazy. So often enough they just won't check results, assuming it is going to work out fine because they think they are using the APIs correctly. This leads to increased risk of errors going unnoticed, leading to bad crashes. If you look into the ffmpeg sources, you will see quite a lot of calls where results are not checked (especially the older code). Using exceptions has the benefit that errors cannot go unnoticed.

But in the end it is personal preference. I had this kind of discussions with other devs as well. If your team agrees that it is the best approach for them, and you are strict in checking each and every result, then you should use that approach. My teams were pretty successful without.

[yikuo123]

Hello, is there any update?

I think the current solution is good enough for our current use case.

[brabebhin]

It just needs approval now i think.

[brabebhin]

Nuget package will be uploaded tomorrow.