Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Latest version of image support for toot #319

Closed
wants to merge 85 commits into from

Conversation

danschwarz
Copy link
Collaborator

@danschwarz danschwarz commented Feb 25, 2023

Currently supports Unicode half-cell rendering, plus Kitty and iTerm2 pixel rendering in supported terminals. Sixel support is a future planned enhancement. (Thanks @AnonymouX47 !)

I've done some cleanup and simplification of the code.

image

image

danschwarz and others added 11 commits February 22, 2023 19:40
This replaces ansiwidget.py, but offers the same functionality
on terminals that don't support more advanced protocols.
On Kitty and Wezterm, you get pixel-resolution images in
24 bit color. Should also work in iTerm2 but testing is TBD.

Some quirks to sort out, but it works pretty well!

Note: currently requires you build the urwid-widget branch
of term-image and install to your python venv.

Term-image has a minimum Python version of 3.7

https://github.com/AnonymouX47/term-image/tree/urwid-widget
Also, the app now uses the UrwidImageScreen class, which clears any
stray Kitty or iTerm2 images upon app startup
This works just like urwid.raw_display.Screen, but properly
clears kitty and iterm2 images on startup and shutdown, even
if exceptions are encountered
This detects whether the term-image UrwidImages are being
rendered with half-cells or single pixels. If single pixels,
we can use less screen real estate to show a given image
and maintain legibility
If we are to support 256 color rendering of character cells
for terminals that don't support truecolor
(Apple Terminal, for example) this support will have to be
added in the term-image UrwidImage widget itself.
If we are rendering with pixels, we can use less screen
real estate for images.  If we are rendering with cells,
we need to use more.
In future the image widget may be able to display animation
These images are (potentially) animatable, although all display
as static images now
The term-image widget does not yet support 256 color mode
(this is a future planned enhancement). Until it does, I
have removed the --256 color option from toot tui.
@danschwarz danschwarz marked this pull request as draft February 25, 2023 01:59
@AnonymouX47
Copy link
Contributor

It's a pleasure 😃

As for the 256-color support, I've actually made quite a lot of progress behind the scenes these past two days, refining my initial idea for implementation. I usually prefer not to get ahead of myself (being that I haven't released this version yet) but when ideas start popping into my head, all I can do is put 'em down ASAP.

Been researching on sixels of recent also. More on that later.

Comment on lines 166 to 175


def can_render_pixels():
# temp image to make AutoImage happy
img = Image.new("RGB", (16, 16))
ai = AutoImage(img)

# BlockImage renders to half-cells
# KittyImage and ITerm2Image render to pixels
return not isinstance(ai, BlockImage)
Copy link
Contributor

@AnonymouX47 AnonymouX47 Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of instantiating an image class, I would suggest using auto_image_class() (was previously named auto_style() but future plans necessitated the change) with GraphicsImage (instead of BlockImage, mainly to make it future-proof as I'll be adding more image classes later and to simplify the logic):

return issubclass(auto_image_class(), GraphicsImage)

GraphicsImage is the baseclass of all image classes implementing graphics-based render styles. See the class heirachy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@danschwarz
Copy link
Collaborator Author

danschwarz commented Feb 25, 2023

It's a pleasure 😃

As for the 256-color support, I've actually made quite a lot of progress behind the scenes these past two days, refining my initial idea for implementation. I usually prefer not to get ahead of myself (being that I haven't released this version yet) but when ideas start popping into my head, all I can do is put 'em down ASAP.

Been researching on sixels of recent also. More on that later.

Re: sixels- @saitoha's libsixel with Python bindings works well, but the project hasn't seen any updates in 4 years.
The libsixel/libsixel fork is actively maintained and is probably the best thing to start with unless you're up for implementing your own sixel encoder from scratch...

Have you seen @GuardKenzie's Python bindings for Chafa? It incorporates Sixel support. I've done some tests with it, and was able to get it rendering images using Sixel when using its Loader class, but haven't spent the time to get it working without using the Loader class (Loader introduces an ImageMagick dependency, which I'd prefer to avoid.)

Chafa is actively updated. It also has a more sophisticated implementation of Unicode block rendering, using more symbols, with higher fidelity results in some cases.

@GuardKenzie
Copy link

GuardKenzie commented Feb 25, 2023

