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

New pull request about to integrate HEIF support (read only) using system based libheif #1233

Closed
wants to merge 23 commits into from

Conversation

cgilles
Copy link
Collaborator

@cgilles cgilles commented Jun 9, 2020

Hi

This PR is a new revision of my previous one, but it use now the libheif dependency from the host system.

the option to support HEIF in read only is disable by default in cmake. There is an option to enable it.

Patch README accordingly.

Important : with the last libheif release, new format are support, as HEIX and ... AVIF (using libaom in background). So this is full transparent from libexiv2, as all is done in background by libheif...

https://github.com/strukturag/libheif/releases

Best Regards
Gilles Caulier

@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #1233 into master will increase coverage by 0.66%.
The diff coverage is 56.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
+ Coverage   71.24%   71.91%   +0.66%     
==========================================
  Files         148      152       +4     
  Lines       19457    17674    -1783     
==========================================
- Hits        13862    12710    -1152     
+ Misses       5595     4964     -631     
Impacted Files Coverage Δ
include/exiv2/error.hpp 90.90% <ø> (ø)
include/exiv2/types.hpp 96.00% <ø> (-0.56%) ⬇️
samples/exiv2json.cpp 81.88% <ø> (+2.26%) ⬆️
samples/geotag.cpp 82.44% <ø> (+1.45%) ⬆️
src/RemoteIo.cpp 0.00% <0.00%> (ø)
src/actions.cpp 74.50% <0.00%> (-0.30%) ⬇️
src/canonmn_int.cpp 88.69% <ø> (+0.41%) ⬆️
src/error.cpp 87.87% <ø> (-2.37%) ⬇️
src/fujimn_int.cpp 100.00% <ø> (ø)
src/image.cpp 83.24% <ø> (-1.04%) ⬇️
... and 110 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e863d2...3a12d48. Read the comment docs.

@clanmills
Copy link
Collaborator

Thank You for your hard work on this, @cgilles

I won't be accepting this on the 0.27-maintenance branch at this time as it's in Code Freeze for v0.27.3 which is scheduled for release on 2020-06-30.

I see you are requesting that information about this be added to README.md and I'll take care of this at the time of releasing 0.28. Can you open a feature request, assign it to me and milestone 0.28.

This PR takes us into the hotly disputed subject of reading ISOBMFF files. I have set up a Team Exiv2 Meeting to discuss this.

Topic: Exiv2 and ISOBMFF Support
Time: Jun 14, 2020 13:00 London

Join Zoom Meeting
https://us02web.zoom.us/j/82136730279?pwd=M3hCbll4cWN3ellJd2pCZkxjVEx3Zz09

Meeting ID: 821 3673 0279
Password: 1fDNUV

@clanmills
Copy link
Collaborator

I'm unable to apply the patch to master:

842 rmills@rmillsmbp:~/gnu/github/exiv2/master $ curl https://patch-diff.githubusercontent.com/raw/Exiv2/exiv2/pull/1233.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 3957k    0 3957k    0     0  2181k      0 --:--:--  0:00:01 --:--:-- 2180k
<stdin>:56402: trailing whitespace.
    
<stdin>:119542: trailing whitespace.
    
error: patch failed: src/properties.cpp:196
error: src/properties.cpp: patch does not apply
<stdin>:44484: new blank line at EOF.
+
<stdin>:44689: new blank line at EOF.
+
<stdin>:62468: new blank line at EOF.
+
error: patch failed: include/exiv2/error.hpp:253
error: include/exiv2/error.hpp: patch does not apply
error: patch failed: include/exiv2/image_types.hpp:34
error: include/exiv2/image_types.hpp: patch does not apply
error: patch failed: po/nl.po:3
error: po/nl.po: patch does not apply
error: patch failed: src/CMakeLists.txt:10
error: src/CMakeLists.txt: patch does not apply
error: patch failed: src/error.cpp:165
error: src/error.cpp: patch does not apply
error: patch failed: src/image.cpp:35
error: src/image.cpp: patch does not apply
error: patch failed: CMakeLists.txt:15
error: CMakeLists.txt: patch does not apply
error: patch failed: cmake/config.h.cmake:53
error: cmake/config.h.cmake: patch does not apply
error: patch failed: cmake/printSummary.cmake:44
error: cmake/printSummary.cmake: patch does not apply
error: patch failed: src/CMakeLists.txt:10
error: src/CMakeLists.txt: patch does not apply
error: patch failed: src/image.cpp:35
error: src/image.cpp: patch does not apply
843 rmills@rmillsmbp:~/gnu/github/exiv2/master $ 

