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

[Michael Niedermayer]

On Sat, Feb 07, 2009 at 10:25:09AM +0200, Kostya wrote:
> 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).

argh ive missed that :(

what about placing the DISCARD logic prior to ff_rm_parse_packet() ?


>   
> > 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.

the file should have AAC in its name :)


>  
> > 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(), ...)

getting rid of this memcpy is welcome


> 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.

but that flow is broken in old and new format, and i really dont see the
advantage of this change

and again, if you can simplify the code (that implicates reducing the number
of statements and lines) that is welcome.

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20090207/f8e5cea9/attachment.pgp>