-
Notifications
You must be signed in to change notification settings - Fork 71
Add codec selection to VideoEncoder API #1038
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 codec selection to VideoEncoder API #1038
Conversation
2a49a3d to
94ddcbb
Compare
94ddcbb to
976bd2c
Compare
| Returns: | ||
| Tensor: The raw encoded bytes as 4D uint8 Tensor. | ||
| Tensor: The raw encoded bytes as 1D uint8 Tensor. |
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.
Drive by change: this should say 1D.
src/torchcodec/_core/ops.py
Outdated
| crf: Optional[int] = None, | ||
| codec: Optional[str] = None, | ||
| pixel_format: Optional[str] = None, | ||
| crf: Optional[int] = 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.
Drive by changes: Reordering these params to keep the order consistent.
test/test_encoders.py
Outdated
| frame_rate=30, | ||
| ) | ||
| getattr(encoder, method)(**valid_params, codec=codec) | ||
|
|
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.
Let's add a test against the CLI to make sure the parameter is properly taken into account? It will probably have to be a separate test (not part of the giant test_against_cli one), since the codec will obviously vary per format.
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.
Let's also assert that using "libx264" gives the exact same result as using "h264", i.e. that there is an equivalence between codec spec and codec implem.
…o codec_select_encode_option
NicolasHug
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.
Nice work, thanks @Dan-Flores !
| codec, | ||
| " not found. To see available codecs, run: ffmpeg -encoders"); | ||
| } else { | ||
| avCodec = avcodec_find_encoder(avFormatContext_->oformat->video_codec); |
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 just realized oformat is a pointer so let's add
TORCH_CHECK(avFormatContext_->oformat != nullptr, ...);
|
|
||
| frames_spec = self.decode(spec_output).data | ||
| frames_impl = self.decode(impl_output).data | ||
| torch.testing.assert_close(frames_spec, frames_impl, rtol=0, atol=0) |
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.
Great tests!
This PR adds
codecas an option in the VideoEncoder API.It accepts codec specs or codec implems, similar to FFmpeg's find_codec function by using
avcodec_find_encoder_by_nameoravcodec_descriptor_get_by_name+avcodec_find_encoder.