There doesn't appear to be anything added to the test suite for this.

844 rmills@rmillsmbp:~/gnu/github/exiv2/master $ curl https://patch-diff.githubusercontent.com/raw/Exiv2/exiv2/pull/1233.patch 2>/dev/null | grep '^--- a/'
--- a/src/properties.cpp
--- a/include/exiv2/error.hpp
--- a/include/exiv2/image_types.hpp
--- a/po/nl.po
--- a/src/CMakeLists.txt
--- a/src/error.cpp
--- a/src/image.cpp
--- a/CMakeLists.txt
--- a/cmake/config.h.cmake
--- a/cmake/printSummary.cmake
--- a/src/CMakeLists.txt
--- a/src/image.cpp
--- a/src/libheif/LibHeifRules.cmake
--- a/src/heifimage.cpp
--- a/src/heifimage.cpp
--- a/src/heifimage.cpp
--- a/src/heifimage_int.hpp
--- a/src/libheif/LibHeifRules.cmake
--- a/src/libheif/README
--- a/src/libheif/box_fuzzer.cc
--- a/src/libheif/encoder_fuzzer.cc
--- a/src/libheif/file_fuzzer.cc
--- a/src/libheif/README
--- a/src/libheif/heif_emscripten.h
--- a/include/exiv2/error.hpp
--- a/include/exiv2/image_types.hpp
--- a/po/nl.po
--- a/src/CMakeLists.txt
--- a/src/error.cpp
--- a/src/image.cpp
--- a/CMakeLists.txt
--- a/cmake/config.h.cmake
--- a/cmake/printSummary.cmake
--- a/src/CMakeLists.txt
--- a/src/image.cpp
--- a/src/libheif/LibHeifRules.cmake
--- a/src/heifimage.cpp
--- a/src/heifimage.cpp
--- a/src/heifimage.cpp
--- a/src/heifimage_int.hpp
--- a/src/libheif/LibHeifRules.cmake
--- a/src/libheif/README
--- a/src/libheif/box_fuzzer.cc
--- a/src/libheif/encoder_fuzzer.cc
--- a/src/libheif/file_fuzzer.cc
--- a/src/libheif/README
--- a/src/libheif/heif_emscripten.h
--- a/README.md
--- a/src/CMakeLists.txt
--- a/src/libheif/LibHeifRules.cmake
--- a/src/libheif/README
--- a/src/libheif/bitstream.cc
--- a/src/libheif/bitstream.h
--- a/src/libheif/box.cc
--- a/src/libheif/box.h
--- a/src/libheif/error.cc
--- a/src/libheif/error.h
--- a/src/libheif/heif.cc
--- a/src/libheif/heif.h
--- a/src/libheif/heif_api_structs.h
--- a/src/libheif/heif_colorconversion.cc
--- a/src/libheif/heif_colorconversion.h
--- a/src/libheif/heif_context.cc
--- a/src/libheif/heif_context.h
--- a/src/libheif/heif_cxx.h
--- a/src/libheif/heif_file.cc
--- a/src/libheif/heif_file.h
--- a/src/libheif/heif_hevc.cc
--- a/src/libheif/heif_hevc.h
--- a/src/libheif/heif_image.cc
--- a/src/libheif/heif_image.h
--- a/src/libheif/heif_limits.h
--- a/src/libheif/heif_plugin.cc
--- a/src/libheif/heif_plugin.h
--- a/src/libheif/heif_plugin_registry.cc
--- a/src/libheif/heif_plugin_registry.h
--- a/src/libheif/heif_version.h.in
--- a/src/libheif/logging.h
845 rmills@rmillsmbp:~/gnu/github/exiv2/master 

@D4N
Copy link
Member

D4N commented Jun 9, 2020

@cgilles Please squash the commits to remove all the intermediate steps and address the comments that were left for #1044 as these still apply.

@clanmills
Copy link
Collaborator

