gfx-rs / gfx

[maintenance mode] A low-overhead Vulkan-like GPU API for Rust.
http://gfx-rs.github.io/
Apache License 2.0
5.35k stars 549 forks source link

OpenGL backend lists all known formats as supported #3683

Closed mbrla0 closed 3 years ago

mbrla0 commented 3 years ago

This issue describes a problem with the OpenGL backend (gfx-backend-gl), in which inaccurate reporting of image format properties can easily lead to a crash when creating images in those formats.

Rationale

The main way one would go about checking for valid image creation parameters in the HAL, for use in Device::create_image, is by calling PhysicalDevice::image_format_properties (Whose Vulkan equivalent is vkGetPhysicalDeviceImageFormatProperties). One calls this function with parameters very close to those of Device::create_image and gets in return an Option<ImageFormatProperties>, which encodes in itself both whether the given set of parameters is supported or not and the ranges for which image parameters in the given format are valid.

However, the implementation of PhysicalDevice::image_format_properties in the OpenGL backend reports every format known to gfx_backend_gl::conv::describe_format as supported, which is definitely not the case for quite a number of those known formats in OpenGL ES. This leads to the use of formats like Format::Bgra8Unorm, crashing the application as a GL_INVALID_OPERATION is triggered by some drivers upon the backend calling glTexImage2D.

How to reproduce

I came across this problem in the Mesa 20.2.6 implementation of OpenGL ES 3.2, with the Mesa Intel(R) UHD Graphics 620 (KBL GT2) renderer. Keep in mind that this issue may not happen in more lenient implementations.

In order to verify and trigger this bug, one only needs to create an image through Device::create_image with parameter combinations not specified in the OpenGL ES 3 documentation for glTexImage2D, that PhysicalDevice::image_format_properties indicates as supported.

The following log is from my program, which I believe is easy enough to understand in context.

[2021-03-16T20:52:47Z TRACE creeper::graphics] [12/768] Probing image with usage 0x00000004, format of Bgra8Unorm and tiling of Optimal
[2021-03-16T20:52:47Z TRACE creeper::graphics] Image format properties: FormatProperties { max_extent: Extent { width: 4294967295, height: 4294967295, depth: 4294967295 }, max_levels: 255, max_layers: 65535, sample_count_mask: 127, max_resource_size: 18446744073709551615 }
Mesa: User error: GL_INVALID_OPERATION in glTexImage2D(format = GL_BGRA, type = GL_UNSIGNED_BYTE, internalformat = GL_RGBA8)
[2021-03-16T20:52:47Z ERROR gfx_backend_gl] [API/Error] ID 1 : GL_INVALID_OPERATION in glTexImage2D(format = GL_BGRA, type = GL_UNSIGNED_BYTE, internalformat = GL_RGBA8)
thread 'main' panicked at 'Error creating image: InvalidOperation for kind D2(10, 10, 1, 1) of Bgra8Unorm', /home/mbr/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx-backend-gl-0.7.1/src/device.rs:1548:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a601302ff0217b91589b5a7310a8a23adb843fdc/library/std/src/panicking.rs:495:5
   1: std::panicking::begin_panic_fmt
             at /rustc/a601302ff0217b91589b5a7310a8a23adb843fdc/library/std/src/panicking.rs:437:5
   2: <gfx_backend_gl::device::Device as gfx_hal::device::Device<gfx_backend_gl::Backend>>::create_image
             at /home/mbr/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx-backend-gl-0.7.1/src/device.rs:1548:13
   3: creeper::graphics::ImageProfile::for_image
   4: creeper::graphics::Adapter::probe_images
   5: creeper::graphics::Adapter::collect
   6: creeper::graphics::Graphics::collect::{{closure}}
             at ./creeper/src/graphics.rs:29:19
   7: core::iter::adapters::map_try_fold::{{closure}}
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/adapters/mod.rs:912:28
   8: core::iter::traits::iterator::Iterator::try_fold
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:1888:21
   9: <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::try_fold
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/adapters/mod.rs:938:9
  10: core::iter::traits::iterator::Iterator::find_map
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2263:9
  11: <core::iter::adapters::FilterMap<I,F> as core::iter::traits::iterator::Iterator>::next
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/adapters/mod.rs:1245:9
  12: <alloc::vec::Vec<T> as alloc::vec::SpecFromIterNested<T,I>>::from_iter
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec.rs:2082:32
  13: <alloc::vec::Vec<T> as alloc::vec::SpecFromIter<T,I>>::from_iter
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec.rs:2226:20
  14: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec.rs:1959:9
  15: core::iter::traits::iterator::Iterator::collect
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:1670:9
  16: creeper::graphics::Graphics::collect
             at ./creeper/src/graphics.rs:27:18
  17: creeper_dump::backends::unix::GraphicsBackends::collect
  18: creeper_dump::main
             at ./creeper/dump/src/main.rs:28:17
  19: core::ops::function::FnOnce::call_once
             at /home/mbr/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Proposed Solution

My proposed solution for this problem, which I plan to implement in a follow-up to this issue is to add a filter to PhysicalDevice::image_format_properties that removes input parameter combinations forbidden in the OpenGL ES specification, and to selectively lift some of these filters based on a combination of version and possibly relevant extensions.

Using version information to gate features is not a new thing to the OpenGL backend, as shown by functions such as gfx_backend_gl::PhysicalDevice::new_adapter. However, those are not as restrictive as a proper image format properties filter would be, which might pose a challenge when it comes to existing code targetting the HAL that relies on specific formats always being present.

kvark commented 3 years ago

Thank you for writing this down! I don't think there is a huge concern about breaking any apps here: they don't work on GL backend today. All we want to do is to expose the restrictions that are already in place.