-
Notifications
You must be signed in to change notification settings - Fork 235
feat(v2): add video support #972
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
Conversation
422a3f6 to
658c247
Compare
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
658c247 to
dc957d1
Compare
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
…ideo-v2 Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
|
|
||
|
|
||
| class VideoTensorMixin: | ||
| @staticmethod |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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]There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Signed-off-by: anna-charlotte <[email protected]>
samsja
left a comment
There was a problem hiding this 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
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
…ideo-v2 Signed-off-by: anna-charlotte <[email protected]>
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. |
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
|
📝 Docs are deployed on https://ft-feat-add-video-v2--jina-docs.netlify.app 🎉 |
| """ | ||
|
|
||
| url: Optional[VideoUrl] | ||
| audio: Optional[Audio] = Audio() |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay makes sense
cannot approve bc of knee problem
Add support for video files to docarray v2
Goals:
VideoUrlVideoTensorwithVideoNdarrayandVideoTorchTensorVideopredefined doc (with optional attrsVideo.tensor,Video.key_frame_indices,Video.urlandVideo.embedding)