Fixes #5291: add ImageView support with Sixel rendering and optimize re-rendering#5292
Conversation
…state updates for terminal drivers
|
Thanks for the PR! I have a few minor technical suggestions after reviewing the implementation:
Overall, the Sixel optimization logic and the new |
- Switch ConcurrentDictionary to Dictionary for _attributeCache - Use integer math for scaling logic to avoid double conversions - Implement reusable buffer for scaled image to reduce allocations during resize
…g and add coordinate conversion methods
…nt potential null reference exceptions
|
Thank you! I'm aware of the first two. I tested the last bullet earlier and did not experience it. Maybe I broke something recently; will take a look. Are folks aligned with this direction? I can work to address all three of these (although the middle one is not a regression; it's the current behavior) if this seems like a reasonable direction. We should really move the rendering logic out of the ConcurrentQueue in OutputBase and into the view itself but I figured we could do that iteratively so I would maybe suggest ignoring the middle improvement for now. |
|
To ignore the middle one for now then just render the sixel after the dialog box is closed. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ImageView control for rendering Color[,] pixel buffers (cell-based fallback or sixel when available) and extends the driver/output pipeline to detect sixel support and avoid re-emitting sixel data every frame by tracking a dirty flag.
Changes:
- Added
Terminal.Gui.Views.ImageViewwith scaling and sixel/cell-based rendering paths. - Added driver-level sixel capability detection (
IDriver.SixelSupport) and output-levelSixelToRender.IsDirty/AlwaysRenderbehavior to skip redundant sixel writes. - Updated UICatalog’s
Imagesscenario and added/updated tests for the new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTestsParallelizable/Views/ImageViewTests.cs | New unit tests covering ImageView scaling/rendering and sixel helpers. |
| Tests/UnitTestsParallelizable/Drivers/Output/OutputBaseTests.cs | New tests verifying OutputBase.Write skips/clears dirty sixels and DriverImpl.SixelSupport storage. |
| Tests/UnitTestsParallelizable/Application/MainLoopCoordinatorTests.cs | Adds coverage around sixel detection startup behavior (but one test name/behavior mismatch). |
| Terminal.Gui/Views/Markdown/MarkdownView.Drawing.cs | Queues markdown sixels with AlwaysRender=true to keep them visible under the new dirty model. |
| Terminal.Gui/Views/ImageView.cs | New ImageView implementation with sixel/cell rendering and scaling. |
| Terminal.Gui/Drivers/Output/OutputBase.cs | Skips sixel writes when not dirty (unless AlwaysRender) and clears IsDirty after writing. |
| Terminal.Gui/Drivers/IDriver.cs | Adds SixelSupport to the public driver interface. |
| Terminal.Gui/Drivers/DriverImpl.cs | Implements SixelSupport storage and internal setter. |
| Terminal.Gui/Drawing/Sixel/SixelToRender.cs | Adds IsDirty and AlwaysRender flags. |
| Terminal.Gui/App/MainLoop/MainLoopCoordinator.cs | Runs sixel detection during driver initialization for non-legacy consoles. |
| Examples/UICatalog/Scenarios/Images.cs | Refactors scenario to use the new ImageView and driver-level sixel detection. |
* fix(image): address PR gui-cs#5292 review comments and regressions - Optimize ImageView scaling with integer arithmetic and ArrayPool - Fix memory leaks in Images scenario by disposing ImageSharp images - Fix regression in Images scenario where Sixel support detection didn't update UI - Implement proper SixelToRender reuse and cleanup in MarkdownView - Add default implementation for IDriver.SixelSupport to avoid breaking changes - Improve test cleanup by disposing IApplication instances * refactor: simplify ImageView aspect ratio calculations and optimize Sixel rendering logic * refactor: change SixelSupport from a property with a default getter to an abstract property definition * feat: add SixelSupportChanged event to driver interface and implement change notification * test: add unit tests for SixelSupportChanged event and update image resizing type in UICatalog example --------- Co-authored-by: Matt Razza <504088+mrazza@users.noreply.github.com>
|
@BDisp While I was not able to repro your third issue, I suspect it was caused by removing a sixel from the queue during Fire rendering. I've addressed that risk. This is more evidence that the ConcurrentQueue of sixels rendered at the end of the draw pipeline is very very hacky but hoping to punt a fix for that into a future PR. I also addressed the other two regressions you identified. Thanks! Supports Sixel now correctly displays resolved value: Fire renders at the same time as the sixel: Also addressed copilot comments. |
Interesting. Not noticing that on foot on Linux; maybe the redraw is so fast it's not noticeable or maybe something else is causing that area of the screen to not invalidate on my machine. Thanks for the powershell validation. Not sure what the solution is yet. Importantly, it's not clear to me why this would be different. I suspect this is caused by something marking that area of the OutputBuffer as IsDirty and there being a delay between when it is re-rendered (rendering nothing - ) and the Sixel fully flushing at the end of the draw cycle ( ). However, this existed before my change. |
…nt to window initialization in Images scenario
|
I have an ugly solution involving reaching into the content buffer and marking the impacted cells as not dirty. I'll push this but I don't feel good about it. Something like this: |
…content to prevent flickering also, introduce FitImageInViewportCells for accurate aspect-ratio-preserving scaling Note: This IsDirty hack will likely not play nicely with transparent sixels placed ON TOP of other views.
|
As I struggle to repro this myself, @BDisp would appreciate if you could pull this down and give it a test. |
|
I added metrics and internal logs when i was optimising layout and redraw. Theres a markdown file on it. You could add sixel redraws to it. It used dotnet counters tool |
|
Looking more closely at the first GIF, it's not just the left side that's flickering, but also the bottom side. I suspect it's not due to the border, but rather another reason I haven't yet been able to figure out. But it's worth investigating because it might even be a completely different problem unrelated to the changes in this PR. Do you agree? |
|
@BDisp Thank you! I could probably resolve the remaining flickering by scaling and positioning the image so that its bounds are on exact cell boundaries that I clear the IsDirty flag on... either by effectively shrinking the image within the Viewport or by expanding the IsDirty clearing by one cell in all directions. The former is likely preferred between these two options otherwise the borders would never draw. Even so, I'm not sure it's worth doing. The flickering is most common when doing rapid buffer refreshes like in the fire animation which may be uncommon and defaulting to rendering smaller images seems less than ideal (in particular if the renderable area is small; which is the case in my app --- constraining by an entire cell width/height will result in an extremely small image). I believe, in the situations where the flickering is problematic, a consumer could resolve this themselves by adding Padding.Thickness of (1, 1, 1, 1) to the ImageView. TL;DR: This is an existing problem and I believe the flickering on the edges we're seeing is because the bounds of the sixel do not cleanly align with character Cells used when rendering on-sixel graphics and so portions of the sixel extend beyond the Cells I clear the |
|
Please update ImageSharp from 3.1.12 to 4.0.0 as part of this. |
|
It seems, as of 4.0, you are now required to have a license for ImageSharp? https://sixlabors.com/pricing/#plans
https://docs.sixlabors.com/articles/imagesharp/index.html?tabs=tabid-1#license Not sure what you want to do with that @tig |
|
I just applied for their oss license. |
|
Any next steps here? |
I think we have to wait until they provide the license. I just submitted a 2nd request. |
|
ACK. I got a license in less than a day. You should immediately see a 2 month license and within 24hrs I got a 1 year license in my email. |








Creates a ImageView view and uses it in UICatalog.
This view takes no new dependencies in Terminal.Gui so it operates on raw Color[,] arrays. The performance for resizing these is poor. As a result, it may make sense to take an optional lambda to allow the constructor to provide a more efficient resizing implementation using their image library of choice.
Fixes
Proposed Changes/Todos
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)