Add a video dataset reader/writer

[daniel-falk]

Description

This PR adds a video dataset to read and write video files. The idea was discussed in issue #1295.

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.

Development notes

I would gladly accept help to finish the last details of this PR.
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 containers and codecs
• Write unittests
• Test it with versioned dataset, partitioned dataset etc
• Write documentation

Here is a sample pipeline using the VideoDataSet both for reading and writing:
https://github.com/daniel-falk/kedro-video-example/tree/use-kedro-fork

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes

[datajoely]

Thank you for putting so much time and effort into this @daniel-falk 🙏 It's looking great!

[merelcht]

Hi @daniel-falk, are you able to continue working on this PR?

[daniel-falk]

Hi @MerelTheisenQB, I definitelly intend to continue working on the commit - it just got shadowed by a lot of other work I needed to get done first. It will likelly take until May before I can focus more on this.

My plan is to write some unit tests next.

[merelcht]

Hi @MerelTheisenQB, I definitelly intend to continue working on the commit - it just got shadowed by a lot of other work I needed to get done first. It will likelly take until May before I can focus more on this.

My plan is to write some unit tests next.

Okay great, thanks for the heads-up! And do let us know if you need any help at all completing this 🙂

[daniel-falk]

Here is a first set of unit tests. These test mostly the interface, I intend to add more tests for the actual encoding and decoding of video. Once I have done that and verified that the current implementation is good I would appreciate help from you to test it together with the rest of the integrations, such as other datasets (e.g. versioned dataset) and the different infrastructures it should integrate with.

[daniel-falk]

How should the OpenCV requirement be installed? I have added it to a video_requires in setup.py so that it is installed with install_extra video or all, but it seems like it is not installed in the CI job? Seems like that only installs test_requirements.txt and requirements.txt?

[datajoely]

@noklam would mind helping @daniel-falk getting this awesome contribution over the line?

[noklam]

@daniel-falk Could you change the relative import to an absolute path and remove the suppression block for now? I suspect it hides the actual error from the CI log.

You'll also need to include the necessary dependencies in test_requirements.txt. Most of the CI jobs just install requirements.txt and test_requirements.txt.

[daniel-falk]

@noklam Do you have a Windows based environment setup? In that case, could you help me get the CI through for Windows?

• Windows unit test for save to S3 fails, not sure why (the codec-container combo is passing in the test_video_codecs test).
• pip compile fails for windows (not for Linux) not sure why.
• lint jobs fail, if I understand correctly due to files that I have not made any changes in.
• Linux unit test job fails, if I understand it correctly it is just because I don't have 100% test coverage yet. I'm working on this.

The container vs codec support on Windows and Linux looks like this:

EDIT: This table is not correct, OpenCV silently defaults to a supported codec if the one specified is not compatible with the container format. This results in a warning in the log, but the test did not fail. Test is updated and a new table in on the way...

Codec (fourcc)	Container	Linux	Windows
avc1	mjpeg	-	-
avc1	avi	-	-
avc1	mp4	-	OK
x264	mjpeg	-	-
x264	avi	-	-
x264	mp4	-	OK
mp4v	mjpeg	OK	OK
mp4v	avi	OK	OK
mp4v	mp4	OK	OK
mjpg	mjpeg	OK	OK
mjpg	avi	OK	OK
mjpg	mp4	OK	OK
xvid	mjpeg	OK	OK
xvid	avi	OK	OK
xvid	mp4	OK	OK
divx	mjpeg	OK	OK
divx	avi	OK	OK
divx	mp4	OK	OK
fmp4	mjpeg	OK	OK
fmp4	avi	OK	OK
fmp4	mp4	OK	OK

[noklam]

Don't worry about it for now, I have seen the same error in my other PR which is also blocked by some Window CI, once that get fixed I am pretty sure this would be passed.

Thanks a lot for your effort! Really appreciate it.

[daniel-falk]

Here is the codec-container compatibility matrix as from the CI pipeline. Some codecs can be installed separately (such as e.g. h264 codec which has a bad licence preventing it from being shipped together with libraries)

