[FFmpeg-soc] [soc]: r2021 - in?eac3:?ac3dec.h?ac3dec_data.c?ac3dec_data.h eac3dec.c

[Michael Niedermayer]

On Wed, Mar 26, 2008 at 07:42:59PM +0100, Bartlomiej Wolowiec wrote:
> On środa, 26 marca 2008, Michael Niedermayer wrote:
> > >      while (buf_size > 0) {
> > >          int size_needed= s->frame_size ? s->frame_size : s->header_size;
> > >          len = s->inbuf_ptr - s->inbuf;
> > >
> > > @@ -56,7 +64,14 @@
> > >                      memmove(s->inbuf, s->inbuf + 1, s->header_size - 1);
> > >                      s->inbuf_ptr--;
> > >                  } else {
> > > -                    s->frame_size = len;
> > > +                    if(!s->stream_type) {
> > > +                        if(s->inbuf != s->inbuf_tab) {
> > > +                            *poutbuf = s->inbuf_tab;
> > > +                            *poutbuf_size = s->inbuf - s->inbuf_tab;
> > > +                            s->inbuf_ptr = s->inbuf_tab;
> > > +                            s->frame_size = 0;
> > > +                            break;
> > > +                        }
> >
> > this is VERY hackish, stream_type has from the point of view of a
> > generic (AAC+AC3+EAC3) parser no relation to the number of frames over
> > which channels are split.
> >
> > >                      /* update codec info */
> > >                      avctx->sample_rate = s->sample_rate;
> > >                      /* allow downmixing to stereo (or mono for AC3) */
> > > @@ -71,15 +86,18 @@
> > >                      }
> > >                      avctx->bit_rate = s->bit_rate;
> > >                      avctx->frame_size = s->samples;
> > > +                    } else {
> > > +                        //TODO update bit_rate
> > > +                        avctx->frame_size += s->samples;
> > > +                    }
> > > +                    s->frame_size = len;
> > >                  }
> > >              }
> > >          } else {
> > >
> > >              if(s->inbuf_ptr - s->inbuf == s->frame_size){
> > > -                *poutbuf = s->inbuf;
> > > -                *poutbuf_size = s->frame_size;
> > > +                s->inbuf += s->frame_size;
> > >                  s->inbuf_ptr = s->inbuf;
> > >                  s->frame_size = 0;
> > > -                break;
> > >              }
> >
> > this causes additional copying and delay i think for normal AC3 & AAC
> 
> Hi,
> Maybe this:
> if(eac3 stream){
>     // new code
> }else{
>     // existing code
> }
> will be a good solution for these problems?

I do not like generic code full of
If(codecA)
else

The sync function should return if it has a complete frame or if (maybe) not.
That is

flags= FRAME_START | FRAME_END;


You do this in stream_type but this name is totally wrong this is not a
stream_type. It is not even constant for a stream. NEVER even think of using
a name from the a52* spec, the people who wrote it are idiots.
Its a flag indicating the start/end of a frame. And IMHO it should be
returned more directly than as a context variable but thats rather minor
the name is a major issue. It totally confused me until looked it up in the
spec.

Code should be understandable without looking things up in the specs.

Also the "stream_type" has the wrong type (should be enum) and the types used
in AAC-AC3 code must be generic types not *AC3 specific types in headers which
arent #included.


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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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-soc/attachments/20080326/0ad3025c/attachment.pgp>