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

[Michael Niedermayer]

On Thu, Nov 13, 2008 at 11:39:18AM +0200, Yoav Steinberg wrote:
> 
> 
> 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.

could you provide some mp3s that are not handlded optimally?
anyway, this patch is definitly not ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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-devel/attachments/20090104/c6ec5f7d/attachment.pgp>