1748 Video Support in V1.0: part 1/3 : support MatroskaViedo

[mohamedchebbii]

./bin/exiv2 -p x  ~/Downloads/sample_640x360.mkv
Xmp.video.FileName                           XmpText    68  /home/mohamed.chebbi/Downloads/sample_640x360.mkv
Xmp.video.FileSize                           XmpText     8  0.546518
Xmp.video.MimeType                           XmpText    14  video/matroska
Xmp.video.EBMLVersion                        XmpText     1  1
Xmp.video.EBMLReadVersion                    XmpText     1  1
Xmp.video.DocType                            XmpText     8  matroska
Xmp.video.DocTypeVersion                     XmpText     1  4
Xmp.video.DocTypeReadVersion                 XmpText     1  2
Xmp.video.TimecodeScale                      XmpText     5  0.001
Xmp.video.MuxingApp                          XmpText    13  Lavf57.83.100
Xmp.video.WritingApp                         XmpText    13  Lavf57.83.100
Xmp.video.Duration                           XmpText     5  13346
Xmp.video.TotalStream                        XmpText     1  1
Xmp.video.FrameRate                          XmpText     5  29.97
Xmp.video.Width                              XmpText     3  640
Xmp.video.Height                             XmpText     3  360
Xmp.video.AspectRatio                        XmpText     4  16:9

[mohamedchebbii]

Hello @kevinbackhouse ,
Can you please trigger CI tests on my three PRs to check if I need to fix something else ?
#2413
#2415
#2416
Best Regards,

[mohamedchebbii]

Hello @kevinbackhouse , Can you please trigger CI tests on my three PRs to check if I need to fix something else ? #2413 #2415 #2416 Best Regards,

Thank you @kevinbackhouse ,
It's seems most CI tests fail with the following error :

ERROR: zlib/1.2.13: Cannot load recipe.
Error loading conanfile at '/home/runner/.conan/data/zlib/1.2.13/_/_/export/conanfile.py': Current Conan version (1.52.0) does not satisfy the defined one (>=1.53.0).
Error: Process completed with exit code 1.

Is there something that I can do in my side to fix that ; or It's an issue with the CI ?

[codecov]

Codecov Report

Merging #2413 (4bcbbde) into main (0f65c85) will decrease coverage by 0.88%.
The diff coverage is 5.74%.

@@            Coverage Diff             @@
##             main    #2413      +/-   ##
==========================================
- Coverage   65.43%   64.54%   -0.89%     
==========================================
  Files         118      120       +2     
  Lines       21661    21992     +331     
  Branches    10467    10597     +130     
==========================================
+ Hits        14173    14194      +21     
- Misses       5296     5600     +304     
- Partials     2192     2198       +6     

Impacted Files	Coverage Δ
src/image.cpp	71.51% <ø> (+0.38%)	⬆️
src/matroskavideo.cpp	4.08% <4.08%> (ø)
include/exiv2/matroskavideo.hpp	46.15% <46.15%> (ø)
src/pentaxmn_int.cpp	72.34% <0.00%> (ø)

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

[piponazo]

Thanks for your contribution!

I approved your contributions so that now the CI jobs should trigger automatically each time you push something new into your branch.

I do not have any knowledge about the metadata format in video files, but I provided some general feedback about the code your wrote.

In general it looks quite good and with some minor improvements I would be happy to accept the changes 👍

[mohamedchebbii]

Hello @piponazo,
I changed the code as you recommanded , could you please have look ?
Regards

[benmccann]

Thank you @kevinbackhouse and @piponazo for your reviews on this PR. I was wondering if you had any additional comments on it?

[clanmills]

The Ubuntu CI fails to install. The last message before death concerns geotag.

My PR #2437 suffers the same fate.

[piponazo]

Hi @mohamedchebbii, @benmccann . Sorry for the late response. I just re-approved the changes so that the CI would retrigger the jobs, and there are some failures in the windows builds. I would be happy to approve the changes once all the CI jobs pass.

[mohamedchebbii]

Hi @piponazo ,
I fixed the windows build issue,
Could you please re-run CI tests ?
Thanks

[benmccann]

I see @piponazo mentioned above (#2413 (review)) that the CI should trigger automatically:

I approved your contributions so that now the CI jobs should trigger automatically each time you push something new into your branch.

It seems like that's not happening for whatever reason. I'm not quite sure how to set that up on GitHub, but I believe that after you have a commit in a repo it will begin happening automatically, so since this PR is so close perhaps the easiest solution is just to trigger it manually and merge it if it passes. Then hopefully the CI will run automatically on your other pending PRs

[mohamedchebbii]

I see @piponazo mentioned above (#2413 (review)) that the CI should trigger automatically:

I approved your contributions so that now the CI jobs should trigger automatically each time you push something new into your branch.

It seems like that's not happening for whatever reason. I'm not quite sure how to set that up on GitHub, but I believe that after you have a commit in a repo it will begin happening automatically, so since this PR is so close perhaps the easiest solution is just to trigger it manually and merge it if it passes. Then hopefully the CI will run automatically on your other pending PRs

Hello @benmccann ,
Yes indeed , I run CI tests in my repo and they are passed : https://github.com/mohamedchebbii/exiv2/actions/runs/3803047118
It seems , as you said, that for some reasons tests are not triggered automatically each time I push.

[piponazo]

Finally all CI jobs are passing and the code looks good to me. Is it fine to merge this now? Or do you expect to merge the 3 related PRs together?

[mohamedchebbii]

HI @piponazo ,
Thank you for approving my changes.
I would prefer to merge this PR now because the three PRs are not really related :)