tats
Auto inline images
Deduce an appropriate value for inline_img_protocol rather than requiring it to be set manually with eg. -o auto_inline_img=4 for kitty.
Tangentially, should struct _termialImage {...} TerminalImage at image.c:11,27 be _terminalImage? Not important currently since nothing uses the struct tag as opposed to the TerminalImage typedef, but could surprise someone eventually
Some general remarks:
- This should be one commit, not three.
- Missing documentation update.
As someone who does not use images, what problem does this solve?
As someone who does not use images, what problem does this solve?
It avoids having to manually set the inline image output method (since different methods work best depending on your environment). Normally you'd have to go through each option yourself; this patch does the detection automatically.
Some comments on the implementation:
- Doing terminal capability detection in two passes seems needlessly inefficient. Could we merge the sixel test with get_pixel_per_cell?
- As rkta says, we don't need a new source file for this. Just put it in terms.c and/or image.c.
- autodetection should be an option (in rc.c) like the actual image protocols, not a CLI parameter. Then you can default to "autodetect" in new configs, without breaking existing configs. We might want to use a separate variable to store the final chosen image display method.
- Kitty display depends on ImageMagick, and sixel display needs img2sixel. So we probably want to check if these programs exist on the system at all.
- IIRC, the iTerm2 implementation does not support image cropping, unlike the sixel implementation (which also works in iTerm2). Personally, I'd just remove the iTerm2 test; correct image dimensions are more important than optimal color output.
- This should be one commit, not three.
Ah my apologies, that's my bad habit of committing WIP as I go so I remember what order I've done things in. But can't the merge simply be squashed? Or should I rebase and squash it myself and open a different PR? (with changes moved to image.c ofc)
- Missing documentation update.
As far as I could see, -o inline_img_protocol has no documentation to update, though do point me towards it if I missed it
As someone who does not use images, what problem does this solve?
Not every user is aware of the different protocols available, nor which if any are supported by their terminal -- I was unaware that more recent Konsole versions support sixel, iterm2 and kitty images, since I'd been using it since before those versions, and xterm and xfce4-terminal both have undocumented sixel support. Of course, on Wayland the w3mimgdisplay X11 fallback doesn't work
Also, for -o inline_img_protocol=x it isn't clear to the user which values of x correspond to which protocol; even if this were documented, it would still be unintuitive and not easy to remember.
- Doing terminal capability detection in two passes seems needlessly inefficient. Could we merge the sixel test with get_pixel_per_cell?
I don't think so, get_pixel_per_cell uses XTWINOPS CSI Ps ; Ps ; Ps t, test_for_sixel uses CSI Ps c
- autodetection should be an option (in rc.c) like the actual image protocols, not a CLI parameter. Then you can default to "autodetect" in new configs, without breaking existing configs. We might want to use a separate variable to store the final chosen image display method.
I'm not sure existing config would be broken -- if inline_img_protocol is set in a config, it would override default autodetection. I would have liked to use -o inline_img_protocol=-1, since that's what happens under the hood, so perhaps changing to P_CHARINT is the answer
- Doing terminal capability detection in two passes seems needlessly inefficient. Could we merge the sixel test with get_pixel_per_cell?
I don't think so, get_pixel_per_cell uses XTWINOPS
CSI Ps ; Ps ; Ps t, test_for_sixel usesCSI Ps c
Ah, no, I meant sending them together in a single batch. Terminal query code usually does this in one pass, putting primary DA in the end, because one write/read/timeout cycle will always be faster than two. (Also, pretty much every terminal emulator responds to a primary DA but not necessarily to other queries, so sending it as the final query is a nice timeout prevention measure.)
We can live without that for now, since we only have two queries, both seem to be widely supported, and get_pixel_per_cell often returns without sending a query at all. Just a suggestion for improvement.
I'm not sure existing config would be broken
Yeah, I now see you're right.
The only problem with the current solution remains that it doesn't let the user pick "autodetect" and then stick with it, which would be nice for when you use w3m in different terminals. (Even if you could pick -1 in its current form, it would automatically switch to another option as soon as you change something else in the option setting panel.) Hence my suggestion to use a separate internal variable for the actual image protocol choice.
In the absence of any further comments on the clutter of the commit history, I'll just push any further commits and potentially clean up history later (I know `clean up later' is never a good thing to hear ;). The diff against master still looks fairly clean. This implements most of the changes just discussed
Ah, no, I meant sending them together in a single batch
Oh I get it
[...]and get_pixel_per_cell often returns without sending a query at all
inline_img_protocol_autodetect() may also itself return without sending the XTWINOPS query (if protocol_test_environment() returns nonzero), so implementing that may end up being more trouble than it's worth. For startup speed I'm more concerned about the exec call to find img2sixel in have_img2sixel() in image.c, even though it's the same way that img2sixel is found when it's needed in put_image_sixel() in terms.c.
The only problem with the current solution remains that it doesn't let the user pick "autodetect" and then stick with it, which would be nice for when you use w3m in different terminals. (Even if you could pick -1 in its current form, it would automatically switch to another option as soon as you change something else in the option setting panel.)
I'm afraid I don't follow here -- by the time you get to the option setting panel, if autodetect was set it will already have run at startup and resolved to a different value INLINE_IMG_*. Or do you mean it should not run only at startup? The way I see it right now, if the user picks a different setting for inline_img_protocol, this by design should override the auto-detected value
The auto detect should be an option in the options panel, so that a user does not have to use the command line option in order to enable it.
Re: commit history, you can squash the commits and just do a force push into your branch.
Remember to also update the docs, when you are done with the implementation.
The auto detect should be an option in the options panel, so that a user does not have to use the command line option in order to enable it.
Yeah, that's what I meant. You could do something like:
/* in fm.h */
#define INLINE_IMG_AUTODETECT -1
/* maybe move this to fm.h too */
global signed char enable_inline_image init(INLINE_IMG_NONE);
/* ...and set above variable based on this */
global signed char enable_inline_image_config init(INLINE_IMG_AUTODETECT);
So if enable_inline_image_config == INLINE_IMG_AUTODETECT, detect the method and set enable_inline_image to that. Otherwise, set enable_inline_image to enable_inline_image_config as is. (Also, make enable_inline_image_config the user-facing variable you set in rc.c.)
Remember to also update the docs, when you are done with the implementation.
What docs? :P
OK, there's doc/README.sixel, which is a bit outdated and only talks about the sixel method. Also, doc/README.img, which only talks about the X/framebuffer method. I think it would be best to merge the two, and also add some info on the other methods (+ how to select them).
It's kind of my fault for never updating the docs when working on this, so I'll prepare a PR for it once this patch is finalized.
On Mon, Jul 22, 2024 at 01:32:28PM -0700, bptato wrote:
Remember to also update the docs, when you are done with the implementation.
What docs? :P
Well, adding a command line option should at least update the man page and probably also the MANUAL.html.
Well, adding a command line option should at least update the man page and probably also the MANUAL.html.
Wait what? I thought we decided we don't need a command line option...
Looking at the man page and MANUAL.html, neither did ever mention image support. I've started working on an overhauled README.img, I'd say it's easiest to just link to that from MANUAL.html once it's finished.
On Tue, Jul 23, 2024 at 07:43:47AM -0700, bptato wrote:
Well, adding a command line option should at least update the man page and probably also the MANUAL.html.
Wait what? I thought we decided we don't need a command line option...
Did we? That's good, because that's what I preferred. :-D
Looking at the man page and MANUAL.html, neither did ever mention image support. I've started working on an overhauled README.img, I'd say it's easiest to just link to that from MANUAL.html once it's finished.
Sounds good.
Alright, I've squashed the commits from up til now, and the second commit adds the separate variables for enable_inline_image_config, and INLINE_IMAGE_AUTODETECT to the option table in rc.c. The original enable_inline_image can be unsigned now, so there's no possibility of a negative value being taken as a false positive in a boolean test.
Only issue is now the user can't see in the options panel which protocol was selected automagically, but that's not the end of the world. If it does become necessary, maybe that information would be more suited to debug output on stderr anyway.
Also, looks like enable_inline_image can be ifdef'd out after all
JFTR, I have this on my radar. In the current form it's not easy to review, because there seem to be a lot of unrelated changes mixed into the commits, but I will try to work it out when my time permits.
- Doing terminal capability detection in two passes seems needlessly inefficient. Could we merge the sixel test with get_pixel_per_cell?
I don't think so, get_pixel_per_cell uses XTWINOPS
CSI Ps ; Ps ; Ps t, test_for_sixel usesCSI Ps cAh, no, I meant sending them together in a single batch. Terminal query code usually does this in one pass, putting primary DA in the end, because one write/read/timeout cycle will always be faster than two. (Also, pretty much every terminal emulator responds to a primary DA but not necessarily to other queries, so sending it as the final query is a nice timeout prevention measure.)
We can live without that for now, since we only have two queries, both seem to be widely supported, and get_pixel_per_cell often returns without sending a query at all. Just a suggestion for improvement.
Is this still valid? If yes, can you make that improvement @el-remph , please.
Sorry I was away, here's a mockup that seems to work, but I'm not sure if this does optimise anything after all, because to know if we need pixel/char we need to know whether we're using enable_inline_img or not, so (if kitty and mlterm protocols are unavailable) we need to know if the terminal supports sixel before we query pixel/char.
(There's more that could be done to make this prettier, but I'm leaving that until I know if this optimises anything or not)
My 'solution' in this commit, is just call get_pixel_per_char() unconditionally, which will lead to an unnecessary check for pixel/char for those without any supported inline image protocol. On most systems, I don't think this will use XTWINOPS, since TIOCGWINSZ is usually available, so this works out to an extra ioctl call for the typical (I guess) user. I don't know the cost of ioctl so this may not be much, but the only benefits are for those on systems without TIOCGWINSZ so I doubt it's worth it.
Unless anyone has any ideas, I'll rollback this commit
When I wrote "We can live without that for now", I meant it. It isn't that relevant with just two queries, especially when one of those is just a fallback.
The current (as in, before this patch) pixel per column/line logic seems flawed anyway, as the same variables are used for tables too (width attribute, etc.) So right now, toggling inline image support influences table rendering. Querying cell dimensions unconditionally would be preferable, but then it has to be moved outside the image ifdefs, and we have to figure out what to do with the pixel per char/line options... I think it's far beyond the scope of this PR.
When I wrote "We can live without that for now", I meant it.
Wrote the commit in response to @rkta here, but yes, I agree; rolling back as planned
Rebased with detection of ghostty, which has had support for kitty images since very early on afaict.
Anyone know the status of this pr?
On Wed, Jan 15, 2025 at 07:10:09PM -0800, el-remph wrote: [...]
Anyone know the status of this pr?
I didn't follow the last changes that closely, but I'd be willing to accept this into my fork when all current issues are addressed. I'd like to have an OK from bptato and maybe from j4james first though.
Also I'd prefer if you would send it the mailing list1 then I will do the final review.
Chances that it will be merged here are next to zero, I guess.
Rebased with the changes recommended:
- Primary devattr querying moved to terms.c, reading from tty into a dynamically resized buffer
- Test for sixel before testing for mlterm OSC5379, to catch newer versions of mlterm which support sixel but not OSC5379
- Made rkta's style changes
Also I'd prefer if you would send it the mailing list[1] then I will do the final review.
Oh, I didn't realise about the mailing list. If these changes pass muster here then I'll submit it to the mailing list
I've tested the latest version:
- Piping to stdin works now, thanks.
- It seems initImage gets called twice for some reason, so detection is performed twice too :/ I guess that's why the activeImage check is there before getCharSize? Maybe you could reuse it.
- Obscure, but SyncTERM returns
\033[=to primary DA instead of\033[?; might as well support it:
diff --git a/terms.c b/terms.c
index f75dcfc6..d663d18e 100644
--- a/terms.c
+++ b/terms.c
@@ -924,7 +924,8 @@ img_protocol_test_for_sixel(void)
}
/* first useful iteration; validate response */
- if (nchars_read + len >= 3 && memcmp(response, "\033[?", 3) != 0) {
+ if (nchars_read + len >= 3 && memcmp(response, "\033[?", 3) != 0 &&
+ memcmp(response, "\033[=", 3) != 0) {
fputs("Malformed terminal attributes\n", stderr);
return INLINE_IMG_NONE;
}
I still think, this needs to be documented somewhere.
Well, but the image docs in your fork are now different... I can update them in a separate patch, if you're OK with that.
- Obscure, but SyncTERM returns
\033[=to primary DA instead of\033[?; might as well support it:
@bptato I don't think accepting the \033[= prefix is going to help, though, because the rest of the DA report isn't DEC-compatible. You can't search for a 4 parameter to check for Sixel support - it's just an ASCII encoding of the "CTerm" library name and a version number. So you'd probably have to check specifically for the parameter sequence 67;84;101;114;109 at the start (assuming all versions supported Sixel).
Ah, no, by "support" I meant "don't print an error and stop looping" :P
SyncTERM (at least the version I have, from a few months ago) doesn't do private color registers, so you have to manually configure W3M_IMG2SIXEL for it anyway.
- Obscure, but SyncTERM returns \033[= to primary DA instead of \033[?; might as well support it
@bptato Yep that's obscure alright (: Done that and also skipped processing the response if it's SyncTERM
- It seems initImage gets called twice for some reason, so detection is performed twice too :/ I guess that's why the activeImage check is there before getCharSize? Maybe you could reuse it.
afaict activeImage is only set to TRUE once and never set back to FALSE, so if w3m is started and initImage called without auto-detection, and the user then enables auto-detection, that won't work. I haven't been able to solve this.
One option could be to cache the result of auto-detection once it's done, then reuse that on subsequent calls -- is it possible for the result of auto-detection to change during a session? For example, if the user starts w3m in GNU screen in a non-sixel terminal, then reattaches in a sixel terminal? Is it possible for the TERM environment variable to be modified during a session?
afaict activeImage is only set to TRUE once and never set back to FALSE, so if w3m is started and initImage called without auto-detection, and the user then enables auto-detection, that won't work. I haven't been able to solve this.
Ah, I see. I would have expected that I have to restart w3m, but it's definitely nicer if it works immediately :)
One option could be to cache the result of auto-detection once it's done, then reuse that on subsequent calls -- is it possible for the result of auto-detection to change during a session? For example, if the user starts w3m in GNU screen in a non-sixel terminal, then reattaches in a sixel terminal?
On GNU screen, img2sixel only seems to work with the "penetrate" hack. (But maybe I'm missing something.)
For a working example, see tmux: it reports Sixel capability even on terminals that cannot display Sixel, so you can attach both a Sixel-capable and a non-Sixel-capable terminal at the same time (and see a placeholder when appropriate).
In other words: I wouldn't worry about this.
Is it possible for the TERM environment variable to be modified during a session?
No.