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

Reuse matching ispe box if one exists. #1175

Merged
merged 4 commits into from
Jun 10, 2024
Merged

Conversation

dukesook
Copy link
Contributor

@dukesook dukesook commented May 21, 2024

Reusing identical properties will help clean up the ipco box. It becomes cumbersome when there are many images like this:
image

@bradh
Copy link
Contributor

bradh commented May 21, 2024

I like the concept, but it seems like we should do it for more than just ispe. For example, adding pixi would also make it simpler, and hvcC would likely save significant space.

I haven't got all the way through the process, but maybe we can always make the box, and do an operator == implementation on the box class to check if its the same. The complexity would be to check equivalence, but having it in the box implementation would encapsulate it for changes (e.g. if a new version got added, we wouldn't forget to update the check over in file.cc.

If that worked out, we could change:

int index = m_ipco_box->append_child_box(ispe_clap_pixi_whatever);

to be something like:

int index = m_ipco_box->find_or_append_child_box(ispe_clap_pixi_whatever);

Thoughts?

@farindk
Copy link
Contributor

farindk commented May 22, 2024

This absolutely makes sense. I also think that there should be an operator== to do this check for all box types. Maybe the equivalence could be tested directly on the binary data. Then we have it automatically for every box and there is no need to maintain the operator== for each class.

@farindk
Copy link
Contributor

farindk commented May 22, 2024

@dukesook Looks good. Maybe you could test short_type for equivalence before writing out both boxes to increase efficiency.

@farindk
Copy link
Contributor

farindk commented May 22, 2024

Thank you. That looks good. I'll have a closer look and merge this in the coming days.

@dukesook
Copy link
Contributor Author

I updated the PR with the provided feedback.

The Box class now:

  1. Overrides the == operator
  2. Provides an equal() function to work with shared_ptrs
  3. Provides a generic find_or_append_child_box() function to prevent redundant boxes. (This works great with pixi & ispe, but the hvcC box is added first and later populated with values)

@bradh
Copy link
Contributor

bradh commented May 26, 2024

LGTM. I think it would be worth unit testing, so sent a PR for that.

@farindk farindk merged commit 1e75661 into strukturag:master Jun 10, 2024
35 checks passed
@farindk
Copy link
Contributor

farindk commented Jun 10, 2024

Thank you. This is great to have.

@farindk
Copy link
Contributor

farindk commented Jun 10, 2024

I've made the operator== virtual so that we can still add more efficient equality comparisons.

farindk added a commit that referenced this pull request Jun 10, 2024
farindk added a commit that referenced this pull request Jun 10, 2024
farindk added a commit that referenced this pull request Jun 10, 2024
farindk added a commit that referenced this pull request Jun 10, 2024
@dukesook dukesook deleted the unique_ispe branch June 13, 2024 12:37
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