We weren't callingav_frame_clone() here before. Was that an error?

No, that was not a mistake. The function has changed. Previously it combined 2 operations: 1) conversion of the frame with filtergraph, 2) creating a tensor. The frame converted by filtergraph lived locally insideUniqueAVFrame and we just released it in the end to the tensordeleter. I, the schema was:

torch::Tensor func(const UniqueAVFrame& input) {   UniqueAVFrame output = filtergraph->convert(input);   AVFrame* outputPtr = output.release();   return torch::from_blob(output->data, deleter(outputPtr), ...);}

In the new code I moved frame conversion with filtergraph outside of the function. And function signature has changed - it started to accept just a constant reference to a frame without knowing whether it will be further needed or not. But we still need to pass a reference to thedeleter to make sure that tensor will unreference it once it's not needed. To achieve that we clone the frame which references the same data and pass it to the deleter. In this way caller of the function can still do something with the frame if needed, or just unreference it (what we actually are doing). So, schema now is:

torch::Tensor func(const UniqueAVFrame& frame) {   AVFrame* frameClone = av_frame_clone(frame.get());   return torch::from_blob(frameClone->data, deleter(frameClone), ...);}

I don't love that we're doing FFmpeg versions checks here, but we also do this in several other places inCudaDeviceInterface.cpp. In the rest of the C++ code, we've kept such checks hidden behind functions inFFMPEGCommon. Most of those, however, are about a particular field of a struct being renamed. This logic is very specific to filtergraph, and it does make sense to keep that here.

There's no action to take based on this comment, I'm just pointing out it's an awkward situation. I might end up refactoring it in some of my decoder-native transform work.

@dvrogozh QQ - do we expect frames to come out asAV_PIX_FMT_RGB24? I'm a bit surprised that this would be the case, I'd assume most encoded frames are in YUV or something similar?

This change here in CPU device interface is due to CPU fallback in CUDA device interface to handle ffmpeg-4.4 10-bit streams:
https://github.com/dvrogozh/torchcodec/blob/f298fa7e722e9e0932813545e819e7da1a31994d/src/torchcodec/_core/CudaDeviceInterface.cpp#L242-L247
The filter graph gets setup in CUDA device interface which outputs RGB24, then the output needs to be converted to tensor. Which happens here in CPU device interface.

Above being said, I think there might be 2 cases in the future where we might see RGB24 coming out of decoders:

1. Images support and "image" codecs such as motion jpeg
2. HW decoders sometimes blend decoding and color space/scaling into single pass for the optimization purposes.