8000 Fix default blend colorspace for contrast equalizer module and dummy IOP by glasl · Pull Request #10798 · darktable-org/darktable · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix default blend colorspace for contrast equalizer module and dummy IOP #10798

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 18, 2022
Merged

Fix default blend colorspace for contrast equalizer module and dummy IOP #10798

merged 1 commit into from
Jan 18, 2022

Conversation

glasl
Copy link
@glasl glasl commented Jan 4, 2022

Fix Issue #10080:

  • Set default blend mode colorspace to scene referred.
  • Adjust default correspondingly in boilerplate file 'useless.c' in case future modules will be based on it

@glasl glasl changed the title [Issue #10080] Fix default blend colorspace for contrast equalizer module and dummy IOP Fix default blend colorspace for contrast equalizer module and dummy IOP Jan 4, 2022
@TurboGit
Copy link
Member

@aurelienpierre : I guess this is ok as for the previous same change in other iop and that this can go in 3.8.1, right?

@TurboGit TurboGit added the feature: redesign current features to rewrite label Jan 18, 2022
@aurelienpierre
Copy link
Member

yes

@TurboGit TurboGit added this to the 3.8.1 milestone Jan 18, 2022
@TurboGit
Copy link
Member

@aurelienpierre : Thanks! Merging now.

TurboGit
TurboGit approved these changes Jan 18, 2022
View reviewed 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.

Thanks!

@TurboGit TurboGit merged commit e1c8746 into darktable-org:master Jan 18, 2022
@TurboGit
Copy link
Member

Merged in master and darktable-3.8.x branch.

@TurboGit
Copy link
Member

Sadly this breaks integration test 0024:

Test 0024-contrast-equalizer
      Image mire1.cr2
      Expected CPU vs. current CPU report :
      ----------------------------------
      Max dE                   : 32.95917
      Avg dE                   : 4.18790
      Std dE                   : 3.94097
      ----------------------------------
      Pixels below avg + 0 std : 64.59 %
      Pixels below avg + 1 std : 86.84 %
      Pixels below avg + 3 std : 97.71 %
      Pixels below avg + 6 std : 99.98 %
      Pixels below avg + 9 std : 100.00 %
      ----------------------------------
      Pixels above tolerance   : 61.60 %
 
  FAILS: image visually changed
         see diff.png for visual difference
         (2.72239e+06 pixels changed)

Total test 1
Errors     1
   - 0024-contrast-equalizer
see test-20220118-125551.log

TurboGit added a commit that referenced this pull request Jan 18, 2022
@TurboGit
atrous: Revert to iop_cs_Lab (#10798).
9dee4ff 
This is because it introduces regression on corresponding test:

Test 0024-contrast-equalizer
      Image mire1.cr2
      Expected CPU vs. current CPU report :
      ----------------------------------
      Max dE                   : 32.95917
      Avg dE                   : 4.18790
      Std dE                   : 3.94097
      ----------------------------------
      Pixels below avg + 0 std : 64.59 %
      Pixels below avg + 1 std : 86.84 %
      Pixels below avg + 3 std : 97.71 %
      Pixels below avg + 6 std : 99.98 %
      Pixels below avg + 9 std : 100.00 %
      ----------------------------------
      Pixels above tolerance   : 61.60 %

  FAILS: image visually changed
         see diff.png for visual difference
         (2.72239e+06 pixels changed)

Total test 1
Errors     1
   - 0024-contrast-equalizer
see test-20220118-141420.log
TurboGit added a commit that referenced this pull request Jan 18, 2022
@TurboGit
atrous: Revert to iop_cs_Lab (#10798).
5514240 
This is because it introduces regression on corresponding test:

Test 0024-contrast-equalizer
      Image mire1.cr2
      Expected CPU vs. current CPU report :
      ----------------------------------
      Max dE                   : 32.95917
      Avg dE                   : 4.18790
      Std dE                   : 3.94097
      ----------------------------------
      Pixels below avg + 0 std : 64.59 %
      Pixels below avg + 1 std : 86.84 %
      Pixels below avg + 3 std : 97.71 %
      Pixels below avg + 6 std : 99.98 %
      Pixels below avg + 9 std : 100.00 %
      ----------------------------------
      Pixels above tolerance   : 61.60 %

  FAILS: image visually changed
         see diff.png for visual difference
         (2.72239e+06 pixels changed)

Total test 1
Errors     1
   - 0024-contrast-equalizer
see test-20220118-141420.log
@TurboGit
Copy link
Member

I have reverted the change on master & darktable-3.8.x for now.

@ralfbrown
Copy link
Collaborator

The function default_colorspace tells the pixpipe what colorspace to use for the iop's input buffer - so changing it from Lab to RGB for atrous.c means that the pixpipe adds conversion if the previous module used Lab and omits the existing conversion if the previous module used RGB.

@TurboGit
Copy link
Member

Right, and this conversion do change a lot the pixels, so clearly this is not an option for this module as we will break all current edits.

@ralfbrown
Copy link
Collaborator

Took another look at things. An iop has three optional functions: input_colorspace, output_colorspace, and blend_colorspace. If not defined, these default to calling the required default_colorspace. So what should work for atrous.c is to add

int blend_colorspace(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece)
{
  return iop_cs_rgb;
}

This will presumably add a pair of colorspace conversions any time blending is enabled if the following module takes Lab input. If the following module takes RGB, we need a conversion either way.

@TurboGit
Copy link
Member

Right, and this conversion do change a lot the pixels, so clearly this is not an option for this module as we will break all current edits.

Yes, but do we want that... After all the module is Lab so do we gain something having blending in rgb by default? This looks like a corner case to me and I'm really leaning toward a do-nothing solution on this at this stage.

@TurboGit
Copy link
Member

Having default blend to rgb we at least loose one thing : we cannot select Lab blending mode anymore.

@glasl
Copy link
Author
glasl commented Jan 19, 2022

Took another look at things. An iop has three optional functions: input_colorspace, output_colorspace, and blend_colorspace. If not defined, these default to calling the required default_colorspace. So what should work for atrous.c is to add

Thanks for all of your feedback - I noticed the blend_colorspace() api yesterday, too - still getting familiar with the code base.
Wanted to create another pull request, but I'm glad this discussion came first.

@TurboGit is right in that it's not possible to select Lab blending any more.

Blending is behaves different between Lab and rgb colorspace, so it might impact current edits done with Lab blending?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TurboGit TurboGit TurboGit approved these changes

@aurelienpierre aurelienpierre Awaiting requested review from aurelienpierre

Assignees
No one assigned
Labels
feature: redesign current features to rewrite
Projects
None yet
Milestone
3.8.1
Development

Successfully merging this pull request may close these issues.
4 participants
@glasl @TurboGit @aurelienpierre @ralfbrown
0