-
Notifications
You must be signed in to change notification settings - Fork 278
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
Handling of corrupted frames in libFLAC #464
Comments
Yes, this is correct. If you rewind to the last seen framesync you get an infinite loop as you noticed. Previously, in FLAC 1.3.4 and before, libFLAC would start looking for a new frame directly after detecting an error. However, this could result in it skipping a valid frame as well, if it overread the previous frame. That's why it rewinds to just after the start of the corrupt frame for a valid one.
Yes, perhaps the error isn't really clear. It could be replaced with
I agree that could be improved.
I'm not sure this is a good idea. FLAC is a lossless codec, and error concealment does not fit with that. In case of an error I'd like it to be loud and clear. Furthermore, here the stream contains samples that do not fit within the bit per sample of the stream. If further processing is done on these, that could result in undefined behaviour. |
Oh, and by the way, thanks for reporting! |
Completely understandable. Thanks for the clarification.
Maybe, would be nice to know what exactly happens without having to debug for that specific case. I understand the hesitation about adding a new error to the enum, but currently it is a bit difficult to know what the error is (in this scenario). Anyway, this is a really strange scenario, so I'm happy to change the error by the one suggested
I know, that's why I suggested to add this behind a flag in the decoder. Client must explicitly ask for this. For what I can see in code, if the frame is correct, it should go straight to this block of code flac/src/libFLAC/stream_decoder.c Line 2220 in a2be4d2
For the mentioned case: flac/src/libFLAC/stream_decoder.c Line 3189 in a2be4d2
It seems here that if the bits per sample defined originally does not match the one in the frame (and frame's is smaller), then it adds that difference into the samples (?). The comment says On the other hand, it does not seem it should affect the decoder state (I might be wrong here!). The frame has been already decoded, and the operations are on the raw samples, sum that with the error sent to client, it should know that this frame has something wrong. I would really have that option, but if that is not acceptable, I will understand.
Thanks for the fast answer!! |
If such a thing is added, an interface would need to be provided that gives the client as much information as possible: what happened and which sample is the problem (if that is known). Otherwise you won't know what happened anyway.
That code is used when seeking. It throws out
I'll keep this issue open, but I won't make any promises. |
Could be a good idea, in terms of error handling for such scenario. If it is possible to add this new interface to the decoder, would be nice.
Understood, thanks for explaining to me!!
Understandable. I will keep an eye of this issue, and if any help is required, I will be glad to. One thing I just though, should I create a separated issue for this?
|
No, I don't think it is necessary to open another issue. FLAC decoding is already extremely fast, so it doesn't seem pressing in any way. |
Just as a note to self: to improve troubleshooting, maybe it is a good idea to add extra FLAC__Frame subframe types. Maybe something like FLAC__SUBFRAME_TYPE_OOB (out-of-bounds) and FLAC__SUBFRAME_TYPE_FILLER (for filling gaps between two frames when the frame in between was unreadable/unparsable). Then we have a simple way to pass data, so the client can do with it whatever it wants. |
So, I've just looked into this again. I'll probably add two new error statusses: To do this, I'll need some input to test with. These out-of-bounds inputs aren't easy to create. Do you perhaps have a short sample? |
Hi there! I do not have a shorter example. But I think I managed to shorten the original file keeping the out-of-bounds using |
Hello flac team!! A user reopened an issue on my repo (a node bindings to
libFLAC
) pointing out that there is like half a second of silence when decoding certain flac file (included in the issue).libFLAC
version in use are1.3.4
(user's) and1.4.0
/1.4.1
(mine).First investigation
The first thing I do is to check the file and effectively it has some kind of corruption in it.
ffmpeg
can decode the file fine without complains, butlibFLAC
inserts some silence just in the corrupted frame. Doing an analysis withflac -a ...
to that file I get the following errors:Debugging code!
So I setup my repo to debug some C/C++ code and run a test file to decode the file using the stream decoder using a file. Put a breakpoint here https://github.com/xiph/flac/blob/master/src/libFLAC/stream_decoder.c#L2118 and wait to trigger the error. And it hits twice (one per channel). The two hits of the breakpoint gives me the following values:
channel
i
decoder->private_->output[channel][i]
NOTE: did not check
read_subframe_
, it is dense and I suppose it is correct.After this, it goes into here
flac/src/libFLAC/stream_decoder.c
Line 2192 in b6fbd6b
decoder->private_->last_seen_framesync
is8392003
but the start of the frame is at8392001
. From the output of the analysis command (the corrupted frame):Not sure if the
last_seen_framesync
should be two bytes after the start of the frame, but in case this is wrong this line might be the issue (reads the position after reading two bytes of the frame):flac/src/libFLAC/stream_decoder.c
Line 2014 in b6fbd6b
Okey then, we've got a corrupted frame, the decoder then tries to rewind but it does not goes exactly at the start of the frame. Then when trying to find a frame from the
FLAC__stream_decoder_process_single
, it gets lost. When finds the next frame, it inserts silence for the whole corrupted frame (as stated here).Trying something...
If I try to "fix" the
last_seen_framesync
by pointing at the start of the frame, theFLAC__stream_decoder_process_single
will get stuck in an infinite loop because:And it seems there is no way to stop the loop and tell the frame is corrupted (apart from the
FLAC__STREAM_DECODER_ERROR_STATUS_FRAME_CRC_MISMATCH
error).Recap
last_seen_framesync
might be pointing at +2 bytes offset from the start of the framelast_seen_framesync
points to the start of the frame, an infinite loop is causedMy opinion
The first thing I see is because there is some corrupted data (samples outside the bits per sample range), the decoder emits
FLAC__STREAM_DECODER_ERROR_STATUS_FRAME_CRC_MISMATCH
. I think this error is a bit confusing and it does not exactly reflect the real of the error. Or I just get this completely wrong. The description of the error says The frame's data did not match the CRC in the footer. but in this case that's not what happens. CRCs match but the data is not valid for the declared bits per sample.Next thing is the
FLAC__STREAM_DECODER_ERROR_STATUS_FRAME_CRC_MISMATCH
error (or whatever error is sent in that scenario). In my opinion, should be sent only once. It is not necessary, and maybe reduces some CPU time by stopping just when the error is detected - and not checking the rest of the channels.And last, as a developer, I would like to get the corrupted frame anyway (either by this case or a real CRC mismatch) and do whatever I would like to do with that. Like maybe try to fix somehow the samples or whatever. I already know there is something wrong with that frame (because of the
FLAC__STREAM_DECODER_ERROR_STATUS_FRAME_CRC_MISMATCH
). Maybe as an optional flag that could be set to send the frame instead of inserting silence... With aFLAC__stream_decoder_set_XXX()
...If there is anything not clear or need more information, I would be glad to help you. As well as I am open to discuss everything I put in this issue :)
The text was updated successfully, but these errors were encountered: