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

Add support for CIELab, EYCC and CMYK #559

Merged
merged 1 commit into from
Aug 21, 2015
Merged

Add support for CIELab, EYCC and CMYK #559

merged 1 commit into from
Aug 21, 2015

Conversation

szukw000
Copy link
Contributor

Declares three new functions
Calls the three new functions
Collects data for CIELab, sets the color_space for EYCC and CMYK

@mayeut mayeut changed the title Defines three new functions Add support for CIELab, EYCC and CMYK Jul 30, 2015
@mayeut
Copy link
Collaborator

mayeut commented Jul 30, 2015

@szukw000

Thanks for the PR
A few remarks:

  • changing formatting in src/bin/common/color.c makes the diff harder to read (I know this is messy from file to file... Uniformize code syntax  #128)
  • same goes for unrelated changes: some opj_malloc changed to op_calloc with no apparent reason making this harder to read.

Now for the functional part,

  • CIE lab 'colr' box reading: allocation is not checked. In case header length is not 7 or 35, the buffer contains uninitialized values.
  • icc_profile_buf: I don't know if this is the right thing to use this one (it has the advantage of not breaking ABI). Seems to be pretty specific. At least a comment shall be added in openjpeg.h for this member.
  • icc_profile_len: This one shall be properly set.

@mayeut
Copy link
Collaborator

mayeut commented Jul 30, 2015

One more thing,
I suppose you have test data that goes with these changes. It's always a good thing to add tests for new functions. You can open a PR on uclouvain/openjpeg-data with the data & update this PR with corresponding tests.

@mayeut mayeut mentioned this pull request Jul 30, 2015
Declares three new functions
Calls the three new functions
Collects data for CIELab, sets the color_space for EYCC and CMYK
@szukw000
Copy link
Contributor Author

@mayeut ,

  1. replacing malloc() with calloc() was necessary, because the number of info entries
    may be greater than the number of components: thus info entry values change from
    time to time (100, 15xyz, 32xyz, etc). 'issue414-5C-cdef.jp2' has 6 info entries and
    5 components.

  2. I used the icc_profile_buf with icc_profile_len = 0 to avoid ABI change AND to use
    a 'structure' that is otherwise unused. If you want to use icc_profile_len > 0, then
    you must introduce another color_profile value.

  3. Yes, I have some test data:

    eci-090-CIELab.jp2
    eci-091-CIELAB.jp2
    issue326-CIELAB.jp2

    img0-CMYK.j2k
    issue205-CMYK.jp2
    issue327-18_1805_a4_1-CMYK.jp2

    issue236-ESYCC-CDEF.jp2

    But I can not open a PR on uclouvain/openjpeg-data with the data & update this PR with
    corresponding tests: I do not know how to do that. GITHUB is a mystery.

winfried

@szukw000 szukw000 closed this Jul 31, 2015
@szukw000
Copy link
Contributor Author

szukw000 commented Aug 4, 2015

@mayeut ,

you can download the example files here:

http://home/arcor.de/szukw000/images-for-mayeut.tar.xz

I remove this archive on Oct 31, 2015.

I suppose you have test data that goes with these changes. It's always a good thing to
add tests for new functions. You can open a PR on uclouvain/openjpeg-data with the
data & update this PR with corresponding tests.

I know that there are 2 branches in openjpeg-data:

openjpeg-data/{baseline|input}

each with 2 branches conformance and nonregression.

winfried

@szukw000 szukw000 reopened this Aug 4, 2015
mayeut added a commit that referenced this pull request Aug 21, 2015
Add support for CIELab, EYCC and CMYK
@mayeut mayeut merged commit 3109759 into uclouvain:master Aug 21, 2015
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