[FFmpeg-devel] [PATCH] Handle AAC in RM similar to other audio codecs

[Kostya]

On Fri, Feb 06, 2009 at 11:34:05PM +0100, Michael Niedermayer wrote:
> On Wed, Feb 04, 2009 at 09:06:59AM +0200, Kostya wrote:
> > On Tue, Feb 03, 2009 at 05:23:59PM +0200, Kostya wrote:
> > > On Tue, Feb 03, 2009 at 02:06:40PM +0100, Michael Niedermayer wrote:
> > > > On Tue, Feb 03, 2009 at 09:57:02AM +0200, Kostya wrote:
> > > > > With this patch RM demuxer handles AAC in the same way as other audio codecs -
> > > > > it loads it into temporary buffer and passes chunks from it as requested.
> > > > > (Demuxer still should be cleaned, refactored and maybe rewritten from scratch)
> > > > > 
> > > > > This patch also fixes -an behaviour with AAC sound.
> > > > 
> > > > your patch makes the demuxer more complex not less
> > > > also it appears that an additional copy pass is added (if so this is a
> > > > immedeate reason for rejection)
> > > 
> > > Another reason to rewrite this demuxer from scratch...
> > 
> > Well, how's this approach (like video frames)?
> > I think I can convert other audio codec handling to be like this
> > (hence no temporary buffer and one copy less for them). 
> 
> this too is more complex than it was, if you can reduce complexity this
> is welcome, if you can make it faster this is welcome as well.
> 
> to fix your bug, try:
> 
> Index: libavformat/rmdec.c
> ===================================================================
> --- libavformat/rmdec.c	(revision 17003)
> +++ libavformat/rmdec.c	(working copy)
> @@ -650,6 +650,7 @@
>      if(  (st->discard >= AVDISCARD_NONKEY && !(*flags&2))
>         || st->discard >= AVDISCARD_ALL){
>          av_free_packet(pkt);
> +        rm->audio_pkt_cnt= 0;
>          return -1;
>      }

Of course I've tried that - it makes demuxer resync (i.e. read all audio
packets data byte by byte until it finds next packet header). While that's
not that bad, it tends to hit false positives several bytes before actual
packet start and resulting packets are garbage (which is bad).
  
> ive not tested it as i have no aac+rv file

Then download one -
http://samples.mplayerhq.hu/real/VC-RV40/from_RP10/irobot_fox_550k_fs.rmvb
shows that effect.
 
> and about rewriting from scratch, first you have to understand the code
> to judge that it is worse than what you have in mind.

For now we have three approaches in the code:
1. Video frame approach - if not all data is read from the current packet,
  rm->remaining_len is set to the length of the leftover and in the next
  pass demuxer continues from that position with the same stream parsing
  that packet. (i.e. parse() - read(), parse() - read(), ...)
2. Most RM audio codecs (with fixed frame size) allocate temporary buffer
  at start, during parsing frame read all data there at once and memcpy()
  from there during subsequent calls
  (i.e. parse() - read(), cache() - memcpy(), parse - read(), ...)
3. With AAC subpacket sizes may vary, so parsing records their sizes and
  they are used in cache() calls
  (i.e. parse() - read(), cache() - read(), cache() - read(), ...)

I just propose moving that reading from cache() back to parse(), so it
will be executed in the same flow.

For now it is:
 if(audio subpackets left)
  cache();
 else{
resync:
   sync();
   if(!parse()) goto resync;
 }

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