Video Support in V1.0: part 1/3 : support matroskavideo #2370

mohamedchebbii · 2022-09-27T21:10:25Z

Hello,
I want to retsore the support of video files, I begin with matroskavideo and I will add tiff and asf format during next days.
Here a sample test results:

./bin/exiv2 -p x  ~/Downloads/sample_960x400_ocean_with_audio.mkv
Xmp.video.FileName                           XmpText    85  /home/mohamed.chebbi/Downloads/sample_960x400_ocean_with_audio.mkv
Xmp.video.FileSize                           XmpText     7  16.5413
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  Lavf58.45.100
Xmp.video.WritingApp                         XmpText    13  Lavf58.45.100
Xmp.video.Duration                           XmpText     5  46616
Xmp.video.TotalStream                        XmpText     1  1
Xmp.video.FrameRate                          XmpText     6  23.976
Xmp.video.Width                              XmpText     3  960
Xmp.video.Height                             XmpText     3  400
Xmp.video.AspectRatio                        XmpText     3  2.4

Please let me know If I miss anything, I will be happy to add It.
Best Regards,

postscript-dev · 2022-09-28T18:58:39Z

@mohamedchebbii

Please let me know If I miss anything, I will be happy to add It.

Since the video support was removed, @neheb has continued using clang-tidy to modernize the code to be compatible with C++17. I don't know which C++ version the video code currently supports but some small changes may be needed to comply. @neheb, do you have any thoughts on this?

@mohamedchebbii, please can you add at least one (small) test file to demonstrate that the code in your PR works. I don't know if we have a valid file in the 0.27-maintenance branch, perhaps the code was untested by the CI? Also, I remember that Exiv2 discussed storing larger files outside of the Exiv2 repository (e.g., #896) and downloading them as needed, but I'm not sure what we decided.

neheb · 2022-09-28T20:52:43Z

I'd run through clang-tidy, yes. On a quick glance, I see constructors not marked as delete and not marked public. I'm guessing this is copy/paste from somewhere.

neheb · 2022-09-28T23:30:10Z

include/exiv2/matroskavideo.hpp

+    /*!
+      @brief Helper structure for the Matroska tags lookup table.
+     */
+    struct MatroskaTags {


A simple std::pair is good enough I think.

Done 65a01dc

neheb · 2022-09-28T23:30:42Z

include/exiv2/matroskavideo.hpp

+        void aspectRatio();
+
+    private:
+        //! @name NOT Implemented


Not implemented functions should be public and marked = delete;

done here 65a01dc

hmm? I don't see = delete;

my bad, It's here : 49f6acd

include/exiv2/matroskavideo.hpp

src/matroskavideo.cpp

codecov · 2022-09-28T23:41:49Z

Codecov Report

Merging #2370 (f14bf95) into main (597e372) will decrease coverage by 0.76%.
The diff coverage is 11.55%.

@@            Coverage Diff             @@
##             main    #2370      +/-   ##
==========================================
- Coverage   64.20%   63.43%   -0.77%     
==========================================
  Files         119      121       +2     
  Lines       21082    21361     +279     
  Branches    10393    10513     +120     
==========================================
+ Hits        13535    13550      +15     
- Misses       5393     5651     +258     
- Partials     2154     2160       +6

Impacted Files	Coverage Δ
include/exiv2/matroskavideo.hpp	`0.00% <0.00%> (ø)`
include/exiv2/types.hpp	`81.81% <ø> (ø)`
src/matroskavideo.cpp	`4.71% <4.71%> (ø)`
src/image.cpp	`70.84% <91.66%> (+0.39%)`	⬆️

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

…de and clean up

…sts and some minor fix

include/exiv2/types.hpp

src/image.cpp

…windows compilation issue and some format issues

kevinbackhouse · 2022-10-04T09:08:48Z

Before this gets merged, I'd like to run the fuzzer on it for a few days. I'll wait until the rest of the code review is done, so please ping me when you think it's ready to merge.

mergify · 2022-10-04T21:46:12Z

⚠️ The sha of the head commit of this PR conflicts with #2373. Mergify cannot evaluate rules on this PR. ⚠️

… compilation issue and some format issues

… clang-format-12

… clang-format-12 patch 2

neheb · 2022-10-08T20:55:10Z

Needs rebasing.

kevinbackhouse · 2022-10-08T21:14:07Z

I started the fuzzer and it found a couple of crashes within a few minutes. The crash files are here: crash.tar.gz

Please could you read the README in the fuzz sub-directory and run the fuzzer yourself to check for other bugs?

kevinbackhouse · 2022-10-08T21:18:20Z

I notice that the only test in this PR is a very basic unit test. For a new source file like this, we need some proper test files in test/data with associated test scripts in tests/.

kevinbackhouse · 2022-10-08T21:19:48Z

include/exiv2/matroskavideo.hpp

+    @param buf Pointer to the memory area with the tag information.
+    @param size Size of \em buf.
+   */
+  void contentManagement(const MatroskaTags* mt, const byte* buf, long size);


Please don't use the long type. We're trying to eliminate it from the codebase. We usually use size_t for sizes.

understood, thank you very much @kevinbackhouse for your time.
I will analyse the crash report and run locally the fuzz test to fix them

replacing long by size_t done here f14bf95

…l?id=51707 Resize buf to avoid buffer overflow.

…l?id=51241 Fix bounds checking bug.

* Fix BmffImage::writeMetadata() error id/message * Add exceptions setting Exif/IPTC/XMP in BMFF * Add missing header for new functions

Easier to read with lambdas. Signed-off-by: Rosen Penev <[email protected]>

…l?id=51751 Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52053 Avoid null pointer deref and heap buffer overflow.

Video Support in V1.0: part 1/4 : support matroskavideo Video Support in V1.0: part 1/3 : support matroskavideo : upgrade code and clean up Video Support in V1.0: part 1/3 : support matroskavideo : clang-format Video Support in V1.0: part 1/3 : support matroskavideo : Add unit tests and some minor fix Video Support in V1.0: part 1/3 : support matroskavideo : try to fix windows compilation issue and some format issues Video Support in V1.0: part 1/3 : support matroskavideo : Fix windows compilation issue and some format issues Video Support in V1.0: part 1/3 : support matroskavideo : Format with clang-format-12 Video Support in V1.0: part 1/3 : support matroskavideo : Format with clang-format-12 patch 2

…g by size_t and fix rebase issue

mohamedchebbii · 2022-10-22T10:33:19Z

Needs rebasing.

done here f14bf95

Mohamed Ali added 2 commits September 27, 2022 22:19

Video Support in V1.0: part 1/4 : support matroskavideo

c65a822

Video Support in V1.0: part 1/4 : support matroskavideo

f8bd38f

neheb reviewed Sep 28, 2022

View reviewed changes

include/exiv2/matroskavideo.hpp Outdated Show resolved Hide resolved

neheb reviewed Sep 28, 2022

View reviewed changes

src/matroskavideo.cpp Outdated Show resolved Hide resolved

neheb reviewed Sep 28, 2022

View reviewed changes

src/matroskavideo.cpp Outdated Show resolved Hide resolved

Mohamed Ali added 2 commits September 29, 2022 22:46

Video Support in V1.0: part 1/3 : support matroskavideo : upgrade co…

65a01dc

…de and clean up

Video Support in V1.0: part 1/3 : support matroskavideo : clang-format

ff81f20

mohamedchebbii marked this pull request as draft September 30, 2022 10:55

Video Support in V1.0: part 1/3 : support matroskavideo : Add unit te…

047e619

…sts and some minor fix

mohamedchebbii marked this pull request as ready for review October 2, 2022 20:13

benmccann reviewed Oct 3, 2022

View reviewed changes

include/exiv2/types.hpp Outdated Show resolved Hide resolved

src/image.cpp Outdated Show resolved Hide resolved

Video Support in V1.0: part 1/3 : support matroskavideo : try to fix …

c6f1112

…windows compilation issue and some format issues

kevinbackhouse self-requested a review October 4, 2022 09:03

mohamedchebbii closed this Oct 4, 2022

mohamedchebbii deleted the 1748 branch October 4, 2022 21:24

mohamedchebbii restored the 1748 branch October 4, 2022 22:02

mohamedchebbii reopened this Oct 4, 2022

Mohamed Ali Chebbi added 3 commits October 5, 2022 00:37

Video Support in V1.0: part 1/3 : support matroskavideo : Fix windows…

49f6acd

… compilation issue and some format issues

Video Support in V1.0: part 1/3 : support matroskavideo : Format with…

a8a51c2

… clang-format-12

Video Support in V1.0: part 1/3 : support matroskavideo : Format with…

2050293

… clang-format-12 patch 2

kevinbackhouse reviewed Oct 8, 2022

View reviewed changes

kevinbackhouse and others added 14 commits October 20, 2022 23:02

Regression test for #2366

24af649

Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detai…

3c20d2e

…l?id=51707 Resize buf to avoid buffer overflow.

nul-terminate string

5441ab4

clang-format

22a4b68

Regression test for #2352

7dfc57c

Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detai…

7e55222

…l?id=51241 Fix bounds checking bug.

Fix/add exceptions for BMFF based files (#2364)

0fd477b

* Fix BmffImage::writeMetadata() error id/message * Add exceptions setting Exif/IPTC/XMP in BMFF * Add missing header for new functions

get rid of nested if conditions

1768028

Easier to read with lambdas. Signed-off-by: Rosen Penev <[email protected]>

Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detai…

31ccc2a

…l?id=51751 Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52053 Avoid null pointer deref and heap buffer overflow.

clang-format

5cdecb9

Merge remote-tracking branch 'origin/main' into main

e557b77

Merge branch 'HEAD' into 1748

4eeae49

Video Support in V1.0: part 1/3 : support matroskavideo : replace lon…

f14bf95

…g by size_t and fix rebase issue

mohamedchebbii closed this by deleting the head repository Nov 2, 2022

kevinbackhouse mentioned this pull request Jan 2, 2023

OSS-Fuzz issues in new video code #2450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video Support in V1.0: part 1/3 : support matroskavideo #2370

Video Support in V1.0: part 1/3 : support matroskavideo #2370

mohamedchebbii commented Sep 27, 2022

postscript-dev commented Sep 28, 2022

neheb commented Sep 28, 2022 •

edited

Loading

neheb Sep 28, 2022

mohamedchebbii Sep 29, 2022

neheb Sep 28, 2022

mohamedchebbii Sep 29, 2022

neheb Sep 29, 2022

mohamedchebbii Oct 4, 2022

codecov bot commented Sep 28, 2022 •

edited

Loading

kevinbackhouse commented Oct 4, 2022

mergify bot commented Oct 4, 2022

neheb commented Oct 8, 2022

kevinbackhouse commented Oct 8, 2022

kevinbackhouse commented Oct 8, 2022

kevinbackhouse Oct 8, 2022

mohamedchebbii Oct 10, 2022

mohamedchebbii Oct 22, 2022

mohamedchebbii commented Oct 22, 2022

Video Support in V1.0: part 1/3 : support matroskavideo #2370

Video Support in V1.0: part 1/3 : support matroskavideo #2370

Conversation

mohamedchebbii commented Sep 27, 2022

postscript-dev commented Sep 28, 2022

neheb commented Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

kevinbackhouse commented Oct 4, 2022

mergify bot commented Oct 4, 2022

neheb commented Oct 8, 2022

kevinbackhouse commented Oct 8, 2022

kevinbackhouse commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedchebbii commented Oct 22, 2022

neheb commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading