Skip to content

Conversation

@shashwat1002
Copy link
Contributor

@shashwat1002 shashwat1002 commented Jun 6, 2022

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.
  • I have mentioned this change in the changelog.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

On determination that the imgops functions have lowest dependency and need to be used as-is, that is where I have begun porting.

The PR introduces (as of now) a function with an identical signature to rgb_to_hsv with rest of the code unchanged. The DISABLE_RUST flag is used to decide if the Rust version is compiled or the C version.
Edit: hardsubx_imgops is now fully ported

The compilation works and the executable runs properly on the files I have. However, the who suite of tests must be run as soon as the imgops module is ported.

Ps: I discovered that when building using cmake, Rust compilation is disabled by default and I had to explicitly set the DWITHOUT_RUST=OFF
I am unsure if this is the expected behaviour.
Edit: apparently this is because cmake caches options

Cc: @PunitLodha, @cfsmp3
please look this over once to check if I am not doing something obviously dangerous. I will continue after feedback.

@shashwat1002 shashwat1002 changed the title [WIP] Port hardsubx imgops Port hardsubx imgops Jun 7, 2022
@shashwat1002
Copy link
Contributor Author

shashwat1002 commented Jun 7, 2022

PR updated
@PunitLodha please review

Copy link
Member

@PunitLodha PunitLodha left a comment

Choose a reason for hiding this comment

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

Also fix the formatting issues by using rustfmt and clang-format

@shashwat1002
Copy link
Contributor Author

Please check now @PunitLodha

Also note that, the code now is mathematically different for the rgb_to_lab function
I was looking into the actual algorithm so as to write better comments, I noticed that the existing disagreed with several references I found. I may be missing something, but I am not sure

Source:

@shashwat1002 shashwat1002 force-pushed the port_hardsubx_imgops branch from 76178df to dcd6d8a Compare June 10, 2022 21:47
@shashwat1002
Copy link
Contributor Author

shashwat1002 commented Jun 13, 2022

@PunitLodha
Rewrote functions using the palette crate

The result is definitely different from the original C code
There is still a lot of hallucination (both the old and the new version)

I believe the new version is leading to slightly less hallucinations when it comes to lines which sort of make sense in the result. I don't have a good metric as of now.

Ps: Also it seems the rust compiler does not support deconstruction on left side in assignment statements because it worked on my system but not in the checks, so I just converted and committed

@PunitLodha
Copy link
Member

The result is definitely different from the original C code

This is expected. Once we update the preprocessing code, we should be seeing better results than before

Ps: Also it seems the rust compiler does not support deconstruction on left side in assignment statements because it worked on my system but not in the checks, so I just converted and committed

This is because the tests use rust 1.56.0, and destructuring was added in 1.59.0

Copy link
Member

@PunitLodha PunitLodha left a comment

Choose a reason for hiding this comment

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

Please fix the formatting, https://github.com/CCExtractor/ccextractor/runs/6866810135?check_suite_focus=true

Always make sure that the CI checks are not failing

#![allow(non_camel_case_types)]
#![allow(non_snake_case)]

extern crate palette;
Copy link
Member

Choose a reason for hiding this comment

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

@shashwat1002
Copy link
Contributor Author

Please fix the formatting, https://github.com/CCExtractor/ccextractor/runs/6866810135?check_suite_focus=true

Always make sure that the CI checks are not failing

I wanted to ask about that actually. I ran clang formatter, which one should I use then?

@PunitLodha PunitLodha merged commit 0f90afa into CCExtractor:master Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants