Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jiahaog
Copy link
Member

@jiahaog jiahaog commented Mar 8, 2024

Internally, when targeting x86 Android, these files fail to compile:

impeller/renderer/backend/vulkan/yuv_conversion_vk.cc:33:22: error: conditional expression is ambiguous; 'const impeller::vk::SamplerYcbcrConversion' can be converted to 'unsigned long long' and vice versa
  return conversion_ ? conversion_.get() : VK_NULL_HANDLE;
                     ^ ~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~
impeller/renderer/backend/vulkan/pipeline_vk.cc:360:25: error: conditional expression is ambiguous; 'vk::Sampler' can be converted to 'unsigned long long' and vice versa
      immutable_sampler ? immutable_sampler->GetSampler() : VK_NULL_HANDLE;
                        ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~

The cause is that internally, VULKAN_HPP_TYPESAFE_CONVERSION is set but not for the GN build here. Copying from the vulkan docs:

On 64-bit platforms Vulkan-Hpp supports implicit conversions between C++ Vulkan handles and C Vulkan handles. On 32-bit platforms all non-dispatchable handles are defined as uint64_t, thus preventing type-conversion checks at compile time which would catch assignments between incompatible handle types. Due to that Vulkan-Hpp does not enable implicit conversion for 32-bit platforms by default and it is recommended to use a static_cast for the conversion like this: VkImage = static_cast<VkImage>(cppImage) to prevent converting some arbitrary int to a handle or vice versa by accident. If you're developing your code on a 64-bit platform, but want compile your code for a 32-bit platform without adding the explicit casts you can define VULKAN_HPP_TYPESAFE_CONVERSION to 1 in your build system or before including vulkan.hpp. On 64-bit platforms this define is set to 1 by default and can be set to 0 to disable implicit conversions.

I'm not sure if doing the static_cast on the VK_NULL_HANDLE in this PR is right though. When targeting x86, this macro ends up being defined as 0LL. But at the same time, putting the cast on the other ternary branch (e.g. static_cast<vk::Sampler>(immutable_sampler->GetSampler())) doesn't resolve the error.

List which issues are fixed by this PR. You must list at least one issue.
b/328355475

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@jiahaog
Copy link
Member Author

jiahaog commented Mar 8, 2024

Chinmay, can you take a look if this is right?

@chinmaygarde
Copy link
Member

This is a bit of a head scratcher. I could not get the error to reproduce on our build even after setting VULKAN_HPP_TYPESAFE_CONVERSION to zero as stated in the docs. That seems to be because the header just checks for the definition of the macro here. Not what it it is defined to.

This seems like a bug. But I am honestly not sure.

Your patch seems fine to me to unblock the roll and we should land it. But I don't think we have a way of avoiding this same issue in the future. Can you file a separate issue for 32 bit platforms please? Otherwise LGTM on this patch. Thanks!

@jiahaog jiahaog marked this pull request as ready for review March 11, 2024 00:30
@jiahaog jiahaog added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 11, 2024
@jiahaog
Copy link
Member Author

jiahaog commented Mar 11, 2024

Thanks! I was quite confused as well - here's my understanding of the issue. These ternary expressions in this PR fails to build when targeting 32-bit platforms, when the VULKAN_HPP_TYPESAFE_CONVERSION define is enabled. So we actually need to enable VULKAN_HPP_TYPESAFE_CONVERSION to reproduce the error. I was able to reproduce it by setting VULKAN_HPP_TYPESAFE_CONVERSION on :vulkan_headers_config here (with ./flutter/tools/gn --android --android-cpu=x86 --runtime-mode=debug && ninja -C out/android_debug_x86).

I have filed flutter/flutter#144916 to track this :)

@auto-submit auto-submit bot merged commit 210f84e into flutter:main Mar 11, 2024
@jiahaog jiahaog deleted the static-check branch March 11, 2024 07:08
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants