Auto inline images #302

el-remph · 2024-07-12T19:05:25Z

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

rkta · 2024-07-14T06:24:52Z

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?

rkta · 2024-07-14T06:25:26Z

image.c

@@ -16,7 +16,7 @@ static int image_index = 0;

 /* display image */

-typedef struct _termialImage {
+typedef struct _termialImage /* [sic]? */ {


Just fix it.

rkta · 2024-07-14T06:26:48Z

main.c

@@ -127,7 +127,7 @@ static int searchKeyNum(void);
 #define help() fusage(stdout, 0)
 #define usage() fusage(stderr, 1)

-int enable_inline_image;
+int enable_inline_image = -1; /* TODO: can't we remove this ifndef USE_IMAGE? */


I don't see an USE_IMAGE here and probably no ;)

I think it's unused without USE_IMAGE, but you also have to ifdef out a few enable_inline_image checks (and the CLI param assignments).

rkta · 2024-07-14T06:27:58Z

inline_img_protocol_autodetect.c

@@ -0,0 +1,127 @@
+/* The only externally visible symbol exported by this module is


This code should follow the same style as the code in other units. And I think a better place for this is image.c

rkta · 2024-07-14T06:28:29Z

inline_img_protocol_autodetect.c

@@ -0,0 +1,127 @@
+/* The only externally visible symbol exported by this module is
+ * inline_img_protocol_autodetect(). I appreciate that this name does not
+ * roll off the tongue


Are you trolling?

No? I certainly intended no offence, I must have just been in a good mood while writing that, sorry

rkta · 2024-07-14T06:30:00Z

main.c

@@ -720,6 +720,8 @@ main(int argc, char **argv)
 		}
 	    }
 #endif
+	    else if (!strcmp("-auto-inline-img", argv[i]))


Does this need to have a command line switch? It's should be an option anyways and the can be set via -o.

I considered that, but thought that inline_img_protocol=5 or enable_inline_image == 5 would imply a built-in protocol rather than use of w3mimgdisplay for any code looking for a positive value to mean that, while auto-detection can resolve to 0 (ie. use w3mimgdisplay). Also, if a new image protocol were to become relevant, such as Terminology's, having auto-detection in the middle of a list of protocols would make no sense. Trouble is, set_param() doesn't allow negative values for P_INT and I thought changing that could have excessively far-reaching implications for anything assuming P_INT was >= 0.

Looking at set_param again, this could fixed by making auto_inline_image with P_SHORT or P_CHARINT and changing the type of enable_inline_image accordingly, but at the time I just went for the easiest and simplest way so I could test it

bptato · 2024-07-14T17:51:05Z

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.

el-remph · 2024-07-14T18:24:40Z

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.

el-remph · 2024-07-14T19:56:16Z

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

bptato · 2024-07-15T15:49:49Z

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

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.

* Remove iterm2 test * Move image protocol detection functions into image.c * Replace -auto-inline-img with -o inline_img_protocol=-1, by changing inline_img_protcol to a P_CHARINT * Test for presence of img2sixel command before allowing sixel protocol selection

el-remph · 2024-07-21T19:01:23Z

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

rkta · 2024-07-22T07:20:37Z

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.

bptato · 2024-07-22T20:32:07Z

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.

rkta · 2024-07-23T05:45:17Z

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.

bptato · 2024-07-23T14:43:26Z

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.

rkta · 2024-07-23T15:18:59Z

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.

inline_img_protocol based on the invoking terminal

* Make auto-selection a true option for inline_img_protocol, with a separate variable for configuration

el-remph · 2024-08-19T04:12:49Z

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

rkta · 2024-09-06T13:49:26Z

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.

rkta · 2024-09-28T13:15:27Z

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

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.

Is this still valid? If yes, can you make that improvement @el-remph , please.

rkta reviewed Jul 14, 2024

View reviewed changes

el-remph added 2 commits August 19, 2024 03:02

By default, with -o inline_img_protocol=-1, determine the value of

c05f80c

inline_img_protocol based on the invoking terminal

* Compile out enable_inline_img ifndef USE_IMAGE

7d07f7f

* Make auto-selection a true option for inline_img_protocol, with a separate variable for configuration

el-remph force-pushed the auto-inline-images branch from 3629d57 to 7d07f7f Compare August 19, 2024 04:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto inline images #302

Auto inline images #302

el-remph commented Jul 12, 2024

rkta commented Jul 14, 2024

rkta Jul 14, 2024

rkta Jul 14, 2024

bptato Jul 14, 2024

rkta Jul 14, 2024

rkta Jul 14, 2024

el-remph Jul 14, 2024

rkta Jul 14, 2024

el-remph Jul 14, 2024

bptato commented Jul 14, 2024

el-remph commented Jul 14, 2024

el-remph commented Jul 14, 2024

bptato commented Jul 15, 2024

el-remph commented Jul 21, 2024

rkta commented Jul 22, 2024

bptato commented Jul 22, 2024

rkta commented Jul 23, 2024 via email

bptato commented Jul 23, 2024

rkta commented Jul 23, 2024 via email

el-remph commented Aug 19, 2024

rkta commented Sep 6, 2024

rkta commented Sep 28, 2024

		@@ -0,0 +1,127 @@
		/* The only externally visible symbol exported by this module is

Auto inline images #302

Are you sure you want to change the base?

Auto inline images #302

Conversation

el-remph commented Jul 12, 2024

rkta commented Jul 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bptato commented Jul 14, 2024

el-remph commented Jul 14, 2024

el-remph commented Jul 14, 2024

bptato commented Jul 15, 2024

el-remph commented Jul 21, 2024

rkta commented Jul 22, 2024

bptato commented Jul 22, 2024

rkta commented Jul 23, 2024 via email

bptato commented Jul 23, 2024

rkta commented Jul 23, 2024 via email

el-remph commented Aug 19, 2024

rkta commented Sep 6, 2024

rkta commented Sep 28, 2024