Skip to content

Refactor context structs to use constructors #869

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.

Sign up for GitHub

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

Merged
scotts merged 3 commits into from
Sep 5, 2025
Merged

Refactor context structs to use constructors #869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c48023
Refactor context structs to use constructors
scotts Sep 4, 2025
d6e39e9
Better defaults on SwsFrameContext
scotts Sep 5, 2025
942c2bb
Merge branch 'main' of github.com:pytorch/torchcodec into struct_init
scotts Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 28 additions & 19 deletions src/torchcodec/_core/CpuDeviceInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ static bool g_cpu = registerDeviceInterface(

} // namespace

CpuDeviceInterface::SwsFrameContext::SwsFrameContext(
int inputWidth,
int inputHeight,
AVPixelFormat inputFormat,
int outputWidth,
int outputHeight)
: inputWidth(inputWidth),
inputHeight(inputHeight),
inputFormat(inputFormat),
outputWidth(outputWidth),
outputHeight(outputHeight) {}

bool CpuDeviceInterface::SwsFrameContext::operator==(
const CpuDeviceInterface::SwsFrameContext& other) const {
return inputWidth == other.inputWidth && inputHeight == other.inputHeight &&
Expand Down Expand Up @@ -97,13 +109,12 @@ void CpuDeviceInterface::convertAVFrameToFrameOutput(
// And we sometimes re-create them because it's possible for frame
// resolution to change mid-stream. Finally, we want to reuse the colorspace
// conversion objects as much as possible for performance reasons.
SwsFrameContext swsFrameContext;

swsFrameContext.inputWidth = avFrame->width;
swsFrameContext.inputHeight = avFrame->height;
swsFrameContext.inputFormat = frameFormat;
swsFrameContext.outputWidth = expectedOutputWidth;
swsFrameContext.outputHeight = expectedOutputHeight;
SwsFrameContext swsFrameContext(
avFrame->width,
avFrame->height,
frameFormat,
expectedOutputWidth,
expectedOutputHeight);

outputTensor = preAllocatedOutputTensor.value_or(allocateEmptyHWCTensor(
expectedOutputHeight, expectedOutputWidth, torch::kCPU));
Expand All @@ -128,22 +139,20 @@ void CpuDeviceInterface::convertAVFrameToFrameOutput(
} else if (colorConversionLibrary == ColorConversionLibrary::FILTERGRAPH) {
// See comment above in swscale branch about the filterGraphContext_
// creation. creation
FiltersContext filtersContext;

filtersContext.inputWidth = avFrame->width;
filtersContext.inputHeight = avFrame->height;
filtersContext.inputFormat = frameFormat;
filtersContext.inputAspectRatio = avFrame->sample_aspect_ratio;
filtersContext.outputWidth = expectedOutputWidth;
filtersContext.outputHeight = expectedOutputHeight;
filtersContext.outputFormat = AV_PIX_FMT_RGB24;
filtersContext.timeBase = timeBase;

std::stringstream filters;
filters << "scale=" << expectedOutputWidth << ":" << expectedOutputHeight;
filters << ":sws_flags=bilinear";

filtersContext.filtergraphStr = filters.str();
FiltersContext filtersContext(
avFrame->width,
avFrame->height,
frameFormat,
avFrame->sample_aspect_ratio,
expectedOutputWidth,
expectedOutputHeight,
AV_PIX_FMT_RGB24,
filters.str(),
timeBase);

if (!filterGraphContext_ || prevFiltersContext_ != filtersContext) {
filterGraphContext_ =
Expand Down
18 changes: 13 additions & 5 deletions src/torchcodec/_core/CpuDeviceInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,19 @@ class CpuDeviceInterface : public DeviceInterface {
const UniqueAVFrame& avFrame);

struct SwsFrameContext {
int inputWidth;
int inputHeight;
AVPixelFormat inputFormat;
int outputWidth;
int outputHeight;
int inputWidth = 0;
int inputHeight = 0;
AVPixelFormat inputFormat = AV_PIX_FMT_NONE;
int outputWidth = 0;
int outputHeight = 0;

SwsFrameContext() = default;
SwsFrameContext(
int inputWidth,
int inputHeight,
AVPixelFormat inputFormat,
int outputWidth,
int outputHeight);
bool operator==(const SwsFrameContext&) const;
bool operator!=(const SwsFrameContext&) const;
};
Expand Down
20 changes: 20 additions & 0 deletions src/torchcodec/_core/FilterGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,26 @@ extern "C" {

namespace facebook::torchcodec {

FiltersContext::FiltersContext(
int inputWidth,
int inputHeight,
AVPixelFormat inputFormat,
AVRational inputAspectRatio,
int outputWidth,
int outputHeight,
AVPixelFormat outputFormat,
const std::string& filtergraphStr,
AVRational timeBase)
: inputWidth(inputWidth),
inputHeight(inputHeight),
inputFormat(inputFormat),
inputAspectRatio(inputAspectRatio),
outputWidth(outputWidth),
outputHeight(outputHeight),
outputFormat(outputFormat),
filtergraphStr(filtergraphStr),
timeBase(timeBase) {}

bool operator==(const AVRational& lhs, const AVRational& rhs) {
return lhs.num == rhs.num && lhs.den == rhs.den;
}
Expand Down
15 changes: 14 additions & 1 deletion src/torchcodec/_core/FilterGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,24 @@ struct FiltersContext {
int outputWidth = 0;
int outputHeight = 0;
AVPixelFormat outputFormat = AV_PIX_FMT_NONE;

std::string filtergraphStr;
AVRational timeBase = {0, 0};
UniqueAVBufferRef hwFramesCtx;

FiltersContext() = default;

@dvrogozh dvrogozh Sep 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should default initializers be removed from the above then?

@scotts scotts Sep 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we can't do that as they are default-initialized as members of CpuDeviceInterface: https://github.com/pytorch/torchcodec/blob/da3eddaa6746d093b09ad1413a42d7244686c58e/src/torchcodec/_core/CpuDeviceInterface.h#L64-L67
Before we decode anything, they start out empty. We could instead make the members unique pointers, but I think that's more complexity than it's worth.

@dvrogozh dvrogozh Sep 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, you are right. But in this case, I just realized that I forgot to add default initializers for SwsFrameContext struct. Can you help to fix within this PR?

@scotts scotts Sep 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dvrogozh, I believe I added them here: https://github.com/pytorch/torchcodec/pull/869/files#diff-454252ad2b209f4b84958d7a3a01c56e49c2cbc21f3aea12acdaa0cada3e8ee2R52

@dvrogozh dvrogozh Sep 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You might wish to adjust a format as AV_PIX_FMT_NONE defined as -1 (https://github.com/FFmpeg/FFmpeg/blob/6891314db18c96afa35f5e3765d60f4e257fc665/libavutil/pixfmt.h#L72):

AVPixelFormat inputFormat = AV_PIX_FMT_NONE;

@scotts scotts Sep 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dvrogozh, oh, you mean for the individual fields. Yes, got it.

@dvrogozh dvrogozh Sep 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes. Thank you for the change.

FiltersContext(FiltersContext&&) = default;
FiltersContext& operator=(FiltersContext&&) = default;
FiltersContext(
int inputWidth,
int inputHeight,
AVPixelFormat inputFormat,
AVRational inputAspectRatio,
int outputWidth,
int outputHeight,
AVPixelFormat outputFormat,
const std::string& filtergraphStr,
AVRational timeBase);

bool operator==(const FiltersContext&) const;
bool operator!=(const FiltersContext&) const;
};
Expand Down
Loading