[FFmpeg-soc] extension of ac3 parser

[Michael Niedermayer]

On Mon, Apr 21, 2008 at 01:04:58PM +0200, Bartlomiej Wolowiec wrote:
> On poniedziałek, 21 kwietnia 2008, Michael Niedermayer wrote:
> > On Mon, Apr 21, 2008 at 12:49:41AM +0200, Bartlomiej Wolowiec wrote:
> > > On niedziela, 20 kwietnia 2008, Michael Niedermayer wrote:
> > > > > Index: libavcodec/ac3_parser.h
> > > > > ===================================================================
> > > > > --- libavcodec/ac3_parser.h	(wersja 12910)
> > > > > +++ libavcodec/ac3_parser.h	(kopia robocza)
> > > > > @@ -35,14 +35,15 @@
> > > > >
> > > > >  /**
> > > > >   * Parses AC-3 frame header.
> > > > > - * Parses the header up to the lfeon element, which is the first 52
> > > > > or 54 bits - * depending on the audio coding mode.
> > > > > + * Parses the header up to the lfeon element, which is the first 52,
> > > > > 54, or 104 bits + * depending on the audio coding mode and
> > > > > read_channel_map. * @param buf[in] Array containing the first 7 bytes
> > > > > of the frame.
> > > >
> > > >                                                 ^^^^^^^
> > > >
> > > > 7 bytes and 104 bits ?
> > > >
> > > > >   * @param hdr[out] Pointer to struct where header info is written.
> > > > >
> > > > > + * @param read_channel_map[in] Determines whether channel_map is to
> > > > > be set
> > > >
> > > > why is this conditional and not always done?
> > > >
> > > > [...]
> > >
> > > Because  e.g. while parsing in buffer there are just 8 bytes, which won't
> > > be enough to read full information. According to solution proposed by
> > > you, parser firstly reads 7-byte headers, then, when full set of frames
> > > is in buffer, it sets number of channels etc.
> >
> > Well in that case i think it might be better to have these in seperate
> > functions.
> >
> > That is we need the following patches (if you agree and theres no issue iam
> > missing)
> > * change ff_ac3_parse_header() to take a GetBitContext instead of const
> > char * * add a ff_ac3_parse_header_full() which calls ff_ac3_parse_header()
> > and then reads the channel_map stuff.
> 
> Ok, the only case which is questionable in my opinion, it's using it in ac3 
> decoder. 

> Should before call ff_ac3_parse_header() new GetBitContext be 
> created, so that the old one won't be changed? (decoder must also read mix 
> levels)

No this would completely defeat the sense of the whole change.
No new GetBitContext should be created!
The intent of using GetBitContext is to get rid of the duplicated read/skip
code. If this requires further factorization and changes so be it.

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20080421/6bd42fbf/attachment.pgp>