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

Pan at full resolution and fix iso12646 white border jumps #15084

Merged
merged 5 commits into from Aug 22, 2023

Conversation

dterrahe
Copy link
Member

@dterrahe dterrahe commented Aug 19, 2023

When zoomed into the central darkroom image or secondary preview window, you can drag the image to move it around. As soon as the available full size preview no longer matches the location on screen, we instead get a low resolution preview upscaled to fill the window, while the full size preview is being recalculated. This causes a jump to a blocky image and a jump back to quality image when done.

This PR instead continues to use the high res buffered image but correctly repositions it and temporarily fills the rest of the image (the corners) with the upscaled preview. When an updated high res image arrives it is being used, even if we have continued to move around in the mean time; it probably still better covers the window than the original one.

This PR also fixes issues (#13441 (comment)) with the size of the white border when iso12646 color assessment is turned on; this was done differently while in low res and high res modes and in the secondary window (only in high res), causing jumps/flashes when the calculation completed.

Double buffering was removed from dt_view_paint_surface because gtk3 now takes care of this for all widget repainting.

A lot of confusing/obfuscating/duplicate code around zoom scale and border calculation was removed. Obviously much more could be done here.

EDIT: use same approach for zooming in/out.

@dterrahe dterrahe added bugfix pull request fixing a bug feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters difficulty: hard big changes across different parts of the code base scope: UI user interface and interactions scope: performance doing everything the same but faster scope: codebase making darktable source code easier to manage labels Aug 19, 2023
@dterrahe dterrahe added this to the 4.6 milestone Aug 19, 2023
@dterrahe
Copy link
Member Author

dterrahe commented Aug 19, 2023

This change could allow a further improvement where the size of a recalculated zoomed image could be increased to cover more than the currently visible area, so it would not be immediately necessary to recalculate again on a small further move, as long as the calculated area still completely covered the visible area. You'd probably only want that while moving, because otherwise each parameter change would trigger a larger-than-necessary recalc, slowing down responsiveness.

Edit: I do not intend to implement that as part of this PR.

@TurboGit
Copy link
Member

I have tested this and found some issues about buffering. When you drag the image the borders that need new content (out of the view before, right border when dragging to the left for example) is reconstructed by band (no buffering).

Also when I double click on a portrait image the top/bottom is refreshed with a little delay and when I click on a landscape image the left/right is refreshed with a little delay. It seems that first the square part is displayed and then the borders. Again I suspect a buffering issue. This is reproduced more easily with a large image say 30Mpix or above.

@dterrahe
Copy link
Member Author

dterrahe commented Aug 21, 2023

That is exactly what this PR is meant to do.

Any area that was not previously in view (and where we therefore do not have any high resolution results) will as before fall back to an upscaled section of the preview image (also shown in the navigator top-left). This is all we used to do previously, until a newly calculated high res image for the exact new area became available (seconds later, for a complex pipe).

In this PR for the area where we do have a better image still, namely the one that was visible before the move/zoom, it is panned, and zoomed where necessary and overlaid on top of the low quality image. There's nothing else we can do until the recalculation is finished and we have again a high res image for the whole view. But at least this prevents the image we were looking at temporarily degrading in quality (while some new area is being built up).

To see even better which areas need recalculation, switch on focus-peaking mode. Previously, while we were waiting for the recalculation, no focus-peaking markings were shown at all (so they would flash off and back on as we moved/zoomed).

Buffering issues look different; they occur when the results of some redrawing actions, for example wiping the window clean, are already shown on the screen while we are still drawing. Sometimes this only happens for part of the screen while it is refreshing, so you may see a break between top and bottom, which is corrected within the next screen refresh 1/60th of a second later. The result is quick flashes and tears.

EDIT: Unless I completely misunderstand what you are saying and you see completely white bands appearing on the top/bottom right/left? When you say "double click" do you mean middle-click (zoom)? Double click should switch to lighttable?

@TurboGit
Copy link
Member

TurboGit commented Aug 21, 2023

@dterrahe : I was probably not clear, on the lighttable if you double-click on an image to go to the darkroom I have indeed a two step display. First the center square area and then the top/bottom for portrait images or left/right for landscape images.

EDIT: it is a fast effect but quite disturbing and a regression compared to what we have today.

@dterrahe
Copy link
Member Author

I see what you mean and will investigate. I suspect the full image size dimensions of one of the pipes doesn't get updated instantaneously, so they get temporarily out of sync.

@dterrahe
Copy link
Member Author

if you double-click on an image to go to the darkroom I have indeed a two step display. First the center square area and then the top/bottom for portrait images or left/right for landscape images.

I believe this is now fixed.

@dterrahe dterrahe marked this pull request as draft August 22, 2023 13:18
@dterrahe dterrahe marked this pull request as ready for review August 22, 2023 14:25
@dterrahe
Copy link
Member Author

Last commit fixes crashing due to insufficient locking of the preview buffer. Was especially notificable when changing aspect ration, for example by rotating.
Also more aggressively backfills with preview in case of window aspect changes.

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

Working for me now. The pan experience is lot better. Thanks!

@TurboGit TurboGit merged commit 10652b3 into darktable-org:master Aug 22, 2023
8 checks passed
@TurboGit
Copy link
Member

We need a release note entry, TIA.

@dterrahe dterrahe deleted the pan_full_resolution branch August 22, 2023 18:24
@dterrahe
Copy link
Member Author

Release Notes:
Performance Improvements:

  • When panning or zooming, a low resolution placeholder used to be shown until the image was fully recalculated. Now the part of the previous high quality preview that is still visible is moved or resized and only the edges are temporarily shown in low quality.

Bug Fixes:

  • The white borders for ISO 12464 color assessment (toggled with CTRL+B) are now correctly sized and placed at all zoom levels and don't flash when switching between low and high quality preview, both in the center view and secondary preview window.

@jenshannoschwalm
Copy link
Collaborator

Really cool :-)

