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

Turn gain map flag on by default. #2513

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

maryla-uc
Copy link
Collaborator

Remove 'EXPERIMENTAL' from its name.

Remove 'EXPERIMENTAL' from its name.
@maryla-uc maryla-uc requested a review from wantehchang December 2, 2024 11:44
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request.

Can we remove the AVIF_ENABLE_GAIN_MAP option?

Remove EXPERIMENTAL from AVIF_ENABLE_EXPERIMENTAL_JPEG_GAIN_MAP_CONVERSION

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

I read the whole pull request. Other than whether AVIF_ENABLE_GAIN_MAP should be kept or removed, it looks good. Here are some more comments.

.github/workflows/ci-fuzztest.yml Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
tests/data/README.md Outdated Show resolved Hide resolved
@@ -676,7 +677,7 @@ if(AVIF_BUILD_APPS OR (AVIF_BUILD_TESTS AND (AVIF_ENABLE_FUZZTEST OR AVIF_ENABLE
find_package(JPEG REQUIRED)
endif()

if(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP)
if(AVIF_ENABLE_GAIN_MAP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove EXPERIMENTAL from AVIF_ENABLE_EXPERIMENTAL_JPEG_GAIN_MAP_CONVERSION?

Can do this in a follow-up CL.

@maryla-uc
Copy link
Collaborator Author

I kept the AVIF_ENABLE_GAIN_MAP option for now just in case we needed to revert it to OFF for some reason, and to keep this PR simple (even though the rename means that there are still quite a few changes).
I'm happy to remove it though. It can be done in another PR.

@maryla-uc
Copy link
Collaborator Author

As for AVIF_ENABLE_EXPERIMENTAL_JPEG_GAIN_MAP_CONVERSION, since it's still off by default, I kept the "experimental" name for now. Again, can be changed later.

@maryla-uc maryla-uc merged commit 3196438 into AOMediaCodec:main Dec 3, 2024
34 checks passed
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