Skip to content

Conversation

@dimaxano
Copy link

I've just rewrote function for frames reading from skvideo to OpenCV

Copy link
Owner

@rizkiarm rizkiarm left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions! It is a very good idea to add OpenCV support. But please do note that this addition would incur some cost to the user as it further complicates setup, given that it is widely known that OpenCV doesn't play well with some OS as well as very picky about dependencies.

That's why we believe that it would be better to keep skvideo as a default and give the choice to use OpenCV to the user. We would also like to keep the requirement as it is and put cv2 only as an optional requirement.

def get_video_frames(self, path):
def get_video_frames_skvideo(self, path):
videogen = skvideo.io.vreader(path)
#print("aaaa")
Copy link
Owner

Choose a reason for hiding this comment

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

Please delete this line

Copy link
Author

Choose a reason for hiding this comment

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

Done!

return mouth_frames

def get_video_frames(self, path):
def get_video_frames_skvideo(self, path):
Copy link
Owner

Choose a reason for hiding this comment

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

Don't change the current method of video loading as it has been proven to be reliable and doesn't require an additional installation of OpenCV, which is sometimes problematic (not to mention that the program itself sometimes doesn't work as expected).

Instead, provide an additional argument for the user in the from_video method. Something like this: from_video(self, path, use_open_cv=False). Please keep Sk-video to be the default method.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

from scipy import ndimage
from scipy.misc import imresize
import skvideo.io
import cv2
Copy link
Owner

Choose a reason for hiding this comment

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

Handle cv2 import failure (when users don't have OpenCV) as we want to give options to users to opt-out from using the feature

Copy link
Author

Choose a reason for hiding this comment

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

I've decided to import cv2 later in the code

return frames


def get_video_frames(self, path):
Copy link
Owner

Choose a reason for hiding this comment

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

Rename the method to signify the use of opencv

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@dimaxano dimaxano closed this Jul 24, 2018
@dimaxano dimaxano reopened this Jul 24, 2018
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