Sixel support #4828
Sixel support #4828
Conversation
| Argument{ "file", 'f', Args::Type::Manifest, Resource::String::SourceListUpdatedNever, ArgumentType::Positional }, | ||
| Argument{ "aspect-ratio", 'a', Args::Type::AcceptPackageAgreements, Resource::String::SourceListUpdatedNever, ArgumentType::Standard }, | ||
| Argument{ "transparent", 't', Args::Type::AcceptSourceAgreements, Resource::String::SourceListUpdatedNever, ArgumentType::Flag }, | ||
| Argument{ "color-count", 'c', Args::Type::ConfigurationAcceptWarning, Resource::String::SourceListUpdatedNever, ArgumentType::Standard }, | ||
| Argument{ "width", 'w', Args::Type::AdminSettingEnable, Resource::String::SourceListUpdatedNever, ArgumentType::Standard }, | ||
| Argument{ "height", 'h', Args::Type::AllowReboot, Resource::String::SourceListUpdatedNever, ArgumentType::Standard }, | ||
| Argument{ "stretch", 's', Args::Type::AllVersions, Resource::String::SourceListUpdatedNever, ArgumentType::Flag }, | ||
| Argument{ "repeat", 'r', Args::Type::Name, Resource::String::SourceListUpdatedNever, ArgumentType::Flag }, | ||
| Argument{ "out-file", 'o', Args::Type::BlockingPin, Resource::String::SourceListUpdatedNever, ArgumentType::Standard }, |
There was a problem hiding this comment.
Nit: Probably not too important for a debug command, but the arg types and strings just feel wrong...
There was a problem hiding this comment.
Yes, the goal was to not introduce any new arguments just for debugging purposes. Rather than trying to find good type name fits, I just let autocomplete decide for me. I then cheekily chose the string "Never" for the descriptions.
If the arbitrary type names are too much, I could define macros to make it easier to read.
There was a problem hiding this comment.
And that is what I did.
src/AppInstallerCLICore/Sixel.cpp
Outdated
| UINT32 result = static_cast<UINT32>(input); | ||
| result *= 100; | ||
| UINT32 fractional = result % 255; | ||
| result /= 255; | ||
| return result + (fractional >= 128 ? 1 : 0); |
There was a problem hiding this comment.
I believe (input * 100 + 128) / 255 should roughly achieve the same. You could also opt to using a 256-ary LUT for better performance, although that likely won't help - I bet this function is not going to be a bottleneck. 😅
| { | ||
| try | ||
| { | ||
| ConsoleInputModeRestore inputMode; |
There was a problem hiding this comment.
ConsoleModeRestoreBase restores the original console mode (= no VT when run under conhost) on destruction, right? Doesn't this mean that inputMode should be preserved in a class member so that the restore is performed only when winget exits?
There was a problem hiding this comment.
While there are some mode flags that could probably be set for the entire process, disabling echo input is one that definitely needs to be done only while we receive the VT response. We do have some cases where we prompt the user and I prefer the echo there.
|
|
||
| // Response is of the form AICLI_VT_CSI ? <conformance level> ; (<extension number> ;)* c | ||
| std::string sequence = ExtractSequence(inStream, "[?", "c"); | ||
| std::vector<std::string> values = Utility::Split(sequence, ';'); |
There was a problem hiding this comment.
BTW, if you ever need it, I wrote a strtok alternative here: https://github.com/microsoft/terminal/blob/a7e47b711a2adc7b9e80eddea8168089f7d3b11e/src/inc/til/string.h#L351-L398
(It should be called tokenize or something instead of prefix_split.)
There's also a small-vector implementation that may be useful: https://github.com/microsoft/terminal/blob/a7e47b711a2adc7b9e80eddea8168089f7d3b11e/src/inc/til/small_vector.h
| const ConsoleModeRestore& ConsoleModeRestore::Instance() | ||
| { | ||
| static ConsoleModeRestore s_instance; | ||
| return s_instance; | ||
| } | ||
|
|
||
| ConsoleInputModeRestore::ConsoleInputModeRestore() : ConsoleModeRestoreBase(STD_INPUT_HANDLE) | ||
| { | ||
| m_token = InitializeMode(STD_INPUT_HANDLE, m_previousMode, { ENABLE_EXTENDED_FLAGS | ENABLE_VIRTUAL_TERMINAL_INPUT }, ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT | ENABLE_QUICK_EDIT_MODE); |
There was a problem hiding this comment.
I'm surprised you're disabling the quick edit mode. Is that intentional? Without it, users can't easily select text while winget is running.
| m_token = false; | ||
| } | ||
| } | ||
|
|
||
| ConsoleModeRestore::ConsoleModeRestore() : ConsoleModeRestoreBase(STD_OUTPUT_HANDLE) | ||
| { | ||
| m_token = InitializeMode(STD_OUTPUT_HANDLE, m_previousMode, { ENABLE_VIRTUAL_TERMINAL_PROCESSING | DISABLE_NEWLINE_AUTO_RETURN, ENABLE_VIRTUAL_TERMINAL_PROCESSING }); |
There was a problem hiding this comment.
FWIW, I'd personally always recommend against using the old mode as a basis for setting the new mode. After all, why should a previous console application be allowed to determine what modes you want?
| } | ||
|
|
||
| // Extracts a VT sequence, expected one of the form ESCAPE + prefix + result + suffix, returning the result part. | ||
| std::string ExtractSequence(std::istream& inStream, std::string_view prefix, std::string_view suffix) |
There was a problem hiding this comment.
If you assume that your suffix is a single character you could use inStream.getline() to do all the seeking for you automatically. That would also avoid reading past the end of the DA1 response if you believe that may be a concern.
Personally, at least in the beginning, I'd not care about consuming more input than needed or handling situations where the sequence isn't close to the start of the input buffer, so I'd do something like this:
char buf[1024];
const auto len = inStream.readsome(&buf[0], std::size(buf));
std::string_view response{ &buf[0], std::max(0, len) };
const auto beg = response.find('\x1b');
const auto end = response.find('c', beg);
std::string result;
if (beg < end) {
result.assign(response, beg, end - beg);
}
return result;
| @@ -21,6 +21,8 @@ namespace AppInstaller::Caching | |||
| IndexV2_PackageVersionData, | |||
| // Manifests for index V2. | |||
| IndexV2_Manifest, | |||
| // Icons | |||
| Icons, | |||
There was a problem hiding this comment.
nit: "Icon" since it's only for one file?
| result.Path = GetPackagePath(); | ||
| result.Create = false; | ||
| if (path == PathName::ImageAssets) | ||
| { | ||
| result.Path /= (IsReleaseBuild() ? s_ImageAssetsDirectoryRelativeRelease : s_ImageAssetsDirectoryRelativePreview); |
There was a problem hiding this comment.
Curious why do we have different paths for release and preview?
There was a problem hiding this comment.
Because I wasn't thinking through it. It should be different paths for the different repositories.
| result.Path /= s_ImageAssetsDirectoryRelativePreview; | ||
| if (!std::filesystem::is_directory(result.Path)) | ||
| { | ||
| result.Path.clear(); |
There was a problem hiding this comment.
Is the only case the directory exists is when it's an unpackaged wingetdevcli?
| // TODO: Consider theme based on current background color. | ||
| bool IsIconBetter(const Manifest::Icon& current, const Manifest::Icon& alternative) | ||
| { | ||
| static constexpr std::array<uint8_t, ToIntegral(Manifest::IconResolutionEnum::Square256) + 1> s_iconResolutionOrder |
There was a problem hiding this comment.
nit: probably add some comment that if Icon enum changes, this list needs to be updated as well
| // Determines icon fit given two options. | ||
| // Targets an 80x80 icon as the best resolution for this use case. | ||
| // TODO: Consider theme based on current background color. | ||
| bool IsIconBetter(const Manifest::Icon& current, const Manifest::Icon& alternative) |
There was a problem hiding this comment.
nit: IsSecondIconBetter? Usually we have IsFirstBetter but this one is opposite
| // Create palette from full image | ||
| std::filesystem::path imageAssetsRoot = Runtime::GetPathTo(Runtime::PathName::ImageAssets); | ||
|
|
||
| // This image matches the target pixel size. If changing the target size, choose the most appropriate image. |
There was a problem hiding this comment.
We should check if imageAssetsRoot is empty before using it?
| m_out << blockOn; | ||
| } | ||
| // This image matches the target pixel size. If changing the target size, choose the most appropriate image. | ||
| std::filesystem::path imageAssetsRoot = Runtime::GetPathTo(Runtime::PathName::ImageAssets); |
There was a problem hiding this comment.
Check empty path here too?
| auto infoOut = reporter.Info(); | ||
| VirtualTerminal::ConstructedSequence indent; | ||
|
|
||
| if (reporter.SixelsEnabled()) |
There was a problem hiding this comment.
Do we want to try CATCH_LOG since I saw a bunch of THROW_IF_FAILED in sixels.cpp code?
|
The progress bar could be a nyancat. |
Hackathon
As part of a yearly event to work on personal choice projects, I implemented support for rendering sixels in a few different scenarios. You can try it out in any terminal that supports sixels, such as the latest Windows Terminal Preview build. Both @lhecker and @j4james were very helpful with guidance on the nuances of sixels.
Change
The foundational work is a wrapper around the Windows Imaging Component (WIC) APIs and a renderer for converting the indexed images to sixel format. WIC does all the heavy lifting of decoding images, palette optimization and dithering. My additions are a simple compositor and sixel renderer. There is also some new code for detecting VT extension support, including only enabling sixel rendering if the terminal advertises the extension.
Sixel use was added in various ways. The winget icon is output any time the header would be (during
winget --infoand any time we output help):The package icon (if present) is displayed during
winget show. This is an example using a local manifest updated to point to an image located on the github repository of the package; actual package icon will likely be different. Package icons extracted from actual installs are expected to arrive soon:A new progress visualization is available using sixels. The indefinite progress (the current character spinner that iterates through various slashes, dash and pipe) looks like:
while the progress bar is a conveyor belt bringing your package to you:
The conveyor belt base image is only the finest of dev art; it is very open to someone with more artistic skill to improve upon.
The tearing present in this gif does exist, but it is exaggerated by the capture and the slow steady progress. Actual progress is much less steady, making any tearing that may occur far less noticeable.
Settings
Use of sixels (including even the attempt to detect support in the terminal) is gated behind enabling them in the settings. Two new/updated settings are available.
.visual.enableSixels(trueorfalse).visual.progressBar"sixel"causes progress to use the sixel versions shown above."disabled"disables progress output.Validation
Many, many manual runs of various scenarios using the added
debugcommands.Added a few unit tests to cover primary paths.
Microsoft Reviewers: Open in CodeFlow