Decoding Async API

[onepiecefreak3]

This is the current design for the async API. Since I'm more familiar with the decoding pipeline, I only implemented it for decoding and I'd like a review of it, if the design is ok like that. I'm especially interested in feedback to the AsyncOperation class and how I changed the BaseBcBlockDecoder and its usage of ReadOnlyMemory.

If the design is approved for decoding, I will implement it for encoding. A change like switching from ReadOnlySpan to ReadOnlyMemory is not necessary in the encoding.

[Nominom]

Looks good now. You can make it similarily for the encoding.

Thank you for all the help btw. I really appreciate it. You're the first contributor I've had besides some people adding issues.

[onepiecefreak3]

Sure, no problem. Of course I want to use your library in the end as well, but I saw potential so I focus on it to try and improve it.

[onepiecefreak3]

I know how it feels to have no other (active) contributors to your projects. I have the same with Kuriimu2. ^^

This is not the last commit. I missed to add a check against the cancellation in non-parallel encoding.

[Nominom]

I got a quick look and it's looking good so far. I'll scan over the code more thoroughly tomorrow. Do you want to add tests or shall I? At least one test for each of the async functions (just check that they work basically) and one for cancelling a parallel operation and another to cancel a non-parallel one.

I was thinking, does the Parallel.For throw when it's cancelled? Seems like the Microsoft examples use token.ThrowIfCancelled or something like that. If parallel.for throws I think the non parallel should throw as well.

[onepiecefreak3]

Yes, the Parallel.For currently thorws when cancelled. I guess that is the expected behaviour, so I will add throwing to the non-parallel cancellations.

And testing each async method could be quite heavy, wouldn't it? Wouldn't it double the test load (since nearly every public method has an async overload)?

[Nominom]

The Quality option can be set to Fast for the Async tests. With fast the tests take like 100ms to complete.

[onepiecefreak3]

I was rather referring to the number of tests, not their runtime ^^;

[Nominom]

Ah, well... It's like 10? more tests if we don't check every encoding and just assume if one works, they all work. I feel like the more tests, the safer it is to make changes in the future.

[onepiecefreak3]

Basically we can actually test one and know that all would work. All async methods are just one-liners referencing the synchronous method basically and just pass through the token. So if the syncrhonous calls work, what we need to test is actually if the cancellation works. And we can quickly observe that all async methods start a task by looking at them.

Beside that, I did add tests to test the cancellation itself for now.

[Nominom]

Maybe they don't all need a test. But I don't see a major downside to adding them. They act as more of a safeguard if some refactoring changes something unexpectedly some day.

[onepiecefreak3]

Then I will add cancellation tests for each decode and encode overload tomorrow.

[onepiecefreak3]

DecodeAllMipMapsAsync - TestGradient1; created KTX contains mipmaps with wrong sizes EncodeToDdsAsync - TestGradient1; created DDS contains mipmaps with wrong sizes

Those 2 descriptions are reminders to check why those 2 tests are not successful. The current reason is as described, but I don't know how it comes to that, or how to fix it. So the test build will fail for this commit.

[Nominom]

Seems like you did some refactoring work on the tests while you were at it. It's looking good.

[onepiecefreak3]

Since Ktx and Dds are used multiple times to their respective loaders, disposing them anywhere would interfere with the remaining tests. Otherwise I don't see any more FileStreams that are not closed by a using statement in the end. Therefore I will not do more changes to the current tests.