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

v1.0.0 #1513

Merged
merged 4 commits into from
Aug 28, 2023
Merged

v1.0.0 #1513

merged 4 commits into from
Aug 28, 2023

Conversation

wantehchang
Copy link
Collaborator

No description provided.

@wantehchang
Copy link
Collaborator Author

CHANGELOG.md Outdated
@@ -6,6 +6,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [1.0.0] - 2023-08-23
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wrote this pull request by following the instructions at https://github.com/AOMediaCodec/libavif/wiki/Release-Checklist.

I reviewed the commits in the range v0.11.1...HEAD. I may have missed a small number that should be mentioned in the changelog.

We probably should also call out 1.0.0 implies the API will be more stable from now on.

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should also call out 1.0.0 implies the API will be more stable from now on.

Do we also want to provide a stable 1.0.0 ABI? (I believe for C, library users expect it) If so, we should also reserve some padding to the structs annotated with "Version 1.0.0 ends here" now, so that adding fields later won't break the ABI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we also want to provide a stable 1.0.0 ABI. The three structs annotated with "Version 1.0.0 ends here" must be allocated by calling their Create() function, so we will be able to extend those three structs without breaking the ABI. I will add comments before those three structs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I forgot that these are not allocated by user. But then avifRGBImage would be in trouble, since this struct is allocated by user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yuan: That's correct. Vignesh, Yannis, and I reviewed the current members of the avifRGBImage struct and concluded they are adequate. So we didn't add any reserved fields (padding) to avifRGBImage. Since you are also familiar with this struct, could you take a look? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I take a look at the history of avifRGBImage, and there are some fields that were added due to external move or need: alphaPremultiplied is for premultiplied alpha, isFloat is for half float support, and maxThreads is for threaded conversion.

I feel that if we were at 2020, it's hard to predict that we will end up having these now, so I'd prefer adding a few padding, just in case new need arises in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yuan: Thank you for taking a look. Note that some of the potential new features of RGB-YUV conversion, such as honoring the chroma sample position, can be supported by using the color space information in the avifImage struct. Also, the avifRGBImage struct actually has several bytes of unneeded space:

typedef struct avifRGBImage
{
    uint32_t width;
    uint32_t height;
    uint32_t depth;
    avifRGBFormat format;
    avifChromaUpsampling chromaUpsampling;
    avifChromaDownsampling chromaDownsampling;
    avifBool avoidLibYUV;
    avifBool ignoreAlpha;
    avifBool alphaPremultiplied;
    avifBool isFloat;
    int maxThreads;
    uint8_t * pixels;
    uint32_t rowBytes;
} avifRGBImage;

For example, it uses an int (avifBool) to represent a boolean. So if we require the struct be initialized by avifRGBImageSetDefaults() or memset() before use, then we have the option of repurposing some of the bytes or bits for a new member. I wrote a pull request to document this initialization requirement:
#1520

Copy link
Contributor

Choose a reason for hiding this comment

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

we have the option of repurposing some of the bytes or bits for a new member

This is still somethat a breaking API change. We definitely won't change typedef int avifBool, so we have to change the type of some field from avifBool to a smaller integer type. Say some user faithfully assigning that field with a variable of avifBool type, and the compiler failed to constant folding that variable therefore doesn't know that it can only be 0 or 1, this will cause a C4244 for MSVC or -Wconversion for GCC/Clang. If the user is also doing the same -Werror thing like libavif, then the previously working code is now broken.

@wantehchang wantehchang requested review from vigneshvg and removed request for joedrago August 24, 2023 22:20
Copy link
Collaborator

@vigneshvg vigneshvg left a comment

Choose a reason for hiding this comment

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

lgtm.

i will also create an android release an upload it to maven central once this PR is merged.

@wantehchang wantehchang merged commit bc41fc5 into AOMediaCodec:main Aug 28, 2023
14 checks passed
@wantehchang wantehchang deleted the release-1-0-0 branch August 28, 2023 22:03
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.

None yet

4 participants