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

Reverting pipe stuff 501 #18362

Conversation

jenshannoschwalm
Copy link
Collaborator

No description provided.

@jenshannoschwalm jenshannoschwalm added this to the 5.0.1 milestone Feb 5, 2025
@jenshannoschwalm jenshannoschwalm added the bugfix pull request fixing a bug label Feb 5, 2025
@wpferguson
Copy link
Member

@TurboGit, @jenshannoschwalm do you want me to test this too?

@TurboGit
Copy link
Member

TurboGit commented Feb 5, 2025

@wpferguson : Yes please would be good to be sure that all is ok on 5.0.1 especially since I plan to cut the sources this week-end. TIA.

@wpferguson
Copy link
Member

I processed 47 images and got stuck in the "working" loop 4 times.

My setup was clone master, checkout release-5.0.0, pull the PR and apply it, compile and test. Is this an accurate way to test or should I have done something different?

@jenshannoschwalm
Copy link
Collaborator Author

It is the correct way. This is possibly one of the expose loops that has been fixed in master. Could you provide a log? -d pipe -d screen would be sufficient. I saw those in some of your earlier logs ...

@wpferguson
Copy link
Member

-d screen isn't a valid flag

@jenshannoschwalm
Copy link
Collaborator Author

-d pipe -d expose :-)

@wpferguson
Copy link
Member

wpferguson commented Feb 5, 2025

I guessed right :-D, here you go....

501_test1.txt

Stopped after the error occurred, so work backwards from the end

EDIT: Every time the loop occurred it was after I loaded the image and the style was applied, but before I started any processing.

EDIT 2: Here's another one,
501_test2.txt

@jenshannoschwalm
Copy link
Collaborator Author

Yes, exactly.

That's the dimension recalculating i guess. I will have a look over the next 2 days - not sure if we can fix that with one safe commit. Maybe you'll have to test again :-)

My guess would be f89705c you might cherry pick and test. If i remember correctly that was done while investigating these type of loops.

@jenshannoschwalm
Copy link
Collaborator Author

Here you go for testing

@wpferguson
Copy link
Member

wpferguson commented Feb 6, 2025

Processed 12 images, got a loop.
501_test3.txt

EDIT: Processed 16 more, another loop,
501_test4.txt

@jenshannoschwalm
Copy link
Collaborator Author

Both logs don't show that ???

@wpferguson
Copy link
Member

Sorry, forgot the flags. I had it baked into the command earlier.

@wpferguson
Copy link
Member

With logging on I can't reproduce. Processed 55 images without a problem.

@wpferguson
Copy link
Member

Kept testing, finally reproduced.
501_test6.txt

@jenshannoschwalm
Copy link
Collaborator Author

Aah, yes. I am sure this is not related to the original issue you bisected. For me it's a redrawing issue requesting a new dimension calculation. Unfortunately no idea how that is triggered or how/where to fix.

Can you confirm, that the added commit is at least making this a lot more stable from your testing?

@wpferguson
Copy link
Member

wpferguson commented Feb 6, 2025

With logging on it's very hard to reproduce. Without logging it occurs more often, but still less than it was.

I think a "normal" user probably isn't going to experience this, I do have some ideas about what is happening that I would like to test out (things I'm doing, not pixelpipe code).

All in all, I think it's good for 5.0.1.

src/common/iop_order.c Outdated Show resolved Hide resolved
This reverts commit 48282d4.

Some static fixes for iop_order
As the hash is based upon position in the pipe - this is not iop_order - we have to find and use that.
Only enabled pieces/modules should be integrated into the hash
dt_dev_pixelpipe_change() calls the possibly costly dt_dev_pixelpipe_get_dimensions()
in all cases in addition to possibly syncing pipe nodes.

In dt_dev_process_image_job() we check for situations where we *must* call dt_dev_pixelpipe_change()
as we don't have the pipe dimensions available in addition to pipe->changed != DT_DEV_PIPE_UNCHANGED.

This leads to a significant performance gain due to less "pixelpipe-runs" just done to get the dimension.

In dt_dev_pixelpipe_change() we reduce the amount of syncing if possible as there is another active
dt_dev_pixelpipe_change_t flag that includes work to be done.

Some log improvements reducing misleading noise.
@jenshannoschwalm jenshannoschwalm force-pushed the reverting_pipe_stuff_501 branch from 26dea41 to 98d2505 Compare February 7, 2025 16:36
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.

Thanks!

@TurboGit TurboGit merged commit f98dab9 into darktable-org:darktable-5.0.x Feb 7, 2025
@jenshannoschwalm jenshannoschwalm deleted the reverting_pipe_stuff_501 branch February 7, 2025 16:58
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants