search

InternationalColorConsortium / DemoIccMAX

Demonstration Implementation for iccMAX color profiles
Other
121 stars 37 forks source link

documenting tags #80

Open kateniolet opened 4 months ago

kateniolet commented 4 months ago

I'd like to start documenting enums in IccProfLib/icProfileHeader.h something like this, which could be hyperlinked to specification in generated docs, and wanted to check if the format and such before I start doing a bunch:

` /**

/ Private tags/ icSigEmbeddedV5ProfileTag = 0x49434335, / 'ICC5' / } icTagSignature; `

xsscx commented 4 months ago

Hello @kateniolet

Documenting enums in IccProfLib/icProfileHeader.h, this would be a nice feature.

Perhaps a file located contrib/IccProfLib/icProfileHeader.html with the documented enums, and/or add to Doxygen Config, what are your thoughts?

With respect to refactoring, agreed. As we submit PR's others will do same. If you have the time, please consider a PR with any refactored code. I will try to include your suggestion of documenting enums in IccProfLib/icProfileHeader.h in future, but hope you will consider providing the PR. Thank you for sharing, great idea!

xsscx commented 4 months ago

Hello @kateniolet

I had a few thoughts for Linking Source -> Specifications for your PR.

Confirm the Sources

The ICC publishes the Specifications at URL https://www.color.org/icc_specs2.xalter

ICC Links to Specs

Friendly URI's

Generate the friendly URI's using location.hash with anchors when converting the pdf->html

Perhaps something like:

/**
 * @enum icTagSignature
 * @brief Enum for ICC tag signatures.
 *
 * These tags are defined in the ICC.2:2023 specification.
 * For more details, refer to the specific sections in the ICC specification.
 *
 * @see <a href="https://www.color.org/specification/ICC.2-2023.html#section9.2.1">ICC.2:2023 Section 9.2.1</a>
 * @see <a href="https://www.color.org/specification/ICC.2-2023.html#section9.2.2">ICC.2:2023 Section 9.2.2</a>
 * @see <a href="https://www.color.org/specification/ICC.2-2023.html#section9.2.3">ICC.2:2023 Section 9.2.3</a>
 */
typedef enum {
    icSigAToB0Tag = 0x41324230, /**< 'A2B0' §9.2.1 */
    icSigAToB1Tag = 0x41324231, /**< 'A2B1' §9.2.2 */
    icSigAToB2Tag = 0x41324232, /**< 'A2B2' §9.2.3 */
    // ... (continue with other tags)
} icTagSignature;

Summary: The Source -> Specification hyperlinked would be very helpful. Please consider a PR if you have time.

kateniolet commented 4 months ago

I'm not 100% sure I follow your suggestion as URL's like https://www.color.org/specification/ICC.2-2023.html#section9.2.3 do not work currently (and I have no idea what/who would be involved in getting that to work)

And we probably wouldn't want to hard-code URL's into the source files anyway as that is pretty fragile and verbose, but rather use doxygen to recognize the tags when generating html documentation

If that sounds acceptable I'll do a PR with those changes.

Probably best, IMHO, is if the format is something like:

icSigAToB0Tag = 0x41324230, // @TAG A2B0 @ICC 2:2023§9.2.1

that way doxygen should (AFAIK, i'm not a doxygen expert by any means) be able to pick it up and create the URL links if we write the accompanying rules.

petervwyatt commented 4 months ago

What you are looking for is called "PDF Fragment Identifiers" - see RFC 8118 Section 3.

Unfortunately, the ICC specification PDFs hosted at https://www.color.org/specification/index.xalter do not include the necessary PDF data structures to make the "best" PDF fragment identifiers, such as using nameddest where the destination might be a nicely recognizable section/clause number such as nameddest=7.3.2.

However you can always use a PDF Fragment Identifier that opens to a specific page - and this will work across all browsers: e.g. https://www.color.org/specification/ICC.2-2019.pdf#page=39

Note that this uses 1-based page numbers and won't match the page numbers visible in footers for the ICC PDFs because the first many pages are labelled with Roman numerals: e.g. https://www.color.org/specification/ICC.2-2019.pdf#page=9 opens to page "ix" for the Foreword

PS. (Shameless self-promotion) I recently wrote about PDF Fragment Identifiers at the PDF Association website. Note also that not all browsers support all PDF Fragment Identifiers, but page= seems to be supported.

xsscx commented 4 months ago

Hello @kateniolet @petervwyatt

Sample Doxygen for DemoIccMAX at URL https://xss.cx/public/docs/DemoIccMAX/

I did a quick mockup for a few URI's in the Source File at URL https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h_source.html for the look and feel.

Example

typedef enum {
    [icSigAToB0Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a48a680ba3049e0c37a44da9f08e411c7)                          = 0x41324230,  /* 'A2B0' */ [�9.2.1](https://www.color.org/specification/ICC.2-2019.pdf#page=48) 
    [icSigAToB1Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5ab04125e571eb02f134e54fb3b0bcdb45)                          = 0x41324231,  /* 'A2B1' */ [�9.2.2](https://www.color.org/specification/ICC.2-2019.pdf#page=49) 
    [icSigAToB2Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a58f2209b5887e2dd73b279b5b2c6f4b2)                          = 0x41324232,  /* 'A2B2' */ [�9.2.3](https://www.color.org/specification/ICC.2-2019.pdf#page=49)  
    [icSigAToB3Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a19b39e0d70b49c0c8864ec3b5ee2f1ba)                          = 0x41324233,  /* 'A2B3' */ [�9.2.4](https://www.color.org/specification/ICC.2-2019.pdf#page=49) 
    [icSigAToM0Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a4101fbfb53a9e422f925a0eb9b9754dc)                          = 0x41324d30,  /* 'A2M0' */ [�9.2.5](https://www.color.org/specification/ICC.2-2019.pdf#page=49) 
    [icSigBlueColorantTag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5af4720cf718f11479079abbd8e2cf4cd8)                   = 0x6258595A,  /* 'bXYZ' */
    [icSigBlueMatrixColumnTag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a19e7177ed729ed7835222428eef4c2a3)               = 0x6258595A,  /* 'bXYZ' */
    [icSigBlueTRCTag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a8b6202952425dbdc400d2626e650c903)                        = 0x62545243,  /* 'bTRC' */
    [icSigBrdfColorimetricParameter0Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5ae6f4fd41479c1274ea14e0bb432f0253)     = 0x62637030,  /* 'bcp0' */ [�9.2.6](https://www.color.org/specification/ICC.2-2019.pdf#page=50) 
    [icSigBrdfColorimetricParameter1Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5af4af5bcf72d59f49965aa40c48908244)     = 0x62637031,  /* 'bcp1' */ [�9.2.7](https://www.color.org/specification/ICC.2-2019.pdf#page=50) 
    [icSigBrdfColorimetricParameter2Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a238f3512751f428723e6c394c9ec213d)     = 0x62637032,  /* 'bcp2' */ [�9.2.8](https://www.color.org/specification/ICC.2-2019.pdf#page=50) 
    [icSigBrdfColorimetricParameter3Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a09cd92af38d6cbff7b89298d89f51eeb)     = 0x62637033,  /* 'bcp3' */ [�9.2.9](https://www.color.org/specification/ICC.2-2019.pdf#page=51) 
    [icSigBrdfSpectralParameter0Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a341a5307f022df0104358887c08c4115)         = 0x62737030,  /* 'bsp0' */ [�9.2.10](https://www.color.org/specification/ICC.2-2019.pdf#page=51) 
    [icSigBrdfSpectralParameter1Tag](https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h.html#ac82636cc27ba918bd737e94a62715af5a3d4231c55723c0b2d71820be0c94c969)         = 0x62737031,  /* 'bsp1' */ [�9.2.11](https://www.color.org/specification/ICC.2-2019.pdf#page=51)
...

Any feedback appreciated, then will modify the Doxygen Config.

petervwyatt commented 4 months ago

@xsscx https://xss.cx/public/docs/DemoIccMAX/d3/d1f/ic_profile_header_8h_source.html is working nicely for me, except for the strange symbol (not valid UTF8??) that is before the mentioned clause number... did you mean it to be § (&sect;)?

And won't you want this inside the C++ comment rather than outside so that it will also work in certain IDEs that auto-detect URLs?

xsscx commented 4 months ago

@petervwyatt thank you, good feedback. I'll check it and fix/update over weekend and will check with some IDE's. If any other thoughts and ideas, please do share. thank you!

xsscx commented 3 months ago

Hello @kateniolet and @petervwyatt:

I had been working on this enum and Documentation item, then started to check the enums, and now need to resolve things like:

undefined-behavior in iccDumpProfile.cpp:228:69: runtime error: load of value 2543294359, which is not a valid value for type 'icPlatformSignature'
undefined-behavior in IccUtil.cpp:1845:11: runtime error: load of value 2543294359, which is not a valid value for type 'icPlatformSignature'
undefined-behavior in IccUtil.cpp:1865:27: runtime error: load of value 2543294359, which is not a valid value for type 'icPlatformSignature'

I will return to Documenting the emuns once I can get these ubsan and other issues resolved. If either of you or anyone else can pickup this Documentation for these enums items that would be great.

petervwyatt commented 3 months ago

Hi @xsscx, not quite understanding what you're asking... do you mean for us to actually edit and commit changes to IccProfLib/icProfileHeader.h that add appropriate URLs (w/ PDF fragment identifiers) to the relevant ICC spec? Is this on main or a branch?

I do have some scripts and data from my SafeDocs work where I did a multi-way comparison between tags-as-ASCII and tags-as-hex in both this C++ code and the ICC.1 and ICC.2 specs to identify things like mismatched hex tags, mismatched ASCII tags, coded but missing documentation, and documented but missing code. But this was against older versions of both ICC docs and would need updating. Another thing that I found useful was to ensure that ASCII tags were always single-quoted in comments (so as to be sure SPACE was correctly captured).

xsscx commented 3 months ago

@petervwyatt

I should have provided more info. Having the enums documented and linked to the Spec would be helpful. I did the mockup of the idea from @kateniolet, but when I checked the enums, I saw the ubsan issues and my attention shifted.

As you suggested, a PR with appropriate URLs (including PDF fragment identifiers) linking to the relevant sections of the ICC spec on the main branch, if you have the time.

With respect to the SafeDocs works, this looks very interesting:

Scope: ICC.1 and ICC.2

Functions: tags-as-ASCII and tags-as-hex

Heuristics: mismatched hex tags mismatched ASCII tags coded but missing documentation documented but missing code

Having the SafeDocs code to review would help educate me and provide ideas for a refactored mashup of iccScan, IccDumpProfile and other ideas to indicate compliance, potential security and other issues.

Please consider sharing your SafeDocs works via PR to aid in the identification and reduction of Bug Classes.

Hopefully the documentation of enums will be worked into a PR, if you have time.

Let me know if I can provide any additional info and/or reach out via email.

Summary

  1. Documentation enhancements by adding the enums and link to the Spec.

  2. Switch Statements Comparing Different Enumerations: We found switch statements comparing values from different enumeration types (icColorSpaceSignature and icSpectralColorSignature). This incompatibility has been causing compiler warnings and UBSan errors.

  3. Case Values Not in Enumerated Type: We found that the switch statements also include case labels with values not defined within the expected enumeration type. This often occurs when using macros or hard-coded values that don't align with the expected enum.

Issues to be addressed in a future PR.