On Thu, Dec 11, 2014 at 11:12:25AM +0100, Martijn van Beurden wrote:> Op 11-12-14 om 10:53 schreef Martijn van Beurden: > > Op 11-12-14 om 10:05 schreef Miroslav Lichvar: > >> but I'd rather see the real seeking bug fixed instead > > > > I think I might have a fix [...]So the problem is that FLAC__stream_decoder_process_single returns error before it finds a valid frame?> Another solution might be to 'just try again' somewhere else > when seeking fails, but maybe there are good reasons not to do > so? The decoder might get stuck in a loop?I think that would be a reasonable solution. In one iteration of the root-finding algorithm, don't give up when decoding fails, but try also a limited number of different positions (say 10) dividing the interval between the lower and upper bound evenly. Does that make sense? -- Miroslav Lichvar
2014-12-11 14:34 GMT+01:00 Miroslav Lichvar <mlichvar at redhat.com>:> > On Thu, Dec 11, 2014 at 11:12:25AM +0100, Martijn van Beurden wrote: > > Op 11-12-14 om 10:53 schreef Martijn van Beurden: > > > Op 11-12-14 om 10:05 schreef Miroslav Lichvar: > > >> but I'd rather see the real seeking bug fixed instead > > > > > > I think I might have a fix [...] > > So the problem is that FLAC__stream_decoder_process_single returns > error before it finds a valid frame? >I'm not sure whether we mean the same thing, but I think the problem is that seek_to_absolute_sample_ calls FLAC__stream_decoder_process_single, which calls read_frame_, which calls read_subframe_, which calls either read_subframe_fixed_ or read_subframe_lpc_, which call read_residual_partitioned_rice_. The return false set there is propagated all the way down. So, because the decoding of the frame is aborted upon finding a situation in which a heap overflow might be in order (but which will usually just be a bogus header), the decoder considers the seeking proces failed. This is only a problem with seeking and not with normal decoding, because in a valid stream, the decoder doesn't have to look for a header (so it doesn't have the chance to run into a fake one) because the next header is always right after the previously decoded frame.> > > Another solution might be to 'just try again' somewhere else > > when seeking fails, but maybe there are good reasons not to do > > so? The decoder might get stuck in a loop? > > I think that would be a reasonable solution. > > In one iteration of the root-finding algorithm, don't give up when > decoding fails, but try also a limited number of different positions > (say 10) dividing the interval between the lower and upper bound > evenly. Does that make sense?Yes, I think it does. -------------- next part -------------- An HTML attachment was scrubbed... URL: http://lists.xiph.org/pipermail/flac-dev/attachments/20141211/2e7cf2d5/attachment.htm
On Thu, Dec 11, 2014 at 04:50:24PM +0100, Martijn van Beurden wrote:> 2014-12-11 14:34 GMT+01:00 Miroslav Lichvar <mlichvar at redhat.com>: > > So the problem is that FLAC__stream_decoder_process_single returns > > error before it finds a valid frame? > > > > I'm not sure whether we mean the same thing, but I think the problem is > that seek_to_absolute_sample_ calls FLAC__stream_decoder_process_single, > which calls read_frame_, which calls read_subframe_, which calls either > read_subframe_fixed_ or read_subframe_lpc_, which call > read_residual_partitioned_rice_. The return false set there is propagated > all the way down.After few hours of a script seeking randomly in my FLAC library, I have now a reproducer for the problem. It seems to be as you say, the false value propagates through all layers and the seeking process stops. So we need to return true as a non-fatal error, but stop any further decoding on the invalid frame. I think the best fix would be to move the check for invalid predictor or partition order one layer up to the subframe decoding functions. I'll send a patch for review shortly. -- Miroslav Lichvar