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] VideoManager read function failed on multiple videos #107

Merged

Conversation

@ivan23kor
Copy link
Contributor

@ivan23kor ivan23kor commented Jun 11, 2019

Traceback:

"scenedetect/video_manager.py", line 757, in read
::self._downscale_factor, ::self._downscale_factor, :]
TypeError: 'NoneType' object is not subscriptable

Reason:

  1. Initializing a VideoManager object with multiple video paths
  2. Set the downscale factor, e.g. with set_downscale_factor()
  3. Invoke read() function of the created VideoManager object, e.g. via passing it to the SceneManager detect_scenes() as frame_source argument

This happens because of the bug in video_manager.py around lines 750. If one of the captured videos is over, an empty numpy.ndarray is returned on line 745 and its further use leads to the aforementioned error.
The commit contains the bug fix in video_manager.py as well as an integration test (VideoManager + SceneManager) in tests/test_video_manager.py

if not read_frame and not self._get_next_cap():
break
# Switch to the next capture when the current one is over
if not read_frame:
Copy link
Owner

@Breakthrough Breakthrough Jun 11, 2019

Choose a reason for hiding this comment

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

I think this should be changed to while not read_frame instead now, since if for some reason the new cap doesn't return a frame, it will still be used below. Changing this to a while() would advance to the next cap, if any.

Loading

Copy link
Owner

@Breakthrough Breakthrough Jun 11, 2019

Choose a reason for hiding this comment

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

Sorry I realized just now, a while loop is not the solution actually - have a better suggestion.

Loading

tests/test_video_manager.py Outdated Show resolved Hide resolved
Loading
@Breakthrough
Copy link
Owner

@Breakthrough Breakthrough commented Jun 11, 2019

Hi @ivan23kor;

Great find!! I have one small favour to ask - as part of the review, I think the if statement should be changed to a while to handle the case where a video doesn't have any frames for some reason.

Do you agree/disagree? Just want to get your feedback on that before merging this.

Thank you so much for the fix, much appreciated!

Loading

scenedetect/video_manager.py Outdated Show resolved Hide resolved
Loading
@Breakthrough
Copy link
Owner

@Breakthrough Breakthrough commented Jun 11, 2019

Hi @ivan23kor;

Sorry my mistake - no need for nested loops, I think it would be better to just change line 752 to continue instead. Thoughts?

Thanks!!

Loading

@ivan23kor
Copy link
Contributor Author

@ivan23kor ivan23kor commented Jun 13, 2019

The second commit is better, check it out!

Loading

@Breakthrough
Copy link
Owner

@Breakthrough Breakthrough commented Jun 13, 2019

Very nice @ivan23kor! Thank you so much!!

Loading

@Breakthrough Breakthrough merged commit 266b33a into Breakthrough:master Jun 13, 2019
@Breakthrough Breakthrough added this to the v0.6 milestone Jun 13, 2019
@Breakthrough Breakthrough removed this from the v0.6 milestone Jul 13, 2019
@Breakthrough Breakthrough added this to the v0.5.1 milestone Jul 13, 2019
@ivan23kor ivan23kor deleted the many_videos_video_manager_fix branch Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants