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

Document the policy on internal.h vs. avif.h #1589

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wantehchang
Copy link
Collaborator

No description provided.

@vrabaud
Copy link
Collaborator

vrabaud commented Sep 14, 2023

Unrelated but not so much: is there any will to split avif.h (e.g. in encode/decode/extras) to make it easier to read and also to enable the distribution of a decode-only libavif version (which would reduce sizes (useful for TV boxes) and risks (by limiting the available API)) ?

@vigneshvg
Copy link
Collaborator

Unrelated but not so much: is there any will to split avif.h (e.g. in encode/decode/extras) to make it easier to read and also to enable the ditribution of a decode-only libavif version (which would reduce sizes (useful for TV boxes) and risks (by limiting the available API)) ?

libavif in itself should be fairly lightweight. TV boxes that only require the decoder are better off building libavif without the underlying AV1 encoder libraries, which is where the bulk of the binary size comes from.

Ideally, libavif can be split too into encoder and decoder components but the savings isn't gonna be much.

@wantehchang
Copy link
Collaborator Author

Vincent,

The second thing you asked about, the distribution of a decode-only libavif version, is already done in the alternative build systems for libavif in Google's internal repository and in Chrome. It is true that the decode-only libavif version still uses the regular avif.h, which contains declarations of unimplemented functions.

The first thing you asked about, splitting avif.h to make it easier to read, also came up in my review of #1565. This is why I may not merge this pull request.

Note: This pull request documents my understanding of the original two headers of the libavif library, and Joe Drago's review confirms I understand it correctly.

@vrabaud
Copy link
Collaborator

vrabaud commented Sep 20, 2023

This PR is good as is as it makes explicit the current structure/convention.

Here is my opinion though and please feel free to agree to disagree :)

  • headers that are bigger than 1K lines are hard to read/seek through. Splitting avif.h in encoder/decoder could be a way to reduce that. We could also envision splitting types and or animations
  • splitting encoder/decoder will most likely bring little size gain indeed @vigneshvg because libavif is so small but it could help us clean the code/CMake into what is for encoding/decoding/common to both. Cleaner means better tested, faster to compile, easier to re-use without bringing in random dependencies.
  • having both private and public headers in ./include is confusing to the user. They have to read them to understand if they can use them or if they will be installed (ehnce this PR).
  • having a flat hierarchy in ./src forces us to limit the number of files and therefore to bloat them (5K lines for read.c ...). We could envision the following folders: codecs, encode, decode, common (to both enc and dec), utils (utils being totally independent from AVIF e.g. gcd)
  • we will lose git history for sections in big files but we will gain clean history for the future small files containing the moved sections (and not polluted by the rest of the big files). And there actually is git-cp. I have not had to check git history in the past year on read.c; on the other hand, I have been slowed down by its size going from 4.2K to 5.3K lines.

The codebase will continue to grow because of new codecs/features. I believe it is just a matter of time before we need to re-organize files.
Again, there is no convention (or actually there are many), so feel free to agree to disagree :)

This PR LGTM.

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.

4 participants