I've done some tests with it, and was able to get it rendering images using Sixel when using its Loader class, but haven't spent the time to get it working without using the Loader class (Loader introduces an ImageMagick dependency, which I'd prefer to avoid.)

It looks like you are using PIL for this PR which can be used to load images for use with chafa.py. Basically, if you can convert the image data into an array of RGB(A) values, you're golden!

Edit: I don't know how well chafa.py would interface with URWID. That might produce some ghosts.

@AnonymouX47
Copy link
Contributor

Yeah, libsixel seems to be the most likely right now but it has a couple of deficiencies, the major one for me being lack of support for transparency (same goes for chafa). NotCurses implements this, so I know it's possible. Also https://github.com/libsixel/libsixel is not that well maintained, there hasn't been any much development since (See the release notes here) and there are a quite a number of open issues unattended to on both the fork and @saitoha's repo.

I intend to to start off with libsixel and then write my own encoder later on.

Have you seen @GuardKenzie's Python bindings for Chafa?

Yes, I have but I'm yet to try it out.

It also has a more sophisticated implementation of Unicode block rendering.

Yeah, I recently explored this and it's literally mind-blowing. @GuardKenzie, do your bindings expose these features?

@GuardKenzie
Copy link

Yeah, I recently explored this and it's literally mind-blowing. @GuardKenzie, do your bindings expose these features?

Yes! You can specify symbols or a range of symbols to be used when rendering an image. Many of these are also included in predefined "symbol tags" for easy use (including some of the block characters)

@AnonymouX47
Copy link
Contributor

AnonymouX47 commented Feb 25, 2023

I see, thanks.

By the way, i was just going through your docs now. That's some really great work you've done. Well done!

@AnonymouX47
Copy link
Contributor

AnonymouX47 commented Feb 25, 2023

On second thought...

I've been going through the docs of chafa.py for a while now and I've come to realize that using chafa as sort of a render back-end could open a whole lot of new possibilities, way beyond what I already had planned.

Using chafa, we'll get:

  • I don't have to re-invent so many wheels 🥲.
  • Performance that just can't be achieved with pure python.
  • Sixel support (and I intend to discuss with chafa's author about the transparency support).
  • Support for all text color modes I can think of (except 88-color).
  • Support for various Unicode symbol maps.
  • Dithering
  • etc...

@danschwarz what're your thoughts on this?

@GuardKenzie I would very much appreciate your help in making this happen, if possible. Doesn't have to be via code, just suggestions and the likes will go a really great length e.g @danschwarz's suggestions went a really long way in making AnonymouX47/term-image#73 what it is, even this PR probably wouldn't be happening if not for his help.

Thanks 🙇🏾‍♂️.

@danschwarz
Copy link
Collaborator Author

danschwarz commented Feb 25, 2023

I think it's worth trying chafa. We're adding a dependency either way - libsixel or chafa - and at least we know chafa is being maintained. Chafa is really impressive, and I bet the developer will be able to assist with transparency support. (For what it's worth, lack of transparency support isn't a deal-breaker from my point of view, but I'd have to remove the nice rounded corners I'm using when displaying images in toot.)

The other alternatives are: write a sixel encoder from scratch, or try Notcurses (if someone writes and documents proper Python bindings 😆 )

@GuardKenzie
Copy link

By the way, i was just going through your docs now. That's some really great work you've done. Well done!

Thank you, I really appreciate it 🧡
I like to read nice docs so I did my best to write some haha.

@GuardKenzie I would very much appreciate your help in making this happen, if possible. Doesn't have to be via code, just suggestions and the likes will go a really great length e.g @danschwarz's suggestions went a really long way in making AnonymouX47/term-image#73 what it is, even this PR probably wouldn't be happening if not for his help.

Of course I'm willing to help wherever I can! Code contributions might be a bit out of reach for me atm since I'm knee deep in uni work but I'll make time to answer any questions or try to help in other ways of I'm able!

@AnonymouX47
Copy link
Contributor

at least we know chafa is being maintained

True, that's key.

I'd have to remove the nice rounded corners I'm using when displaying images in toot.

With terminal BG color detection (which I have already implemented), lack of transparency is usually not so perceptible (except on highly transparent terminal emulator windows).

write a sixel encoder from scratch

I thought about this again and figured I'd rather contribute to chafa instead. C is my second go-to language after all.

try Notcurses (if someone writes and documents proper Python bindings

Most likely not happening anytime soon. Also, I noticed the project isn't exactly designed to expose the graphics renderers/encoders for use as such.

@AnonymouX47
Copy link
Contributor

Of course I'm willing to help wherever I can!

Thanks, I'll reach out soon enough.

This feature is enabled for terminals that support rendering
images with pixels. For other terminals, custom server emoji
are displayed as text -- :shortcode: , as before.
@AnonymouX47
Copy link
Contributor

AnonymouX47 commented Feb 27, 2023

Good news!

I reached out to @hpjansson (at hpjansson/chafa#133) and he has confirmed that chafa can encode transparent sixels.

I've also tested it out and it checks out. Turned out the terminal emulator (konsole) on which I tested it initially was the one that didn't support transparent sixels.

Seems all roads lead to chafa. 😃

@danschwarz
Copy link
Collaborator Author

Update on this PR: Now supports rendering Mastodon custom server emoji in display names (if you're running on a terminal that supports pixel rendering. If not; the emoji are shown as :text: , same as before.)

See below for an example user who has :python: and :go: emoji in his display name. Those emoji are unique to the bolha.us server.

It'd be nice to support rendering custom emoji in status comment text as well, but there's not an Urwid widget capable of rendering multi-line text blocks with embedded images 😞 .

image

@danschwarz danschwarz marked this pull request as ready for review March 30, 2023 22:18
@danschwarz
Copy link
Collaborator Author

Congratulations to @AnonymouX47 on releasing term-image 0.6.0! With the release, I'm now able to pin toot to use 0.6.0 as a dependency, and I've switched this PR from draft to open.

@AnonymouX47
Copy link
Contributor

Thanks so much.

Sorry it took a while, I needed to get a couple things in shape. I ought to have announced it here ASAP, but I was stuck getting the new image viewer repo to a somewhat presentable state before any announcement.

As soon as I'm done with the above, mainly just the docs though. I'll finish things up with the TextEmbed and other widgets (at https://github.com/AnonymouX47/urwidgets).

It's been quite a long way coming... Thanks for everything.

@AnonymouX47
Copy link
Contributor

Hi! Here again.

I've stayed quiet cos i was yet to fulfill my promise at danschwarz#2. Now, that's done.

I have a couple inquiries to make:

  1. What's the word on dropping Python 3.6 support?
  2. Why are the workflow runs for other versions getting cancelled?
  3. Last I checked, the tests were failing due to a warning issued by term-image, has that been handled?

I hope to hear from you soon. Thanks.

@danschwarz
Copy link
Collaborator Author

danschwarz commented Apr 28, 2023

Hey, this is really terrific! I haven't been able to watch the video yet, but I've been reading the code, and I'm looking forward to trying it out! I think the first thing I do will be to use TextEmbed to add OCS 8 hyperlink support within status message displays. It'll be a real improvement for terminals that support the feature.

I'm afraid I don't have answers to your other questions yet. @ihabunek hasn't been active here for a little while- I assume he's busy with other things and will return and catch up when he can. At that time we can address the question of Python 3.6 support and the test failures- they're due to the GitHub test environment not being able to resolve the term-image-0.6.0 dependency via pip. I have no idea why this is. The same configuration works locally for me. After one or two Python test runs fail, the test system automatically cancels the other runs. So that's why that's occurring.

@AnonymouX47
Copy link
Contributor

It'll be a real improvement for terminals that support the feature.

True

At that time we can address the question of Python 3.6 support

Does this mean this PR waits till then?

If you prefer not so, it's possible to restrict the dependency with ; python_version>="3.7" and then within the code the import can be conditionally handled to exclude Python 3.6 and any definition imported from term_image replaced with a compatible placeholder.
This way, it'll be pretty easy to clean things up when Python 3.6 support is dropped.

After one or two Python test runs fail, the test system automatically cancels the other runs.

Oh, I see... probably due to the Py3.6 failure.

The term-image library, needed for image display, is Python 3.7+
so I have made what I think are the minimum set of changes to
move to Python 3.7 - testing with 3.7+ only, as a start.

Unchanged- vermin target is still 3.6 because changing it to 3.7
fails for an unknown reason. Seems harmless to leave it at 3.6
for the time being.
@danschwarz
Copy link
Collaborator Author

@AnonymouX47 you were right, the dependency import failure was entirely due to Python 3.6. I've taken the liberty of changing the test suite for this PR to test under Python 3.7+ only, and all tests pass now.

@ihabunek ihabunek mentioned this pull request Jun 22, 2023
@ihabunek
Copy link
Owner

Hey guys, I finally gathered some time and energy to work on toot a bit. I've had family issues and my landlord asked us to move out soon so been looking for a new place.

Firstly thank you @AnonymouX47 for the great work on term_image, and @danschwarz on integrating it with toot. I've been using this branch for a bit and seems to work very well. But I'm a bit hesitant to merge it yet.

I'm always cautious of adding dependencies and term_image is still unstable, e.g. term_image 0.7.0 does not work with toot at the moment, so we'd need to pin it to >=0.6,<0.7 or similar. Also it's mostly not packaged by popular distros which package toot.

One option would be to vendor it (copy the whole term_image inside toot) until a stable release is published, would you be ok with that @AnonymouX47? Alternatively we could make it an optional dependency by refactoring the code a bit to work without term_image installed. The latter might encourage maintainers to add it to their distro. WDYT?

Lastly, keeping images in memory is a bad idea since it will just keep using up more and more. We should download them to ~/.cache/, implement some kind of invalidation and load from there when a post is loaded. I'd be happy to handle this bit.

I made a squashed version of this pull #372 since this is getting very messy with all the merges from master to branch.

@danschwarz
Copy link
Collaborator Author

danschwarz commented Jun 22, 2023

@ihabunek I have a one line patch for the code that will allow us to run on 0.7.0. Need to test first. That said, your concerns are valid, bundling vs. making it a soft dependency, I'd like users to have the feature without a lot of effort. Maybe bundle it in until it hits 1.0, and then move to a soft dependency?

As for caching, no objection if you want to have a go at implementing cache eviction to disk or similar.

My concern about the image feature right now is a strange interaction that toot has with the latest urwid prerelease, see the issue I recently opened. Urwid seems to stall TUI updates after async op until a key or mouse event is received. This happens with current master on the title screen when running urwid 2.1.3 prerelease; it happens much more- on every image display- when running with image support, because we are doing a lot of async image loads and widget repaints. Either we lock to 2.1.2 or we work with the urwid devs to see what's broken and get it fixed before 2.1.3 releases. I've isolated it to a particular commit on their side where they've changed their idle emulation, but I don't understand the async code at a deep enough level to understand what's causing this problem.

@danschwarz
Copy link
Collaborator Author

Ref: urwid/urwid#575

@AnonymouX47
Copy link
Contributor

AnonymouX47 commented Jun 22, 2023

Hey guys, I finally gathered some time and energy to work on toot a bit. I've had family issues and my landlord asked us to move out soon so been looking for a new place.

Glad you're back. I hope everything gets resolved on time (if not yet resolved).

Firstly thank you @AnonymouX47 for the great work on term_image, and @danschwarz on integrating it with toot. I've been using this branch for a bit and seems to work very well. But I'm a bit hesitant to merge it yet.

You're welcome. I have to give a lot of credit to @danschwarz on this one. If not for him, this PR probably wouldn't exist.

I'm always cautious of adding dependencies and term_image is still unstable, e.g. term_image 0.7.0 does not work with toot at the moment, so we'd need to pin it to >=0.6,<0.7 or similar. Also it's mostly not packaged by popular distros which package toot.

One option would be to vendor it (copy the whole term_image inside toot) until a stable release is published, would you be ok with that @AnonymouX47? Alternatively we could make it an optional dependency by refactoring the code a bit to work without term_image installed. The latter might encourage maintainers to add it to their distro. WDYT?

Understandable... Pinning to a minor version is definitely necessary pre-1.0.

Whichever of the alternatives you deem fit is okay by me. As for vendoring, i think it's allowed between the licenses (in this case). Not so sure but might require noting that certain part of the codebase is vendored with a link to the license or a copy of the license [statement] itself.

Lastly, keeping images in memory is a bad idea since it will just keep using up more and more. We should download them to ~/.cache/, implement some kind of invalidation and load from there when a post is loaded. I'd be happy to handle this bit.

Yeah, I also thought about that but assumed @danschwarz had implemented some form of eviction. Anyways, downloading to disk is definitely less expensive.

@danschwarz
Copy link
Collaborator Author

danschwarz commented Jun 23, 2023

Yeah, I also thought about that but assumed @danschwarz had implemented some form of eviction.

😳 Whoops. I was so happy to get image display working properly that I didn't go back and implement this.

@danschwarz
Copy link
Collaborator Author

Closing this in favor of #372

@danschwarz danschwarz closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants