This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Impeller] remove Vulkan command encoder abstraction, use command buffer vk. #55680
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonahwilliams
commented
Oct 6, 2024
| if (auto command_buffer = GetCommandBuffer()) { | ||
| command_buffer.insertDebugUtilsLabelEXT(label_info); | ||
| } | ||
| if (queue_) { |
Contributor
Author
There was a problem hiding this comment.
TBD: what was this for?
Contributor
Author
There was a problem hiding this comment.
Asking because if I remove this, the command buffer no longer needs a reference to the queue.
Member
There was a problem hiding this comment.
I was inserting the same marker in both the queue for the buffer for ease of debugging. But AFAICT, having the marker (push-pop) in the buffer is sufficient. This complicates other things like reset. So let's get rid of it.
chinmaygarde
approved these changes
Oct 7, 2024
Contributor
Author
|
Need to update to remove queue reference. |
matanlurey
approved these changes
Oct 7, 2024
jonahwilliams
added
the
autosubmit
gaaclarke
reviewed
Oct 7, 2024
Member
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I like the refactor, just a few notes.
| for (const std::shared_ptr<CommandBuffer>& buffer : buffers) { | ||
| auto encoder = CommandBufferVK::Cast(*buffer).GetEncoder(); | ||
| if (!encoder->EndCommandBuffer()) { | ||
| CommandBufferVK& command_buffer = CommandBufferVK::Cast(*buffer); |
Member
There was a problem hiding this comment.
This should be CommandBufferVK*. We are discarding the fact that this can be null.
| ~CommandBufferVK() override; | ||
|
|
||
| const std::shared_ptr<CommandEncoderVK>& GetEncoder(); | ||
| // Encoder Functionality |
Member
There was a problem hiding this comment.
Suggested change
| // Encoder Functionality |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 7, 2024
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Oct 7, 2024
…156364) flutter/engine@e84e303...427302e 2024-10-07 30870216+gaaclarke@users.noreply.github.com Speculative fix for memory issues related to retrying image decompression (flutter/engine#55704) 2024-10-07 jonahwilliams@google.com [Impeller] disable surface control on API 29. (flutter/engine#55708) 2024-10-07 jonahwilliams@google.com [Impeller] remove Vulkan command encoder abstraction, use command buffer vk. (flutter/engine#55680) 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 matanl@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
nick9822
pushed a commit
to nick9822/flutter
that referenced
this pull request
Dec 18, 2024
…fer vk. (flutter/engine#55680) The Vulkan command buffer wrapper delegates almost all of its functionality to the "CommandEncoderVK". I did not find that this separation was useful, as they have a 1-1 relationship and an identical lifecycle. Lets combine them to reduce the number of things to worry about.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Vulkan command buffer wrapper delegates almost all of its functionality to the "CommandEncoderVK". I did not find that this separation was useful, as they have a 1-1 relationship and an identical lifecycle. Lets combine them to reduce the number of things to worry about.