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

Avoid roi_in over-expanding #15885

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

jenshannoschwalm
Copy link
Collaborator

Due to imprecisions or ceil functions we might set roi_in size to be larger than what we provide via piece->buf_in.width/height.
Relevant for modules that do scaling in the pipe like demosaic or finalscale.

Fixes #15876

We might want to do all these "safety checks" not in the module's code but in pixelpipe while we do all the modify roi_in calls, as this PR is meant for 4.6 and 4.8 branch just this fix.

While being here it doesn't hurt to fix some comments too.

@jenshannoschwalm jenshannoschwalm added bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels labels Dec 16, 2023
@jenshannoschwalm jenshannoschwalm modified the milestones: 4.6, 4.8 Dec 16, 2023
@TurboGit TurboGit modified the milestones: 4.8, 4.6.1 Dec 16, 2023
@gi-man
Copy link
Contributor

gi-man commented Dec 16, 2023

I tested the PR on windows 11 and it works as when using the 0/1080 values.

The issue with 1/1080 is still happening. I will start a new Issue for it. Basically the scale is being calculated as a very small number (close to zero), so the resulting image is too small.

    22.2689 [dt_imageio_export] [export] imgid 1, 4936x3296 --> 1x0 (scale 0.000203). upscale=no, hq=yes
    22.2689 pixelpipe starting CL      [export]                                (   0/   0)    1x   0 scale=0.0002 --> (   0/   0)    1x   0 scale=0.0002 device=0 (nvidiacudanvidiageforcertx3060)

@jenshannoschwalm
Copy link
Collaborator Author

so the resulting image is too small.

Yes, that's an UI issue for me. We should always restrict exported image sizes to something meaningful.

@gi-man
Copy link
Contributor

gi-man commented Dec 16, 2023

I was thinking of using the range limit in the field, but 0 is a valid number. I'm thinking it is not a significant issue. I will just leave it alone.

Due to imprecisions or ceil functions we might set roi_in size to be larger than what we provide
via piece->buf_in.width/height.

Relevant for modules that do scaling in the pipe like demosaic or finalscale.
Also ensure a minimal size to avoid crashes.
@jenshannoschwalm
Copy link
Collaborator Author

Latest squash-force-pushed commit ensures a minimal size to be processed to avoid crashing and finish the export pipe.
@TurboGit ready for review ...

@TurboGit
Copy link
Member

@jenshannoschwalm : If I read this correctly we now do not allow to export a picture smaller than 16 x 16. Right?

@jenshannoschwalm
Copy link
Collaborator Author

Yes. It is not a bad restriction - at least i think so. Also it doesn't make sense for most algos. Let me know if you don't agree.

@TurboGit
Copy link
Member

Yes. It is not a bad restriction

I fully agree, just to be sure I'm understanding the code properly.

@TurboGit TurboGit merged commit f638fad into darktable-org:master Dec 18, 2023
4 checks passed
@TurboGit
Copy link
Member

Thanks, merged in master and darktable-4.6.x branch.

@TurboGit TurboGit self-requested a review December 18, 2023 07:08
@jenshannoschwalm jenshannoschwalm deleted the demosaic_roi_in branch December 18, 2023 16:25
@jenshannoschwalm
Copy link
Collaborator Author

Release note for 4.6 and 4.6

Fix spurious crashes while exporting to some output sizes

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 priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when exporting (Windows)
3 participants