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

Set frame rate of video extracting #382

Closed
wants to merge 1 commit into from
Closed

Conversation

zliang7
Copy link
Contributor

@zliang7 zliang7 commented Apr 9, 2019

This value means how many image files generated per second.
To reduce the number of image files, the value must be less than
the video FPS.

@zliang7
Copy link
Contributor Author

zliang7 commented Apr 9, 2019

This PR is trying to fix #194. To reduce the number of image files, this PR use frame rate value(FPS) Instead of using step value of frame.
@nmanovic , PTAL

if frame_rate > 0:
output_opts += ' -r ' + str(frame_rate)
else:
output_opts += ' -vsync 0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option -vsync 0 is very important. It allows to generate non-duplicated frames. -r can lead to duplicated frames and in general it is bad

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to keep the vsync options. But if both -vsync and -r options are specified, ffmpeg reports error:
"Using -vsync 0 and -r can produce invalid output files"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zliang7, this is why '-r' option is not good here. See my thoughts below.

@nmanovic
Copy link
Contributor

nm/rest_api should replace develop very soon. Could you please submit your changes into nm/rest_api branch?

@@ -272,13 +272,17 @@ def rq_handler(job, exc_type, exc_value, traceback):
############################# Internal implementation for server API

class _FrameExtractor:
def __init__(self, source_path, compress_quality, flip_flag=False):
def __init__(self, source_path, compress_quality, flip_flag=False, frame_rate=0):
Copy link
Contributor

@nmanovic nmanovic Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem with the approach is you will get annotation file which doesn't correspond to the video file. For example, originally you have a video file with 30FPS. If someone annotates the whole video file we will get <frame000000>, <frame000001>, ... inside the annotation file for every frame. Now you say that we can provide "custom" FPS. You will get another <frame000000>, <frame000001>, ... which don't correspond to the original video file. How to match if you don't store extra parameters (e.g. custom frame rate) inside the annotation file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the internals of backend. Is there any difference between upload a video directly with upload all frame images pre-extracted locally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zliang7, if you still have questions it is better to organize a meeting and discuss.

@nmanovic
Copy link
Contributor

nmanovic commented Apr 10, 2019

Let me clarify my vision for the feature:

  • Need several extra fields:

    • start frame. It should be 0 by default, >=0, less than stop frame. It will be good if you can specify it as frame number or as time
    • stop frame. It should be empty by default (corresponds to the end), >=0, more than start frame. It will be good if you can specify it as frame number or as time
    • step. It should be empty by default (corresponds to the minimum possible step), >0. It will be good if you can specify it as frame number or as time
  • Inside dump file you should have exact correspondence between the video file and dumped frames. For example, if you annotate only 1 frame from 30 frames you should be <frame000000>, <frame000029>, ... inside the annotation file.

  • As I said previously the patch should be submitted into nm/rest_api branch. It will be merged into develop branch very soon.

@nmanovic
Copy link
Contributor

  • start frame. It should be 0 by default, >=0, less than stop frame. It will be good if you can specify it as frame number or as time

May be for unification the default value for start frame should be also empty.

@nmanovic
Copy link
Contributor

One more important note. The same approach can work not only for a video file. It can work for a set of images or an archive. All these parameters above should be as a part of rest api and go to DB as well.

This value means how many image files generated per second.
To reduce the number of image files, the value must be less than
the video FPS.
@nmanovic
Copy link
Contributor

@zliang7 , should we expect a new patch for the issue or just close the PR?

@zliang7
Copy link
Contributor Author

zliang7 commented Apr 15, 2019

@nmanovic , it's OK to close this PR. I will try to submit again after your restful api PR get merged. But before that, I need some time to understand your huge change to rebase/change this one.

I still have some questions about your thoughts. Instead of setup a meeting to discuss, I prefer to discuss in the issue page. So everyone can see it and join the discussion, since
cvat is an opensource project. Please see my question at #194

@nmanovic
Copy link
Contributor

OK. Let's make one more iteration with the PR. I will close it for now.

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.

2 participants