I'm not OK about this. This is a huge change. There's no test code. I've called a meeting on Sunday to discuss ISOBMFF.

@cgilles
Copy link
Collaborator Author

cgilles commented Jun 9, 2020

Sorry i post on wrong github file. Look my comment here 👍
#1229 (comment)

@clanmills
Copy link
Collaborator

Gilles: This is a mess. Please start again with a clean and simple PR wich includes updates to the test suite.

@clanmills clanmills closed this Jun 9, 2020
@cgilles
Copy link
Collaborator Author

cgilles commented Jun 10, 2020

A "mess". You are serious ????

[gilles@localhost exiv2]$ pwd
/mnt/devel/GIT/exiv2
[gilles@localhost exiv2]$ git branch

  • master
    [gilles@localhost exiv2]$ git log -1
    commit 1ea63cc (HEAD -> master, origin/master, origin/HEAD)
    Author: Gilles Caulier caulier.gilles@gmail.com
    Date: Sun Jun 7 10:37:13 2020 +0200

    fix broken compilation under ARM. tested with my Raspberry PIV and Raspbian 64bits.
    [gilles@localhost exiv2]$
    [gilles@localhost exiv2]$ git apply --stat --check heifv2.patch.txt
    README.md | 2
    cmake/config.h.cmake | 3
    cmake/printSummary.cmake | 1
    include/exiv2/error.hpp | 1
    include/exiv2/image_types.hpp | 1
    src/CMakeLists.txt | 15 ++
    src/error.cpp | 4
    src/heifimage.cpp | 390 +++++++++++++++++++++++++++++++++++++++++
    src/heifimage_int.hpp | 94 ++++++++++
    src/image.cpp | 6 +
    10 files changed, 516 insertions(+), 1 deletion(-)
    [gilles@localhost exiv2]$ git apply heifv2.patch.txt
    heifv2.patch.txt:521: new blank line at EOF.

warning: 1 line adds whitespace errors.
[gilles@localhost exiv2]$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
(use "git add ..." to update what will be committed)
(use "git checkout -- ..." to discard changes in working directory)

    modified:   README.md
    modified:   cmake/config.h.cmake
    modified:   cmake/printSummary.cmake
    modified:   include/exiv2/error.hpp
    modified:   include/exiv2/image_types.hpp
    modified:   src/CMakeLists.txt
    modified:   src/error.cpp
    modified:   src/image.cpp

Why not take time to read my work. Why you is faster to close all my file open on github ? what's the problem. Do you think that i'm a beginner ???

Seriouly i'm become really a waste of time, and my time is precious. If you want to see more contributors in the future, please take a care...

Best regards

Gilles Caulier

@clanmills
Copy link
Collaborator

Yes. I am serious. The patch couldn't be applied. There were about 50 new files and changes to about 30 existing files. And all of this to provide support for something about which the team is unhappy. And it's only readonly support. And there's not test coverage for the feature. Is this your idea of good work?

@tbeu
Copy link
Member

tbeu commented Aug 5, 2020

@cgilles FYI, I successfully built exiv2 on Win from your branch cgilles:heif using libheif v1.7.0.

@cgilles
Copy link
Collaborator Author

cgilles commented Aug 7, 2020

To @tbeu : sure it compile fine under Linux, MacOs and Windows, as i use this kind of patch with digiKam everywhere now.
But if you read this comment :

#1186 (comment)

... You will seen that i'm "WORST OFFENDER" to Exiv2 project and especially for Mr Mills famous project management !
You must know that i contribute to Exiv2 well before than Mr Mill manage this project, and i C++ engineer since 35 year old now, and i have no lesson to hear from him...

I close this parenthesis now, as i have no time to lost with Mr Mills...

@avibrazil
Copy link

Folks, this isn't yet applied?
I'm desperately waiting for HEIF support in Exiv2.

@clanmills
Copy link
Collaborator

#1229 (comment)

@tbeu
Copy link
Member

tbeu commented Nov 4, 2020

@avibrazil Have a look here (for maint) or here (for master).

@avibrazil
Copy link

@avibrazil Have a look here (for maint) or here (for master).

Great to see it truly evolving in code.
But I need it as plain dnf install from popular sources as for wider adoption.

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

5 participants