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
fix: key frame extraction (DEV-1513) #2300
Conversation
4676c77
to
547a3fa
Compare
Before merging I should probably remove the huge .mp4 from the test_data folder as @jnussbaum pointed out to me. Keeping them for now for a convenience for our reviewers. |
in the PR title, "DEV" should be all-caps and in parentheses :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really feel competent to review this one. But I couldn't see anything obvious, so as long as it works, I'm happy with it
Codecov ReportBase: 86.95% // Head: 86.95% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #2300 +/- ##
=======================================
Coverage 86.95% 86.95%
=======================================
Files 242 242
Lines 28102 28102
=======================================
Hits 24435 24435
Misses 3667 3667 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, jsut a minor remark: you added test data but no test, is that correct? Would it be possible to add a test for the 2 issues?
I will remove the "test_data/*.mp4" files before merging. I did not add any tests because:
|
d5ad6be
to
210d5cf
Compare
Fixes bugs which prevent the key frame extraction and matrix file creation of the two files: https://github.com/dasch-swiss/4124-xmlperformance-testdata/blob/main/bitstreams/0.1.mp4 https://github.com/dasch-swiss/4124-xmlperformance-testdata/blob/main/bitstreams/100.mp4 Both files suffer from two different bugs: Fixes processing of very short moving images (0.1.mp4) The file is too short for any frame to be extracted with the previous implementation which assumed at least five keyframes to be extracted. Change the script to start extracting keyframes at time unit 0, i.e. the beginning of the file and use the first extract frame as the start of the matrix instead of a later possibly non existing one. Fix rounding error in calculation of number of matrix files to created (100.mp4) For some reason the calculation of the number of matrix files was rounded from 0.8... to 2 where the next bigger integer should be 1 fixes issue DEV-1513 Additional changes: * Add support for debugging output to export-moving-image-frames.sh Introduce $DEBUG for the export script as a way to turn on debug out * Mount scripts folder into sipi for local dev purposed * Declare function with function keyword * Formatting - use spaces instead of tabs akin to scala source code * Fix mimetype check being skipped/failing Failed with + file -b --mime-type ./1.mp4 -ne video/mp4 Usage: file [-bcCdEhikLlNnprsSvzZ0] [--apple] [--extension] [--mime-encoding] [--mime-type] [-e <testname>] [-F <separator>] [-f <namefile>] [-m <magicfiles>] [-P <parameter=value>] <file> ... file -C [-m <magicfiles>] file [--help] * Use double quotes in order to prevent globbing and word splitting * Remove unused code: function collect, unused num variable,unused framestart variable * Fix warning for read without -r https://github.com/koalaman/shellcheck/wiki/SC2162 * Simplify calculation and rename number_of_frame_files
210d5cf
to
1cfee67
Compare
Issue Number: DEV-1513
Pull Request Checklist
Basic Requirements
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Does this PR change client-test-data?
Other information
This PR contains fixes which prevent the key frame extraction and matrix file creation of the two files:
https://github.com/dasch-swiss/4124-xmlperformance-testdata/blob/main/bitstreams/0.1.mp4
https://github.com/dasch-swiss/4124-xmlperformance-testdata/blob/main/bitstreams/100.mp4
Both files suffer from two different bugs:
Fixes processing of very short moving images (0.1.mp4)
The file is too short for any frame to be extracted with the previous implementation which assumed at least five keyframes to be extracted. Change the script to start extracting keyframes at time unit 0, i.e. the beginning of the file and use the first extract frame as the start of the matrix instead of a later possibly non existing one.
Fix rounding error in calculation of number of matrix files to created (100.mp4)
For some reason the calculation of the number of matrix files was rounded from 0.8... to 2 where the next bigger integer should be 1
fixes issue DEV-1513