#2074 Fix dropped frames at the end of encoders that buffer more than one packet.

[joshpkware]

Fix for #2074. Add a retry mechanism. The test that was modified was asserting incorrectly, since the expected amerge behavior is to add the channels to the file if both the input files are stereo.

[saudet]

Thanks! Why wasn't the test failing 🤔

[joshpkware]

I believe I've identified the root cause of the infinite loop issue. While investigating, I found that the assertions were never reached because no audio frames were being received. I confirmed this by using the FFmpeg CLI, which showed that the audio stream was empty. This seemed unusual, so I looked into the recordSamples method in the FFmpegFrameRecorder class.

Upon inspection, I noticed an issue on line 1293. If the swr_convert call returns 0 samples, the code breaks out of the loop. However, based on my testing, this behavior is incorrect. The swr_convert function can sometimes return 0 when it doesn't yet have enough input samples to process, but that doesn't necessarily mean the conversion is complete.

To address this, I updated the condition. Instead of breaking out of the loop whenever swr_convert returns an out count of 0, I added a check to ensure inputCount is also 0. Here's why:

• If you call swr_convert with a non-zero inputCount and it returns 0, it simply means it doesn't yet have enough input to produce output samples.
• If you call swr_convert with inputCount set to 0, you're explicitly asking it to flush remaining samples.

Therefore, you should continue looping until swr_convert consistently returns an out count of 0 while inputCount is also 0, indicating there are no more samples to process or flush. The updated code looks like this:

if ((ret = swr_convert(samples_convert_ctx, plane_ptr2, outputCount, plane_ptr, inputCount)) < 0) {
    throw new Exception("swr_convert() error " + ret + ": Cannot convert audio samples.");
} else if (ret == 0 && inputCount == 0) { // Added inputCount == 0 condition
    break;
}

With this change, I was able to remove the check for a null frame entirely within the encoding loop. As a result, Vorbis no longer triggered an infinite loop with that test. The infinite loop occurs when no valid frames were encoded before flushing. In such cases, avcodec_receive_packet always returns 0 but logs the following error:

Warning: [vorbis @ 0x10301bcf0] Trying to remove 1024 samples, but the queue is empty.

To handle cases when we flush before encoding, I think we have two options:

1. Retain the current workaround of retries.
2. Implement a check to see if any valid frames have been sent before breaking out of the loop early.

Either solution prevents the infinite loop, but option 2 could be a cleaner and more efficient approach. Let me know what you think!

[saudet]

Looks good, thanks!

Either solution prevents the infinite loop, but option 2 could be a cleaner and more efficient approach. Let me know what you think!

If you believe you have a good fix for this, by all means, please do!

[joshpkware]

Here's what I came up with. Essentially, I set a flag to true whenever we write any samples. While we're encoding the flushed samples, if we haven't written anything yet, we exit the loop after the first iteration. Alternatively, we could just return early from the method if we prefer.

During testing, I found that trying to write a very small number of samples could also trigger the infinite loop. It turns out the swr_context wasn't being flushed properly, so any samples left in the buffer at the end of the file were dropped. If the file was small enough, no samples were ever written, causing the infinite loop.

To fix this, I changed the logic to prevent breaking out of the loop immediately when samples is null in the recordSamples method. Now, the samples_out array is flushed at the end of that method.

[saudet]

This looks alright I think. Anything else you'd like to fix before we merge this though?

[joshpkware]

Nope I think this is good for now! I may put in a different PR later for centralizing some of the flushing concepts, but that'll require a little more restructuring which I think is out of scope for this PR.