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
Contributor

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
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 11, 2024
flutter/engine@d13999c...210f84e

2024-03-11 jiahaog@users.noreply.github.com Disambiguate conditional expressions (flutter/engine#51285)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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