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

avifRWDataRealloc,avifRWDataSet return avifResult #1478

Merged
merged 8 commits into from
Jul 31, 2023

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Jul 26, 2023

Also avifImageSetProfileICC(), avifImageSetMetadataExif(), avifImageSetMetadataXMP() and avifCodecEncodeOutputAddSample(). To catch out-of-memory issues.
Bug: #820

Suggested follow-ups:

  • Propagate AVIF_RESULT_OUT_OF_MEMORY to all sites calling a function listed above (not done in this change to keep it at a reasonable size)
  • Remove abort() in makeRoom()

Also avifImageSetProfileICC(), avifImageSetMetadataExif(),
avifImageSetMetadataXMP() and avifCodecEncodeOutputAddSample().
To catch out-of-memory issues.
Bug: AOMediaCodec#820
@y-guyon y-guyon requested a review from wantehchang July 26, 2023 14:25
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.

Yannis: Thank you very much for writing this pull request. This is something we need to complete before declaring the API as stable (i.e., version 1.0.0).

I have some suggested changes. Please read my comments in rawdata.c first (about the alternative in avifRWDataRealloc()).

apps/shared/iccmaker.c Outdated Show resolved Hide resolved
apps/shared/iccmaker.c Outdated Show resolved Hide resolved
include/avif/avif.h Show resolved Hide resolved
include/avif/avif.h Show resolved Hide resolved
src/exif.c Show resolved Hide resolved
src/rawdata.c Outdated Show resolved Hide resolved
src/read.c Show resolved Hide resolved
src/stream.c Outdated Show resolved Hide resolved
src/write.c Outdated Show resolved Hide resolved
tests/aviftest.c Outdated Show resolved Hide resolved
Free resources in aviftest.c.
Change TODO syntax to match style guide.
Also avifImageSetMetadataExif(), avifImageSetMetadataXMP() and
avifCodecEncodeOutputAddSample().
@y-guyon y-guyon requested a review from wantehchang July 27, 2023 12:17
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.

LGTM. You can merge the pull request after addressing the review comments. I can do a post-commit review. Thanks again for writing this pull request.

include/avif/avif.h Show resolved Hide resolved
src/codec_aom.c Show resolved Hide resolved
src/codec_avm.c Show resolved Hide resolved
src/codec_svt.c Outdated Show resolved Hide resolved
src/exif.c Show resolved Hide resolved
src/rawdata.c Show resolved Hide resolved
src/write.c Outdated Show resolved Hide resolved
src/mem.c Outdated Show resolved Hide resolved
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.

Yannis: I verified that this pull request changes the prototypes of all the remaining public functions that may fail because of memory allocation failures. This is an important step towards a stable v1.0.0 API. Thanks a lot!

@y-guyon
Copy link
Collaborator Author

y-guyon commented Jul 28, 2023

I verified that this pull request changes the prototypes of all the remaining public functions that may fail because of memory allocation failures.

Thanks!

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.

LGTM.

src/exif.c Show resolved Hide resolved
@y-guyon y-guyon merged commit 397f74c into AOMediaCodec:main Jul 31, 2023
14 checks passed
@y-guyon y-guyon deleted the status_rwdata_realloc branch August 7, 2023 09:47
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