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

[Michael Niedermayer]

On Wed, Nov 12, 2008 at 07:02:55PM +0200, Yoav Steinberg wrote:
> Hi,
> Attached are patches witch make some improvements for mp3 parsing.
> skip_vbrtag_and_id3v1.patch:

> - When file contains ID3v1 tag at the end don't attempt to read it as part 
> of a packet. In some cases there are mp3 files where the ID3v1 tag was 
> stuck forcefully at the end of the file inside the last frame boundary. In 
> those cases the patch eliminates attempts to parse/decode the ID3 tag.
> - If we find a VBR tag at the beginning of the file don't attempt to parse 
> it as a valid frame.

These are 2 seperate things which must be in seperate patches.


> parse_all_frames.patch:
> - Buffer packets when we loose sync and start counting "valid" frames until 
> parser returns a valid frame. Then return buffered frames once we're back 
> in sync. This prevents the parser from discarding valid frames in many 
> cases.
>
> The goal of these patches was to improve the parsing of mp3 files so we 
> only parse/decode the actual (and all of) the audio data.
> I also notice there's no CRC checks performed on mpeg audio frames with CRC 
> info - this is a future to do:)
> 	Yoav.

[...]

> @@ -439,8 +446,14 @@
>      }
>  
>      if(frames < 0)
> +    {
> +        url_fseek(s->pb, base, SEEK_SET);
>          return;
> +    }

the brace placement is inconsistant relative to the existing


>  
> +    /* Skip the vbr tag frame */
> +    url_fseek(s->pb, base + vbrtag_size, SEEK_SET);
> +
>      spf = c.lsf ? 576 : 1152; /* Samples per frame, layer 3 */
>      st->duration = av_rescale_q(frames, (AVRational){spf, c.sample_rate},
>                                  st->time_base);

> @@ -452,7 +465,6 @@
>      AVStream *st;
>      uint8_t buf[ID3v1_TAG_SIZE];
>      int len, ret, filesize;
> -    int64_t off;
>  
>      st = av_new_stream(s, 0);
>      if (!st)
> @@ -492,9 +504,7 @@
>          url_fseek(s->pb, 0, SEEK_SET);
>      }
>  
> -    off = url_ftell(s->pb);
> -    mp3_parse_vbr_tags(s, st, off);
> -    url_fseek(s->pb, off, SEEK_SET);
> +    mp3_parse_vbr_tags(s, st, url_ftell(s->pb));

url_ftell() can be called in mp3_parse_vbr_tags()


>  
>      /* the parameters will be extracted from the compressed bitstream */
>      return 0;

> @@ -505,10 +515,20 @@
>  static int mp3_read_packet(AVFormatContext *s, AVPacket *pkt)
>  {
>      int ret, size;
> +    int64_t size_left;
>      //    AVStream *st = s->streams[0];
>  
>      size= MP3_PACKET_SIZE;
>  
> +    /* if we're nearing the end of the input and there's an
> +       ID3v1 tag then make sure we don't read it as a packet */
> +    if (((MPAContext*)s->priv_data)->found_id3v1 && s->file_size && 
> +        (size_left = s->file_size - url_ftell(s->pb) - ID3v1_TAG_SIZE) < size) {
> +        size = size_left;
> +        if (size <= 0)
> +            return AVERROR(EIO);
> +    }
> +
>      ret= av_get_packet(s->pb, pkt, size);
>  
>      pkt->stream_index = 0;

file_size is not known for stdin/pipes


[...]
> 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".

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)

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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/20081112/644fbda6/attachment.pgp>