@piratenpanda
Copy link
Contributor

piratenpanda commented Aug 26, 2023

This is really hard to work with tbh. After zooming or panning around there are blocks which are not completely loaded and visible borders take my attention. When trying to do something, suddenly some parts of the image jump around as the proper image is loaded there. For me this made editing much more uncomfortable as so much of the image is moving now.

@piratenpanda
Copy link
Contributor

Bildschirmaufzeichnung.vom.2023-08-26.12-03-21.webm
Bildschirmaufzeichnung.vom.2023-08-26.12-04-10.webm

Is this how it should be?

@dterrahe
Copy link
Member Author

Is this how it should be?

Yes. Previously the whole image was blurry and popped into focus suddenly (and also the focus-peaking and over exposed marking switched off and back on). It still takes the same amount of time to be fully updated.

It always seemed to me that the blurry scaled preview image was slightly shifted compared to the final image (so there was a jump when switching to the high-res results). I haven't changed this, so now this disconnect can be seen while both parts are overlayed. It could be that this is just an effect of the lower resolution getting scaled up.

@piratenpanda
Copy link
Contributor

I see. I wonder why it "forgets" already rendered parts though when not zooming.

As it is now, it's much more taxing on my brain, but I guess I'm in the minority here.

@dterrahe
Copy link
Member Author

I wonder why it "forgets" already rendered parts though when not zooming.

It depends on the size of your cache, but also each rendered image with a slightly different position is considered separately. When you use the mouse to pan you'll rarely exactly hit the same image. That will happen much more often if you use the arrow keys to pan; try pressing left arrow and then right arrow a few times.

As an aside; I'd considered just rendering the unknown edges and then merging them with the already available part of the image before caching, but this is very non-trivial as it would have to be done at all levels of the cache. A lot of effort for an area where people have happily lived with getting dumped back into low-res while recalculating for years.

Another approach (that vkdt has taken) is to always render in full resolution and pick the view from that. Might be doable on a really fast rig, but would sacrifice speed when just changing parameters and not panning/zooming.

it's much more taxing on my brain

You didn't notice the temporary deterioration of the whole image during panning/zooming before? Brains are different :-)

@piratenpanda
Copy link
Contributor

You didn't notice the temporary deterioration of the whole image during panning/zooming before? Brains are different :-)

Sure, but having a "loading" blurred version of everything was easier than seeing part of the image move around. But I'm also easily distracted.

@dterrahe
Copy link
Member Author

having a "loading" blurred version of everything was easier than seeing part of the image move around.

If you don't get used to it after a while, or if there are a lot of other people with the same "brain" (please chime in here if that's you!) it might be worth implementing a configuration switch to disable this. As always, we want to limit those kinds of choices (especially if they are confusing for new users or end up not being used in the long run but still needing to be supported and thereby adding complexity/bugs/fear) but we don't want to make anyone unhappy either :-)

If someone else is interested in investigating whether the low-res preview is correctly positioned or whether there indeed is a small shift, as there sometimes seems to be during the pop into focus, that would be helpful too. I've by now stared too long at all the parameters that control zoom levels (including dpi) and positioning to be enthusiastic about spending more time on it.

@piratenpanda
Copy link
Contributor

I'd be happy if there'd be a darktablerc setting. Doesn't need to be in the settings dialog. But let's see if I just get used to it.

@zurdo-10
Copy link

Hi, for info not sure what to expect but here is how is behaving on Mac intel i7 64 GB/AMD Radeon Pro 5700 xt 16 GB

dt.378.zoom.and.pan.mp4

@dterrahe
Copy link
Member Author

That looks wrong. Can't see too well on my phone, but it seems the repositioned high-res part is moving faster than the mouse and the low-res part behind it slower? How did it use to behave in 4.4.2? What is the dpi setting of your monitor (in X) and in the preferences/general tab?

@zurdo-10
Copy link

I am on iMac 27 inch Retina display 5120x2880, ppi 218, and default in preferences. In 4.4.2 the effect is different. Here is a recording

dt4.4.2.mp4

@dterrahe
Copy link
Member Author

Mac intel

Ah, I had missed the Mac bit...

That'll make it harder for me to replicate and experiment with/test solutions, since I don't have access to one of those, so I'll dig through the code to see if there used to be any Mac-specific adjustments (probably ppd/cairo_surface_set_device_scale related but also maybe _iso_12646_get_border should be used, or a version for the secondary preview window) and if I find something I'll depend on you to test possible fixes. Mac retina is funny because it seems to pretend internally that it is still a low res display to accommodate older software.

Could you also please try (and later test fixes) for the darkroom secondary preview window (the third button bottom-left) please? And would you be able to test multi-display (with the secondary window on a different display than the main window and with different OS scaling factors set for each display)? Or may another Mac user with a dual monitor setup would be willing to assist here?

@zurdo-10
Copy link

For the secondary preview window the issue is the same when display on the iMac, and also when display on a Eizo CS2420 (1920x1200) as secondary display.

@dterrahe
Copy link
Member Author

Release Note:
#15084 (comment)

@TurboGit
Copy link
Member

Release Note: #15084 (comment)

Missed that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug difficulty: hard big changes across different parts of the code base feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: codebase making darktable source code easier to manage scope: performance doing everything the same but faster scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants