Collaborating on this project

[aferrero2707]

Hi! I just discovered the existence of this VIPS-based image viewer, and I am very much interested in helping you to extend the functionalities of vipsdisp.

At the moment I have just forked the project and added some travis ci configuration that allows to automatically build and generate an AppImage package for the application.

However, I have few suggestions on which I am ready to contribute:

1. add proper color management that takes into account the monitor profile (if I understand correctly, at the moment the image is converted to sRGB)
2. improve zooming speed by pre-computing a bunch of scaled-down copies
3. change the "best fit" zoom mode so that it shows the full image, and center it in the preview area
4. full screen and slideshow modes (but at a second stage)

The idea would be to take advantage of VIPS resizing speed and multi-threading to create a fast and lightweight image viewer with accurate color management, something that seems to be in quite some high demand in the FLOSS community.

What do you think?

[jcupitt]

Hey Andrea, nice to hear from you!

Sure, I've not had time to finish it :( My plan was to get vipsdisp mostly done, then paste it into nip2 to make nip3.

The big issue is flickering: as you zoom in and out, you'll see horrible flashing of the image. I've experimented with a range of solutions and I think it has to be something quite complicated :-(

vipsdisp needs to allocate an off-screen client-side pixel buffer the size of the window (not the image!), and paint the screen from that. When you zoom, instead of discarding the old pipeline and linking the display to the new one, it'll need to make a new pipeline and use that to incrementally update the off-screen buffer.

This means that vipsdisp must handle all scrolling in the window: it'll have to implement the scrollable interface, and handle moves that keep content intelligently. What a pain! The current nip2 image viewer has all this code, but it'll need to be copied over and reworked. I was hoping to use the gtk3 scroll system.

The other big TODO is a proper model-view split for rendering parameters. There needs to be an object that holds things like contrast and scale, and the display painter should be linked to it with a set of change signals. Again, the nip2 image viewer has this (it's called conversion, ie. the thing that manages the pixel -> display conversion), it would need to be copied over.

Anyway, that's the point I got to. On your ideas:

1. Good idea! It should be a simple thing to add. A proper conversion model would make it even simpler.

2. I'd like to be able to exploit things like pyramidal tiffs and openslide images too.

3. Agreed.

4. gtk3 full-screen mode is simple to support (I think).

I like "eog", but vipsdisp could (potentially) be nicer, especially with large images (obviously).

[jcupitt]

I've added you as a collaborator, please push away!

Large changes that need discussion are best done via branches and PRs still, but please feel free to push small stuff to master as you like.

[aferrero2707]

Concerning zooming, I had similar problems to solve for photoflow, but I took a different route... I do not like to use the mouse wheel for zooming, because it means that you will not be able to pan the image with a touchpad. In my experience, using two-fingers horizontal and vertical scrolling is the most intuitive and quick way to look through an image in 1:1 zoom mode.

Moreover, I'm ready to bet that most of the users will basically use two zoom modes, "best fit" to look at the image as a whole, and 1:1 for pixel-peeping and checking the image sharpness. All the rest can be handled with keyboard shortcuts, which IMHO are at least as good (if not better) than mouse wheel zooming.

This means you can use the signals of a standard GTK scrolled window in order to handle the image panning, and simply change the values of the adjustments associated to the horizontal and vertical scroll bars when you change the zoom level.

Do you think this makes sense?

For point 1, I am writing some code that bypasses the slow LCMS2 conversions in some specific (but quite common) cases, like conversions between two matrix-base RGB profiles.

For point 2, I am already using in-memory pyramids in photoflow: whenever a new image is opened, I immediately compute the 1:2, 1:4, 1:8... versions, storing them either in memory or raw disk buffers depending on their size. When computing the final image at a given zoom level, the closest pyramid level is picked as a starting point.

What I do not understand is what you mean by "contrast and scale". Do you have in mind to also allow some edits to the image? I would personally not go along this way, at least not at the beginning... an image viewer is not an editor, and we have already plenty of editors out in the wild ;-)

[jcupitt]

For contrast and scale, I need to be able to put pixels through:

V' = a V + b

with sliders for a and b, and with a on a log scale. I'd like things like false-colour too.

My idea with the mousewheel was just to use the standard bindings, so left drag for pan and wheel for scroll. I usually use keybindings myself, of course.

[jcupitt]

Oh, and contrast/scale just affect the conversion for display, they do not change the image, of course. You need them for medical imaging, and they are useful for photography too, if you want to be able to check pixels in very dark or light areas, especially with 16-bit images.

[aferrero2707]

Now I understand! Given that scaling of values works best in linear gamma, here is the kind of pipeline that I would propose, at least for photographic needs:

• open image
• promote to floating point and normalize to [0..1] (needed for the icc conversions)
• convert to a common linear RGB color space, like Rec2020 or ACEScg
• pre-compute the zoom levels

For displaying the image, here is what I would do:

• pick the closest pyramid level, do the final scaling and insert a tile cache
• apply the aV+b scaling
• convert to the monitor profile and finally to 8-bit integer

Does this make sense?

Il 04/dic/2017 19:10, "John Cupitt" [email protected] ha scritto:

Oh, and contrast/scale just affect the conversion for display, they do not change the image, of course. You need them for medical imaging, and they are useful for photography too, if you want to be able to check pixels in very dark or light areas, especially with 16-bit images.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jcupitt/vipsdisp/issues/1#issuecomment-349051709, or mute the thread https://github.com/notifications/unsubscribe-auth/AF8M9SYBsK_7nF9QgzZyBWOJS6G2mK6wks5s9DWogaJpZM4Q1Afq .

[jcupitt]

Yes, you can see the two-stage pipeline it's using here:

https://github.com/jcupitt/vipsdisp/blob/master/imagedisplay.c#L329

That's doing any geometric stuff, like zoom and subsample, but does not alter pixel values. The output of this stage is used to update the status bar, for example, which shows the pixel values stored in the file, not as sent to the screen.

Stage two takes that and does radiometric stuff, ie. conversion to 8-bit sRGB pixels for the screen:

https://github.com/jcupitt/vipsdisp/blob/master/imagedisplay.c#L365

So the aV+b would slot in there somewhere.

I see now I started adding a separate conversion.c object to manage these transforms:

https://github.com/jcupitt/vipsdisp/blob/master/conversion.c

but I think it's unfinished.

[jcupitt]

I thought about this some more overnight, and I think we might have slightly different aims.

My desire is to have a testbed for the image display component for nip3, and as a side-benefit, have a small tool for image display. So, for me, all the features of the nip2 image display window are must-haves, including things like overlays, which are probably not so useful for you.

Perhaps we should fork the project after all? libvips 8.6 is (finally!!) almost done (the end of this week, hopefully), so I plan to attack nip3 next, and this project is the next thing on that list.

Perhaps you could just do small things on it for a few months, and then when I get it to the point where I can copy-paste it into nip3, you could take over and do whatever you like. What do you think?

I have a new job, by the way. In January I start full time work on babies brains! I've been part-time on diseased lungs for 12 years now, so it'll be a nice change. I'm hoping I'll be able to do some nip3 work in my new role.

Did you see the what-new-in-8.6 post?

http://jcupitt.github.io/libvips/2017/11/28/What's-new-in-8.6.html

[aferrero2707]

Congrats for your new job!

I have forked the project already, and will do some experimenting in my own copy for a while.

First things I want to try are image pyramids for faster zooming, better positioning of the displayed image when it does not fill the drawing area, and full color management.

My initial goal would be to have something like eog, but faster (particularly with big images), with accurate color management, multi-threaded and multi-platform.

[aferrero2707]

I started to put my hands on your code, in my forked repo.

The main thing I added was the creation of image pyramids when a file is loaded (see code starting from here). There is a define switch at the beginning of the file that allows to enable/disable the use of pyramids, to compare performances.

I also changed the magnification factor to floating point, to allow for non-integer factors and use vips_resize() for the final scaling.

Another modification I made is to start the visualisation in "best fit" mode, and added a couple of methods to refresh the preview in "best fit" mode whenever the window size is changed.

I also observed a couple of issues, namely:

• when I try to open some TIFF files, the program stops with this message: (vipsdisp:4594): VIPS-WARNING **: Unknown field with tag 36867 (0x9003) encountered When I open the same with photoflow I also get the message, but then the processing continues... do you have any idea of why this warning is fatal in the vipsdisp case?

• now that the zooming speed is increased by the use of pyramids, I sometimes get crashes due to the use of un-referenced objects. I have no clue why...

Let me know your impressions, if you have some time to test the code...

[jcupitt]

Hiya, on 1) there's a thing in disp.c to enable abort on warning:

