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

Convert color conversion internals to matrices stored as 1D array #1932

Closed
wants to merge 2 commits into from

Conversation

vrabaud
Copy link
Collaborator

@vrabaud vrabaud commented Jan 11, 2024

This is to prevent passing M[3][3] and make sure they are const, cf https://c-faq.com/ansi/constmismatch.html

@vrabaud vrabaud mentioned this pull request Jan 11, 2024
coeffs[1][2] = coeffsTmp[5];
coeffs[2][0] = coeffsTmp[6];
coeffs[2][1] = coeffsTmp[7];
coeffs[2][2] = coeffsTmp[8];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This kind of conversion is not ideal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

include/avif/internal.h Show resolved Hide resolved
@wantehchang
Copy link
Collaborator

Thank you for writing this pull request. The new code is not as readable as the original code, so I think we should ask in the C/C++ discussion group at Google about this problem. The two dimensional array is not exactly the same as https://c-faq.com/ansi/constmismatch.html. Hopefully there is a better solution.

I am also wondering why none of the compilers we use regularly warn about this. Is it because we do not enable a certain warning option?

@wantehchang
Copy link
Collaborator

{{
0.4360747, 0.3850649, 0.1430804, // row 0
0.2225045, 0.7168786, 0.0606169, // row 1
0.0139322, 0.0971045, 0.7141733 // row 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the original still works, the original code seems to be a better initializer for std::array<std::array<double, 3>, 3> (see line 19).

This comment also applies to the second arguments for the ExpectMatrixNear() calls below.

@vrabaud
Copy link
Collaborator Author

vrabaud commented Jan 15, 2024

I tried to play with clang options like '-Wincompatible-pointer-types-discards-qualifiers' to no avail. Ditto for gcc.
The simplest solution is to remove the const from the functions. I just thought we might want to preserve it. It's all internal so we might as well remove the const and preserve the 2d semantics.

@wantehchang
Copy link
Collaborator

Vincent: Removing the const from the function prorotypes is fine by me.

My suggestion was to wrap the two-dimensional array in a struct, e.g.,

typedef struct {
    double x[3][3];
} avifMat;

But this requires more changes to the code, because coeffs[i][j] will become coeffs.d[i][j].

@vrabaud
Copy link
Collaborator Author

vrabaud commented Jan 17, 2024

Right, but the problem would be the same: a const avifMat offers no const guarantee on the data in x.

@vrabaud
Copy link
Collaborator Author

vrabaud commented Jan 17, 2024

Closing in favor of #1946

@vrabaud vrabaud closed this Jan 17, 2024
@wantehchang
Copy link
Collaborator

Right, but the problem would be the same: a const avifMat offers no const guarantee on the data in x.

Are you sure? I guess you may be thinking of a struct like avifRWData:

typedef struct avifRWData
{
    uint8_t * data;
    size_t size;
} avifRWData;

then the following is allowed:

    uint8_t data[16];
    const avifRWData rw = { data, sizeof(data) };
    rw.data[0] = 0;

On the other hand, if we have a struct like the following:

typedef struct FixedSizedData16
{
    uint8_t data[16];
} FixedSizedData16;

then the following is not allowed:

    const FixedSizedData16 fixed;
    fixed.data[0] = 0; 

@vrabaud
Copy link
Collaborator Author

vrabaud commented Jan 17, 2024

I thought the same issue would pop with 2d but it indeed works.

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.

3 participants