Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite Loop in GetAudio for certain files #423

Closed
arch1t3cht opened this issue Aug 4, 2023 · 4 comments
Closed

Infinite Loop in GetAudio for certain files #423

arch1t3cht opened this issue Aug 4, 2023 · 4 comments

Comments

@arch1t3cht
Copy link
Contributor

Hello - I was sent an audio file which makes FFMS2 lock up when reading the entire audio from start to finish.

Audio file

The file is here: https://www.dropbox.com/scl/fi/wljguppohkwgexiq50kqx/out.m4a?rlkey=lem4n70q5jqq5dgvkke9i8s79&dl=0
It seems like this file was produced by concatenating two audio files with ffmpeg, which definitely fits the analysis below.

Steps to reproduce

Open this audio file in FFMS2 and repeatedly call FFMS_GetAudio to read the audio from the beginning to the end. Eventually, FFMS2 will freeze. Specifically, this was reproduced in Aegisub, where opening that audio file (while having the Aegisub's Audio Cache Type set to either RAM or HDD) would work, but closing the file would make Aegisub freeze.

Analysis

Debugging shows that the GetAudio function enters an infinite loop,

while (Count > 0) {
// Find first useful cache block
while (it != Cache.end() && it->Start + it->Samples <= Start) ++it;
// Cache has the next block we want
if (it != Cache.end() && it->Start <= Start) {
int64_t SrcOffset = FFMAX(0, Start - it->Start);
int64_t DstOffset = FFMAX(0, it->Start - Start);
int64_t CopySamples = FFMIN(it->Samples - SrcOffset, Count - DstOffset);
size_t Bytes = static_cast<size_t>(CopySamples * BytesPerSample);
memcpy(Dst + DstOffset * BytesPerSample, it->Data.get() + SrcOffset * BytesPerSample, Bytes);
Start += CopySamples;
Count -= CopySamples;
Dst += Bytes;
++it;
}
// Decode another block
else {
if (Start < CurrentSample && SeekOffset == -1)
throw FFMS_Exception(FFMS_ERROR_SEEKING, FFMS_ERROR_CODEC, "Audio stream is not seekable");
if (SeekOffset >= 0 && (Start < CurrentSample || Start > CurrentSample + DecodeFrame->nb_samples * 5)) {
FrameInfo f;
f.SampleStart = Start;
size_t NewPacketNumber = std::distance(
Frames.begin(),
std::lower_bound(Frames.begin(), Frames.end(), f, SampleStartComp));
NewPacketNumber = NewPacketNumber > static_cast<size_t>(SeekOffset + 15)
? NewPacketNumber - SeekOffset - 15
: 0;
while (NewPacketNumber > 0 && !Frames[NewPacketNumber].KeyFrame) --NewPacketNumber;
// Only seek forward if it'll actually result in moving forward
if (Start < CurrentSample || static_cast<size_t>(NewPacketNumber) > PacketNumber) {
PacketNumber = NewPacketNumber;
CurrentSample = -1;
av_frame_unref(DecodeFrame);
avcodec_flush_buffers(CodecContext);
Seek();
}
}
// Decode until we hit the block we want
if (PacketNumber >= Frames.size())
throw FFMS_Exception(FFMS_ERROR_SEEKING, FFMS_ERROR_CODEC, "Seeking is severely broken");
while (CurrentSample + CurrentFrame->SampleCount <= Start && PacketNumber < Frames.size())
DecodeNextBlock(&it);
if (CurrentSample > Start)
throw FFMS_Exception(FFMS_ERROR_SEEKING, FFMS_ERROR_CODEC, "Seeking is severely broken");
// The block we want is now in the cache immediately before it
--it;
}

which is caused by the previous call to DecodeNextBlock getting AVERROR(EAGAIN) from avcodec_receive_frame.

// ReadPacket may have changed the packet number
CurrentFrame = &Frames[PacketNumber];
CurrentSample = CurrentFrame->SampleStart;
int NumberOfSamples = 0;
AudioBlock *CachedBlock = nullptr;
int Ret = avcodec_send_packet(CodecContext, Packet);
av_packet_unref(Packet);
av_packet_free(&Packet);
av_frame_unref(DecodeFrame);
Ret = avcodec_receive_frame(CodecContext, DecodeFrame);
if (Ret == 0) {
//FIXME, is DecodeFrame->nb_samples > 0 always true for decoded frames? I can't be bothered to find out
NumberOfSamples += DecodeFrame->nb_samples;
if (DecodeFrame->nb_samples > 0) {
if (pos)
CachedBlock = CacheBlock(*pos);
}
}
// Zero sample packets aren't included in the index
if (!NumberOfSamples)
return NumberOfSamples;

This happens somewhere in the middle of the audio file, likely at the point where the two files were concatenated. With this, CurrentFrame is already overwritten, but since avcodec_receive_frame didn't return data yet, no audio frame was added to the cache. Since the condition in line 427 assumes that there's a suitable audio frame in the cache, DecodeNextBlock isn't called any more and the function loops infinitely.

Fixes

A simple band-aid solution would be to check the return value of DecodeNextBlock in line 428 and throw an error if it's zero. Properly fixing this would mean sending more data to avcodec until avcodec_receive_frame works again, but I don't know enough about the code base and the packet counting involved to want to attempt that, if it's even possible.

Finally, a side note... it'd be really great to have a new release of ffms2 sometime soon. Linux distributions still have release 2.40, which still has the seeking regression in #394 , which causes seeking errors in lots of videos. New Aegisub users on Linux or Mac run into this a lot, for example.

@myrsloik
Copy link
Contributor

Give the latest commit a try

@myrsloik myrsloik reopened this Sep 29, 2023
@arch1t3cht
Copy link
Contributor Author

Still happens, unfortunately, at the same timestamp as it did before the rewrite.

@arch1t3cht
Copy link
Contributor Author

After a closer look, it doesn't happen on 16a06e7 but was reintroduced by a624594 .

@myrsloik
Copy link
Contributor

Guess I'll simply revert the last commit then. Great success!

qyot27 pushed a commit to qyot27/ffms2_cplugin that referenced this issue Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants