search

pytorch / torchcodec

PyTorch video decoding
BSD 3-Clause "New" or "Revised" License
77 stars 9 forks source link

Make it clear that the device interface is only for non-CPU devices. This makes the code simpler to reason about #244

Closed ahmadsharif1 closed 1 month ago

ahmadsharif1 commented 1 month ago

This PR makes it clear that the device interface is only for non-CPU devices.

It should only be called from within an if block like this:

if (device.type() != torch::kCPU) {
  deviceFunction();
}

CPU code is already contained in VideoDecoder.cpp. So we can directly execute that code for CPU without the control flow going from VideoDecoder.cpp to CPUOnlyDevice.cpp back to VideoDecoder.cpp. That makes the code easier to reason about.

The CPUOnlyDevice.cpp file will throw errors for every function because it is only linked in for CPU-only versions of torchcodec and CPU-only version does not support any non-CPU device.

The main advantage of this structure is that for CPU code, we will remain within VideoDecoder.cpp.

For CUDA we will go from VideoDecoder.cpp to CudaDevice.cpp.

Before this change, for device="cpu" the code would take this path:

VideoDecoder.cpp -> CPUOnlyDevice.cpp -> VideoDecoder.cpp (for the actual color-conversion, etc.)

After this change, for device="cpu" we remain within VideoDecoder.cpp. We only go outside for non-CPU devices.

NicolasHug commented 1 month ago

CPU-only version does not support any device

Just to make sure I understand: this seems OK at the C++ level, but for Python we will still allow VideoDecoder(..., device="cpu"), including for the cpu-only version of torchcodec. Right?

ahmadsharif1 commented 1 month ago

Just to make sure I understand: this seems OK at the C++ level, but for Python we will still allow VideoDecoder(..., device="cpu"), including for the cpu-only version of torchcodec. Right?

Yes, this doesn't change any python interface.

The main advantage of this structure is that for CPU code, we will remain within VideoDecoder.cpp.

For CUDA we will go from VideoDecoder.cpp to CudaDevice.cpp.

Before this change, for CPU we would go from VideoDecoder.cpp to CPUOnlyDevice.cpp and go back to VideoDecoder.cpp for the actual CPU implementation. This PR avoids that round trip for CPU-device and avoids calling into CPUOnlyDevice.cpp for that code.