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 an ImageInput::valid_file(IOProxy) overload #3826

Merged

Conversation

jessey-git
Copy link
Contributor

Description

Every format that has an existing valid_file(std::string) implementation, and which supports an
ioproxy, now also has a valid_file(IOProxy) implementation for completeness.

The existing std::string overloads have been implemented in terms of the IOProxy overload where
possible to cut down on the boilerplate. This is done at the base of the hierarchy in ImageInput.

The remaining formats that support ioproxy, but do not currently have a valid_file method, will be
submitted in a follow-up PR as the additional logic required for those will require closer review.

Tests

I haven't had much time/luck to sort out running the tests locally on Windows, or setting up a parallel WSL
environment, so I admit this is coming in with no test coverage beyond it working locally for me in a
test app. The app invokes both valid_file methods on a directory filled with various files for the formats in question.

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@jessey-git jessey-git changed the title Valid file proxy Add an ImageInput::valid_file(IOProxy) overload May 3, 2023
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

I like this, thanks!

The only comment I had was very minor, just an organization/documentation tweak about the header.

Check the clang-format test in the CI run to see what's failing.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM

@lgritz
Copy link
Collaborator

lgritz commented May 3, 2023

The CI failure for icc doesn't appear to be related to your changes, it's (temporarily?) unable to download package containing the icc compiler. So I'm going to merge.

@lgritz lgritz merged commit a1d8dbc into AcademySoftwareFoundation:master May 3, 2023
lgritz pushed a commit that referenced this pull request May 17, 2023
…D, and WEBP (#3831)

Following up #3826, this implements efficient `valid_file` support for
DDS, DPX, PSD, HDR, and WEBP.

The quick magic/signature checks were abstracted into small helper
functions where necessary to eliminate duplicate code.

For WEBP, some redundant checks were also removed and there's also a fix
for a `uint64_t` vs `size_t` check which would have caused issues on
32-bit builds.

The PSD changes exposed an issue with array attributes. During a normal
ImageInput create+open sequence, an ImageInput may be opened then closed
and then opened again.

Array attributes are not cleared as part of close and so their contents
will be doubled up next time they are read into the existing `m_spec`.
This PR does not fix that general problem but now that PSD has a proper
valid_file which doesn't do an open, the array attributes are now
correct:

```
This PR
2023-05-06T02:32:05.8774584Z -    photoshop:ColorMode: 3
2023-05-06T02:32:05.8774888Z -    photoshop:ICCProfile: "sRGB IEC61966-2.1"
2023-05-06T02:32:05.8775194Z -    photoshop:LayerName: 1, 2, 3
2023-05-06T02:32:05.8775436Z -    photoshop:LayerText: 1, 2, 3

vs. the current
2023-05-06T02:32:05.8775725Z +    photoshop:ColorMode: 3, 3
2023-05-06T02:32:05.8776023Z +    photoshop:ICCProfile: "sRGB IEC61966-2.1"
2023-05-06T02:32:05.8776494Z +    photoshop:LayerName: 1, 2, 3, 1, 2, 3
2023-05-06T02:32:05.8776746Z +    photoshop:LayerText: 1, 2, 3, 1, 2, 3
```
birsoyo pushed a commit to birsoyo/oiio that referenced this pull request Jun 8, 2023
…D, and WEBP (AcademySoftwareFoundation#3831)

Following up AcademySoftwareFoundation#3826, this implements efficient `valid_file` support for
DDS, DPX, PSD, HDR, and WEBP.

The quick magic/signature checks were abstracted into small helper
functions where necessary to eliminate duplicate code.

For WEBP, some redundant checks were also removed and there's also a fix
for a `uint64_t` vs `size_t` check which would have caused issues on
32-bit builds.

The PSD changes exposed an issue with array attributes. During a normal
ImageInput create+open sequence, an ImageInput may be opened then closed
and then opened again.

Array attributes are not cleared as part of close and so their contents
will be doubled up next time they are read into the existing `m_spec`.
This PR does not fix that general problem but now that PSD has a proper
valid_file which doesn't do an open, the array attributes are now
correct:

```
This PR
2023-05-06T02:32:05.8774584Z -    photoshop:ColorMode: 3
2023-05-06T02:32:05.8774888Z -    photoshop:ICCProfile: "sRGB IEC61966-2.1"
2023-05-06T02:32:05.8775194Z -    photoshop:LayerName: 1, 2, 3
2023-05-06T02:32:05.8775436Z -    photoshop:LayerText: 1, 2, 3

vs. the current
2023-05-06T02:32:05.8775725Z +    photoshop:ColorMode: 3, 3
2023-05-06T02:32:05.8776023Z +    photoshop:ICCProfile: "sRGB IEC61966-2.1"
2023-05-06T02:32:05.8776494Z +    photoshop:LayerName: 1, 2, 3, 1, 2, 3
2023-05-06T02:32:05.8776746Z +    photoshop:LayerText: 1, 2, 3, 1, 2, 3
```
@jessey-git jessey-git deleted the valid_file_proxy branch August 25, 2023 05:24
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