-
Notifications
You must be signed in to change notification settings - Fork 71
Decoder-native resize public implementation #1003
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
Changes from 15 commits
dd24dfa
3a2df84
5344ab4
98cf81b
65c4ad7
f300c70
2c3b7f0
80e84b5
5ac60d8
531b40f
cc333ac
238a8ff
55d362c
0d2492e
a2da767
2cd3f65
4ff0186
0f9eb62
8081298
39ed9ac
6e6815c
363e688
463674d
c20914c
254641a
9b4186a
105c77f
70b5976
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |||||
| import json | ||||||
| import numbers | ||||||
| from pathlib import Path | ||||||
| from typing import Literal, Optional, Tuple, Union | ||||||
| from typing import List, Literal, Optional, Sequence, Tuple, Union | ||||||
|
|
||||||
| import torch | ||||||
| from torch import device as torch_device, Tensor | ||||||
|
|
@@ -19,6 +19,7 @@ | |||||
| create_decoder, | ||||||
| ERROR_REPORTING_INSTRUCTIONS, | ||||||
| ) | ||||||
| from torchcodec.transforms import DecoderTransform, Resize | ||||||
|
|
||||||
|
|
||||||
| class VideoDecoder: | ||||||
|
|
@@ -66,6 +67,10 @@ class VideoDecoder: | |||||
| probably is. Default: "exact". | ||||||
| Read more about this parameter in: | ||||||
| :ref:`sphx_glr_generated_examples_decoding_approximate_mode.py` | ||||||
| transforms (sequence of transform objects, optional): Sequence of transforms to be | ||||||
| applied to the decoded frames by the decoder itself, in order. Accepts both | ||||||
| ``torchcodec.transforms.DecoderTransform`` and ``torchvision.transforms.v2.Transform`` | ||||||
|
||||||
| ``torchcodec.transforms.DecoderTransform`` and ``torchvision.transforms.v2.Transform`` | |
| :class:`~torchcodec.transforms.DecoderTransform` and :class:`~torchvision.transforms.v2.Transform` |
We should also create a doc page for the transforms!
Outdated
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.
Do we want to document this behavior? It seems binding, and we discussed that we may want to reserve the right to change the underlying implementation provided the output are still valid?
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.
We want to reserve the right to change the underlying implementation, but we may not be able to easily change when we apply the transform with respect to the colorspace conversion. That fact is, I think, implied by what we consider to be our reference: a fully decoded frame passed to a TorchVision transform. In that scenario, the transform is always applied after the colorspace conversion.
Then I think the questions are:
- Do we want to document that we consider passing untransformed frames to TorchVision transforms as our reference? I think we do, because I think that's implied by accepting the TorchVision transforms, and it's a easy way to explain the feature to users.
- Is when the transform is applied useful to users? I thought it was, but if it's of little value, we could potentially just not talk about it.
Given how far away the tolerances were when TorchCodec applied the transform in YUV, but TorchVision applied them in RGB, I think that if we ever changed this behavior, it would have to be an option.
scotts marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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 fails if tv_available is False? Because v2 wouldn't exist
EDIT ah no that's probably fine because of the if not tv_available: check above.
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.
Makes me thing we should have a dummy job where we don't install TV that ensures TC still works fine...
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.
On a job which doesn't have TorchVision installed: I agree we need to do something here, but I'd like to punt on this for now. The current testing file imports TorchVision unconditionally. I think we'll want to separate out the tests that require TorchVision from those that don't so that we can test both behaviors, but that will require different .py files. I'd like to deal with that in its own PR.
I actually started to add a step in the current linux wheel test that did not install TorchVision when I realized this.
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.
Yes, we can punt on this. I'm hoping we can do something very simple regarding testing: keep all but one test job using torchvision, and just have one small CI job that doesn't install TV and just runs a few tests, basically just insuring TV is an optional dependency. I'd like to avoid separating tests in different files just for that - we may have more than one optional dependency and that quickly becomes untractable.
Outdated
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.
Nit, I think I would have been less surprised by v2 being actually optional if this were elif.
| if isinstance(transform, v2.Resize): | |
| elif isinstance(transform, v2.Resize): |
scotts marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| # All rights reserved. | ||
| # | ||
| # This source code is licensed under the BSD-style license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
| from ._decoder_transforms import DecoderTransform, Resize # noqa |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,58 @@ | ||||||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | ||||||
| # All rights reserved. | ||||||
| # | ||||||
| # This source code is licensed under the BSD-style license found in the | ||||||
| # LICENSE file in the root directory of this source tree. | ||||||
|
|
||||||
| from abc import ABC, abstractmethod | ||||||
| from dataclasses import dataclass | ||||||
| from typing import Sequence | ||||||
|
|
||||||
|
|
||||||
| @dataclass | ||||||
| class DecoderTransform(ABC): | ||||||
| """Base class for all decoder transforms. | ||||||
| A DecoderTransform is a transform that is applied by the decoder before | ||||||
| returning the decoded frame. The implementation does not live in TorchCodec | ||||||
| itself, but in the underyling decoder. Applying DecoderTransforms to frames | ||||||
scotts marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| should be both faster and more memory efficient than receiving normally | ||||||
| decoded frames and applying the same kind of transform. | ||||||
| Most DecoderTransforms have a complementary transform in TorchVision, | ||||||
| specificially in torchvision.transforms.v2. For such transforms, we ensure | ||||||
|
||||||
| that: | ||||||
| 1. Default behaviors are the same. | ||||||
| 2. The parameters for the DecoderTransform are a subset of the | ||||||
| TorchVision transform. | ||||||
| 3. Parameters with the same name control the same behavior and accept a | ||||||
| subset of the same types. | ||||||
| 4. The difference between the frames returned by a DecoderTransform and | ||||||
| the complementary TorchVision transform are small. | ||||||
| All DecoderTranforms are applied in the output pixel format and colorspace. | ||||||
| """ | ||||||
|
|
||||||
| @abstractmethod | ||||||
| def make_params(self) -> str: | ||||||
| pass | ||||||
|
|
||||||
|
|
||||||
| @dataclass | ||||||
| class Resize(DecoderTransform): | ||||||
scotts marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| """Resize the decoded frame to a given size. | ||||||
| Complementary TorchVision transform: torchvision.transforms.v2.Resize. | ||||||
|
||||||
| Complementary TorchVision transform: torchvision.transforms.v2.Resize. | |
| Complementary TorchVision transform: :class:`~torchvision.transforms.v2.Resize~. |
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 was getting linting errors like: https://github.com/meta-pytorch/torchcodec/actions/runs/19157614790/job/54761644331
Which points to docs which recommend the above change: https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports