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

Add a video dataset #1295

Closed
4 of 8 tasks
daniel-falk opened this issue Feb 27, 2022 · 6 comments · Fixed by #1312
Closed
4 of 8 tasks

Add a video dataset #1295

daniel-falk opened this issue Feb 27, 2022 · 6 comments · Fixed by #1312
Assignees
Labels
Community Issue/PR opened by the open-source community Component: IO Issue/PR addresses data loading/saving/versioning and validation, the DataCatalog and DataSets Issue: Feature Request New feature or improvement to existing feature

Comments

@daniel-falk
Copy link
Contributor

Description

I am working on a video dataset that is capable of loading and saving video files. I would be happy to contribute with this to the community. I currently have a working implementation of it, but there are some more work needed and I would like to hear your comments and see if anyone else is interested in helping with the finalization of it.

The current status is:

  • Load video files
  • Save video files from other VideoDataSet
  • Save video files from list of frames
  • Save video files from generator yielding frames
  • Make it work with fsspec
  • Verify functionality with different formats
  • Write unittests
  • Test it with versioned dataset, partitioned dataset etc

Context

When working with computer vision pipelines on video it is in many cases much more effective to read the frames directly from the video. This removes the need for manual preprocessing to extract the frames and it takes much less space on the disk if the frames are encoded in a video file (with inter frame compression) than to store separate frames as images. One of the few cases where this is not the best way is when accessing the frames frequently, e.g. when training models, then it is inefficient to decode the video each time. In this later case we can however still use the video dataset as a preprocessing node in the pipeline to unpack the frames to an intermediate dataset.

Possible Implementation

My implementation of the VideoDataSet can be seen at this link together with a pipeline using it in a few different ways.
https://github.com/daniel-falk/kedro-video-example

Possible Alternatives

I have not yet figured out how to use the OpenCV video writer together with fsspec. There are however other options available for writing video files.

There is a need to spend some time thinking about the file suffixes and the fourcc codes in the video (codec).

To save video now we need to import the video class from the dataset file, this does not feel like the same design pattern as the other datasets. Perhaps there is a better way to design it?

@daniel-falk daniel-falk added the Issue: Feature Request New feature or improvement to existing feature label Feb 27, 2022
@datajoely
Copy link
Contributor

@daniel-falk this looks really really great - one idea on how you could get it to write to fssepc is to use a NamedTemporaryFile, it's not a perfect solution but I think it could work.

https://stackoverflow.com/a/69671122/2010808

lorenabalan pushed a commit that referenced this issue Feb 28, 2022
[AUTO-MERGE] Merge master into develop via merge-master-to-develop
@merelcht merelcht added the Community Issue/PR opened by the open-source community label Feb 28, 2022
@daniel-falk
Copy link
Contributor Author

@datajoely That's probably the easiest solution. The potential issues I see with it is that then we are limited to processing videos that fits into the tmp-partition. I think that it is quite normal for linux users with /tmp mounted on tmpfs to only have 2GB or so for the /tmp partition. Perhaps there might be other limiting environments, e.g. when using in a docker container I think /tmp will be stored on whatever partition docker is set to use for its layers? How does it work in e.g. Airflow?

I tested some experiments writing to a named pipe instead. This did not work when writing .mp4 files, in that case the VideoWriter needed a seekable file. It did however work for .avi files. I'm not sure however that this is a good idea, 1) it is potentially relying on implementation details of the VideoWriter, 2) it is complex, 3) windows uses some other interface to named pipes than unix. Anyhow, here is the experiment:
https://github.com/daniel-falk/kedro-video-example/blob/master/notebooks/write-video-to-pipe.ipynb

I will look a bit too at other video encoding libraries, it would be good to find one that can write directly to an open file handle.

@datajoely
Copy link
Contributor

Hi @daniel-falk - makes a lot of sense and thank you for testing all of that!

I think we should be practical here and maybe make your constructor something like this:

 def __init__(self, filepath: str, fsspec_write:bool=False):

That way we can support both approaches, defaulting to the simplest and document the limitation in the docstring? Wdyt?

@daniel-falk
Copy link
Contributor Author

Yes, that's probably true. Let's make it simple now and we can think more about options later.

