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

fix: key frame extraction (DEV-1513) #2300

Merged
merged 1 commit into from Nov 24, 2022

Conversation

seakayone
Copy link
Collaborator

@seakayone seakayone commented Nov 23, 2022

Issue Number: DEV-1513

Pull Request Checklist

Basic Requirements

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix: represents bug fixes
  • Refactor: represents production code refactoring
  • Feature: represents a new feature
  • Documentation: documentation changes (no production code change)
  • Chore: maintenance tasks (no production code change)
  • Style: styles updates (no production code change)
  • Test: all about tests: adding, refactoring tests (no production code change)
  • Other... Please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR change client-test-data?

  • Yes (don't forget to update the JS-LIB team about the change)
  • No

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:

  1. 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.

  2. 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

@seakayone seakayone requested review from irinaschubert and subotic and removed request for irinaschubert November 23, 2022 15:24
@seakayone seakayone self-assigned this Nov 23, 2022
@seakayone seakayone force-pushed the fix/key-frame-extraction-DEV-1513 branch from 4676c77 to 547a3fa Compare November 23, 2022 15:27
@seakayone seakayone changed the title Fix/key frame extraction dev 1513 fix: key frame extraction dev-1513 Nov 23, 2022
@swarmia
Copy link

swarmia bot commented Nov 23, 2022

@seakayone
Copy link
Collaborator Author

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.

@BalduinLandolt
Copy link
Collaborator

in the PR title, "DEV" should be all-caps and in parentheses :)

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a 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
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Base: 86.95% // Head: 86.95% // No change to project coverage 👍

Coverage data is based on head (d5ad6be) compared to base (bfe578a).
Patch has no changes to coverable lines.

❗ Current head d5ad6be differs from pull request most recent head 1cfee67. Consider uploading reports for the commit 1cfee67 to get more accurate results

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@seakayone seakayone changed the title fix: key frame extraction dev-1513 fix: key frame extraction DEV-1513 Nov 23, 2022
@seakayone seakayone changed the title fix: key frame extraction DEV-1513 fix: key frame extraction (DEV-1513) Nov 23, 2022
Copy link

@irinaschubert irinaschubert left a 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?

@seakayone
Copy link
Collaborator Author

seakayone commented Nov 24, 2022

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:

  1. There where no test present for it which I could easily extend.
  2. This is a more elaborate workflow and unit testing this bash script is very hard to do.

@seakayone seakayone force-pushed the fix/key-frame-extraction-DEV-1513 branch 2 times, most recently from d5ad6be to 210d5cf Compare November 24, 2022 10:47
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
@seakayone seakayone force-pushed the fix/key-frame-extraction-DEV-1513 branch from 210d5cf to 1cfee67 Compare November 24, 2022 10:50
@seakayone seakayone merged commit 729f071 into main Nov 24, 2022
7 checks passed
@seakayone seakayone deleted the fix/key-frame-extraction-DEV-1513 branch November 24, 2022 12:02
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

3 participants