[FFmpeg-cvslog] r11379 - trunk/libavcodec/ac3dec.c

[Michael Niedermayer]

On Wed, Jan 02, 2008 at 11:52:19PM -0500, Justin Ruggles wrote:
> Rich Felker wrote:
> > On Thu, Jan 03, 2008 at 03:26:30AM +0100, jbr wrote:
> >> Author: jbr
> >> Date: Thu Jan  3 03:26:29 2008
> >> New Revision: 11379
> >>
> >> Log:
> >> add crc check to ac3 decoder
> > 
> > What is the performance impact?? I'm strongly against this change,
> > especially the aspect of outputting _nothing_ (and thus breaking a/v
> > sync!) when crc check fails. If crc check fails, the decoder should
> > instead use error concealment heuristics, and crc should not be
> > checked at all when error concealment is disabled.
> 
> I would definitely rather output audio (either error concealment or
> silence) to keep a/v sync.  But the only reliable way to know if the
> frame size and/or number of channels are correct (and both can change
> mid-stream) is to also check the first CRC code.  And if that fails,
> there is no way to know.
> 
> If other people think it's worth the risk, I can change it.

I really hate it that every time audio error concealment comes up the
awnser is something like "it cant be done" it really is almost never true.
In E-AC3 error concealment is a mandatory part of the spec, not doing
it means your decoder is not compliant.

About the number of channels, if the crc matches and there are no other
obvious errors then you can assume that the channels as stored are correct.
Otherwise ignoring a change in the number of channels seems reasonable.
Its more likely that a mismatching crc and new channel number is a damaged
channel number than a correct one. This is independant of what is otherwise
done with the frame. (though trying to decode a frame with a damaged header
seems like a waste of time ...)

About the number of bytes. One way, though maybe not the best nor only way
to handle it is in the parser:
1. if theres a matching sync word after the frame size the size can be
   considered correct
2. if the crc is matching the size can be considered correct
3. if theres a matching sync word after the frame size (using the size of the
   last frame here) that size is likely correct
4. you have to search for the next sync word to find the frame size, if
   the distance exceeds the max frame size output a frame of the same size
   as the last valid one and try again. If the distance is an exact multiple
   of the last valid frame size then the assumtation of several constant
   sized frames is reasonable and the parser should output such frames.

Always check the crc, or not? id say use AVCodecContext.error_resilience
to decide.

Then it seems ac3_decode_frame() doesnt check if the bit reader overread the
available data or if theres unused data left at the end.

Now what should be done if any error is detected? The damaged part (this
seems to be the whole frame if i understand AC3 correctly) has to be
concealed, to quote the spec:
----
    If the CRC word for an Enhanced AC-3 bit stream is found to be invalid, all blocks in the
frame must be substituted with an appropriate error concealment signal. For most applications,
this can be easily accomplished by simply repeating the last known-good block (before the
overlap-add window process).
----
same can be done with non "E" AC3



> 
> Personally, the only reason I want the CRC check is for debugging, so I
> don't have a problem with only running the check when the log level is
> >= AV_LOG_DEBUG or even putting it in an #ifdef so I can turn it on
> whenever someone reports a sample as buggy which turns out just to be
> damaged.
> 
> As for performance, I don't really know enough about dezicycles to make
> a judgement, but I did a START_TIMER/STOP_TIMER on ac3_decode_frame()

dezicycles == 1/10 cpu cycles, so on a 1ghz cpu 10000000 dezicycles are
1 milli second

PS: the cleanup work you do on E-AC3 is great!

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20080103/fba6098a/attachment.pgp>