Skip to content

Conversation

@anna-charlotte
Copy link
Contributor

@anna-charlotte anna-charlotte commented Jan 3, 2023

Add support for video files to docarray v2

Goals:

  • VideoUrl
  • VideoTensor with VideoNdarray and VideoTorchTensor
  • Video predefined doc (with optional attrs Video.tensor, Video.key_frame_indices, Video.url and Video.embedding)
  • tests
  • check and update documentation, if required. See guide

anna-charlotte added 3 commits January 4, 2023 10:29
Signed-off-by: anna-charlotte <[email protected]>
anna-charlotte added 3 commits January 4, 2023 12:17
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
@samsja samsja added the DocArray v2 This issue is part of the rewrite; not to be merged into main label Jan 6, 2023
@anna-charlotte anna-charlotte mentioned this pull request Jan 6, 2023
47 tasks
anna-charlotte added 7 commits January 11, 2023 09:36


class VideoTensorMixin:
@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

maybe briefly explain why we went for a static method here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In .validate() of VideoNdArray we want to call the validation for general VideoNdArray (by calling super().validate()), and secondly we want to add a video specific shape check (and respectively for torch). As far as I know it's not possible to specify in super() which parent class to go to, instead it just goes to through all the parent classes and stops when it find a corresponding method. Therefore we have to call the second check explicitly. When calling it as as VideoTensorMixin.validate() the information of the child class gets lost though, which we need to get the corresponding comp backend. Therefore the changes in the signature and to staticmethod.

Copy link
Member

Choose a reason for hiding this comment

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

If you wan to use a specific superclass implementation, instead of using super() you can do something like:

SuperclassThatYouWant.foo(*inputs)

Or if you need to pass the subclass cls:

SuperclassThatYouWant.foo.__func__(cls, *inputs)

This unbinds foo from the class, so you can give it any cls argument that you want.

You can see this in the __class_getitem__ implementation of NdArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice, thanks! This works, but mypy complains:

docarray/typing/tensor/video/video_ndarray.py:34: error: "Callable[[VideoTensorMixin], VideoTensorMixin]" has no attribute "__func__"  [attr-defined]

Do u know what to do here to satisfy mypy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok just saw that in __class_getitem__ of NdArray it is being ignored ( # type: ignore), will add that here too then

comp_backend = cls.get_comp_backend()

if (
comp_backend.n_dim(value) not in [3, 4] # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to ignore the type ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it mypy complains, I think because value in validate_shape can be an instance of VideoTensor, in our use case either VideoNdArray or VideoTorchTensor but the .n_dim methods have a specified input class respectiveley to their comp backend (np.ndarray or torch.Tensor). Therefore mypy complains that value is not an NdArray for NumpyCompBackend.n_dim() (or torch tensor for torch comp backend):

docarray/typing/tensor/video/video_tensor_mixin.py:22: error: Argument 1 to "n_dim" of "TorchCompBackend" has incompatible type "T"; expected "Tensor"  [arg-type]
docarray/typing/tensor/video/video_tensor_mixin.py:22: error: Argument 1 to "n_dim" of "NumpyCompBackend" has incompatible type "T"; expected "ndarray[Any, Any]"  [arg-type]

Copy link
Member

Choose a reason for hiding this comment

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

I think this is this issue that has to be fixed using the ugly overloads.

Copy link
Member

Choose a reason for hiding this comment

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

@JohannesMessner need ur help there we did not manage to debug this. I don't understand why it is not working and why we have different TypeVar in AbstractCompBackend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

overload was not working here, because the Mixin should not know about the classes VideoNdArray and VideoTorchTensor

Copy link
Member

@samsja samsja left a comment

Choose a reason for hiding this comment

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

I added some comments. PR looks great ! Looking forward to have video support

@anna-charlotte
Copy link
Contributor Author

Did you manage to resolve the quirk where saved video frames would slightly change? Or did you get more insight into why it happens / why it is okay?

No, still not so sure about this, I think that it is due to the encoding when storing to the file and decoding when reading from a file. I opened an issue on their Gitter as well as Github, but no response yet. I looked at a couple of examples over several iterations of loading and saving and for all it seemed to oscillate a little bit in the RGB values for the first 1 to 3 iterations, and then stayed the same.

@github-actions
Copy link

📝 Docs are deployed on https://ft-feat-add-video-v2--jina-docs.netlify.app 🎉

"""

url: Optional[VideoUrl]
audio: Optional[Audio] = Audio()
Copy link
Member

Choose a reason for hiding this comment

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

why not None as a default value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .load() from VideoUrl returns an AudioNdArray (and some other stuff), which can't be written to video.audio.tensor if video.audio == None.

Copy link
Member

Choose a reason for hiding this comment

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

okay makes sense

@samsja samsja dismissed JohannesMessner’s stale review January 17, 2023 15:16

cannot approve bc of knee problem

@samsja samsja merged commit 9888d93 into feat-rewrite-v2 Jan 17, 2023
@samsja samsja deleted the feat-add-video-v2 branch January 17, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants