Fix Windows build and add Windows CPU tests

[NicolasHug]

This PR fixes the Windows wheel build, and adds tests jobs that mimic the setup we have for other platforms: we build a wheel, and then install and run the tests from that wheel on the separate machine.

The vast majority of tests are passing as-is, without any modification needed. Worth nothing that for comparing frames, we are going through the same path as on OSX, with higher tolerances:

torchcodec/test/utils.py

Lines 82 to 83 in 05a6ff5

	else:
	torch.testing.assert_close(*args, **kwargs, atol=3, rtol=0)

I had a few problems with the audio encoder though. in increasing order of importance:

• We can't always validate parameters, so we (correctly) error later in avcodec_open2(). This is described in this comment: Fix Windows build and add Windows CPU tests #806 (comment)
• Encoding mp3 files seems to be buggy. We can encode, but we're getting weird errors when decoding what we just encoded. I don't think it's a decoding issue though, because the decoder tests are using mp3 files just fine. I have opened Encoding mp3 on Windows is probably incorrect #837 to follow-up.
• We are getting segfaults and memory access errors with Fffmpeg 5 (and only with ffmpeg 5!!). I opened FFmpeg 5 on Windows errors in AudioEncoder's avcodec_open2 when passing bad parameters #836 to follow-up. You'll see more details there but the segfault happens internally within avcodec_open2(), even after validating that the parameters are valid pointers. Because it only happens on Windows and on FFmpeg5, I want to declare that this is an ffmpeg5 bug.

Note: we're not errorring on warnings on Windows, and there are a lot of those. I'll re-enable this as a follow-up:

torchcodec/src/torchcodec/_core/CMakeLists.txt

Lines 14 to 17 in 05a6ff5

	if (WIN32)
	# TODO set warnings as errors on Windows as well.
	# set(TORCHCODEC_WERROR_OPTION "/WX")
	else()

Massive thanks to @traversaro for unblocking us with the #806 (comment) fix.

[NicolasHug]

Above is a drive-by as I was trying to fix the problem with FFmpeg 5. I'm not sure it fixed anything, but it is probably still a good change to have? I can extract it out in another PR if it's preferred.

[Dan-Flores]

Does this code imply that if the AVIOContext stored in avFormatContext_ has an error, we do not need to flush / close it?

[scotts]

Based on my digging, it's also that it's risky to call flush when there's an error state. Seems reasonable to keep it in.

[NicolasHug]

yeah, it because when error isn't 0 then flushing or calling avio_close() are potentially accessing invalid data.

[NicolasHug]

above and in other tests below: on Windows, we're hitting this early return:

torchcodec/src/torchcodec/_core/Encoder.cpp

Lines 35 to 38 in 05a6ff5

	void validateSampleRate(const AVCodec& avCodec, int sampleRate) {
	if (avCodec.supported_samplerates == nullptr) {
	return;
	}

So we're unable to validate some parameters early, and we just fail in the call to avcodec_open2() later:

torchcodec/src/torchcodec/_core/Encoder.cpp

Lines 213 to 217 in 05a6ff5

	int status = avcodec_open2(avCodecContext_.get(), avCodec, nullptr);
	TORCH_CHECK(
	status == AVSUCCESS,
	"avcodec_open2 failed: ",
	getFFMPEGErrorStringFromErrorCode(status));

[NicolasHug]

@traversaro correctly pointed out that this can now be removed, as a consequence of the other cmake_build_type fix below #806 (comment)

[NicolasHug]

Above is the main fix that was needed for the extensions to load properly, something I had been blocked on for a few weeks. The fix is from @traversaro :

NicolasHug#1

IIUC, on windows and with our current setup, cmake is generating multiple build files. And without this fix, the different build files would be inheriting different build configuration. Typically we'd be building some parts with the Release build type, while other would be inheriting a different build type, causing problems at runtime when loading the libraries.

@traversaro thank you so much again for unblocking us!

[NicolasHug]

Will revert to just 3.9 before merging, like in the other jobs. This is just to show that the jobs are green across multiple versions

Also, you probably don't need to review the rest of this file too closely, because it's mostly copy/pasted from our existing test job (for e.g. Linux)

[NicolasHug]

Will remove this before merging. This is just to show that the jobs are green across versions.

Note that we may still try to address the 3.13t issue, but that's for a follow-up.

[Dan-Flores]

Will this condition be hit? It might be duplicate with the check and pytest.skip on line 260

[NicolasHug]

The check above only skips on FFmpeg <= 5, but FFmpeg 6 and 7 will still reach this line :)

[Dan-Flores]

Same question here, is this a duplicate check with line 319? On a similar note, it might be better to explicitly call pytest.skip whenever we are not running a test.

[NicolasHug]

It's not a duplicate because this will be reached by FFmpeg 6 and 7. You're right that we should generally prefer using pytest.skip to just return, or to doing what we're doing here: it's better to know that a test was skipped, rather than seeing it be green when in fact it never ran.

In this specific case I think it's best not to call pytest.skip, because the test is still doing something meaningful just above. Specifically it calls the encoding methods, without error. We are only skipping the assert_close() check.

[Dan-Flores]

Does this change enable the test to pass on Windows? If so, we could parameterize the format and add an IS_WINDOWS check to skip if IS_WINDOWS and format == "mp3", as in tests above.

[NicolasHug]

Correct, I changed from mp3 to flac due to the issues we had on Windows.

I agree that we could parameterize, but I'm not sure it's necessary for the purpose of this test, as it wouldn't make it more robust. Mostly this test is just about checking the number of output channels is respected, which is agnostic to the format.