Codec (fourcc)	Container	Linux	Windows
avc1	mjpeg	-	-
avc1	avi	-	-
avc1	mp4	-	-
x264	mjpeg	-	-
x264	avi	-	-
x264	mp4	-	-
mp4v	mjpeg	-	-
mp4v	avi	OK	OK
mp4v	mp4	OK	OK
mjpg	mjpeg	-	-
mjpg	avi	-	-
mjpg	mp4	-	-
xvid	mjpeg	-	-
xvid	avi	-	-
xvid	mp4	-	-
divx	mjpeg	-	-
divx	avi	-	-
divx	mp4	-	-
fmp4	mjpeg	-	-
fmp4	avi	-	-
fmp4	mp4	-	-

[daniel-falk]

I have added unit tests now to get 100% coverage.

@noklam It seems like it is just one unit test that fails on Windows now. Did you have a Windows environment setup so that you could take a look at this? The problem is not related to S3, it fails to write the file to disk when using the temp-dir, the other tests passes when it writes video with the same format to other paths.

[datajoely]

Amazing work!

[noklam]

Trying to re-run the Window Tests, we have seen the same annoying PEP517 error for a while. It's non-deterministic, let's check if the tests still fail.

[daniel-falk]

Have you had time to look anything more on this @noklam ? Or is there anyone else on the team who could help me get it trough @datajoely ?

[datajoely]

Hi @daniel-falk apologies we we're waiting for this to get out of draft!

[datajoely]

Amazing work both - thanks for pushing @daniel-falk this is an awesome contribution!

[noklam]

Awesome, I have converted this back to draft for now, feel free to make it "Ready for Review" to signal that and I will review it again!

[merelcht]

Hi @daniel-falk do you still want to complete this PR? We'd like to get all PRs related to datasets to be merged soon now we're moving our datasets code to a different package (see our medium blog post for more details)

Otherwise we'll close this PR and ask you to re-open it on the new repo when it's ready.

[daniel-falk]

Hi @merelcht. Sorry for being slow on this, some other projects took priority. It would be nice to get it merged before changes are made, when is the move planned to start?

I just uploaded a new commit which is:

• Rebased to latest main
• Added the missing parameters to the init function
• Squashed and cleaned up the commits
• Considered the failure reason on windows that @noklam found
• Inherited the Versioned dataset

I still need to make a bit of testing to verify that it works as expected in all situations.

I looked a bit into why the tests did not fail due to the missing credentials. As far as I can understand that is not tested by moto. The same issue is true for other datasets, if I remove the credentials argument the tests still pass. I can also replicate it with moto only, this test case passes with no issues:

import pytest
from moto import mock_s3
import os
import boto3

@pytest.fixture(scope='function')
def aws_credentials():
    """Mocked AWS Credentials for moto."""
    os.environ['AWS_ACCESS_KEY_ID'] = 'testing'
    os.environ['AWS_SECRET_ACCESS_KEY'] = 'testing'
    os.environ['AWS_SECURITY_TOKEN'] = 'testing'
    os.environ['AWS_SESSION_TOKEN'] = 'testing'
    os.environ['AWS_DEFAULT_REGION'] = 'us-east-1'

@pytest.fixture(scope='function')
def s3(aws_credentials):
    with mock_s3():
        yield boto3.client('s3', region_name='us-east-1')

def test_create_bucket_creds(s3):
    s32 = boto3.client('s3', region_name='us-east-1', aws_access_key_id="other", aws_secret_access_key="other")
    s32.create_bucket(Bucket="somebucket")

    result = s3.list_buckets()
    assert len(result['Buckets']) == 1
    assert result['Buckets'][0]['Name'] == 'somebucket'

[noklam]

@daniel-falk We aim to move it end of next week with a new Kedro minor release coming.

If it helps I am fine with a more basic version without Versioning at this point, you can add the versioning feature later at the Kedro-dataset package.

[daniel-falk]

@noklam That's a good idea to reduce the size of the test matrix. I have removed the inheritence of the versioned dataset now. I have also tested the dataset in a pipeline, both with local storage and with S3 storage as per this repo.

[noklam]