I did just push a new commit which makes it work with fsspec. It uses a named tempfile for the writing and the filecache in fsspec for reading (which makes a local copy of the file for us). What do you think about that?

 from kedro.io.core import (
     AbstractDataSet,
-    get_filepath_str,
     get_protocol_and_path,
 )
 
@@ -12,6 +11,7 @@ import cv2
 import PIL.Image
 import numpy as np
 import more_itertools
+import tempfile
 
 
 class SlicedVideo:
@@ -184,7 +184,6 @@ class VideoDataSet(AbstractDataSet):
         protocol, path = get_protocol_and_path(filepath)
         self._protocol = protocol
         self._filepath = PurePosixPath(path)
-        self._fs = fsspec.filesystem(self._protocol)
 
     def _load(self) -> AbstractVideo:
         """Loads data from the video file.
@@ -192,29 +191,30 @@ class VideoDataSet(AbstractDataSet):
         Returns:
             Data from the video file as a AbstractVideo object
         """
-        load_path = get_filepath_str(self._filepath, self._protocol)
-        with self._fs.open(load_path, mode="r") as f:
-            return FileVideo(load_path)
+        with fsspec.open("filecache::%s://%s" % (self._protocol, self._filepath), mode="rb") as f:
+            return FileVideo(f.name)
 
     def _save(self, video: AbstractVideo) -> None:
         """Saves image data to the specified filepath.
         """
-        save_path = get_filepath_str(self._filepath, self._protocol)
         # TODO: This assumes that the output file has the same fourcc code as the input file,
         # this might not be the case since we can use one VideoDataSet to read e.g. a mp4 file with H264 video
         # and then save it to another VideoDataSet which should use an .avi file with MJPEG
-        # TODO: There is no way to use the OpenVN VideoWrite to write to an open file, so it does not
-        # work together with fsspec. Investigate this further...
-        writer = cv2.VideoWriter(
-            save_path, cv2.VideoWriter_fourcc(*video.fourcc), video.fps, video.size
-        )
-        try:
-            for frame in video:
-                writer.write(  # PIL images are RGB, opencv expects BGR
-                    np.asarray(frame)[:, :, ::-1]
-                )
-        finally:
-            writer.release()
+        with tempfile.NamedTemporaryFile(suffix=self._filepath.suffix, mode="wb") as tmp:
+            writer = cv2.VideoWriter(
+                tmp.name, cv2.VideoWriter_fourcc(*video.fourcc), video.fps, video.size
+            )
+            try:
+                for frame in video:
+                    writer.write(  # PIL images are RGB, opencv expects BGR
+                        np.asarray(frame)[:, :, ::-1]
+                    )
+            finally:
+                writer.release()
+
+            with fsspec.open("%s://%s" % (self._protocol, self._filepath), "wb") as f_target:
+                with open(tmp.name, "rb") as f_tmp:
+                    f_target.write(f_tmp.read())
 
     def _describe(self) -> Dict[str, Any]:
         return dict(filepath=self._filepath, protocol=self._protocol)

We should however make sure that we do not copy the file if it is a local destination as you say, but it should be enough to check if self._protocol is file://? Or did you have any other thoughts about the parameter to the constructor?

Do you want me to move the dataset file to the kedro repo and create a pull-request, then we can continue development there? Or is it easier to keep it together with the test-pipeline for now?

I can also add that I looked at the fsspec.fuse parts for mounting the file locally and then writing to it. I don't know if they have made anything clever to sync lazily and therefore save disk space. I did however conclude it to be not useful since it is not supported on Windows and on MacOS it was terribly complicated to install libfuse.

@datajoely
Copy link
Contributor

All around amazing work - I wouldn't overdo, the self._protocol works for us elsewhere.

I'd raise the PR against the core Kedro repo and continue development there.

Thanks again for the contribution!

@daniel-falk
Copy link
Contributor Author

There is now a PR for the VideoDataSet: #1312

@merelcht merelcht linked a pull request Mar 7, 2022 that will close this issue
14 tasks
@merelcht merelcht added the Component: IO Issue/PR addresses data loading/saving/versioning and validation, the DataCatalog and DataSets label Mar 15, 2022
@noklam noklam self-assigned this Aug 9, 2022
@merelcht merelcht moved this to Done in Kedro Framework Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Issue/PR opened by the open-source community Component: IO Issue/PR addresses data loading/saving/versioning and validation, the DataCatalog and DataSets Issue: Feature Request New feature or improvement to existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants