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.

Sign up for GitHub

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

Jump to bottom

heif: add CreateCopy support (WIP) #8907

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

heif: add CreateCopy support (WIP) #8907

wants to merge 15 commits into from

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Dec 4, 2023 •
edited

What does this PR do?

Implements CreateCopy for the heif driver. The implementation uses libheif.

This implementation also includes some cleanups to existing code. That could be split out into another PR if desired.

What are related issues/pull requests?

N/A.

Tasklist

  • Add virtual writer
  • Add support for != 8 bits
  • Add support for YUV
  • Add creation options for libheif encoding options
  • Add test case for AV1 encoding
  • Add test case for JPEG encoding
  • Add test case for JPEG2000 encoding
  • Add test case for uncompressed encoding.
  • remove hack for driver check (3ac750b#diff-4462bd609cd228478ce2ea09d926c0edecf4b06c3033f71540cf0fa86cbd664aR311)
  • Decide on default quality level (50 might be a bit low)
  • Add documentation - multiple creation options need to be added
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

N/A.

@bradh bradh marked this pull request as draft December 4, 2023 09:45
@bradh bradh force-pushed the libheif_createcopy_2023-12-03 branch from 03e6545 to ad07518 Compare December 4, 2023 10:22
rouault
rouault reviewed Dec 4, 2023
View reviewed changes
frmts/heif/heifdrivercore.cpp Outdated
@@ -42,33 +42,33 @@ int HEIFDriverIdentifySimplified(GDALOpenInfo *poOpenInfo)
if (poOpenInfo->nHeaderBytes < 12 || poOpenInfo->fpL == nullptr)
return false;

#if LIBHEIF_HAVE_VERSION(1, 4, 0)
const auto res =
Copy link
Member

Choose a reason for hiding this comment

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

we can't call libheif file in this file, since for deferred loaded plugins (https://gdal.org/development/rfc/rfc96_deferred_plugin_loading.html), it will be linked into libgdal core, without libheif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Had not understood, and will need to review the RFC in detail to fix.

Based on an initial review of the RFC, I understand this is a filter that should return true for all supported files. That is, it is probably better to return a false positive (which will delay startup) than a false negative (which will cause the file not to be loaded), within reason. Clearly it needs to be a light test though.

Copy link
Member

Choose a reason for hiding this comment

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

Based on an initial review of the RFC, I understand this is a filter that should return true for all supported files. That is, it is probably better to return a false positive (which will delay startup) than a false negative (which will cause the file not to be loaded), within reason.

yes.
If there are not-sure situations, and mostly if the file might be opened by another driver, you can also return GDAL_IDENTIFY_UNKNOWN, and the Open() method with be invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented. Additional RFC review to do.

frmts/heif/heifdataset.cpp Show resolved Hide resolved
frmts/heif/heifdatasetcreatecopy.cpp Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

Coverage Status

coverage: 67.998% (+0.001%) from 67.997%
when pulling 3ac750b on bradh:libheif_createcopy_2023-12-03
into fba7ec4 on OSGeo:master.

@bradh bradh force-pushed the libheif_createcopy_2023-12-03 branch from 3ac750b to a65fd32 Compare December 5, 2023 06:39
@github-actions GitHub Actions
Copy link

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@github-actions github-actions bot added the stale label May 16, 2024
@bradh
Copy link
Contributor Author

bradh commented May 16, 2024

If there are feedback items on what is currently implemented, that would be appreciated.

@stale stale bot removed the stale label May 16, 2024
rouault
rouault reviewed May 16, 2024
View reviewed changes
Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

a few comments, but looks good. Would need to be rebased on top of latest master to get fresh CI. Do you feel like this PR could be merged as it (with potential future PRs), or is there some extra work required?

autotest/gdrivers/heif.py Outdated Show resolved Hide resolved
frmts/heif/heifdatasetcreatecopy.cpp Outdated Show resolved Hide resolved
@bradh
Copy link
Contributor Author

bradh commented May 17, 2024

a few comments, but looks good. Would need to be rebased on top of latest master to get fresh CI.

Thanks.

Do you feel like this PR could be merged as it (with potential future PRs), or is there some extra work required?

I would not want to merge it without the test cases and documentation, or with the driver check hack. The other parts could probably wait for future PRs.

bradh and others added 14 commits May 18, 2024 17:02
@bradh
heif: implement CreateCopy (WIP)
b2bc640
@bradh
heif: only use compression formats that are available in libheif release
827667e
@bradh
heif: fix errors in deferred driver loading
2637b09
@bradh
heif: PR review comment fixes, additional tests
410f0bc
@bradh
heif: back out uncompressed codec
7ab41d6
@bradh
heif: additional test work
d0d2bd0
@bradh
heif: implement VFS writer
4208c1a
@bradh
heif: mark libheif context as unused
10fe629
@bradh
heif: avoid use of new heif_error_success constant
f19beee
@bradh
heif: add protection against double inclusion of dataset header file
c610c03
@bradh
heif: initial creation documentation
0fc6383
@bradh @rouault
Update autotest/gdrivers/heif.py
6d2d8b2 
Co-authored-by: Even Rouault <[email protected]>
@bradh @rouault
Update frmts/heif/heifdatasetcreatecopy.cpp
c1bf35a 
Co-authored-by: Even Rouault <[email protected]>
@bradh
heif: correct autotest temporary path
2545967
@bradh bradh force-pushed the libheif_createcopy_2023-12-03 branch from fa17bec to 2545967 Compare May 18, 2024 09:23
@bradh bradh force-pushed the libheif_createcopy_2023-12-03 branch from 2adfc1f to d1b3cd5 Compare May 18, 2024 09:53
rouault
rouault reviewed May 18, 2024
View reviewed changes
doc/source/drivers/raster/heif.rst
@@ -31,6 +34,26 @@ Driver capabilities

.. supports_virtualio:: if libheif >= 1.4

Creation support
Copy link
Member

Choose a reason for hiding this comment

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

to fix "AssertionError: Driver HEIF declares DCAP_CREATECOPY but doc does not!" in https://github.com/OSGeo/gdal/actions/runs/9139007180/job/25130835649?pr=8907:

Suggested change
Creation support
.. supports_createcopy::
Creation support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Already fixed in a separate work branch. Still not sure what is going on with the bus error on the Alpine builds....

Copy link
Member

Choose a reason for hiding this comment

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

on the Alpine builds....

perhaps a issue specific with the libheif version they ship

A way to reproduce locally is to use the Docker alpine:edge image

docker run --name gdal_alpine -it -v /path/to/your/GDAL/checkout:/gdal alpine:edge

execute the apk add line at https://github.com/OSGeo/gdal/blob/master/.github/workflows/alpine/Dockerfile.ci#L3
cd /gdal
python3 -m pip install --break-system-packages -U -r autotest/requirements.txt
mkdir build_alpine
cd build_alpine
do the build as in https://github.com/OSGeo/gdal/blob/master/.github/workflows/alpine/build.sh#L16 (drop the line "-DADD_EXTERNAL_DEFERRED_PLUGIN_FOO=/tmp/foo.cpp" to simplify things)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouault rouault rouault left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bradh @coveralls @rouault