DarthAffe / StableDiffusion.NET

C# Wrapper for StableDiffusion.cpp
MIT License
60 stars 10 forks source link

Unsafe code To safe code #1

Open IntptrMax opened 8 months ago

IntptrMax commented 8 months ago

There are some unsafe code, would you like to change it to safe code only?

DarthAffe commented 8 months ago

Currently I don't have any plans on doing this. It would add additional overhead and complexity without any benefit (at least I'm not able to see any). There's nothing unsafe exposed through the API so consumers of the library do not need to enable unsafe code and even if everything would be handled without unsafe blocks the native memory would still need to be handled in an unsafe way.

DGdev91 commented 6 months ago

I see your point, but it still needs to be worked on a bit. Right now, any error on the native code makes the whole application crash even if the calling c# method is under a try/catch block, and this makes impossible to handle errors on application side

DarthAffe commented 6 months ago

I see your point, but it still needs to be worked on a bit. Right now, any error on the native code makes the whole application crash even if the calling c# method is under a try/catch block, and this makes impossible to handle errors on application side

Do you have a specific example of such an error? The only ones I've encountered so far (and they would fit your description of not being catchable) are access violations (which is just another name for segmentation fault) and these can not be catched as they indicate a corrupted stack. There where some (officially unrecommended) options to enable exception handling for them, but they are no longer available in modern .net.

All cases of these need to be fixed and can't be handled by application itself. If the error also occurs in the example application of stable-diffusion.cpp (like it's the case with unsupported chars in the prompt) it needs to be fixed there (nothing I can do), if not it might be a marshalling mistake and would require some steps to reproduce the error.

DGdev91 commented 6 months ago

Do you have a specific example of such an error? The only ones I've encountered so far (and they would fit your description of not being catchable) are access violations (which is just another name for segmentation fault) and these can not be catched as they indicate a corrupted stack. There where some (officially unrecommended) options to enable exception handling for them, but they are no longer available in modern .net.

All cases of these need to be fixed and can't be handled by application itself. If the error also occurs in the example application of stable-diffusion.cpp (like it's the case with unsupported chars in the prompt) it needs to be fixed there (nothing I can do), if not it might be a marshalling mistake and would require some steps to reproduce the error.

Well, for example if the negative prompt is null it chashes after giving this: terminate called after throwing an instance of 'std::logic_error' what(): basic_string::_M_construct null not valid Not a big deal at all of course, as it's pretty straightforward to prevent it, it's just an example.

Also, there's a known bug in stablediffusion.cpp wich makes image generation fail when width/height is too high, when using cuda or rocm (see https://github.com/leejet/stable-diffusion.cpp/issues/156) in that case, if using your application the whole app crashes instead of just giving an exception.

I guess it can also happen when the user hasn't enough vram

DarthAffe commented 6 months ago

I tested both of them (null negative prompt and high image size). null prompt is causing a access violation, which is thrown but as i explained can't be catched; high image size is causing an error code 0xc0000409 (STATUS_STACK_BUFFER_OVERRUN) which is a fail fast exception that causes immediate termination of the process. both of them can't be handled and need to be fixed in stablediffusion.cpp.

What I can do ofc is to validate as much input as possible beforehand. Passing null as the negative prompt should already prompt a warning as it's defined as a non-nullable type, but I'll make sure it's not possible. The size thing though is kinda out of our control as I don't want to limit the input size - it might work with some models / in the future.

FSSRepo commented 6 months ago

I'm one of the individuals directly involved in the development of sd.cpp, and I recognize that the project has many limitations and numerous cases mishandled. With time and more assistance from the community, we could surely achieve something more stable.

Currently, the main barrier is the lack of documentation and the code's structure (almost unreadable code) we currently have. We're exploring better ways to refactor the code for improved readability and to enhance aspects like memory management, as many parts still assume fixed buffer sizes, leading to unexpected behaviors at times.

Some resolutions might cause issues across different models since kernels behave differently and may not account for cases with large tensors (especially in the hip backend of ggml, which hasn't been extensively tested), leading to illegal memory accesses.