https://github.com/aferrero2707/vipsdisp/blob/master/disp.c#L164

it's handy for debugging, but you'll want it off for production.

2. Sorry, some kind of race I guess? They are very annoying to debug :(

If I run the non-pyr version, I can whizz the mousewheel up and down very fast and not see the problem, I don't know if that helps.

Races usually happen when callbacks occur in an order the program is not expecting (eg. a paint after the image has been unreffed), so to debug them you add printf()s to the start of all the relevant functions to you can record execution order, then trigger a crash to a logfile, then read the logfile backwards from the end until you find something happening in the wrong order. It's a really horrible job.

[aferrero2707]

there's a thing in disp.c to enable abort on warning

Thanks, that made the trick!

The race condition seems to be gone after fixing some obvious mistake in the gobject references.

I am now scratching my head to find an elegant way to solve the flickering of the image during zooming/panning. One possibility would be to first draw an approximate image by up-scaling the closest (but smaller) pyramid level, and then refine the result with the accurate vips_resize() method.

Meanwhile I have introduced an ICC transform operation that is adapted to floating-point data, and promoted the input image to floating point for better accuracy.

[jcupitt]

As I said, I think we need our own off-screen buffer and to handle all scrolling and resize events ourselves :( what a pain. I'll have a go next week, maybe.

[aferrero2707]

Another thing that I have noticed is that the image updating by vips_sinkscreen() does not seem to saturate the available CPUs, and the tiles seem to be drawn sequentially more than in parallel.

Is there a way to quantify the threading overhead and the amount of parallelisation?

Thanks!

[jcupitt]

I tried watching top while zooming out of a large jpg, and I hit about 170% CPU peak. A long way off the 12 cores this machine has, you're right.

My guess is that for large jpg image, libvips is unpacking to a temporary file, then allocating a single mmap window, sharing that between threads, and scrolling it up and down. With many threads working over a large area, you'll see horrible thrashing as the window is moved constantly.

The idea was to keep virtual memory use down by sharing input regions, but perhaps that's not a good idea in this case. We should probably experiment with a private window for each thread.

[aferrero2707]

I need to double-check, but my impression is that the same happens when the input image is stored in a memory buffer.

Let me know what I can do to help you on this...

Il 08/dic/2017 17:12, "John Cupitt" [email protected] ha scritto:

I tried watching top while zooming out of a large jpg, and I hit about 170% CPU peak. A long way off the 12 cores this machine has, you're right.

My guess is that for large jpg image, libvips is unpacking to a temporary file, then allocating a single mmap window, sharing that between threads, and scrolling it up and down. With many threads working over a large area, you'll see horrible thrashing as the window is moved constantly.

The idea was to keep virtual memory use down by sharing input regions, but perhaps that's not a good idea in this case. We should probably experiment with a private window for each thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jcupitt/vipsdisp/issues/1#issuecomment-350302735, or mute the thread https://github.com/notifications/unsubscribe-auth/AF8M9SR8AVuaW0XaCBthbPFEgGKf9SWpks5s-V_4gaJpZM4Q1Afq .

[jcupitt]

I looked over the code again, and it keeps a set of windows per image and shares them between threads.

I think this was a good idea back when 32-bit platforms were important and we had to keep VM use under control, but I doubt if it's necessary now. I'll make an issue for experimenting with a set of windows per thread instead.

[jcupitt]

I spent some time rereading the window code and found a terrible problem! It was continually freeing and newing windows instead of reusing them. Very embarrassing. I think that code can’t have been looked over for years and years.

Could you try git master libvips? It’s about 5x faster for zoom out for me.

[aferrero2707]

Will test it asap, and let you know... this demonstrates once more that looking at things from a different perspective can be very helpful! ;-)

[aferrero2707]

I am not sure to see such a speed enhancement... could you give me a simple snippet of code for which the speedup is measurable?

Thanks!

P.S: I am doing most of my tests under OSX, don't know if this is relevant.

[jcupitt]

It has to be an image over 100mb uncompressed, so more than about 6k x 6k.

It should be obvious: vips used to chug a bit zooming out 16:1, you'd see tiles painting in strips, but now it's instant at all zoom levels.

Old and busted 8.6.0:

$ time vips subsample wtc.jpg x.v 16 16
real	0m3.220s
user	0m1.262s
sys	0m0.365s

New hotness git master:

$ time vips subsample wtc.jpg x.v 16 16
real	0m2.352s
user	0m1.322s
sys	0m0.338s

[aferrero2707]

I have started to test the current libvips git master, but I cannot see a visible difference... the preview in vipsdisp is still being visibly built tile-by-tile.

I have prepared a small test benchmark in my vipsdisp fork, which does the following:

• opens an image from disk
• promotes the image data to floating point and normalizes the range to [0,1], using vips_linear1()
• applies my own version of the floating-point ICC transform to linear Rec.2020
• shrinks the image with vips_resize()
• saves the result to memory with vips_image_write_to_memory()

The command is called resize and the corresponding source file is resize.c.

If I process an 50Mpx Jpeg with resize, it runs for few seconds but the CPU usage hardly goes beyond 100% on my 4-core laptop, which seems to be strange... This is using libvips compiled from git master.

Hope this helps!

[jcupitt]

Happy new year!

I've committed a set of changes to imagedisplay.c that make it do its own scrolling. It implements a gtkscrollable interface and redraws the fixed imagedisplay itself via an intermediate buffer.

This means it can keep the screen between updates, and therefore no more flickering on zoom! It looks much nicer. It seems fast and stable for me.

Next I'm going to fix up the "conversion" class and move all of the zoom / unzoom stuff out of imagedisplay.

[jcupitt]

I added centre on zoom out as well. I fixed (I think) a race in vips_sink_screen(): you need HEAD of 8.6, or git master.

[aferrero2707]

COOL!!! I just updated and compiled the current version, and works very well!

From my side, I've been working on three features:

• fractional zooming for better "best fit" mode
• color management for converting to the display profile as well as scaling in linear gamma for better quality
• support for high-dpi screens

Next week I will merge your changes and make a new version, then you can decide if you want to pick some of the changes...

Thanks!

[jcupitt]

Sounds good!

I hope to have imagedisplay split into separate widget and conversion objects today. Splitting it should make adding fancier conversions, like colour management, much easier.

[aferrero2707]

I just committed a couple of fixes for the part of the code that computes the "best fit" scaling factor.

[jcupitt]

Nice, I've merged it to my conversion-object branch.

[aferrero2707]

Meanwhile I have experimented a bit with various resizing methods in imagedisplay_display_image(), and compared three cases:

1. the default: vips_subsample( image, &x, -imagedisplay->mag, -imagedisplay->mag, NULL )
2. vips_affine() with bilinear interpolation: vips_affine( image, &x, -1.f/imagedisplay->mag, 0, 0, -1.f/imagedisplay->mag, "interpolate", vips_interpolate_bilinear_static(), NULL )
3. vips_shrink(): vips_shrink( image, &x, -imagedisplay->mag, -imagedisplay->mag, NULL )

At least on my system, methods 1. and 2. are equally fast, and the tiles are updated almost instantly at any zoom level. However, vips_shrink() gives significantly worse performances, particularly when the image is zoomed at the "best fit" factor or lower. Is this somehow expected? Given that vips_shrink() is part of vips_resize(), I'd like to see if there is a way to make it faster...

[jcupitt]

I've merged my conversion stuff to master. There's now a conversion object which manages how images are transformed for display. You just change a param on that and the image updates automatically.

I added background load too! Try it with a huuuuge image.