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

Restore some video capabilities by restoring quicktimevideo.cpp #2337

Merged
merged 8 commits into from
Aug 24, 2022

Conversation

hassec
Copy link
Member

@hassec hassec commented Aug 22, 2022

  • Revive previously removed quicktime.{h,cpp} files and adapt them to work with the current version of exiv2
  • Add https://github.com/HowardHinnant/date as dependency to have sane handling of dates in c++ (I picked this library because it will be standard in c++20)
  • Add some xmp printer conversions such that e.g. TrackCreateDate isn't just printed as seconds since 1904...
  • add a very small test file to the regression tests to execute some of the newly added code.

Before we can merge this I'd need an ok from @notorand-it to add his file to this repo for testing purposes. Could you adapt your issue #2237 to include that you license the original file as CC0?

Test file is created by myself and I hereby license it under CC0

This would close #2237 (fyi @benmccann), and partially close #1748.

EDIT: date library and pretty printers removed again... I couldn't really figure out how to fix the ci and don't really have the time to go debug these ci issues.

@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2022

This pull request fixes 1 alert when merging 1d6be12 into 89d7798 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@hassec hassec force-pushed the hassec_revive_qt branch 2 times, most recently from 94306d2 to 1b87806 Compare August 22, 2022 15:07
@hassec hassec added video enhancement feature / functionality enhancements labels Aug 22, 2022
@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2022

This pull request fixes 1 alert when merging 1b87806 into 89d7798 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #2337 (b04e48c) into main (89d7798) will decrease coverage by 0.30%.
The diff coverage is 55.46%.

@@            Coverage Diff             @@
##             main    #2337      +/-   ##
==========================================
- Coverage   63.47%   63.16%   -0.31%     
==========================================
  Files         118      119       +1     
  Lines       19584    20433     +849     
  Branches     9569    10164     +595     
==========================================
+ Hits        12431    12907     +476     
- Misses       5088     5403     +315     
- Partials     2065     2123      +58     
Impacted Files Coverage Δ
include/exiv2/basicio.hpp 91.66% <ø> (ø)
src/bmffimage.cpp 76.81% <0.00%> (+0.21%) ⬆️
src/image.cpp 69.51% <ø> (ø)
src/properties.cpp 75.22% <ø> (ø)
src/quicktimevideo.cpp 55.52% <55.52%> (ø)
app/exiv2.cpp 62.54% <0.00%> (+0.05%) ⬆️
src/tags.cpp 67.83% <0.00%> (+1.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hassec hassec force-pushed the hassec_revive_qt branch 8 times, most recently from 00afe0a to 3cdfdfe Compare August 22, 2022 17:19
@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2022

This pull request fixes 1 alert when merging de5a289 into 89d7798 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@piponazo
Copy link
Collaborator

In general it looks good to me, thanks for taking care of restoring this code 👏 .

I see there are some windows errors in CI:
image

If you do not have a windows dev environment, let me know and I can help fixing those.

@hassec
Copy link
Member Author

hassec commented Aug 23, 2022

Thanks @piponazo! I don't have a windows dev env, but let me first try to play a round of trial and error ci ping pong 😂
If that fails, I'll happily forward this to you ;)

@kevinbackhouse
Copy link
Collaborator

I would try replacing uint64_t with size_t throughout this file to fix the Windows build failures. It looks to me like those values are all file sizes/offsets, so size_t is the natural type to use.

@benmccann
Copy link
Contributor

This is great! Thank you so much!

FYI, because you said "partially close #1748" in the PR description it will close that issue when this PR is merged, which doesn't seem like what you intended

Hopefully @notorand-it can give you a quick okay to use the test file, but if you don't hear back or it's holding things up, I should be able to create a small test video in that case.

@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2022

This pull request fixes 1 alert when merging e97855b into 71dc04e - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@hassec
Copy link
Member Author

hassec commented Aug 24, 2022

Added a test file that I created myself, so once the ci is green this is good to go form my side :)

@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2022

This pull request fixes 1 alert when merging b04e48c into 71dc04e - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature / functionality enhancements video
Projects
None yet
4 participants