Refactor logic to close AVIOContext

[Dan-Flores]

This PR consolidates the logic used to flush and close AVIOContext, and should fix the flaky test bug described in #724.

The issue is related to the member variables avFormatContext_ and avioContextHolder_ pointing to the same AVIOContext object, and the logic of close_avio.

torchcodec/src/torchcodec/_core/Encoder.cpp

Line 185 in 50686bb

avFormatContext_->pb = avioContextHolder_->getAVIOContext();

In close_avio, if avioContextHolder_ is not defined (if using to_file), the AVIOContext pointer is correctly set to nullptr, preventing double frees in the member variables destructors. Otherwise, it will be destroyed in the member variable destructors. In those destructors, both will attempt to free the same address, as it is never set to a nullptr:

torchcodec/src/torchcodec/_core/AVIOContextHolder.cpp

Lines 50 to 54 in 50686bb

	AVIOContextHolder::~AVIOContextHolder() {
	if (avioContext_) {
	av_freep(&avioContext_->buffer);
	}
	}

torchcodec/src/torchcodec/_core/FFMPEGCommon.h

Lines 68 to 70 in 50686bb

	using UniqueEncodingAVFormatContext = std::unique_ptr<
	AVFormatContext,
	Deleter<AVFormatContext, void, avformat_free_context>>;

The updated closeAVIOContext function avoids this by

• Returning early if avFormatContext_ or avFormatContext_->pb are set to nullptr (so it is safe to call multiple times).
• Calls avio_close on the AVIOContext pointer if avioContextHolder_ is not defined (when using to_file). Otherwise, the reference in avFormatContext_ will be set to nullptr.
  • During avioContextHolder's destructor, the AVIOContext will be freed (only once!).

[NicolasHug]

Thanks @Dan-Flores , let's try this. The refactor is useful regardless of the logic change!

[NicolasHug]

Can you check my understanding that the main logic difference is that we now set avFormatContext->pb = nullptr; after the if (!avioContextHolder) { ...} block, and not inside it?

[Dan-Flores]

Yes exactly, after and outside of the condition.