[FFmpeg-devel] [PATCH] fixes in mpeg audio parsing

[Yoav Steinberg]

Michael Niedermayer wrote:
>> Index: mpegaudio_parser.c
>> ===================================================================
>> --- mpegaudio_parser.c	(revision 15776)
>> +++ mpegaudio_parser.c	(working copy)
>> @@ -24,15 +24,46 @@
>>  #include "mpegaudio.h"
>>  #include "mpegaudiodecheader.h"
>>  
>> +#define HEADERS_REQUIRED_FOR_SYNC 3
>> +#define MAX_Q_SIZE 4
>> +#if (MAX_Q_SIZE < HEADERS_REQUIRED_FOR_SYNC) || (((MAX_Q_SIZE-1) & MAX_Q_SIZE) != 0)
>> +#error Header queue size too small or not power of 2
>> +#endif
>> +#define ENQ_HDR(ctx,pktptr,pktsize) do { \
>> +    (ctx)->hdr_q_size++; \
>> +    (ctx)->hdr_q_last = ((ctx)->hdr_q_last + 1) & (MAX_Q_SIZE-1); \
>> +    (ctx)->hdr_q[(ctx)->hdr_q_last].ptr = (pktptr); \
>> +    (ctx)->hdr_q[(ctx)->hdr_q_last].size = (pktsize); \
>> +    } while (0)
>> +#define DEQ_HDR(ctx,pktptr,pktsize) do { \
>> +    int idx = ((ctx)->hdr_q_last - ((ctx)->hdr_q_size - 1)) & (MAX_Q_SIZE-1); \
>> +    pktptr = (ctx)->hdr_q[idx].ptr; \
>> +    pktsize = (ctx)->hdr_q[idx].size; \
>> +    if (--(ctx)->hdr_q_size == 0) \
>> +        CLRQ(ctx); \
>> +    } while (0)
>> +#define CLRQ(ctx) do { \
>> +    (ctx)->hdr_q_size = 0; \
>> +    (ctx)->inbuf_ptr = (ctx)->inbuf + ((ctx)->inbuf_ptr - (ctx)->inbuf_pkt_end); \
>> +    (ctx)->inbuf_pkt_end = (ctx)->inbuf; \
>> +    } while (0)
>> +#define PKT_READY_IN_Q(ctx) ((ctx)->hdr_q_size > 1 || ((ctx)->hdr_q_size == 1 && !(ctx)->reading_frame_data))
> 
> ...
> 
> this (and the rest of the patch) looks scarily messy
> also this patch is probably >10 times bigger than needed. And such big redesign
> requires a lengthy justification. besides the code might benefit from using
> ff_combine_frame(), though changing the parser to use ff_combine_frame() has
> to be seperate from any intended changes to its "behavior".

The patch might look messy because I basically replaced the existing 
mpegaudio_parse() so the diff came out messy. In any case I used the 
existing logic for finding the frames in the stream, checking for their 
validity and consistency, and conserving the same exit conditions from 
the function.
I added a "header queue" to keep track of where packets are stored in 
the inbuf, and to easily clear the inbuf or return a valid packet from 
the queue when when needed (when header_count is reset or when it 
reaches a minimum valid count, respectively).
I changed the loop structure to support dequeuing and returning old 
frames if they exist in the queue.
I changes the parsing state structure to contain a queue of positions in 
inbuf where previous packets have been stored. I also created a new 
state variable indicating if we're in the midst of reading frame data 
(which was basically a side functionality of the frame_size state var).
For the sake of clarity I added comments, changed the header_count state 
var to have a positive minimal boundary (instead of obscure negative int 
logic), and used macros and defines for the queue handling and constants 
(previously duplicated hardcoded numbers).
Also I made sure the existing optimization hack of not copying the frame 
to inbuf if its contained in the input buffer was preserved. And used 
the existing inbuf to store the packets to avoid additional memcpy's.

Regarding ff_combine_frame(), I'm not sure I understand its 
functionality, and I'm not sure it'll provide any assistance when 
tracking multiple "future" packets as required here. Additionally a 
quick look at ff_combine_frame() hits to uses of additional memcpy's and 
allocation which aren't really required here.

> 
> Also thinking about this, the parser should not delay or drop the packets
> at all, it should just delay setting the sample_rate/other values. And the
> decoder&ffmpeg  should be fixed to do something reasonable with the first
> damaged packet(s)
> 

This is more of a design issue. I assume that it's the parsers role to 
provide valid frames to the decoder since parsing should be the process 
of determining which frames are valid frames in the stream. If you 
believe a fix in the decoder is more appropriate then maybe you can 
point me out to where/what should be done. Since I'm concerned about 
getting all the valid frames and not dropping any valid frames the 
current patch works great for me.
As a side not, I've tested the patch on a large collection of mp3's 
collected from many sources to verify the deframing logic, which now 
seems much better than before for the (all too common) corrupt mp3 files.