@MerelTheisenQB Something looks weird here, it seems to be our develop code that are not supposed to be here.

Main:
https://github.com/kedro-org/kedro/blob/main/test_requirements.txt#L16

Develop:
https://github.com/kedro-org/kedro/blob/develop/test_requirements.txt#L16

[noklam]

Is this the only place you need this? Would be great if we can remove the extra dependencies from more-itertools as it seems not doing much here.

I am not familiar with this library, I guess the goal is to peek at the first element so you can know the size of the element before iterating the generator. However, it seems to skip the first element somehow? Is this an expected behavior?

@daniel-falk

[daniel-falk]

In your example peek will be chained to include the first element even after peeking. In my code this is self._gen, this is what is used later. frames will be missing the first element after peeking, but that is not used anymore. Your example should instead be:

In [14]: def gen():
    ...:     for i in range(5):
    ...:         yield i
    ...: 
In [15]: 
In [15]: g = gen()
In [16]: p = more_itertools.peekable(g)
In [17]: p.peek()
Out[17]: 0
In [18]: list(p)
Out[18]: [0, 1, 2, 3, 4]

I changed the implementation to use regular itertools instead of more_itertools.

[noklam]

Cool, thanks for explaining. As I don't see the peek function as useful here, it almost behaves the same as next. I think it's nice to remove the more_itertools dependency.

[noklam]

Is this something to be done?

[daniel-falk]

I think this is good enough for now. According to the unit tests mp4v (which this defaults to unless explicitly overridden) supports both avi and mp4 containers on both windows and linux. mp4v is also the only codec that is installed in many installations. Some other formats might fail to encode with this, but then OpenCV will fall back to a codec that works and print a warning message in the log. In the future I do however think it is worth to spend some thoughts to see if there is any better way to derive the best codec.

[noklam]

Thank you, that makes sense!

[merelcht]

Thank you so much for this contribution @daniel-falk ! I'm super impressed with all the tests you've added for this as well ⭐ ⭐

I've left some comments in line and have some additional ones here:

• Don't forget to update the RELEASE.md to mention this new dataset
• One thing I don't fully understand is the purpose of all the different video classes. As a user do I need to specify if I want to use the SequenceVideo vs the GeneratorVideo ?

[daniel-falk]

@merelcht
re: One thing I don't fully understand is the purpose of all the different video classes. As a user do I need to specify if I want to use the SequenceVideo vs the GeneratorVideo ?

If you are only working with video files then you do not need to do that. When you open a video file from a path it will automatically become a FileVideo. In some cases you do however have a bunch of np.ndarrays that you want to create a video from, then you can use the SequenceVideo class to wrap them in, and then save them as a VideoDataSet. The same goes for GeneratorVideo, if you have a generator that yields frames, then you can use that class to create a VideoDataSet from that.

I'll add an example on this too in the docstring.

[merelcht]

I think we should move this to the Major features and improvements section above and format it in a table similar to the added datasets here https://github.com/kedro-org/kedro/blob/main/RELEASE.md#major-features-and-improvements-7

[daniel-falk]

done.

[merelcht]

@merelcht re: One thing I don't fully understand is the purpose of all the different video classes. As a user do I need to specify if I want to use the SequenceVideo vs the GeneratorVideo ?

If you are only working with video files then you do not need to do that. When you open a video file from a path it will automatically become a FileVideo. In some cases you do however have a bunch of np.ndarrays that you want to create a video from, then you can use the SequenceVideo class to wrap them in, and then save them as a VideoDataSet. The same goes for GeneratorVideo, if you have a generator that yields frames, then you can use that class to create a VideoDataSet from that.

I'll add an example on this too in the docstring.

Thank you for explaining and adding the examples! 👍

[datajoely]

This is really really awesome work @daniel-falk !

[merelcht]

Thank you so much for this epic contribution @daniel-falk ! ⭐ 🚀 I really appreciate how much work you've put into this!

[daniel-falk]

Thanks for positive feedback! Pushed a new commit with updated RELEASE file.

[noklam]

@daniel-falk Thank you for your contribution! The release note looks good to me.